From 5ed4f65caba1eeafc0be9a06eeedc7c4b0dc3db2 Mon Sep 17 00:00:00 2001 From: Tom <26638278+tomblind@users.noreply.github.com> Date: Sun, 9 Jun 2019 09:00:21 -0600 Subject: [PATCH 1/7] preventing pack/unpack of rest parameters referenced with spread operators --- src/LuaTransformer.ts | 42 ++++++++++++++----- src/TSHelper.ts | 12 ++++++ .../__snapshots__/transformation.spec.ts.snap | 2 +- .../transformation/functionRestArguments.ts | 4 +- test/unit/functions.spec.ts | 27 ++++++++++++ 5 files changed, 75 insertions(+), 12 deletions(-) diff --git a/src/LuaTransformer.ts b/src/LuaTransformer.ts index 0da4e9b84..e009c1cbc 100644 --- a/src/LuaTransformer.ts +++ b/src/LuaTransformer.ts @@ -25,14 +25,14 @@ interface SymbolInfo { } interface FunctionDefinitionInfo { - referencedSymbols: Set; + referencedSymbols: Map; definition?: tstl.VariableDeclarationStatement | tstl.AssignmentStatement; } interface Scope { type: ScopeType; id: number; - referencedSymbols?: Set; + referencedSymbols?: Map; variableDeclarations?: tstl.VariableDeclarationStatement[]; functionDefinitions?: Map; importStatements?: tstl.Statement[]; @@ -1388,12 +1388,29 @@ export class LuaTransformer { return [paramNames, dotsLiteral, restParamName]; } + protected isRestParameterReferenced(identifier: tstl.Identifier, scope: Scope): boolean { + if (!identifier.symbolId) { + return true; + } + if (scope.referencedSymbols === undefined) { + return false; + } + const references = scope.referencedSymbols.get(identifier.symbolId); + return ( + references !== undefined && + // Ignore references that have spread element applied + references.some(r => r.parent === undefined || !ts.isSpreadElement(r.parent)) + ); + } + protected transformFunctionBody( parameters: ts.NodeArray, body: ts.Block, spreadIdentifier?: tstl.Identifier ): [tstl.Statement[], Scope] { this.pushScope(ScopeType.Function); + const bodyStatements = this.performHoisting(this.transformStatements(body.statements)); + const scope = this.popScope(); const headerStatements = []; @@ -1426,7 +1443,7 @@ export class LuaTransformer { } // Push spread operator here - if (spreadIdentifier) { + if (spreadIdentifier && this.isRestParameterReferenced(spreadIdentifier, scope)) { const spreadTable = this.wrapInTable(tstl.createDotsLiteral()); headerStatements.push(tstl.createVariableDeclarationStatement(spreadIdentifier, spreadTable)); } @@ -1434,10 +1451,6 @@ export class LuaTransformer { // Binding pattern statements need to be after spread table is declared headerStatements.push(...bindingPatternDeclarations); - const bodyStatements = this.performHoisting(this.transformStatements(body.statements)); - - const scope = this.popScope(); - return [headerStatements.concat(bodyStatements), scope]; } @@ -1844,7 +1857,7 @@ export class LuaTransformer { if (!scope.functionDefinitions) { scope.functionDefinitions = new Map(); } - const functionInfo = { referencedSymbols: functionScope.referencedSymbols || new Set() }; + const functionInfo = { referencedSymbols: functionScope.referencedSymbols || new Map() }; scope.functionDefinitions.set(name.symbolId, functionInfo); } return this.createLocalOrExportedOrGlobalDeclaration(name, functionExpression, functionDeclaration); @@ -4543,6 +4556,10 @@ export class LuaTransformer { return innerExpression; } + if (tsHelper.isRestParameter(expression.expression, this.checker)) { + return tstl.createDotsLiteral(expression); + } + const type = this.checker.getTypeAtLocation(expression.expression); if (tsHelper.isArrayType(type, this.checker, this.program)) { return this.createUnpackCall(innerExpression, expression); @@ -5212,9 +5229,14 @@ export class LuaTransformer { //Mark symbol as seen in all current scopes for (const scope of this.scopeStack) { if (!scope.referencedSymbols) { - scope.referencedSymbols = new Set(); + scope.referencedSymbols = new Map(); + } + let references = scope.referencedSymbols.get(symbolId); + if (!references) { + references = []; + scope.referencedSymbols.set(symbolId, references); } - scope.referencedSymbols.add(symbolId); + references.push(identifier); } } } diff --git a/src/TSHelper.ts b/src/TSHelper.ts index 8d701512b..984d7dc04 100644 --- a/src/TSHelper.ts +++ b/src/TSHelper.ts @@ -184,6 +184,18 @@ export class TSHelper { return TSHelper.getCustomDecorators(type, checker).has(DecoratorKind.LuaIterator); } + public static isRestParameter(node: ts.Node, checker: ts.TypeChecker): boolean { + const symbol = checker.getSymbolAtLocation(node); + if (!symbol) { + return false; + } + const declarations = symbol.getDeclarations(); + if (!declarations) { + return false; + } + return declarations.some(d => ts.isParameter(d) && d.dotDotDotToken !== undefined); + } + public static isTupleReturnCall(node: ts.Node, checker: ts.TypeChecker): boolean { if (ts.isCallExpression(node)) { const signature = checker.getResolvedSignature(node); diff --git a/test/translation/__snapshots__/transformation.spec.ts.snap b/test/translation/__snapshots__/transformation.spec.ts.snap index 448f18106..58e99b895 100644 --- a/test/translation/__snapshots__/transformation.spec.ts.snap +++ b/test/translation/__snapshots__/transformation.spec.ts.snap @@ -256,6 +256,7 @@ end" exports[`Transformation (functionRestArguments) 1`] = ` "function varargsFunction(self, a, ...) local b = ({...}) + local c = b end" `; @@ -319,7 +320,6 @@ end function MyClass.prototype.____constructor(self) end function MyClass.prototype.varargsFunction(self, a, ...) - local b = ({...}) end" `; diff --git a/test/translation/transformation/functionRestArguments.ts b/test/translation/transformation/functionRestArguments.ts index 3b015a4d6..63649f09b 100644 --- a/test/translation/transformation/functionRestArguments.ts +++ b/test/translation/transformation/functionRestArguments.ts @@ -1 +1,3 @@ -function varargsFunction(a: string, ...b: string[]): void {} +function varargsFunction(a: string, ...b: string[]): void { + const c = b; +} diff --git a/test/unit/functions.spec.ts b/test/unit/functions.spec.ts index cbcd4ad87..83a16a0b3 100644 --- a/test/unit/functions.spec.ts +++ b/test/unit/functions.spec.ts @@ -504,3 +504,30 @@ test("Function rest binding pattern", () => { expect(result).toBe("defxyzabc"); }); + +test("Function rest parameter", () => { + const code = ` + function foo(a: unknown, ...b: string[]) { + return b.join(""); + } + return foo("A", "B", "C", "D"); + `; + + expect(util.transpileAndExecute(code)).toBe("BCD"); +}); + +test("Function rest forward", () => { + const code = ` + function foo(a: unknown, ...b: string[]) { + const c = [...b]; + return c.join(""); + } + function bar(a: unknown, ...b: string[]) { + return foo(a, ...b); + } + return bar("A", "B", "C", "D"); + `; + + expect(util.transpileString(code)).not.toMatch("b = ({...})"); + expect(util.transpileAndExecute(code)).toBe("BCD"); +}); From 3e1e58aade0392b8313809d1a9f1b0fda7fb06a3 Mon Sep 17 00:00:00 2001 From: Tom <26638278+tomblind@users.noreply.github.com> Date: Mon, 10 Jun 2019 06:46:38 -0600 Subject: [PATCH 2/7] fixed referencing rest parameters in nested functions --- src/LuaTransformer.ts | 26 +++++++++++++++++++++----- test/unit/functions.spec.ts | 29 +++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/LuaTransformer.ts b/src/LuaTransformer.ts index e009c1cbc..c84d2d0f7 100644 --- a/src/LuaTransformer.ts +++ b/src/LuaTransformer.ts @@ -1388,7 +1388,11 @@ export class LuaTransformer { return [paramNames, dotsLiteral, restParamName]; } - protected isRestParameterReferenced(identifier: tstl.Identifier, scope: Scope): boolean { + protected isRestParameterReferenced( + identifier: tstl.Identifier, + scope: Scope, + parameters: ts.NodeArray + ): boolean { if (!identifier.symbolId) { return true; } @@ -1398,8 +1402,14 @@ export class LuaTransformer { const references = scope.referencedSymbols.get(identifier.symbolId); return ( references !== undefined && - // Ignore references that have spread element applied - references.some(r => r.parent === undefined || !ts.isSpreadElement(r.parent)) + // Ignore references that have spread element applied (unless the reference is in a nested function) + references.some(r => { + if (r.parent === undefined || !ts.isSpreadElement(r.parent)) { + return true; + } + const scopeFunction = tsHelper.findFirstNodeAbove(r, ts.isFunctionLike); + return scopeFunction === undefined || scopeFunction.parameters !== parameters; + }) ); } @@ -1443,7 +1453,7 @@ export class LuaTransformer { } // Push spread operator here - if (spreadIdentifier && this.isRestParameterReferenced(spreadIdentifier, scope)) { + if (spreadIdentifier && this.isRestParameterReferenced(spreadIdentifier, scope, parameters)) { const spreadTable = this.wrapInTable(tstl.createDotsLiteral()); headerStatements.push(tstl.createVariableDeclarationStatement(spreadIdentifier, spreadTable)); } @@ -4557,7 +4567,13 @@ export class LuaTransformer { } if (tsHelper.isRestParameter(expression.expression, this.checker)) { - return tstl.createDotsLiteral(expression); + const scopeFunction = tsHelper.findFirstNodeAbove(expression, ts.isFunctionLike); + if (scopeFunction) { + const symbol = this.checker.getSymbolAtLocation(expression.expression); + if (symbol && scopeFunction.parameters.some(p => symbol === this.checker.getSymbolAtLocation(p.name))) { + return tstl.createDotsLiteral(expression); + } + } } const type = this.checker.getTypeAtLocation(expression.expression); diff --git a/test/unit/functions.spec.ts b/test/unit/functions.spec.ts index 83a16a0b3..451e9a8e1 100644 --- a/test/unit/functions.spec.ts +++ b/test/unit/functions.spec.ts @@ -516,6 +516,20 @@ test("Function rest parameter", () => { expect(util.transpileAndExecute(code)).toBe("BCD"); }); +test("Function nested rest parameter", () => { + const code = ` + function foo(a: unknown, ...b: string[]) { + function bar() { + return b.join(""); + } + return bar(); + } + return foo("A", "B", "C", "D"); + `; + + expect(util.transpileAndExecute(code)).toBe("BCD"); +}); + test("Function rest forward", () => { const code = ` function foo(a: unknown, ...b: string[]) { @@ -531,3 +545,18 @@ test("Function rest forward", () => { expect(util.transpileString(code)).not.toMatch("b = ({...})"); expect(util.transpileAndExecute(code)).toBe("BCD"); }); + +test("Function nested rest forward", () => { + const code = ` + function foo(a: unknown, ...b: string[]) { + function bar() { + const c = [...b]; + return c.join(""); + } + return bar(); + } + return foo("A", "B", "C", "D"); + `; + + expect(util.transpileAndExecute(code)).toBe("BCD"); +}); From ae3460f491303c6ad145cbb8c3f3eba775bb58de Mon Sep 17 00:00:00 2001 From: Tom <26638278+tomblind@users.noreply.github.com> Date: Mon, 10 Jun 2019 07:04:47 -0600 Subject: [PATCH 3/7] moved some general logic into a helper --- src/LuaTransformer.ts | 17 +++++------------ src/TSHelper.ts | 9 +++++++++ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/LuaTransformer.ts b/src/LuaTransformer.ts index c84d2d0f7..a6a0c4636 100644 --- a/src/LuaTransformer.ts +++ b/src/LuaTransformer.ts @@ -1388,11 +1388,7 @@ export class LuaTransformer { return [paramNames, dotsLiteral, restParamName]; } - protected isRestParameterReferenced( - identifier: tstl.Identifier, - scope: Scope, - parameters: ts.NodeArray - ): boolean { + protected isRestParameterReferenced(identifier: tstl.Identifier, scope: Scope): boolean { if (!identifier.symbolId) { return true; } @@ -1408,7 +1404,7 @@ export class LuaTransformer { return true; } const scopeFunction = tsHelper.findFirstNodeAbove(r, ts.isFunctionLike); - return scopeFunction === undefined || scopeFunction.parameters !== parameters; + return scopeFunction === undefined || !tsHelper.hasParameter(scopeFunction.parameters, r, this.checker); }) ); } @@ -1453,7 +1449,7 @@ export class LuaTransformer { } // Push spread operator here - if (spreadIdentifier && this.isRestParameterReferenced(spreadIdentifier, scope, parameters)) { + if (spreadIdentifier && this.isRestParameterReferenced(spreadIdentifier, scope)) { const spreadTable = this.wrapInTable(tstl.createDotsLiteral()); headerStatements.push(tstl.createVariableDeclarationStatement(spreadIdentifier, spreadTable)); } @@ -4568,11 +4564,8 @@ export class LuaTransformer { if (tsHelper.isRestParameter(expression.expression, this.checker)) { const scopeFunction = tsHelper.findFirstNodeAbove(expression, ts.isFunctionLike); - if (scopeFunction) { - const symbol = this.checker.getSymbolAtLocation(expression.expression); - if (symbol && scopeFunction.parameters.some(p => symbol === this.checker.getSymbolAtLocation(p.name))) { - return tstl.createDotsLiteral(expression); - } + if (scopeFunction && tsHelper.hasParameter(scopeFunction.parameters, expression.expression, this.checker)) { + return tstl.createDotsLiteral(expression); } } diff --git a/src/TSHelper.ts b/src/TSHelper.ts index 984d7dc04..44a74f568 100644 --- a/src/TSHelper.ts +++ b/src/TSHelper.ts @@ -196,6 +196,15 @@ export class TSHelper { return declarations.some(d => ts.isParameter(d) && d.dotDotDotToken !== undefined); } + public static hasParameter( + parameters: ts.NodeArray, + parameter: ts.Node, + checker: ts.TypeChecker + ): boolean { + const symbol = checker.getSymbolAtLocation(parameter); + return symbol !== undefined && parameters.some(p => symbol === checker.getSymbolAtLocation(p.name)); + } + public static isTupleReturnCall(node: ts.Node, checker: ts.TypeChecker): boolean { if (ts.isCallExpression(node)) { const signature = checker.getResolvedSignature(node); From bf75ebfdacb2ea3ea8cc6e9ea7db51d898ab2472 Mon Sep 17 00:00:00 2001 From: Tom <26638278+tomblind@users.noreply.github.com> Date: Mon, 10 Jun 2019 15:59:02 -0600 Subject: [PATCH 4/7] @elipsisForward directive to replace implicit optimization --- src/Decorator.ts | 3 ++ src/LuaTransformer.ts | 44 ++++++++++----- src/TSHelper.ts | 10 ++-- src/TSTLErrors.ts | 4 ++ test/unit/functions.spec.ts | 103 ++++++++++++++++++++++++++++++------ 5 files changed, 128 insertions(+), 36 deletions(-) diff --git a/src/Decorator.ts b/src/Decorator.ts index b8a4f3ad3..3932ad0c3 100644 --- a/src/Decorator.ts +++ b/src/Decorator.ts @@ -29,6 +29,8 @@ export class Decorator { return DecoratorKind.NoSelf; case "noselfinfile": return DecoratorKind.NoSelfInFile; + case "elipsisforward": + return DecoratorKind.ElipsisForward; } return undefined; @@ -61,4 +63,5 @@ export enum DecoratorKind { LuaTable = "LuaTable", NoSelf = "NoSelf", NoSelfInFile = "NoSelfInFile", + ElipsisForward = "ElipsisForward", } diff --git a/src/LuaTransformer.ts b/src/LuaTransformer.ts index a6a0c4636..455d2088b 100644 --- a/src/LuaTransformer.ts +++ b/src/LuaTransformer.ts @@ -1398,14 +1398,13 @@ export class LuaTransformer { const references = scope.referencedSymbols.get(identifier.symbolId); return ( references !== undefined && - // Ignore references that have spread element applied (unless the reference is in a nested function) - references.some(r => { - if (r.parent === undefined || !ts.isSpreadElement(r.parent)) { - return true; - } - const scopeFunction = tsHelper.findFirstNodeAbove(r, ts.isFunctionLike); - return scopeFunction === undefined || !tsHelper.hasParameter(scopeFunction.parameters, r, this.checker); - }) + // Ignore references using @elipsisForward + references.some( + r => + r.parent === undefined || + !ts.isCallExpression(r.parent) || + !tsHelper.isElipsisForwardType(r.parent.expression, this.checker) + ) ); } @@ -4562,11 +4561,11 @@ export class LuaTransformer { return innerExpression; } - if (tsHelper.isRestParameter(expression.expression, this.checker)) { - const scopeFunction = tsHelper.findFirstNodeAbove(expression, ts.isFunctionLike); - if (scopeFunction && tsHelper.hasParameter(scopeFunction.parameters, expression.expression, this.checker)) { - return tstl.createDotsLiteral(expression); - } + if ( + ts.isCallExpression(expression.expression) && + tsHelper.isElipsisForwardType(expression.expression.expression, this.checker) + ) { + return tstl.createDotsLiteral(expression); } const type = this.checker.getTypeAtLocation(expression.expression); @@ -4652,6 +4651,25 @@ export class LuaTransformer { return tstl.createIdentifier("nil"); } + // Validate @elipsisForward + if (tsHelper.isElipsisForwardType(identifier, this.checker)) { + const callExpression = tsHelper.findFirstNodeAbove(identifier, ts.isCallExpression); + if (!callExpression || !callExpression.parent || !ts.isSpreadElement(callExpression.parent)) { + throw TSTLErrors.InvalidElipsisForward( + identifier, + "@elipsesForward can only be used on a function, and called in a spread expression." + ); + } else if ( + callExpression.arguments.length !== 1 || + !tsHelper.isRestParameter(callExpression.arguments[0], this.checker) + ) { + throw TSTLErrors.InvalidElipsisForward( + callExpression, + "@elipsesForward function can only be passed a single rest parameter." + ); + } + } + const text = this.hasUnsafeIdentifierName(identifier) ? this.createSafeName(this.getIdentifierText(identifier)) : this.getIdentifierText(identifier); diff --git a/src/TSHelper.ts b/src/TSHelper.ts index 44a74f568..8ea17fa6f 100644 --- a/src/TSHelper.ts +++ b/src/TSHelper.ts @@ -196,13 +196,9 @@ export class TSHelper { return declarations.some(d => ts.isParameter(d) && d.dotDotDotToken !== undefined); } - public static hasParameter( - parameters: ts.NodeArray, - parameter: ts.Node, - checker: ts.TypeChecker - ): boolean { - const symbol = checker.getSymbolAtLocation(parameter); - return symbol !== undefined && parameters.some(p => symbol === checker.getSymbolAtLocation(p.name)); + public static isElipsisForwardType(node: ts.Node, checker: ts.TypeChecker): boolean { + const type = checker.getTypeAtLocation(node); + return type !== undefined && TSHelper.getCustomDecorators(type, checker).has(DecoratorKind.ElipsisForward); } public static isTupleReturnCall(node: ts.Node, checker: ts.TypeChecker): boolean { diff --git a/src/TSTLErrors.ts b/src/TSTLErrors.ts index 26d08d1ca..f0f492e77 100644 --- a/src/TSTLErrors.ts +++ b/src/TSTLErrors.ts @@ -204,4 +204,8 @@ export class TSTLErrors { node ); }; + + public static InvalidElipsisForward = (node: ts.Node, message: string) => { + return new TranspileError(`Invalid use of @elipsisForward: ${message}`, node); + }; } diff --git a/test/unit/functions.spec.ts b/test/unit/functions.spec.ts index 451e9a8e1..4b8988e4e 100644 --- a/test/unit/functions.spec.ts +++ b/test/unit/functions.spec.ts @@ -530,33 +530,104 @@ test("Function nested rest parameter", () => { expect(util.transpileAndExecute(code)).toBe("BCD"); }); -test("Function rest forward", () => { +test("Function nested rest spread", () => { const code = ` function foo(a: unknown, ...b: string[]) { - const c = [...b]; + function bar() { + const c = [...b]; + return c.join(""); + } + return bar(); + } + return foo("A", "B", "C", "D"); + `; + + expect(util.transpileAndExecute(code)).toBe("BCD"); +}); + +test("@elipsisForward", () => { + const tsHeader = ` + /** @elipsisForward */ declare function elipsisForward(args: unknown): unknown[]; + `; + const code = ` + function foo(a: unknown, ...b: unknown[]) { + const c = [...elipsisForward(b)]; return c.join(""); } - function bar(a: unknown, ...b: string[]) { - return foo(a, ...b); + function bar(a: unknown, ...b: unknown[]) { + return foo(a, ...elipsisForward(b)); } return bar("A", "B", "C", "D"); `; - expect(util.transpileString(code)).not.toMatch("b = ({...})"); - expect(util.transpileAndExecute(code)).toBe("BCD"); + expect(util.transpileString(tsHeader + code)).not.toMatch("b = ({...})"); + expect(util.transpileAndExecute(code, undefined, undefined, tsHeader)).toBe("BCD"); }); -test("Function nested rest forward", () => { +test("invalid non-ambient @elipsisForward", () => { const code = ` - function foo(a: unknown, ...b: string[]) { - function bar() { - const c = [...b]; - return c.join(""); - } - return bar(); - } - return foo("A", "B", "C", "D"); + /** @elipsisForward */ function elipsisForward(args: unknown): unknown[] { return []; } `; + expect(() => util.transpileString(code)).toThrow( + TSTLErrors.InvalidElipsisForward( + ts.createEmptyStatement(), + "@elipsesForward can only be used on a function, and called in a spread expression." + ).message + ); +}); - expect(util.transpileAndExecute(code)).toBe("BCD"); +test.each([ + "const a = elipsisForward(args);", + "const a = [...[elipsisForward()]]", + "for (const v of elipsisForward(args)) {}", + "console.log(elipsisForward);", + "elipsisForward.call(null, 0, 0, 0);", + "let array = [0, elipsisForward, 1];", + "const call: any; call(elipsisForward);", +])("invalid @elipsisForward use (%p)", statement => { + const code = ` + /** @elipsisForward */ declare function elipsisForward(args: unknown): unknown[]; + function foo(...args: unknown[]) { + ${statement} + }`; + + expect(() => util.transpileString(code)).toThrow( + TSTLErrors.InvalidElipsisForward( + ts.createEmptyStatement(), + "@elipsesForward can only be used on a function, and called in a spread expression." + ).message + ); +}); + +test.each(["unknown", "unknown[]", "Array<[]>", "Iterable"])( + "invalid @elipsisForward argument type (%p)", + type => { + const code = ` + /** @elipsisForward */ declare function elipsisForward(args: unknown): unknown[]; + function foo(a: ${type}) { + const x = [...elipsisForward(a)]; + }`; + + expect(() => util.transpileString(code)).toThrow( + TSTLErrors.InvalidElipsisForward( + ts.createEmptyStatement(), + "@elipsesForward function can only be passed a single rest parameter." + ).message + ); + } +); + +test.each(["", "a, 0", "a, 0, 1"])("invalid @elipsisForward argument count (%p)", args => { + const code = ` + /** @elipsisForward */ declare function elipsisForward(...args: unknown[]): unknown[]; + function foo(...a: unknown[]) { + const x = [...elipsisForward(${args})]; + }`; + + expect(() => util.transpileString(code)).toThrow( + TSTLErrors.InvalidElipsisForward( + ts.createEmptyStatement(), + "@elipsesForward function can only be passed a single rest parameter." + ).message + ); }); From 85c1f1b9a3d679f85531a6613d63c24fc3c22ff6 Mon Sep 17 00:00:00 2001 From: Tom <26638278+tomblind@users.noreply.github.com> Date: Mon, 10 Jun 2019 17:07:38 -0600 Subject: [PATCH 5/7] fixed reference tracking when noHoisting is set --- src/LuaTransformer.ts | 4 +++- test/unit/functions.spec.ts | 32 ++++++++++++++++++++++++-------- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/LuaTransformer.ts b/src/LuaTransformer.ts index 455d2088b..e4eecc475 100644 --- a/src/LuaTransformer.ts +++ b/src/LuaTransformer.ts @@ -5252,7 +5252,9 @@ export class LuaTransformer { if (declaration && identifier.pos < declaration.pos) { throw TSTLErrors.ReferencedBeforeDeclaration(identifier); } - } else if (symbolId !== undefined) { + } + + if (symbolId !== undefined) { //Mark symbol as seen in all current scopes for (const scope of this.scopeStack) { if (!scope.referencedSymbols) { diff --git a/test/unit/functions.spec.ts b/test/unit/functions.spec.ts index 4b8988e4e..3c61a2d82 100644 --- a/test/unit/functions.spec.ts +++ b/test/unit/functions.spec.ts @@ -505,7 +505,7 @@ test("Function rest binding pattern", () => { expect(result).toBe("defxyzabc"); }); -test("Function rest parameter", () => { +test.each([{}, { noHoisting: true }])("Function rest parameter", compilerOptions => { const code = ` function foo(a: unknown, ...b: string[]) { return b.join(""); @@ -513,10 +513,10 @@ test("Function rest parameter", () => { return foo("A", "B", "C", "D"); `; - expect(util.transpileAndExecute(code)).toBe("BCD"); + expect(util.transpileAndExecute(code, compilerOptions)).toBe("BCD"); }); -test("Function nested rest parameter", () => { +test.each([{}, { noHoisting: true }])("Function nested rest parameter", compilerOptions => { const code = ` function foo(a: unknown, ...b: string[]) { function bar() { @@ -527,10 +527,10 @@ test("Function nested rest parameter", () => { return foo("A", "B", "C", "D"); `; - expect(util.transpileAndExecute(code)).toBe("BCD"); + expect(util.transpileAndExecute(code, compilerOptions)).toBe("BCD"); }); -test("Function nested rest spread", () => { +test.each([{}, { noHoisting: true }])("Function nested rest spread", compilerOptions => { const code = ` function foo(a: unknown, ...b: string[]) { function bar() { @@ -542,10 +542,10 @@ test("Function nested rest spread", () => { return foo("A", "B", "C", "D"); `; - expect(util.transpileAndExecute(code)).toBe("BCD"); + expect(util.transpileAndExecute(code, compilerOptions)).toBe("BCD"); }); -test("@elipsisForward", () => { +test.each([{}, { noHoisting: true }])("@elipsisForward", compilerOptions => { const tsHeader = ` /** @elipsisForward */ declare function elipsisForward(args: unknown): unknown[]; `; @@ -561,7 +561,23 @@ test("@elipsisForward", () => { `; expect(util.transpileString(tsHeader + code)).not.toMatch("b = ({...})"); - expect(util.transpileAndExecute(code, undefined, undefined, tsHeader)).toBe("BCD"); + expect(util.transpileAndExecute(code, compilerOptions, undefined, tsHeader)).toBe("BCD"); +}); + +test.each([{}, { noHoisting: true }])("@elipsisForward mixed with rest spread", compilerOptions => { + const tsHeader = ` + /** @elipsisForward */ declare function elipsisForward(args: unknown): unknown[]; + `; + const code = ` + function foo(a: unknown, ...b: unknown[]) { + const c = [...elipsisForward(b)]; + const d = [...b]; + return c.join("") + d.join(""); + } + return foo("A", "B", "C", "D"); + `; + + expect(util.transpileAndExecute(code, compilerOptions, undefined, tsHeader)).toBe("BCDBCD"); }); test("invalid non-ambient @elipsisForward", () => { From 8423e5c505e976ae4e9426344b1b112313d33301 Mon Sep 17 00:00:00 2001 From: Tom <26638278+tomblind@users.noreply.github.com> Date: Tue, 11 Jun 2019 08:13:48 -0600 Subject: [PATCH 6/7] Allowing @elipsisForward to take no args so it can be used to access global elipsis --- src/LuaTransformer.ts | 5 +++-- test/unit/functions.spec.ts | 18 +++++++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/LuaTransformer.ts b/src/LuaTransformer.ts index e4eecc475..69d9466e3 100644 --- a/src/LuaTransformer.ts +++ b/src/LuaTransformer.ts @@ -4660,8 +4660,9 @@ export class LuaTransformer { "@elipsesForward can only be used on a function, and called in a spread expression." ); } else if ( - callExpression.arguments.length !== 1 || - !tsHelper.isRestParameter(callExpression.arguments[0], this.checker) + callExpression.arguments.length > 1 || + (callExpression.arguments.length === 1 && + !tsHelper.isRestParameter(callExpression.arguments[0], this.checker)) ) { throw TSTLErrors.InvalidElipsisForward( callExpression, diff --git a/test/unit/functions.spec.ts b/test/unit/functions.spec.ts index 3c61a2d82..30d12d9a8 100644 --- a/test/unit/functions.spec.ts +++ b/test/unit/functions.spec.ts @@ -633,7 +633,7 @@ test.each(["unknown", "unknown[]", "Array<[]>", "Iterable"])( } ); -test.each(["", "a, 0", "a, 0, 1"])("invalid @elipsisForward argument count (%p)", args => { +test.each(["a, 0", "a, 0, 1"])("invalid @elipsisForward argument count (%p)", args => { const code = ` /** @elipsisForward */ declare function elipsisForward(...args: unknown[]): unknown[]; function foo(...a: unknown[]) { @@ -647,3 +647,19 @@ test.each(["", "a, 0", "a, 0, 1"])("invalid @elipsisForward argument count (%p)" ).message ); }); + +test.each([{}, { noHoisting: true }])("@elipsisForward with no argument", compilerOptions => { + const tsHeader = ` + /** @elipsisForward */ declare function elipsisForward(args?: unknown): unknown[]; + `; + const code = ` + function foo(a: unknown, ...b: unknown[]) { + const c = [...elipsisForward()]; + return c.join(""); + } + return foo("A", "B", "C", "D"); + `; + + expect(util.transpileString(tsHeader + code)).not.toMatch("b = ({...})"); + expect(util.transpileAndExecute(code, compilerOptions, undefined, tsHeader)).toBe("BCD"); +}); From 21c5f2e52d0d019863a7b14ec74f0ed3a8e93d0d Mon Sep 17 00:00:00 2001 From: Tom <26638278+tomblind@users.noreply.github.com> Date: Wed, 12 Jun 2019 07:38:02 -0600 Subject: [PATCH 7/7] updated to use @varArg type instead of @elipsisForward function --- src/Decorator.ts | 6 +- src/LuaTransformer.ts | 40 ++--------- src/TSHelper.ts | 4 +- test/unit/functions.spec.ts | 135 +++++++++++------------------------- 4 files changed, 53 insertions(+), 132 deletions(-) diff --git a/src/Decorator.ts b/src/Decorator.ts index 3932ad0c3..d9c7a6d87 100644 --- a/src/Decorator.ts +++ b/src/Decorator.ts @@ -29,8 +29,8 @@ export class Decorator { return DecoratorKind.NoSelf; case "noselfinfile": return DecoratorKind.NoSelfInFile; - case "elipsisforward": - return DecoratorKind.ElipsisForward; + case "vararg": + return DecoratorKind.VarArg; } return undefined; @@ -63,5 +63,5 @@ export enum DecoratorKind { LuaTable = "LuaTable", NoSelf = "NoSelf", NoSelfInFile = "NoSelfInFile", - ElipsisForward = "ElipsisForward", + VarArg = "VarArg", } diff --git a/src/LuaTransformer.ts b/src/LuaTransformer.ts index 69d9466e3..a93c26c97 100644 --- a/src/LuaTransformer.ts +++ b/src/LuaTransformer.ts @@ -1396,15 +1396,12 @@ export class LuaTransformer { return false; } const references = scope.referencedSymbols.get(identifier.symbolId); - return ( - references !== undefined && - // Ignore references using @elipsisForward - references.some( - r => - r.parent === undefined || - !ts.isCallExpression(r.parent) || - !tsHelper.isElipsisForwardType(r.parent.expression, this.checker) - ) + if (!references) { + return false; + } + // Ignore references to @varArg types in spread elements + return references.some( + r => !r.parent || !ts.isSpreadElement(r.parent) || !tsHelper.isVarArgType(r, this.checker) ); } @@ -4561,10 +4558,7 @@ export class LuaTransformer { return innerExpression; } - if ( - ts.isCallExpression(expression.expression) && - tsHelper.isElipsisForwardType(expression.expression.expression, this.checker) - ) { + if (ts.isIdentifier(expression.expression) && tsHelper.isVarArgType(expression.expression, this.checker)) { return tstl.createDotsLiteral(expression); } @@ -4651,26 +4645,6 @@ export class LuaTransformer { return tstl.createIdentifier("nil"); } - // Validate @elipsisForward - if (tsHelper.isElipsisForwardType(identifier, this.checker)) { - const callExpression = tsHelper.findFirstNodeAbove(identifier, ts.isCallExpression); - if (!callExpression || !callExpression.parent || !ts.isSpreadElement(callExpression.parent)) { - throw TSTLErrors.InvalidElipsisForward( - identifier, - "@elipsesForward can only be used on a function, and called in a spread expression." - ); - } else if ( - callExpression.arguments.length > 1 || - (callExpression.arguments.length === 1 && - !tsHelper.isRestParameter(callExpression.arguments[0], this.checker)) - ) { - throw TSTLErrors.InvalidElipsisForward( - callExpression, - "@elipsesForward function can only be passed a single rest parameter." - ); - } - } - const text = this.hasUnsafeIdentifierName(identifier) ? this.createSafeName(this.getIdentifierText(identifier)) : this.getIdentifierText(identifier); diff --git a/src/TSHelper.ts b/src/TSHelper.ts index 8ea17fa6f..90c4cbcf6 100644 --- a/src/TSHelper.ts +++ b/src/TSHelper.ts @@ -196,9 +196,9 @@ export class TSHelper { return declarations.some(d => ts.isParameter(d) && d.dotDotDotToken !== undefined); } - public static isElipsisForwardType(node: ts.Node, checker: ts.TypeChecker): boolean { + public static isVarArgType(node: ts.Node, checker: ts.TypeChecker): boolean { const type = checker.getTypeAtLocation(node); - return type !== undefined && TSHelper.getCustomDecorators(type, checker).has(DecoratorKind.ElipsisForward); + return type !== undefined && TSHelper.getCustomDecorators(type, checker).has(DecoratorKind.VarArg); } public static isTupleReturnCall(node: ts.Node, checker: ts.TypeChecker): boolean { diff --git a/test/unit/functions.spec.ts b/test/unit/functions.spec.ts index 30d12d9a8..0201dd66a 100644 --- a/test/unit/functions.spec.ts +++ b/test/unit/functions.spec.ts @@ -545,121 +545,68 @@ test.each([{}, { noHoisting: true }])("Function nested rest spread", compilerOpt expect(util.transpileAndExecute(code, compilerOptions)).toBe("BCD"); }); -test.each([{}, { noHoisting: true }])("@elipsisForward", compilerOptions => { - const tsHeader = ` - /** @elipsisForward */ declare function elipsisForward(args: unknown): unknown[]; +test.each([{}, { noHoisting: true }])("Function rest parameter (unreferenced)", compilerOptions => { + const code = ` + function foo(a: unknown, ...b: string[]) { + return "foobar"; + } + return foo("A", "B", "C", "D"); `; + + expect(util.transpileString(code, compilerOptions)).not.toMatch("b = ({...})"); + expect(util.transpileAndExecute(code, compilerOptions)).toBe("foobar"); +}); + +test.each([{}, { noHoisting: true }])("@varArg", compilerOptions => { const code = ` - function foo(a: unknown, ...b: unknown[]) { - const c = [...elipsisForward(b)]; + /** @varArg */ type LuaVarArg = A & { __luaVarArg?: never }; + function foo(a: unknown, ...b: LuaVarArg) { + const c = [...b]; return c.join(""); } - function bar(a: unknown, ...b: unknown[]) { - return foo(a, ...elipsisForward(b)); + function bar(a: unknown, ...b: LuaVarArg) { + return foo(a, ...b); } return bar("A", "B", "C", "D"); `; - expect(util.transpileString(tsHeader + code)).not.toMatch("b = ({...})"); - expect(util.transpileAndExecute(code, compilerOptions, undefined, tsHeader)).toBe("BCD"); + const lua = util.transpileString(code, compilerOptions); + expect(lua).not.toMatch("b = ({...})"); + expect(lua).not.toMatch("unpack"); + expect(util.transpileAndExecute(code, compilerOptions)).toBe("BCD"); }); -test.each([{}, { noHoisting: true }])("@elipsisForward mixed with rest spread", compilerOptions => { - const tsHeader = ` - /** @elipsisForward */ declare function elipsisForward(args: unknown): unknown[]; - `; +test.each([{}, { noHoisting: true }])("@varArg array access", compilerOptions => { const code = ` - function foo(a: unknown, ...b: unknown[]) { - const c = [...elipsisForward(b)]; - const d = [...b]; - return c.join("") + d.join(""); + /** @varArg */ type LuaVarArg = A & { __luaVarArg?: never }; + function foo(a: unknown, ...b: LuaVarArg) { + const c = [...b]; + return c.join("") + b[0]; } return foo("A", "B", "C", "D"); `; - expect(util.transpileAndExecute(code, compilerOptions, undefined, tsHeader)).toBe("BCDBCD"); + expect(util.transpileAndExecute(code, compilerOptions)).toBe("BCDB"); }); -test("invalid non-ambient @elipsisForward", () => { +test.each([{}, { noHoisting: true }])("@varArg global", compilerOptions => { const code = ` - /** @elipsisForward */ function elipsisForward(args: unknown): unknown[] { return []; } + /** @varArg */ type LuaVarArg = A & { __luaVarArg?: never }; + declare const arg: LuaVarArg; + const arr = [...arg]; + const result = arr.join(""); `; - expect(() => util.transpileString(code)).toThrow( - TSTLErrors.InvalidElipsisForward( - ts.createEmptyStatement(), - "@elipsesForward can only be used on a function, and called in a spread expression." - ).message - ); -}); - -test.each([ - "const a = elipsisForward(args);", - "const a = [...[elipsisForward()]]", - "for (const v of elipsisForward(args)) {}", - "console.log(elipsisForward);", - "elipsisForward.call(null, 0, 0, 0);", - "let array = [0, elipsisForward, 1];", - "const call: any; call(elipsisForward);", -])("invalid @elipsisForward use (%p)", statement => { - const code = ` - /** @elipsisForward */ declare function elipsisForward(args: unknown): unknown[]; - function foo(...args: unknown[]) { - ${statement} - }`; - - expect(() => util.transpileString(code)).toThrow( - TSTLErrors.InvalidElipsisForward( - ts.createEmptyStatement(), - "@elipsesForward can only be used on a function, and called in a spread expression." - ).message - ); -}); -test.each(["unknown", "unknown[]", "Array<[]>", "Iterable"])( - "invalid @elipsisForward argument type (%p)", - type => { - const code = ` - /** @elipsisForward */ declare function elipsisForward(args: unknown): unknown[]; - function foo(a: ${type}) { - const x = [...elipsisForward(a)]; - }`; - - expect(() => util.transpileString(code)).toThrow( - TSTLErrors.InvalidElipsisForward( - ts.createEmptyStatement(), - "@elipsesForward function can only be passed a single rest parameter." - ).message - ); - } -); - -test.each(["a, 0", "a, 0, 1"])("invalid @elipsisForward argument count (%p)", args => { - const code = ` - /** @elipsisForward */ declare function elipsisForward(...args: unknown[]): unknown[]; - function foo(...a: unknown[]) { - const x = [...elipsisForward(${args})]; - }`; - - expect(() => util.transpileString(code)).toThrow( - TSTLErrors.InvalidElipsisForward( - ts.createEmptyStatement(), - "@elipsesForward function can only be passed a single rest parameter." - ).message - ); -}); + const luaBody = util.transpileString(code, compilerOptions, false); + expect(luaBody).not.toMatch("unpack"); -test.each([{}, { noHoisting: true }])("@elipsisForward with no argument", compilerOptions => { - const tsHeader = ` - /** @elipsisForward */ declare function elipsisForward(args?: unknown): unknown[]; - `; - const code = ` - function foo(a: unknown, ...b: unknown[]) { - const c = [...elipsisForward()]; - return c.join(""); - } - return foo("A", "B", "C", "D"); + const lua = ` + function test(...) + ${luaBody} + return result + end + return test("A", "B", "C", "D") `; - expect(util.transpileString(tsHeader + code)).not.toMatch("b = ({...})"); - expect(util.transpileAndExecute(code, compilerOptions, undefined, tsHeader)).toBe("BCD"); + expect(util.executeLua(lua)).toBe("ABCD"); });