Skip to content

Fixed invalid switch statement lua when default was not the last case #1016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Perryvw merged 3 commits into from
Apr 28, 2021
Merged

Fixed invalid switch statement lua when default was not the last case #1016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8baab77
Made switch default case always last
Perryvw Apr 26, 2021
cba7a2f
Actually fixed switch statement this time
Perryvw Apr 26, 2021
5e9cbff
Added extra default fallthrough test
Perryvw Apr 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/transformation/visitors/switch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@ export const transformSwitchStatement: FunctionVisitor<ts.SwitchStatement> = (st

let statements: lua.Statement[] = [];

const caseClauses = statement.caseBlock.clauses.filter(ts.isCaseClause);

// Starting from the back, concatenating ifs into one big if/elseif statement
const concatenatedIf = caseClauses.reduceRight((previousCondition, clause, index) => {
const concatenatedIf = statement.caseBlock.clauses.reduceRight((previousCondition, clause, index) => {
Copy link
Member

@lolleko lolleko Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not, related to this PR. But I find this reduceRight call extremely hard to read. If you want to you can refactor it. maybe by splitting the generation of the statements and the concatenation of the statements?

const ifStatementsForCases = statement.caseBlock.clauses.filter(!ts.isDefaultClause(clause)).map(
    ...
    return lua.createIfStatement(condition, lua.createBlock([goto])));
);
const concatenatedIf = ifStatementsForCases.reduceRight((prev, ifStatement) => {
    prev.elseBlock = ifStatement;
    return ifStatement;
});

If not, I am fine with merging the PR as it is.

if (ts.isDefaultClause(clause)) {
// Skip default clause here (needs to be included to ensure index lines up with index later)
return previousCondition;
}

// If the clause condition holds, go to the correct label
const condition = lua.createBinaryExpression(
switchVariable,
Expand Down
0 ...t/__snapshots__/conditionals.spec.ts.snap → test/unit/__snapshots__/switch.spec.ts.snap
View file
Open in desktop
File renamed without changes.
297 changes: 0 additions & 297 deletions test/unit/conditionals.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import * as tstl from "../../src";
import { unsupportedForTarget } from "../../src/transformation/utils/diagnostics";
import * as util from "../util";

test.each([0, 1])("if (%p)", inp => {
Expand Down Expand Up @@ -52,302 +51,6 @@ test.each([0, 1, 2, 3])("ifelseifelse (%p)", inp => {
`.expectToMatchJsResult();
});

test.each([0, 1, 2, 3])("switch (%p)", inp => {
util.testFunction`
let result: number = -1;

switch (<number>${inp}) {
case 0:
result = 0;
break;
case 1:
result = 1;
break;
case 2:
result = 2;
break;
}
return result;
`.expectToMatchJsResult();
});

test.each([0, 1, 2, 3])("switchdefault (%p)", inp => {
util.testFunction`
let result: number = -1;

switch (<number>${inp}) {
case 0:
result = 0;
break;
case 1:
result = 1;
break;
case 2:
result = 2;
break;
default:
result = -2;
break;
}
return result;
`.expectToMatchJsResult();
});

test.each([0, 0, 2, 3, 4, 5, 7])("switchfallthrough (%p)", inp => {
util.testFunction`
let result: number = -1;

switch (<number>${inp}) {
case 0:
result = 0;
case 1:
result = 1;
break;
case 2:
result = 2;
case 3:
case 4:
result = 4;
break;
case 5:
result = 5;
case 6:
result += 10;
break;
case 7:
result = 7;
default:
result = -2;
break;
}

return result;
`.expectToMatchJsResult();
});

test.each([0, 1, 2, 3])("nestedSwitch (%p)", inp => {
util.testFunction`
let result: number = -1;

switch (${inp} as number) {
case 0:
result = 0;
break;
case 1:
switch(${inp} as number) {
case 0:
result = 0;
break;
case 1:
result = 1;
break;
default:
result = -3;
break;
}
break;
case 2:
result = 2;
break;
default:
result = -2;
break;
}
return result;
`.expectToMatchJsResult();
});

test("switch cases scope", () => {
util.testFunction`
switch (0 as number) {
case 0:
let foo: number | undefined = 1;
case 1:
foo = 2;
case 2:
return foo;
}
`.expectToMatchJsResult();
});

test("variable in nested scope does not interfere with case scope", () => {
util.testFunction`
let foo: number = 0;
switch (foo) {
case 0: {
let foo = 1;
}

case 1:
return foo;
}
`.expectToMatchJsResult();
});

test("switch using variable re-declared in cases", () => {
util.testFunction`
let foo: number = 0;
switch (foo) {
case 0:
let foo = true;
case 1:
return foo;
}
`.expectToMatchJsResult();
});

test.each([0, 1, 2])("switch with block statement scope (%p)", inp => {
util.testFunction`
let result: number = -1;

switch (<number>${inp}) {
case 0: {
let x = 0;
result = 0;
break;
}
case 1: {
let x = 1;
result = x;
}
case 2: {
let x = 2;
result = x;
break;
}
}
return result;
`.expectToMatchJsResult();
});

test.each([0, 1, 2, 3])("switchReturn (%p)", inp => {
util.testFunction`
switch (<number>${inp}) {
case 0:
return 0;
break;
case 1:
return 1;
case 2:
return 2;
break;
}

return -1;
`.expectToMatchJsResult();
});

test.each([0, 1, 2, 3])("switchWithBrackets (%p)", inp => {
util.testFunction`
let result: number = -1;

switch (<number>${inp}) {
case 0: {
result = 0;
break;
}
case 1: {
result = 1;
break;
}
case 2: {
result = 2;
break;
}
}
return result;
`.expectToMatchJsResult();
});

test.each([0, 1, 2, 3])("switchWithBracketsBreakInConditional (%p)", inp => {
util.testFunction`
let result: number = -1;

switch (<number>${inp}) {
case 0: {
result = 0;
break;
}
case 1: {
result = 1;

if (result == 1) break;
}
case 2: {
result = 2;
break;
}
}
return result;
`.expectToMatchJsResult();
});

test.each([0, 1, 2, 3])("switchWithBracketsBreakInInternalLoop (%p)", inp => {
util.testFunction`
let result: number = -1;

switch (${inp} as number) {
case 0: {
result = 0;

for (let i = 0; i < 5; i++) {
result++;

if (i >= 2) {
break;
}
}
}
case 1: {
result++;
break;
}
case 2: {
result = 2;
break;
}
}
return result;
`.expectToMatchJsResult();
});

test("switch uses elseif", () => {
test("array", () => {
util.testFunction`
let result: number = -1;

switch (2 as number) {
case 0: {
result = 200;
break;
}

case 1: {
result = 100;
break;
}

case 2: {
result = 1;
break;
}
}

return result;
`
.expectLuaToMatchSnapshot()
.expectToMatchJsResult();
});
});

test("switch not allowed in 5.1", () => {
util.testFunction`
switch ("abc") {}
`
.setOptions({ luaTarget: tstl.LuaTarget.Lua51 })
.expectDiagnosticsToMatchSnapshot([unsupportedForTarget.code]);
});

test.each([
{ input: "true ? 'a' : 'b'" },
{ input: "false ? 'a' : 'b'" },
Expand Down
Loading