Conversation
src/transformation/utils/symbols.ts
Outdated
| } | ||
|
|
||
| markSymbolAsReferencedInCurrentScopes(context, symbolId, identifier); | ||
| if (!isOptimizableVarArgSpread(context, symbol, identifier)) { |
There was a problem hiding this comment.
Could you add a comment explaining why in this case this is not needed?
There was a problem hiding this comment.
I'm sorry but I still don't understand this. Could you explain it from the perspective of this trackSymbolReference? Why do we not track this as a reference if the identifier is a vararg? Isn't that a reference?
| } | ||
| // Check for scopes which would stop optimization | ||
| const scope = findScope(context, ScopeType.Function | ScopeType.Try | ScopeType.Catch | ScopeType.File); | ||
| if (scope?.type === ScopeType.File && isExtensionValue(context, symbol, ExtensionKind.VarargConstant)) { |
There was a problem hiding this comment.
This if statement and the next one (L26) are really hard to read, maybe they can be refactored into an extra function? similar to line 30/37/41
| export function transformCompoundAssignmentExpression( | ||
| context: TransformationContext, | ||
| expression: ts.Expression, | ||
| // TODO: Change type to ts.LeftHandSideExpression? |
There was a problem hiding this comment.
Is this a question or a todo? Either way I think we should do a separate PR where we address all the TS deprecation issues. IRRC we have a few places where we use old API.
There was a problem hiding this comment.
Should I remove this? It was just copied over from where the code was previously.
There was a problem hiding this comment.
Oh, in that case just keep it. Until we figure out what exactly was meant by this.
| return false; | ||
| } | ||
| // Identifier must be a var arg in the local function scope | ||
| function isSpreadParameter(p: ts.ParameterDeclaration) { |
There was a problem hiding this comment.
Not sure what out policy is about inline functions is, but personally I prefer arrow functions for that.
| } | ||
| return test("a", "b", "c"); | ||
| ` | ||
| .expectLuaToMatchSnapshot() |
There was a problem hiding this comment.
Not really sure, if we need expectLuaToMatchSnapshot and expectToMatchJsResult. I think if the behaviour of JS and Lua matches, it does not matter what Lua output looks like.
There was a problem hiding this comment.
You could argue in this case it does matter, since the current unoptimized lua also works, but is not exactly what we want. So we should either expect it to match a snapshot (I'm fine with this), or somehow assert the resulting code indeed has the optimizations.
There was a problem hiding this comment.
Good Point. I think checking if the generated code does not contain unpack could be more explicit. But I am also fine with using a snapshot.
There was a problem hiding this comment.
Yeah, this was my thinking - we need to test that the optimization occurs and that it functions correctly. Testing for unpack feels a bit messy. If something breaks it will be clearly visible in the snapshot.
| } | ||
| return test("a", "b", "c"); | ||
| ` | ||
| .expectLuaToMatchSnapshot() |
There was a problem hiding this comment.
You could argue in this case it does matter, since the current unoptimized lua also works, but is not exactly what we want. So we should either expect it to match a snapshot (I'm fine with this), or somehow assert the resulting code indeed has the optimizations.
Perryvw
left a comment
Perryvw
left a comment
There was a problem hiding this comment.
Could you add the following test case to your spread tests:
function foo(...args: string[]) {
return args;
}This should be optimized right?
Other than that looks good to me.
src/transformation/utils/symbols.ts
Outdated
| } | ||
|
|
||
| markSymbolAsReferencedInCurrentScopes(context, symbolId, identifier); | ||
| if (!isOptimizableVarArgSpread(context, symbol, identifier)) { |
There was a problem hiding this comment.
I'm sorry but I still don't understand this. Could you explain it from the perspective of this trackSymbolReference? Why do we not track this as a reference if the identifier is a vararg? Isn't that a reference?
|
@Perryvw That won't be optimized. The function returns an array so the vararg needs to be wrapped. The only way to do what you're thinking there is using |
3 participants
This PR:
@varargannotation$varargconstant language extension for accessing the global ellipsis operatorVararg optimization will occur in any case that's viable. It will NOT be optimized:
closes #963