Skip to content

Commit a2046bd

Browse files
tomblindtomblind
authored
Identifier name fixes (#596)
* Catching lua keyword ambient identifiers in object literal shorthand * treating undeclared variables as ambient when checking for lua keyword names * dis-allowing ambient/undeclared identifiers with invalid lua names * catching undeclared invalid lua identifiers in object literal shorthand * correctly naming exports with invalid lua names, also some refactoring on tests * class with invalid lua names now keep correct name property * applying suggestion * fixed decorated, exported classes fixes #605 * forgot assertion in test
1 parent 908db36 commit a2046bd

File tree

4 files changed

+198
-57
lines changed

4 files changed

+198
-57
lines changed

‎src/LuaTransformer.ts‎

Lines changed: 54 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -264,11 +264,7 @@ export class LuaTransformer {
264264
exportedIdentifier = this.transformIdentifier(specifier.propertyName);
265265
} else {
266266
const exportedSymbol = this.checker.getExportSpecifierLocalTargetSymbol(specifier);
267-
if (exportedSymbol !== undefined) {
268-
exportedIdentifier = this.createIdentifierFromSymbol(exportedSymbol, specifier.name);
269-
} else {
270-
exportedIdentifier = this.transformIdentifier(specifier.name);
271-
}
267+
exportedIdentifier = this.createShorthandIdentifier(exportedSymbol, specifier.name);
272268
}
273269

274270
return tstl.createAssignmentStatement(
@@ -483,10 +479,13 @@ export class LuaTransformer {
483479
}
484480

485481
let className: tstl.Identifier;
482+
let classNameText: string;
486483
if (nameOverride !== undefined) {
487484
className = nameOverride;
485+
classNameText = nameOverride.text;
488486
} else if (statement.name !== undefined) {
489487
className = this.transformIdentifier(statement.name);
488+
classNameText = statement.name.text;
490489
} else {
491490
throw TSTLErrors.MissingClassName(statement);
492491
}
@@ -593,6 +592,7 @@ export class LuaTransformer {
593592
statement,
594593
className,
595594
localClassName,
595+
classNameText,
596596
extendsType
597597
);
598598
result.push(...classCreationMethods);
@@ -727,6 +727,7 @@ export class LuaTransformer {
727727
statement: ts.ClassLikeDeclarationBase,
728728
className: tstl.Identifier,
729729
localClassName: tstl.Identifier,
730+
classNameText: string,
730731
extendsType?: ts.Type
731732
): tstl.Statement[]
732733
{
@@ -752,7 +753,7 @@ export class LuaTransformer {
752753
result.push(
753754
tstl.createAssignmentStatement(
754755
tstl.createTableIndexExpression(tstl.cloneIdentifier(localClassName), tstl.createStringLiteral("name")),
755-
tstl.createStringLiteral(localClassName.text),
756+
tstl.createStringLiteral(classNameText),
756757
statement
757758
)
758759
);
@@ -3440,15 +3441,8 @@ export class LuaTransformer {
34403441
properties.push(tstl.createTableFieldExpression(expression, name, element));
34413442

34423443
} else if (ts.isShorthandPropertyAssignment(element)) {
3443-
let identifier: tstl.Expression | undefined;
34443444
const valueSymbol = this.checker.getShorthandAssignmentValueSymbol(element);
3445-
if (valueSymbol !== undefined
3446-
// Ignore declarations for things like NaN
3447-
&& !tsHelper.isStandardLibraryDeclaration(valueSymbol.valueDeclaration, this.program)) {
3448-
identifier = this.createIdentifierFromSymbol(valueSymbol, element.name);
3449-
} else {
3450-
identifier = this.transformIdentifierExpression(element.name);
3451-
}
3445+
let identifier = this.createShorthandIdentifier(valueSymbol, element.name);
34523446
if (tstl.isIdentifier(identifier) && valueSymbol !== undefined && this.isSymbolExported(valueSymbol)) {
34533447
identifier = this.createExportedIdentifier(identifier);
34543448
}
@@ -5265,34 +5259,62 @@ export class LuaTransformer {
52655259
return tstl.createBinaryExpression(expression, tstl.createNumericLiteral(1), tstl.SyntaxKind.AdditionOperator);
52665260
}
52675261

5268-
protected createIdentifierFromSymbol(symbol: ts.Symbol, tsOriginal?: ts.Node): tstl.Identifier {
5269-
const name = this.hasUnsafeSymbolName(symbol)
5270-
? this.createSafeName(symbol.name)
5271-
: symbol.name;
5272-
return tstl.createIdentifier(name, tsOriginal, this.symbolIds.get(symbol));
5262+
protected createShorthandIdentifier(
5263+
valueSymbol: ts.Symbol | undefined,
5264+
propertyIdentifier: ts.Identifier
5265+
): tstl.Expression
5266+
{
5267+
let name: string;
5268+
if (valueSymbol !== undefined) {
5269+
name = this.hasUnsafeSymbolName(valueSymbol, propertyIdentifier)
5270+
? this.createSafeName(valueSymbol.name)
5271+
: valueSymbol.name;
5272+
5273+
} else {
5274+
const propertyName = this.getIdentifierText(propertyIdentifier);
5275+
if (luaKeywords.has(propertyName) || !tsHelper.isValidLuaIdentifier(propertyName)) {
5276+
// Catch ambient declarations of identifiers with bad names
5277+
throw TSTLErrors.InvalidAmbientIdentifierName(propertyIdentifier);
5278+
}
5279+
name = this.hasUnsafeIdentifierName(propertyIdentifier)
5280+
? this.createSafeName(propertyName)
5281+
: propertyName;
5282+
}
5283+
5284+
const identifier = this.transformIdentifierExpression(ts.createIdentifier(name));
5285+
tstl.setNodeOriginal(identifier, propertyIdentifier);
5286+
if (valueSymbol !== undefined && tstl.isIdentifier(identifier)) {
5287+
identifier.symbolId = this.symbolIds.get(valueSymbol);
5288+
}
5289+
return identifier;
52735290
}
52745291

52755292
protected isUnsafeName(name: string): boolean {
52765293
return luaKeywords.has(name) || luaBuiltins.has(name) || !tsHelper.isValidLuaIdentifier(name);
52775294
}
52785295

5279-
protected hasUnsafeSymbolName(symbol: ts.Symbol): boolean {
5280-
if (luaKeywords.has(symbol.name) || luaBuiltins.has(symbol.name)) {
5281-
// lua keywords are only unsafe when non-ambient and not exported
5282-
const isNonAmbient = symbol.declarations.find(d => !tsHelper.isAmbient(d)) !== undefined;
5283-
return isNonAmbient && !this.isSymbolExported(symbol);
5296+
protected hasUnsafeSymbolName(symbol: ts.Symbol, tsOriginal?: ts.Identifier): boolean {
5297+
const isLuaKeyword = luaKeywords.has(symbol.name);
5298+
const isInvalidIdentifier = !tsHelper.isValidLuaIdentifier(symbol.name);
5299+
const isAmbient = symbol.declarations.some(d => tsHelper.isAmbient(d));
5300+
if ((isLuaKeyword || isInvalidIdentifier) && isAmbient) {
5301+
// Catch ambient declarations of identifiers with bad names
5302+
throw TSTLErrors.InvalidAmbientIdentifierName(tsOriginal || ts.createIdentifier(symbol.name));
5303+
}
5304+
5305+
if (this.isUnsafeName(symbol.name)) {
5306+
// only unsafe when non-ambient and not exported
5307+
return !isAmbient && !this.isSymbolExported(symbol);
52845308
}
5285-
return this.isUnsafeName(symbol.name);
5309+
return false;
52865310
}
52875311

52885312
protected hasUnsafeIdentifierName(identifier: ts.Identifier): boolean {
52895313
const symbol = this.checker.getSymbolAtLocation(identifier);
52905314
if (symbol !== undefined) {
5291-
if (luaKeywords.has(symbol.name) && symbol.declarations.find(d => !tsHelper.isAmbient(d)) === undefined) {
5292-
// Catch ambient declarations of identifiers with lua keyword names
5293-
throw TSTLErrors.InvalidAmbientLuaKeywordIdentifier(identifier);
5294-
}
5295-
return this.hasUnsafeSymbolName(symbol);
5315+
return this.hasUnsafeSymbolName(symbol, identifier);
5316+
} else if (luaKeywords.has(identifier.text) || !tsHelper.isValidLuaIdentifier(identifier.text)) {
5317+
throw TSTLErrors.InvalidAmbientIdentifierName(identifier);
52965318
}
52975319
return false;
52985320
}
@@ -5556,7 +5578,7 @@ export class LuaTransformer {
55565578
declaration: ts.ClassLikeDeclaration
55575579
): tstl.AssignmentStatement | undefined {
55585580
const className = declaration.name !== undefined
5559-
? this.transformIdentifier(declaration.name)
5581+
? this.addExportToIdentifier(this.transformIdentifier(declaration.name))
55605582
: tstl.createAnonymousIdentifier();
55615583

55625584
const decorators = declaration.decorators;
@@ -5585,3 +5607,4 @@ export class LuaTransformer {
55855607
);
55865608
}
55875609
}
5610+

‎src/TSTLErrors.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,9 +195,9 @@ export class TSTLErrors {
195195
return new TranspileError(`Unsupported object destructuring in for...of statement.`, node);
196196
};
197197

198-
public static InvalidAmbientLuaKeywordIdentifier = (node: ts.Identifier) => {
198+
public static InvalidAmbientIdentifierName = (node: ts.Identifier) => {
199199
return new TranspileError(
200-
`Invalid use of lua keyword "${node.text}" as ambient identifier name.`,
200+
`Invalid ambient identifier name "${node.text}". Ambient identifiers must be valid lua identifiers.`,
201201
node
202202
);
203203
};

‎test/unit/classDecorator.spec.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,3 +188,16 @@ test("Throws error if decorator function has void context", () => {
188188
TSTLErrors.InvalidDecoratorContext(util.nodeStub),
189189
);
190190
});
191+
192+
test("Exported class decorator", () => {
193+
const code = `
194+
function decorator<T extends any>(c: T): T {
195+
c.bar = "foobar";
196+
return c;
197+
}
198+
199+
@decorator
200+
export class Foo {}`;
201+
202+
expect(util.transpileExecuteAndReturnExport(code, "Foo.bar")).toBe("foobar");
203+
});

‎test/unit/identifiers.spec.ts‎

Lines changed: 129 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,24 @@ import * as util from "../util";
33
import { luaKeywords } from "../../src/LuaKeywords";
44
import { TSTLErrors } from "../../src/TSTLErrors";
55

6-
test.each(["$$$", "ɥɣɎɌͼƛಠ", "_̀ः٠‿"])("invalid lua identifier name (%p)", name => {
6+
const invalidLuaCharNames = ["$$$", "ɥɣɎɌͼƛಠ", "_̀ः٠‿"];
7+
const validTsInvalidLuaKeywordNames = [
8+
"and",
9+
"elseif",
10+
"end",
11+
"goto",
12+
"local",
13+
"nil",
14+
"not",
15+
"or",
16+
"repeat",
17+
"then",
18+
"until",
19+
];
20+
const invalidLuaNames = [...invalidLuaCharNames, ...luaKeywords.values()];
21+
const validTsInvalidLuaNames = [...invalidLuaCharNames, ...validTsInvalidLuaKeywordNames];
22+
23+
test.each(validTsInvalidLuaNames)("invalid lua identifier name (%p)", name => {
724
const code = `
825
const ${name} = "foobar";
926
return ${name};`;
@@ -19,42 +36,33 @@ test.each([...luaKeywords.values()])("lua keyword as property name (%p)", keywor
1936
expect(util.transpileAndExecute(code)).toBe("foobar");
2037
});
2138

22-
test.each(["and", "elseif", "end", "goto", "local", "nil", "not", "or", "repeat", "then", "until"])(
23-
"destructuring lua keyword (%p)",
24-
keyword => {
25-
const code = `
39+
test.each(validTsInvalidLuaKeywordNames)("destructuring lua keyword (%p)", keyword => {
40+
const code = `
2641
const { foo: ${keyword} } = { foo: "foobar" };
2742
return ${keyword};`;
2843

29-
expect(util.transpileAndExecute(code)).toBe("foobar");
30-
},
31-
);
44+
expect(util.transpileAndExecute(code)).toBe("foobar");
45+
});
3246

33-
test.each(["and", "elseif", "end", "goto", "local", "nil", "not", "or", "repeat", "then", "until"])(
34-
"destructuring shorthand lua keyword (%p)",
35-
keyword => {
36-
const code = `
47+
test.each(validTsInvalidLuaKeywordNames)("destructuring shorthand lua keyword (%p)", keyword => {
48+
const code = `
3749
const { ${keyword} } = { ${keyword}: "foobar" };
3850
return ${keyword};`;
3951

40-
expect(util.transpileAndExecute(code)).toBe("foobar");
41-
},
42-
);
52+
expect(util.transpileAndExecute(code)).toBe("foobar");
53+
});
4354

44-
test.each(["$$$", "ɥɣɎɌͼƛಠ", "_̀ः٠‿", ...luaKeywords.values()])(
45-
"lua keyword or invalid identifier as method call (%p)",
46-
name => {
47-
const code = `
55+
test.each(invalidLuaNames)("lua keyword or invalid identifier as method call (%p)", name => {
56+
const code = `
4857
const foo = {
4958
${name}(arg: string) { return "foo" + arg; }
5059
};
5160
return foo.${name}("bar");`;
5261

53-
expect(util.transpileAndExecute(code)).toBe("foobar");
54-
},
55-
);
62+
expect(util.transpileAndExecute(code)).toBe("foobar");
63+
});
5664

57-
test.each(["$$$", "ɥɣɎɌͼƛಠ", "_̀ः٠‿", ...luaKeywords.values()])(
65+
test.each(invalidLuaNames)(
5866
"lua keyword or invalid identifier as complex method call (%p)",
5967
name => {
6068
const code = `
@@ -84,10 +92,107 @@ test.each([
8492
const foo = local;`;
8593

8694
expect(() => util.transpileString(code)).toThrow(
87-
TSTLErrors.InvalidAmbientLuaKeywordIdentifier(ts.createIdentifier("local")).message,
95+
TSTLErrors.InvalidAmbientIdentifierName(ts.createIdentifier("local")).message,
8896
);
8997
});
9098

99+
test.each([
100+
"var $$$: any;",
101+
"let $$$: any;",
102+
"const $$$: any;",
103+
"const foo: any, bar: any, $$$: any;",
104+
"class $$$ {}",
105+
"namespace $$$ { export const bar: any; }",
106+
"module $$$ { export const bar: any; }",
107+
"enum $$$ {}",
108+
"function $$$() {}",
109+
])("ambient identifier must be a valid lua identifier (%p)", statement => {
110+
const code = `
111+
declare ${statement}
112+
const foo = $$$;`;
113+
114+
expect(() => util.transpileString(code)).toThrow(
115+
TSTLErrors.InvalidAmbientIdentifierName(ts.createIdentifier("$$$")).message,
116+
);
117+
});
118+
119+
test.each(validTsInvalidLuaNames)(
120+
"ambient identifier must be a valid lua identifier (object literal shorthand) (%p)",
121+
name => {
122+
const code = `
123+
declare var ${name}: any;
124+
const foo = { ${name} };`;
125+
126+
expect(() => util.transpileString(code)).toThrow(
127+
TSTLErrors.InvalidAmbientIdentifierName(ts.createIdentifier(name)).message,
128+
);
129+
},
130+
);
131+
132+
test.each(validTsInvalidLuaNames)(
133+
"undeclared identifier must be a valid lua identifier (%p)",
134+
name => {
135+
expect(() => util.transpileString(`const foo = ${name};`)).toThrow(
136+
TSTLErrors.InvalidAmbientIdentifierName(ts.createIdentifier(name)).message,
137+
);
138+
},
139+
);
140+
141+
test.each(validTsInvalidLuaNames)(
142+
"undeclared identifier must be a valid lua identifier (object literal shorthand) (%p)",
143+
name => {
144+
expect(() => util.transpileString(`const foo = { ${name} };`)).toThrow(
145+
TSTLErrors.InvalidAmbientIdentifierName(ts.createIdentifier(name)).message,
146+
);
147+
},
148+
);
149+
150+
test.each(validTsInvalidLuaNames)(
151+
"exported values with invalid lua identifier names (%p)",
152+
name => {
153+
const code = `export const ${name} = "foobar";`;
154+
const lua = util.transpileString(code);
155+
expect(lua.indexOf(`"${name}"`)).toBeGreaterThanOrEqual(0);
156+
expect(util.executeLua(`return (function() ${lua} end)()["${name}"]`)).toBe("foobar");
157+
},
158+
);
159+
160+
test.each(validTsInvalidLuaNames)("class with invalid lua name has correct name property", name => {
161+
const code = `
162+
class ${name} {}
163+
return ${name}.name;`;
164+
165+
expect(util.transpileAndExecute(code)).toBe(name);
166+
});
167+
168+
test.each(validTsInvalidLuaNames)("decorated class with invalid lua name", name => {
169+
const code = `
170+
function decorator<T extends any>(c: T): T {
171+
c.bar = "foobar";
172+
return c;
173+
}
174+
175+
@decorator
176+
class ${name} {}
177+
return (${name} as any).bar;`;
178+
179+
expect(util.transpileAndExecute(code)).toBe("foobar");
180+
});
181+
182+
test.each(validTsInvalidLuaNames)("exported decorated class with invalid lua name", name => {
183+
const code = `
184+
function decorator<T extends any>(c: T): T {
185+
c.bar = "foobar";
186+
return c;
187+
}
188+
189+
@decorator
190+
export class ${name} {}`;
191+
192+
const lua = util.transpileString(code);
193+
expect(util.executeLua(`return (function() ${lua} end)()["${name}"].bar`)).toBe("foobar");
194+
});
195+
91196
describe("lua keyword as identifier doesn't interfere with lua's value", () => {
92197
test("variable (nil)", () => {
93198
const code = `

0 commit comments

Comments
 (0)