From 863634281397b83a9f8029bf128759e6696dc020 Mon Sep 17 00:00:00 2001 From: Tom Date: Mon, 23 Aug 2021 07:00:24 -0600 Subject: [PATCH 1/6] fixes and refactors for hoisting to fix switch statements and make logic more clear --- src/transformation/utils/lua-ast.ts | 16 ++-- src/transformation/utils/scope.ts | 112 ++++++++++++++++++++------ src/transformation/visitors/switch.ts | 16 ++-- 3 files changed, 106 insertions(+), 38 deletions(-) diff --git a/src/transformation/utils/lua-ast.ts b/src/transformation/utils/lua-ast.ts index 5f3a21e4a..091cd73f4 100644 --- a/src/transformation/utils/lua-ast.ts +++ b/src/transformation/utils/lua-ast.ts @@ -175,7 +175,7 @@ export function createLocalOrExportedOrGlobalDeclaration( const isTopLevelVariable = scope.type === ScopeType.File; if (context.isModule || !isTopLevelVariable) { - if (scope.type === ScopeType.Switch || (!isFunctionDeclaration && hasMultipleReferences(scope, lhs))) { + if (!isFunctionDeclaration && hasMultipleReferences(scope, lhs)) { // Split declaration and assignment of identifiers that reference themselves in their declaration declaration = lua.createVariableDeclarationStatement(lhs, undefined, tsOriginal); if (rhs) { @@ -185,15 +185,13 @@ export function createLocalOrExportedOrGlobalDeclaration( declaration = lua.createVariableDeclarationStatement(lhs, rhs, tsOriginal); } - // Remember local variable declarations for hoisting later - if (!scope.variableDeclarations) { - scope.variableDeclarations = []; - } - - scope.variableDeclarations.push(declaration); + if (!isFunctionDeclaration) { + // Remember local variable declarations for hoisting later + if (!scope.variableDeclarations) { + scope.variableDeclarations = []; + } - if (scope.type === ScopeType.Switch) { - declaration = undefined; + scope.variableDeclarations.push(declaration); } } else if (rhs) { // global diff --git a/src/transformation/utils/scope.ts b/src/transformation/utils/scope.ts index 53169765a..05f392656 100644 --- a/src/transformation/utils/scope.ts +++ b/src/transformation/utils/scope.ts @@ -33,6 +33,11 @@ export interface Scope { functionReturned?: boolean; } +export interface HoistingResult { + statements: lua.Statement[]; + hoistedStatements: lua.Statement[]; +} + const scopeStacks = new WeakMap(); function getScopeStack(context: TransformationContext): Scope[] { return getOrUpdate(scopeStacks, context, () => []); @@ -133,13 +138,54 @@ export function isFunctionScopeWithDefinition(scope: Scope): scope is Scope & { return scope.node !== undefined && ts.isFunctionLike(scope.node); } -export function performHoisting(context: TransformationContext, statements: lua.Statement[]): lua.Statement[] { +export function separateHoistedStatements(context: TransformationContext, statements: lua.Statement[]): HoistingResult { const scope = peekScope(context); - let result = statements; - result = hoistFunctionDefinitions(context, scope, result); - result = hoistVariableDeclarations(context, scope, result); - result = hoistImportStatements(scope, result); - return result; + const allHoistedStatments: lua.Statement[] = []; + const allHoistedIdentifiers: lua.Identifier[] = []; + + let hoistedStatements: lua.Statement[]; + let hoistedIdentifiers: lua.Identifier[]; + + ({ statements, hoistedStatements, hoistedIdentifiers } = hoistFunctionDefinitions(context, scope, statements)); + allHoistedStatments.push(...hoistedStatements); + allHoistedIdentifiers.push(...hoistedIdentifiers); + + ({ statements, hoistedIdentifiers } = hoistVariableDeclarations(context, scope, statements)); + allHoistedIdentifiers.push(...hoistedIdentifiers); + + if (allHoistedIdentifiers.length > 0) { + allHoistedStatments.unshift(lua.createVariableDeclarationStatement(allHoistedIdentifiers)); + } + + ({ statements, hoistedStatements } = hoistImportStatements(scope, statements)); + allHoistedStatments.unshift(...hoistedStatements); + + return { statements, hoistedStatements: allHoistedStatments }; +} + +export function performHoisting(context: TransformationContext, statements: lua.Statement[]): lua.Statement[] { + const result = separateHoistedStatements(context, statements); + return [...result.hoistedStatements, ...result.statements]; +} + +function findScopeBlock(node: ts.Node) { + return findFirstNodeAbove( + node, + (n): n is ts.Node => + ts.isBlock(n) || + ts.isFunctionLike(n) || + ts.isSwitchStatement(n) || + ts.isCaseOrDefaultClause(n) || + ts.isDoStatement(n) || + ts.isWhileStatement(n) || + ts.isForStatement(n) || + ts.isForInStatement(n) || + ts.isForOfStatement(n) || + ts.isIfStatement(n) || + ts.isTryStatement(n) || + ts.isCatchClause(n) || + ts.isSourceFile(n) + ); } function shouldHoistSymbol(context: TransformationContext, symbolId: lua.SymbolId, scope: Scope): boolean { @@ -153,6 +199,11 @@ function shouldHoistSymbol(context: TransformationContext, symbolId: lua.SymbolI return false; } + const scopeBlock = findScopeBlock(declaration); + if (scopeBlock && ts.isCaseOrDefaultClause(scopeBlock)) { + return true; + } + if (symbolInfo.firstSeenAtPos < declaration.pos) { return true; } @@ -183,9 +234,9 @@ function hoistVariableDeclarations( context: TransformationContext, scope: Scope, statements: lua.Statement[] -): lua.Statement[] { +): { statements: lua.Statement[]; hoistedIdentifiers: lua.Identifier[] } { if (!scope.variableDeclarations) { - return statements; + return { statements, hoistedIdentifiers: [] }; } const result = [...statements]; @@ -194,7 +245,9 @@ function hoistVariableDeclarations( const symbols = declaration.left.map(i => i.symbolId).filter(isNonNull); if (symbols.some(s => shouldHoistSymbol(context, s, scope))) { const index = result.indexOf(declaration); - assert(index > -1); + if (index < 0) { + continue; // statements array may not contain all statements in the scope (switch-case) + } if (declaration.right) { const assignment = lua.createAssignmentStatement(declaration.left, declaration.right); @@ -204,44 +257,55 @@ function hoistVariableDeclarations( result.splice(index, 1); } - hoistedLocals.push(...declaration.left); - } else if (scope.type === ScopeType.Switch) { - assert(!declaration.right); hoistedLocals.push(...declaration.left); } } - if (hoistedLocals.length > 0) { - result.unshift(lua.createVariableDeclarationStatement(hoistedLocals)); - } - - return result; + return { statements: result, hoistedIdentifiers: hoistedLocals }; } function hoistFunctionDefinitions( context: TransformationContext, scope: Scope, statements: lua.Statement[] -): lua.Statement[] { +): { statements: lua.Statement[]; hoistedStatements: lua.Statement[]; hoistedIdentifiers: lua.Identifier[] } { if (!scope.functionDefinitions) { - return statements; + return { statements, hoistedStatements: [], hoistedIdentifiers: [] }; } const result = [...statements]; - const hoistedFunctions: Array = []; + const hoistedFunctions: lua.AssignmentStatement[] = []; + const hoistedIdentifiers: lua.Identifier[] = []; for (const [functionSymbolId, functionDefinition] of scope.functionDefinitions) { assert(functionDefinition.definition); if (shouldHoistSymbol(context, functionSymbolId, scope)) { const index = result.indexOf(functionDefinition.definition); + if (index < 0) { + continue; // statements array may not contain all statements in the scope (switch-case) + } result.splice(index, 1); - hoistedFunctions.push(functionDefinition.definition); + if (lua.isVariableDeclarationStatement(functionDefinition.definition)) { + assert(functionDefinition.definition.right); + hoistedIdentifiers.push(...functionDefinition.definition.left); + hoistedFunctions.push( + lua.createAssignmentStatement( + functionDefinition.definition.left, + functionDefinition.definition.right + ) + ); + } else { + hoistedFunctions.push(functionDefinition.definition); + } } } - return [...hoistedFunctions, ...result]; + return { statements: result, hoistedStatements: hoistedFunctions, hoistedIdentifiers }; } -function hoistImportStatements(scope: Scope, statements: lua.Statement[]): lua.Statement[] { - return scope.importStatements ? [...scope.importStatements, ...statements] : statements; +function hoistImportStatements( + scope: Scope, + statements: lua.Statement[] +): { statements: lua.Statement[]; hoistedStatements: lua.Statement[] } { + return { statements, hoistedStatements: scope.importStatements ?? [] }; } diff --git a/src/transformation/visitors/switch.ts b/src/transformation/visitors/switch.ts index a82ff0bf0..ac3c31bc2 100644 --- a/src/transformation/visitors/switch.ts +++ b/src/transformation/visitors/switch.ts @@ -3,7 +3,7 @@ import { LuaTarget } from "../../CompilerOptions"; import * as lua from "../../LuaAST"; import { FunctionVisitor } from "../context"; import { unsupportedForTarget } from "../utils/diagnostics"; -import { performHoisting, popScope, pushScope, ScopeType } from "../utils/scope"; +import { separateHoistedStatements, popScope, pushScope, ScopeType } from "../utils/scope"; export const transformSwitchStatement: FunctionVisitor = (statement, context) => { if (context.luaTarget === LuaTarget.Universal || context.luaTarget === LuaTarget.Lua51) { @@ -16,7 +16,8 @@ export const transformSwitchStatement: FunctionVisitor = (st const switchName = `____switch${scope.id}`; const switchVariable = lua.createIdentifier(switchName); - let statements: lua.Statement[] = []; + const statements: lua.Statement[] = []; + const prefixStatements: lua.Statement[] = []; // Starting from the back, concatenating ifs into one big if/elseif statement const concatenatedIf = statement.caseBlock.clauses.reduceRight((previousCondition, clause, index) => { @@ -46,16 +47,21 @@ export const transformSwitchStatement: FunctionVisitor = (st for (const [index, clause] of statement.caseBlock.clauses.entries()) { const labelName = `${switchName}_case_${ts.isCaseClause(clause) ? index : "default"}`; statements.push(lua.createLabelStatement(labelName)); - statements.push(lua.createDoStatement(context.transformStatements(clause.statements))); + const { statements: clauseStatements, hoistedStatements } = separateHoistedStatements( + context, + context.transformStatements(clause.statements) + ); + statements.push(lua.createDoStatement(clauseStatements)); + prefixStatements.push(...hoistedStatements); } statements.push(lua.createLabelStatement(`${switchName}_end`)); - statements = performHoisting(context, statements); + statements.unshift(...prefixStatements); popScope(context); const expression = context.transformExpression(statement.expression); statements.unshift(lua.createVariableDeclarationStatement(switchVariable, expression)); - return statements; + return [lua.createDoStatement(statements, statement)]; }; From a500c59b5dc5f390693f80d08012b73870b4b3e2 Mon Sep 17 00:00:00 2001 From: Tom Date: Mon, 6 Sep 2021 07:36:20 -0600 Subject: [PATCH 2/6] applied hoisting fix to new switch implementation also refactored a few things for clarity and added tests --- src/transformation/utils/scope.ts | 111 ++++++++++---------------- src/transformation/visitors/switch.ts | 20 ++++- test/unit/switch.spec.ts | 65 ++++++++++++++- 3 files changed, 121 insertions(+), 75 deletions(-) diff --git a/src/transformation/utils/scope.ts b/src/transformation/utils/scope.ts index 05f392656..c9f184927 100644 --- a/src/transformation/utils/scope.ts +++ b/src/transformation/utils/scope.ts @@ -36,6 +36,7 @@ export interface Scope { export interface HoistingResult { statements: lua.Statement[]; hoistedStatements: lua.Statement[]; + hoistedIdentifiers: lua.Identifier[]; } const scopeStacks = new WeakMap(); @@ -143,52 +144,42 @@ export function separateHoistedStatements(context: TransformationContext, statem const allHoistedStatments: lua.Statement[] = []; const allHoistedIdentifiers: lua.Identifier[] = []; - let hoistedStatements: lua.Statement[]; - let hoistedIdentifiers: lua.Identifier[]; - - ({ statements, hoistedStatements, hoistedIdentifiers } = hoistFunctionDefinitions(context, scope, statements)); + let { unhoistedStatements, hoistedStatements, hoistedIdentifiers } = hoistFunctionDefinitions( + context, + scope, + statements + ); allHoistedStatments.push(...hoistedStatements); allHoistedIdentifiers.push(...hoistedIdentifiers); - ({ statements, hoistedIdentifiers } = hoistVariableDeclarations(context, scope, statements)); + ({ unhoistedStatements, hoistedIdentifiers } = hoistVariableDeclarations(context, scope, unhoistedStatements)); allHoistedIdentifiers.push(...hoistedIdentifiers); - if (allHoistedIdentifiers.length > 0) { - allHoistedStatments.unshift(lua.createVariableDeclarationStatement(allHoistedIdentifiers)); - } - - ({ statements, hoistedStatements } = hoistImportStatements(scope, statements)); + ({ unhoistedStatements, hoistedStatements } = hoistImportStatements(scope, unhoistedStatements)); allHoistedStatments.unshift(...hoistedStatements); - return { statements, hoistedStatements: allHoistedStatments }; + return { + statements: unhoistedStatements, + hoistedStatements: allHoistedStatments, + hoistedIdentifiers: allHoistedIdentifiers, + }; } export function performHoisting(context: TransformationContext, statements: lua.Statement[]): lua.Statement[] { const result = separateHoistedStatements(context, statements); - return [...result.hoistedStatements, ...result.statements]; -} - -function findScopeBlock(node: ts.Node) { - return findFirstNodeAbove( - node, - (n): n is ts.Node => - ts.isBlock(n) || - ts.isFunctionLike(n) || - ts.isSwitchStatement(n) || - ts.isCaseOrDefaultClause(n) || - ts.isDoStatement(n) || - ts.isWhileStatement(n) || - ts.isForStatement(n) || - ts.isForInStatement(n) || - ts.isForOfStatement(n) || - ts.isIfStatement(n) || - ts.isTryStatement(n) || - ts.isCatchClause(n) || - ts.isSourceFile(n) - ); + const modifiedStatements = [...result.hoistedStatements, ...result.statements]; + if (result.hoistedIdentifiers.length > 0) { + modifiedStatements.unshift(lua.createVariableDeclarationStatement(result.hoistedIdentifiers)); + } + return modifiedStatements; } function shouldHoistSymbol(context: TransformationContext, symbolId: lua.SymbolId, scope: Scope): boolean { + // Always hoist in top-level of switch statements + if (scope.type === ScopeType.Switch) { + return true; + } + const symbolInfo = getSymbolInfo(context, symbolId); if (!symbolInfo) { return false; @@ -199,11 +190,6 @@ function shouldHoistSymbol(context: TransformationContext, symbolId: lua.SymbolI return false; } - const scopeBlock = findScopeBlock(declaration); - if (scopeBlock && ts.isCaseOrDefaultClause(scopeBlock)) { - return true; - } - if (symbolInfo.firstSeenAtPos < declaration.pos) { return true; } @@ -230,21 +216,17 @@ function shouldHoistSymbol(context: TransformationContext, symbolId: lua.SymbolI return false; } -function hoistVariableDeclarations( - context: TransformationContext, - scope: Scope, - statements: lua.Statement[] -): { statements: lua.Statement[]; hoistedIdentifiers: lua.Identifier[] } { +function hoistVariableDeclarations(context: TransformationContext, scope: Scope, statements: lua.Statement[]) { if (!scope.variableDeclarations) { - return { statements, hoistedIdentifiers: [] }; + return { unhoistedStatements: statements, hoistedIdentifiers: [] }; } - const result = [...statements]; - const hoistedLocals: lua.Identifier[] = []; + const unhoistedStatements = [...statements]; + const hoistedIdentifiers: lua.Identifier[] = []; for (const declaration of scope.variableDeclarations) { const symbols = declaration.left.map(i => i.symbolId).filter(isNonNull); if (symbols.some(s => shouldHoistSymbol(context, s, scope))) { - const index = result.indexOf(declaration); + const index = unhoistedStatements.indexOf(declaration); if (index < 0) { continue; // statements array may not contain all statements in the scope (switch-case) } @@ -252,60 +234,53 @@ function hoistVariableDeclarations( if (declaration.right) { const assignment = lua.createAssignmentStatement(declaration.left, declaration.right); lua.setNodePosition(assignment, declaration); // Preserve position info for sourcemap - result.splice(index, 1, assignment); + unhoistedStatements.splice(index, 1, assignment); } else { - result.splice(index, 1); + unhoistedStatements.splice(index, 1); } - hoistedLocals.push(...declaration.left); + hoistedIdentifiers.push(...declaration.left); } } - return { statements: result, hoistedIdentifiers: hoistedLocals }; + return { unhoistedStatements, hoistedIdentifiers }; } -function hoistFunctionDefinitions( - context: TransformationContext, - scope: Scope, - statements: lua.Statement[] -): { statements: lua.Statement[]; hoistedStatements: lua.Statement[]; hoistedIdentifiers: lua.Identifier[] } { +function hoistFunctionDefinitions(context: TransformationContext, scope: Scope, statements: lua.Statement[]) { if (!scope.functionDefinitions) { - return { statements, hoistedStatements: [], hoistedIdentifiers: [] }; + return { unhoistedStatements: statements, hoistedStatements: [], hoistedIdentifiers: [] }; } - const result = [...statements]; - const hoistedFunctions: lua.AssignmentStatement[] = []; + const unhoistedStatements = [...statements]; + const hoistedStatements: lua.Statement[] = []; const hoistedIdentifiers: lua.Identifier[] = []; for (const [functionSymbolId, functionDefinition] of scope.functionDefinitions) { assert(functionDefinition.definition); if (shouldHoistSymbol(context, functionSymbolId, scope)) { - const index = result.indexOf(functionDefinition.definition); + const index = unhoistedStatements.indexOf(functionDefinition.definition); if (index < 0) { continue; // statements array may not contain all statements in the scope (switch-case) } - result.splice(index, 1); + unhoistedStatements.splice(index, 1); if (lua.isVariableDeclarationStatement(functionDefinition.definition)) { assert(functionDefinition.definition.right); hoistedIdentifiers.push(...functionDefinition.definition.left); - hoistedFunctions.push( + hoistedStatements.push( lua.createAssignmentStatement( functionDefinition.definition.left, functionDefinition.definition.right ) ); } else { - hoistedFunctions.push(functionDefinition.definition); + hoistedStatements.push(functionDefinition.definition); } } } - return { statements: result, hoistedStatements: hoistedFunctions, hoistedIdentifiers }; + return { unhoistedStatements, hoistedStatements, hoistedIdentifiers }; } -function hoistImportStatements( - scope: Scope, - statements: lua.Statement[] -): { statements: lua.Statement[]; hoistedStatements: lua.Statement[] } { - return { statements, hoistedStatements: scope.importStatements ?? [] }; +function hoistImportStatements(scope: Scope, statements: lua.Statement[]) { + return { unhoistedStatements: statements, hoistedStatements: scope.importStatements ?? [] }; } diff --git a/src/transformation/visitors/switch.ts b/src/transformation/visitors/switch.ts index 1a05bae41..814dffd9c 100644 --- a/src/transformation/visitors/switch.ts +++ b/src/transformation/visitors/switch.ts @@ -1,7 +1,7 @@ import * as ts from "typescript"; import * as lua from "../../LuaAST"; import { FunctionVisitor, TransformationContext } from "../context"; -import { performHoisting, popScope, pushScope, ScopeType } from "../utils/scope"; +import { popScope, pushScope, ScopeType, separateHoistedStatements } from "../utils/scope"; const containsBreakOrReturn = (nodes: Iterable): boolean => { for (const s of nodes) { @@ -55,7 +55,9 @@ export const transformSwitchStatement: FunctionVisitor = (st // If the switch only has a default clause, wrap it in a single do. // Otherwise, we need to generate a set of if statements to emulate the switch. - let statements: lua.Statement[] = []; + const statements: lua.Statement[] = []; + const prefixStatements: lua.Statement[] = []; + const prefixIdentifiers: lua.Identifier[] = []; const clauses = statement.caseBlock.clauses; if (clauses.length === 1 && ts.isDefaultClause(clauses[0])) { const defaultClause = clauses[0].statements; @@ -124,10 +126,16 @@ export const transformSwitchStatement: FunctionVisitor = (st } // Transform the clause and append the final break statement if necessary - const clauseStatements = context.transformStatements(clause.statements); + const { + statements: clauseStatements, + hoistedStatements, + hoistedIdentifiers, + } = separateHoistedStatements(context, context.transformStatements(clause.statements)); if (i === clauses.length - 1 && !containsBreakOrReturn(clause.statements)) { clauseStatements.push(lua.createBreakStatement()); } + prefixStatements.push(...hoistedStatements); + prefixIdentifiers.push(...hoistedIdentifiers); // Push if statement for case statements.push(lua.createIfStatement(conditionVariable, lua.createBlock(clauseStatements))); @@ -160,7 +168,11 @@ export const transformSwitchStatement: FunctionVisitor = (st } // Hoist the variable, function, and import statements to the top of the switch - statements = performHoisting(context, statements); + statements.unshift(...prefixStatements); + if (prefixIdentifiers.length > 0) { + statements.unshift(lua.createVariableDeclarationStatement(prefixIdentifiers)); + } + popScope(context); // Add the switch expression after hoisting diff --git a/test/unit/switch.spec.ts b/test/unit/switch.spec.ts index 6ed965595..06a92ed4b 100644 --- a/test/unit/switch.spec.ts +++ b/test/unit/switch.spec.ts @@ -369,7 +369,7 @@ test.each([0, 1])("switch empty fallthrough to default (%p)", inp => { case 1: default: out.push("default"); - + } return out; ` @@ -431,7 +431,7 @@ test.each([1, 2])("switch handles side-effects with empty fallthrough (%p)", inp ${new Array(inp).fill("case foo():").join("\n")} default: out.push("default"); - + } out.push(y); @@ -456,7 +456,7 @@ test.each([1, 2])("switch handles side-effects with empty fallthrough (preceding ${new Array(inp).fill("case foo():").join("\n")} default: out.push("default"); - + } out.push(y); @@ -522,3 +522,62 @@ test("switch produces optimal output", () => { test.each([0, 1, 2, 3, 4, 5])("switch produces valid optimal output (%p)", inp => { optimalOutput(inp).expectToMatchJsResult(); }); + +describe("switch hoisting", () => { + test("hoisting between cases", () => { + util.testFunction` + let x = 1; + let result = ""; + switch (x) { + case 1: + result = hoisted(); + break; + case 2: + function hoisted() { + return "hoisted"; + } + break; + } + return result; + `.expectToMatchJsResult(); + }); + + test("indirect hoisting between cases", () => { + util.testFunction` + let x = 1; + let result = ""; + switch (x) { + case 1: + function callHoisted() { + return hoisted(); + } + result = callHoisted(); + break; + case 2: + function hoisted() { + return "hoisted"; + } + break; + } + return result; + `.expectToMatchJsResult(); + }); + + test("hoisting in case expression", () => { + util.testFunction` + let x = 1; + let result = ""; + switch (x) { + case hoisted(): + result = "hoisted"; + break; + case 2: + function hoisted() { + return 1; + } + break; + } + return result; + `.expectToMatchJsResult(); + }); +}); From c7fddc09bb60e62f05b6a2481d8e62a78a27b51c Mon Sep 17 00:00:00 2001 From: Tom Date: Mon, 6 Sep 2021 08:21:34 -0600 Subject: [PATCH 3/6] fixed issues with hoisting from default clause --- src/transformation/visitors/switch.ts | 29 ++++++++++--- test/unit/switch.spec.ts | 62 +++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 5 deletions(-) diff --git a/src/transformation/visitors/switch.ts b/src/transformation/visitors/switch.ts index 814dffd9c..e87d91b92 100644 --- a/src/transformation/visitors/switch.ts +++ b/src/transformation/visitors/switch.ts @@ -66,6 +66,7 @@ export const transformSwitchStatement: FunctionVisitor = (st } } else { // Build up the condition for each if statement + let defaultTransformed = false; let isInitialCondition = true; let condition: lua.Expression | undefined = undefined; for (let i = 0; i < clauses.length; i++) { @@ -137,6 +138,11 @@ export const transformSwitchStatement: FunctionVisitor = (st prefixStatements.push(...hoistedStatements); prefixIdentifiers.push(...hoistedIdentifiers); + // Remember that we transformed default clause so we don't duplicate hoisted statements later + if (ts.isDefaultClause(clause)) { + defaultTransformed = true; + } + // Push if statement for case statements.push(lua.createIfStatement(conditionVariable, lua.createBlock(clauseStatements))); @@ -153,11 +159,24 @@ export const transformSwitchStatement: FunctionVisitor = (st (clause, index) => index >= start && containsBreakOrReturn(clause.statements) ); - // Combine the default and all fallthrough statements - const defaultStatements: lua.Statement[] = []; - clauses - .slice(start, end >= 0 ? end + 1 : undefined) - .forEach(c => defaultStatements.push(...context.transformStatements(c.statements))); + const { + statements: defaultStatements, + hoistedStatements, + hoistedIdentifiers, + } = separateHoistedStatements(context, context.transformStatements(clauses[start].statements)); + + // Only push hoisted statements if this is the first time we're transforming the default clause + if (!defaultTransformed) { + prefixStatements.push(...hoistedStatements); + prefixIdentifiers.push(...hoistedIdentifiers); + } + + // Combine the fallthrough statements + for (const clause of clauses.slice(start + 1, end >= 0 ? end + 1 : undefined)) { + let statements = context.transformStatements(clause.statements); + ({ statements } = separateHoistedStatements(context, statements)); + defaultStatements.push(...statements); + } // Add the default clause if it has any statements // The switch will always break on the final clause and skip execution if valid to do so diff --git a/test/unit/switch.spec.ts b/test/unit/switch.spec.ts index 06a92ed4b..3a027b4f5 100644 --- a/test/unit/switch.spec.ts +++ b/test/unit/switch.spec.ts @@ -580,4 +580,66 @@ describe("switch hoisting", () => { return result; `.expectToMatchJsResult(); }); + + test("hoisting from default clause", () => { + util.testFunction` + let x = 1; + let result = ""; + switch (x) { + case 1: + result = hoisted(); + break; + default: + function hoisted() { + return "hoisted"; + } + break; + } + return result; + `.expectToMatchJsResult(); + }); + + test("hoisting from default clause is not duplicated when falling through", () => { + util.testFunction` + let x = 1; + let result = ""; + switch (x) { + case 1: + result = hoisted(); + break; + case 2: + result = "2"; + default: + function hoisted() { + return "hoisted"; + } + result = "default"; + case 3: + result = "3"; + } + return result; + `.expectToMatchJsResult(); + }); + + test("hoisting from fallthrough clause after default is not duplicated", () => { + util.testFunction` + let x = 1; + let result = ""; + switch (x) { + case 1: + result = hoisted(); + break; + case 2: + result = "2"; + default: + result = "default"; + case 3: + function hoisted() { + return "hoisted"; + } + result = "3"; + } + return result; + `.expectToMatchJsResult(); + }); }); From ea7f5df6726d94a67ab1dae8c05bd283101fcedd Mon Sep 17 00:00:00 2001 From: Tom Date: Mon, 6 Sep 2021 08:44:52 -0600 Subject: [PATCH 4/6] added snapshots for a couple tests and a comment --- src/transformation/utils/scope.ts | 2 + test/unit/__snapshots__/switch.spec.ts.snap | 76 +++++++++++++++++++++ test/unit/switch.spec.ts | 8 ++- 3 files changed, 84 insertions(+), 2 deletions(-) diff --git a/src/transformation/utils/scope.ts b/src/transformation/utils/scope.ts index c9f184927..50962fe00 100644 --- a/src/transformation/utils/scope.ts +++ b/src/transformation/utils/scope.ts @@ -263,7 +263,9 @@ function hoistFunctionDefinitions(context: TransformationContext, scope: Scope, continue; // statements array may not contain all statements in the scope (switch-case) } unhoistedStatements.splice(index, 1); + if (lua.isVariableDeclarationStatement(functionDefinition.definition)) { + // Separate function definition and variable declaration assert(functionDefinition.definition.right); hoistedIdentifiers.push(...functionDefinition.definition.left); hoistedStatements.push( diff --git a/test/unit/__snapshots__/switch.spec.ts.snap b/test/unit/__snapshots__/switch.spec.ts.snap index 0fa333d93..1349374bb 100644 --- a/test/unit/__snapshots__/switch.spec.ts.snap +++ b/test/unit/__snapshots__/switch.spec.ts.snap @@ -34,6 +34,82 @@ end return ____exports" `; +exports[`switch hoisting hoisting from default clause is not duplicated when falling through 1`] = ` +"local ____exports = {} +function ____exports.__main(self) + local x = 1 + local result = \\"\\" + repeat + local ____switch3 = x + local hoisted + function hoisted(self) + return \\"hoisted\\" + end + local ____cond3 = ____switch3 == 1 + if ____cond3 then + result = hoisted(nil) + break + end + ____cond3 = ____cond3 or (____switch3 == 2) + if ____cond3 then + result = \\"2\\" + end + if ____cond3 then + result = \\"default\\" + end + ____cond3 = ____cond3 or (____switch3 == 3) + if ____cond3 then + result = \\"3\\" + break + end + do + result = \\"default\\" + result = \\"3\\" + end + until true + return result +end +return ____exports" +`; + +exports[`switch hoisting hoisting from fallthrough clause after default is not duplicated 1`] = ` +"local ____exports = {} +function ____exports.__main(self) + local x = 1 + local result = \\"\\" + repeat + local ____switch3 = x + local hoisted + function hoisted(self) + return \\"hoisted\\" + end + local ____cond3 = ____switch3 == 1 + if ____cond3 then + result = hoisted(nil) + break + end + ____cond3 = ____cond3 or (____switch3 == 2) + if ____cond3 then + result = \\"2\\" + end + if ____cond3 then + result = \\"default\\" + end + ____cond3 = ____cond3 or (____switch3 == 3) + if ____cond3 then + result = \\"3\\" + break + end + do + result = \\"default\\" + result = \\"3\\" + end + until true + return result +end +return ____exports" +`; + exports[`switch produces optimal output 1`] = ` "require(\\"lualib_bundle\\"); local ____exports = {} diff --git a/test/unit/switch.spec.ts b/test/unit/switch.spec.ts index 3a027b4f5..9b23300ee 100644 --- a/test/unit/switch.spec.ts +++ b/test/unit/switch.spec.ts @@ -618,7 +618,9 @@ describe("switch hoisting", () => { result = "3"; } return result; - `.expectToMatchJsResult(); + ` + .expectToMatchJsResult() + .expectLuaToMatchSnapshot(); }); test("hoisting from fallthrough clause after default is not duplicated", () => { @@ -640,6 +642,8 @@ describe("switch hoisting", () => { result = "3"; } return result; - `.expectToMatchJsResult(); + ` + .expectToMatchJsResult() + .expectLuaToMatchSnapshot(); }); }); From 98d19d739f1a26033f8093a3b97aebe7ab5ed021 Mon Sep 17 00:00:00 2001 From: Tom Date: Mon, 6 Sep 2021 09:02:21 -0600 Subject: [PATCH 5/6] fixed edge case with hoisting in a solo default clause --- src/transformation/visitors/switch.ts | 9 ++++++++- test/unit/switch.spec.ts | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/transformation/visitors/switch.ts b/src/transformation/visitors/switch.ts index e87d91b92..3f576626b 100644 --- a/src/transformation/visitors/switch.ts +++ b/src/transformation/visitors/switch.ts @@ -62,7 +62,14 @@ export const transformSwitchStatement: FunctionVisitor = (st if (clauses.length === 1 && ts.isDefaultClause(clauses[0])) { const defaultClause = clauses[0].statements; if (defaultClause.length) { - statements.push(lua.createDoStatement(context.transformStatements(defaultClause))); + const { + statements: defaultStatements, + hoistedStatements, + hoistedIdentifiers, + } = separateHoistedStatements(context, context.transformStatements(defaultClause)); + prefixStatements.push(...hoistedStatements); + prefixIdentifiers.push(...hoistedIdentifiers); + statements.push(lua.createDoStatement(defaultStatements)); } } else { // Build up the condition for each if statement diff --git a/test/unit/switch.spec.ts b/test/unit/switch.spec.ts index 9b23300ee..4fb72c40c 100644 --- a/test/unit/switch.spec.ts +++ b/test/unit/switch.spec.ts @@ -646,4 +646,19 @@ describe("switch hoisting", () => { .expectToMatchJsResult() .expectLuaToMatchSnapshot(); }); + + test("hoisting in a solo default clause", () => { + util.testFunction` + let x = 1; + let result = ""; + switch (x) { + default: + result = hoisted(); + function hoisted() { + return "hoisted"; + } + } + return result; + `.expectToMatchJsResult(); + }); }); From f29a244053c8d17db83db129cd6bd6a232c520d7 Mon Sep 17 00:00:00 2001 From: Tom Date: Fri, 10 Sep 2021 14:24:20 -0600 Subject: [PATCH 6/6] addressing review feedback --- src/transformation/utils/scope.ts | 17 ++++++++++--- src/transformation/visitors/switch.ts | 35 ++++++++++++++------------- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/src/transformation/utils/scope.ts b/src/transformation/utils/scope.ts index 50962fe00..bed90da76 100644 --- a/src/transformation/utils/scope.ts +++ b/src/transformation/utils/scope.ts @@ -216,7 +216,11 @@ function shouldHoistSymbol(context: TransformationContext, symbolId: lua.SymbolI return false; } -function hoistVariableDeclarations(context: TransformationContext, scope: Scope, statements: lua.Statement[]) { +function hoistVariableDeclarations( + context: TransformationContext, + scope: Scope, + statements: lua.Statement[] +): { unhoistedStatements: lua.Statement[]; hoistedIdentifiers: lua.Identifier[] } { if (!scope.variableDeclarations) { return { unhoistedStatements: statements, hoistedIdentifiers: [] }; } @@ -246,7 +250,11 @@ function hoistVariableDeclarations(context: TransformationContext, scope: Scope, return { unhoistedStatements, hoistedIdentifiers }; } -function hoistFunctionDefinitions(context: TransformationContext, scope: Scope, statements: lua.Statement[]) { +function hoistFunctionDefinitions( + context: TransformationContext, + scope: Scope, + statements: lua.Statement[] +): { unhoistedStatements: lua.Statement[]; hoistedStatements: lua.Statement[]; hoistedIdentifiers: lua.Identifier[] } { if (!scope.functionDefinitions) { return { unhoistedStatements: statements, hoistedStatements: [], hoistedIdentifiers: [] }; } @@ -283,6 +291,9 @@ function hoistFunctionDefinitions(context: TransformationContext, scope: Scope, return { unhoistedStatements, hoistedStatements, hoistedIdentifiers }; } -function hoistImportStatements(scope: Scope, statements: lua.Statement[]) { +function hoistImportStatements( + scope: Scope, + statements: lua.Statement[] +): { unhoistedStatements: lua.Statement[]; hoistedStatements: lua.Statement[] } { return { unhoistedStatements: statements, hoistedStatements: scope.importStatements ?? [] }; } diff --git a/src/transformation/visitors/switch.ts b/src/transformation/visitors/switch.ts index 3f576626b..ae01ff10d 100644 --- a/src/transformation/visitors/switch.ts +++ b/src/transformation/visitors/switch.ts @@ -56,19 +56,19 @@ export const transformSwitchStatement: FunctionVisitor = (st // If the switch only has a default clause, wrap it in a single do. // Otherwise, we need to generate a set of if statements to emulate the switch. const statements: lua.Statement[] = []; - const prefixStatements: lua.Statement[] = []; - const prefixIdentifiers: lua.Identifier[] = []; + const hoistedStatements: lua.Statement[] = []; + const hoistedIdentifiers: lua.Identifier[] = []; const clauses = statement.caseBlock.clauses; if (clauses.length === 1 && ts.isDefaultClause(clauses[0])) { const defaultClause = clauses[0].statements; if (defaultClause.length) { const { statements: defaultStatements, - hoistedStatements, - hoistedIdentifiers, + hoistedStatements: defaultHoistedStatements, + hoistedIdentifiers: defaultHoistedIdentifiers, } = separateHoistedStatements(context, context.transformStatements(defaultClause)); - prefixStatements.push(...hoistedStatements); - prefixIdentifiers.push(...hoistedIdentifiers); + hoistedStatements.push(...defaultHoistedStatements); + hoistedIdentifiers.push(...defaultHoistedIdentifiers); statements.push(lua.createDoStatement(defaultStatements)); } } else { @@ -136,14 +136,14 @@ export const transformSwitchStatement: FunctionVisitor = (st // Transform the clause and append the final break statement if necessary const { statements: clauseStatements, - hoistedStatements, - hoistedIdentifiers, + hoistedStatements: clauseHoistedStatements, + hoistedIdentifiers: clauseHoistedIdentifiers, } = separateHoistedStatements(context, context.transformStatements(clause.statements)); if (i === clauses.length - 1 && !containsBreakOrReturn(clause.statements)) { clauseStatements.push(lua.createBreakStatement()); } - prefixStatements.push(...hoistedStatements); - prefixIdentifiers.push(...hoistedIdentifiers); + hoistedStatements.push(...clauseHoistedStatements); + hoistedIdentifiers.push(...clauseHoistedIdentifiers); // Remember that we transformed default clause so we don't duplicate hoisted statements later if (ts.isDefaultClause(clause)) { @@ -168,19 +168,20 @@ export const transformSwitchStatement: FunctionVisitor = (st const { statements: defaultStatements, - hoistedStatements, - hoistedIdentifiers, + hoistedStatements: defaultHoistedStatements, + hoistedIdentifiers: defaultHoistedIdentifiers, } = separateHoistedStatements(context, context.transformStatements(clauses[start].statements)); // Only push hoisted statements if this is the first time we're transforming the default clause if (!defaultTransformed) { - prefixStatements.push(...hoistedStatements); - prefixIdentifiers.push(...hoistedIdentifiers); + hoistedStatements.push(...defaultHoistedStatements); + hoistedIdentifiers.push(...defaultHoistedIdentifiers); } // Combine the fallthrough statements for (const clause of clauses.slice(start + 1, end >= 0 ? end + 1 : undefined)) { let statements = context.transformStatements(clause.statements); + // Drop hoisted statements as they were already added when clauses were initially transformed above ({ statements } = separateHoistedStatements(context, statements)); defaultStatements.push(...statements); } @@ -194,9 +195,9 @@ export const transformSwitchStatement: FunctionVisitor = (st } // Hoist the variable, function, and import statements to the top of the switch - statements.unshift(...prefixStatements); - if (prefixIdentifiers.length > 0) { - statements.unshift(lua.createVariableDeclarationStatement(prefixIdentifiers)); + statements.unshift(...hoistedStatements); + if (hoistedIdentifiers.length > 0) { + statements.unshift(lua.createVariableDeclarationStatement(hoistedIdentifiers)); } popScope(context);