Skip to content

Commit 0f82db6

Browse files
PerryvwPerryvw
authored
Fixed inconsistent scoping of top-level variable declarations (#795)
* Fixed inconsistent scoping of top-level variable declarations * Added let variables in transformation test * Added handling for variables in block scope, addressed some PR feedback * simplified ast declaration logic * reverted if statement refactor, some changes to tests * Added functional test to check local variables are not in scope * Made object literal tests functional * PR comments * Removed no longer needed getCurrentNamespace * Removed obsolete tests
1 parent ea07f80 commit 0f82db6

File tree

7 files changed

+104
-62
lines changed

7 files changed

+104
-62
lines changed

‎src/transformation/utils/lua-ast.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
1-
import * as ts from "typescript";
21
import * as assert from "assert";
2+
import * as ts from "typescript";
33
import { LuaTarget } from "../../CompilerOptions";
44
import * as lua from "../../LuaAST";
55
import { TransformationContext } from "../context";
6-
import { getCurrentNamespace } from "../visitors/namespace";
76
import { createExportedIdentifier, getIdentifierExportScope } from "./export";
8-
import { findScope, peekScope, ScopeType } from "./scope";
7+
import { peekScope, ScopeType } from "./scope";
98
import { isFunctionType } from "./typescript";
109

1110
export type OneToManyVisitorResult<T extends lua.Node> = T | T[] | undefined;
@@ -122,7 +121,6 @@ export function createLocalOrExportedOrGlobalDeclaration(
122121
let declaration: lua.VariableDeclarationStatement | undefined;
123122
let assignment: lua.AssignmentStatement | undefined;
124123

125-
const isVariableDeclaration = tsOriginal !== undefined && ts.isVariableDeclaration(tsOriginal);
126124
const isFunctionDeclaration = tsOriginal !== undefined && ts.isFunctionDeclaration(tsOriginal);
127125

128126
const identifiers = Array.isArray(lhs) ? lhs : [lhs];
@@ -143,11 +141,10 @@ export function createLocalOrExportedOrGlobalDeclaration(
143141
);
144142
}
145143
} else {
146-
const insideFunction = findScope(context, ScopeType.Function) !== undefined;
147-
148-
if (context.isModule || getCurrentNamespace(context) || insideFunction || isVariableDeclaration) {
149-
const scope = peekScope(context);
144+
const scope = peekScope(context);
145+
const isTopLevelVariable = scope.type === ScopeType.File;
150146

147+
if (context.isModule || !isTopLevelVariable) {
151148
const isPossibleWrappedFunction =
152149
!isFunctionDeclaration &&
153150
tsOriginal &&

‎src/transformation/visitors/namespace.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,8 @@ function moduleHasEmittedBody(
4646
return false;
4747
}
4848

49+
// Static context -> namespace dictionary keeping the current namespace for each transformation context
4950
const currentNamespaces = new WeakMap<TransformationContext, ts.ModuleDeclaration | undefined>();
50-
export const getCurrentNamespace = (context: TransformationContext) => currentNamespaces.get(context);
5151

5252
export const transformModuleDeclaration: FunctionVisitor<ts.ModuleDeclaration> = (node, context) => {
5353
const annotations = getTypeAnnotations(context, context.checker.getTypeAtLocation(node));

‎test/translation/__snapshots__/transformation.spec.ts.snap‎

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,27 @@
11
// Jest Snapshot v1, https://goo.gl/fbAQLP
22

3+
exports[`Transformation (blockScopeVariables) 1`] = `
4+
"do
5+
local a = 1
6+
local b = 1
7+
local ____ = {c = 1}
8+
local c = ____.c
9+
end"
10+
`;
11+
312
exports[`Transformation (characterEscapeSequence) 1`] = `
4-
"local quoteInDoubleQuotes = \\"' ' '\\"
5-
local quoteInTemplateString = \\"' ' '\\"
6-
local doubleQuoteInQuotes = \\"\\\\\\" \\\\\\" \\\\\\"\\"
7-
local doubleQuoteInDoubleQuotes = \\"\\\\\\" \\\\\\" \\\\\\"\\"
8-
local doubleQuoteInTemplateString = \\"\\\\\\" \\\\\\" \\\\\\"\\"
9-
local backQuoteInQuotes = \\"\` \` \`\\"
10-
local backQuoteInDoubleQuotes = \\"\` \` \`\\"
11-
local backQuoteInTemplateString = \\"\` \` \`\\"
12-
local escapedCharsInQuotes = \\"\\\\\\\\ \\\\0 \\\\b \\\\t \\\\n \\\\v \\\\f \\\\\\" ' \`\\"
13-
local escapedCharsInDoubleQuotes = \\"\\\\\\\\ \\\\0 \\\\b \\\\t \\\\n \\\\v \\\\f \\\\\\" ' \`\\"
14-
local escapedCharsInTemplateString = \\"\\\\\\\\ \\\\0 \\\\b \\\\t \\\\n \\\\v \\\\f \\\\\\" ' \`\\"
15-
local nonEmptyTemplateString = \\"Level 0: \\\\n\\\\t \\" .. \\"Level 1: \\\\n\\\\t\\\\t \\" .. \\"Level 3: \\\\n\\\\t\\\\t\\\\t \\" .. \\"Last level \\\\n --\\" .. \\" \\\\n --\\" .. \\" \\\\n --\\" .. \\" \\\\n --\\""
13+
"quoteInDoubleQuotes = \\"' ' '\\"
14+
quoteInTemplateString = \\"' ' '\\"
15+
doubleQuoteInQuotes = \\"\\\\\\" \\\\\\" \\\\\\"\\"
16+
doubleQuoteInDoubleQuotes = \\"\\\\\\" \\\\\\" \\\\\\"\\"
17+
doubleQuoteInTemplateString = \\"\\\\\\" \\\\\\" \\\\\\"\\"
18+
backQuoteInQuotes = \\"\` \` \`\\"
19+
backQuoteInDoubleQuotes = \\"\` \` \`\\"
20+
backQuoteInTemplateString = \\"\` \` \`\\"
21+
escapedCharsInQuotes = \\"\\\\\\\\ \\\\0 \\\\b \\\\t \\\\n \\\\v \\\\f \\\\\\" ' \`\\"
22+
escapedCharsInDoubleQuotes = \\"\\\\\\\\ \\\\0 \\\\b \\\\t \\\\n \\\\v \\\\f \\\\\\" ' \`\\"
23+
escapedCharsInTemplateString = \\"\\\\\\\\ \\\\0 \\\\b \\\\t \\\\n \\\\v \\\\f \\\\\\" ' \`\\"
24+
nonEmptyTemplateString = \\"Level 0: \\\\n\\\\t \\" .. \\"Level 1: \\\\n\\\\t\\\\t \\" .. \\"Level 3: \\\\n\\\\t\\\\t\\\\t \\" .. \\"Last level \\\\n --\\" .. \\" \\\\n --\\" .. \\" \\\\n --\\" .. \\" \\\\n --\\""
1625
`;
1726

1827
exports[`Transformation (classExtension1) 1`] = `
@@ -244,7 +253,7 @@ ____exports.foo = \\"bar\\"
244253
return ____exports"
245254
`;
246255

247-
exports[`Transformation (modulesVariableNoExport) 1`] = `"local foo = \\"bar\\""`;
256+
exports[`Transformation (modulesVariableNoExport) 1`] = `"foo = \\"bar\\""`;
248257

249258
exports[`Transformation (namespacePhantom) 1`] = `
250259
"function nsMember(self)
@@ -257,6 +266,19 @@ exports[`Transformation (returnDefault) 1`] = `
257266
end"
258267
`;
259268

269+
exports[`Transformation (topLevelVariables) 1`] = `
270+
"obj = {value1 = 1, value2 = 2}
271+
value1 = obj.value1
272+
value2 = obj.value2
273+
obj2 = {value3 = 1, value4 = 2}
274+
value3 = obj2.value3
275+
value4 = obj2.value4
276+
function fun1(self)
277+
end
278+
fun2 = function()
279+
end"
280+
`;
281+
260282
exports[`Transformation (unusedDefaultWithNamespaceImport) 1`] = `
261283
"local ____exports = {}
262284
local x = require(\\"module\\")

‎test/translation/transformation/blockScopeVariables.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
const a = 1;
3+
const [b] = [1];
4+
const { c } = { c: 1 };
5+
}

‎test/translation/transformation/topLevelVariables.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
const obj = { value1: 1, value2: 2 };
2+
const value1 = obj.value1;
3+
const { value2 } = obj;
4+
5+
let noValueLet;
6+
let obj2 = { value3: 1, value4: 2 };
7+
let value3 = obj2.value3;
8+
let { value4 } = obj2;
9+
10+
function fun1(): void {}
11+
const fun2 = () => {};

‎test/unit/assignments.spec.ts‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,21 @@
11
import * as util from "../util";
22

3-
test("const declaration", () => {
4-
const lua = util.transpileString(`const foo = true;`);
5-
expect(lua).toBe(`local foo = true`);
3+
test.each(["const", "let"])("%s declaration not top-level is not global", declarationKind => {
4+
util.testModule`
5+
{
6+
${declarationKind} foo = true;
7+
}
8+
// @ts-ignore
9+
return (globalThis as any).foo;
10+
`.expectToEqual(undefined);
611
});
712

8-
test("let declaration", () => {
9-
const lua = util.transpileString(`let foo = true;`);
10-
expect(lua).toBe(`local foo = true`);
13+
test.each(["const", "let"])("%s declaration top-level is global", declarationKind => {
14+
util.testModule`
15+
${declarationKind} foo = true;
16+
// @ts-ignore
17+
return (globalThis as any).foo;
18+
`.expectToEqual(true);
1119
});
1220

1321
test("var declaration is disallowed", () => {

‎test/unit/objectLiteral.spec.ts‎

Lines changed: 33 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,66 +1,65 @@
11
import * as util from "../util";
22

3-
test.each([
4-
{ inp: `{a:3,b:"4"}`, out: '{a = 3, b = "4"}' },
5-
{ inp: `{"a":3,b:"4"}`, out: '{a = 3, b = "4"}' },
6-
{ inp: `{["a"]:3,b:"4"}`, out: '{a = 3, b = "4"}' },
7-
{ inp: `{["a"+123]:3,b:"4"}`, out: '{["a" .. 123] = 3, b = "4"}' },
8-
{ inp: `{[myFunc()]:3,b:"4"}`, out: '{\n [myFunc(_G)] = 3,\n b = "4"\n}' },
9-
{ inp: `{x}`, out: `{x = x}` },
10-
])("Object Literal (%p)", ({ inp, out }) => {
11-
const lua = util.transpileString(`const myvar = ${inp};`);
12-
expect(lua).toBe(`local myvar = ${out}`);
3+
test.each([`{ a: 3, b: "4" }`, `{ "a": 3, b: "4" }`, `{ ["a"]: 3, b: "4" }`, `{ ["a" + 123]: 3, b: "4" }`])(
4+
"Object Literal (%p)",
5+
inp => {
6+
util.testExpression(inp).expectToMatchJsResult();
7+
}
8+
);
9+
10+
test("object literal with function call to get key", () => {
11+
util.testFunction`
12+
const myFunc = () => "a";
13+
return { [myFunc() + "b"]: 3 };
14+
`.expectToMatchJsResult();
15+
});
16+
17+
test("object literal with shorthand property", () => {
18+
util.testFunction`
19+
const x = 5;
20+
return { x };
21+
`.expectToMatchJsResult();
1322
});
1423

1524
describe("property shorthand", () => {
1625
test("should support property shorthand", () => {
17-
const result = util.transpileAndExecute(`
26+
util.testFunction`
1827
const x = 1;
1928
const o = { x };
2029
return o.x;
21-
`);
22-
23-
expect(result).toBe(1);
30+
`.expectToMatchJsResult();
2431
});
2532

2633
test.each([NaN, Infinity])("should support %p shorthand", identifier => {
27-
const result = util.transpileAndExecute(`return ({ ${identifier} }).${identifier}`);
28-
29-
expect(result).toBe(identifier);
34+
util.testExpression`({ ${identifier} }).${identifier}`.expectToMatchJsResult();
3035
});
3136

3237
test("should support _G shorthand", () => {
33-
const result = util.transpileAndExecute(
34-
`return ({ _G })._G.foobar;`,
35-
undefined,
36-
`foobar = "foobar"`,
37-
"declare const _G: any;"
38-
);
39-
40-
expect(result).toBe("foobar");
38+
util.testExpression`({ _G })._G.foobar`
39+
.setTsHeader(`declare const _G: any;`)
40+
.setLuaHeader(`foobar = "foobar"`)
41+
.expectToEqual("foobar");
4142
});
4243

4344
test("should support export property shorthand", () => {
44-
const code = `
45+
util.testModule`
4546
export const x = 1;
4647
const o = { x };
4748
export const y = o.x;
48-
`;
49-
expect(util.transpileExecuteAndReturnExport(code, "y")).toBe(1);
49+
`.expectToMatchJsResult();
5050
});
5151
});
5252

5353
test("undefined as object key", () => {
54-
const code = `const foo = {undefined: "foo"};
55-
return foo.undefined;`;
56-
expect(util.transpileAndExecute(code)).toBe("foo");
54+
util.testFunction`
55+
const foo = {undefined: "foo"};
56+
return foo.undefined;
57+
`.expectToMatchJsResult();
5758
});
5859

5960
test.each([`({x: "foobar"}.x)`, `({x: "foobar"}["x"])`, `({x: () => "foobar"}.x())`, `({x: () => "foobar"}["x"]())`])(
6061
"object literal property access (%p)",
6162
expression => {
62-
const code = `return ${expression}`;
63-
const expectResult = eval(expression);
64-
expect(util.transpileAndExecute(code)).toBe(expectResult);
63+
util.testExpression(expression).expectToMatchJsResult();
6564
}
6665
);

0 commit comments

Comments
 (0)