Skip to content

Commit

Permalink
LibJS: Fix switch skipping case evaluation when hitting the default case
Browse files Browse the repository at this point in the history
When no case match we should not just execute the statements of the
default case but also of any cases below the default case.
  • Loading branch information
davidot authored and linusg committed Sep 30, 2021
1 parent 830ea04 commit e5d48ee
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 28 deletions.
61 changes: 33 additions & 28 deletions Userland/Libraries/LibJS/AST.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2825,6 +2825,7 @@ Value ThrowStatement::execute(Interpreter& interpreter, GlobalObject& global_obj
return {};
}

// 14.12.2 Runtime Semantics: CaseBlockEvaluation, https://tc39.es/ecma262/#sec-runtime-semantics-caseblockevaluation
Value SwitchStatement::execute(Interpreter& interpreter, GlobalObject& global_object) const
{
// FIXME: This needs a massive refactoring, ideally once we start using continue, break, and return completions.
Expand All @@ -2845,16 +2846,46 @@ Value SwitchStatement::execute(Interpreter& interpreter, GlobalObject& global_ob
block_declaration_instantiation(global_object, block_environment);

interpreter.vm().running_execution_context().lexical_environment = block_environment;
Optional<size_t> first_passing_case;
for (size_t i = 0; i < m_cases.size(); ++i) {
auto& switch_case = m_cases[i];
if (switch_case.test()) {
auto test_result = switch_case.test()->execute(interpreter, global_object);
if (interpreter.exception())
return {};
if (is_strictly_equal(discriminant_result, test_result)) {
first_passing_case = i;
break;
}
}
}

// FIXME: we could optimize and store the location of the default case in a member variable.
if (!first_passing_case.has_value()) {
for (size_t i = 0; i < m_cases.size(); ++i) {
auto& switch_case = m_cases[i];
if (!switch_case.test()) {
first_passing_case = i;
break;
}
}
}

auto last_value = js_undefined();

auto execute_switch_case = [&](auto const& switch_case) -> Optional<Value> {
if (!first_passing_case.has_value()) {
return last_value;
}
VERIFY(first_passing_case.value() < m_cases.size());

for (size_t i = first_passing_case.value(); i < m_cases.size(); ++i) {
auto& switch_case = m_cases[i];
for (auto& statement : switch_case.children()) {
auto value = statement.execute(interpreter, global_object);
if (!value.is_empty())
last_value = value;
if (interpreter.exception())
return Value {};
return {};
if (interpreter.vm().should_unwind()) {
if (interpreter.vm().should_unwind_until(ScopeType::Continuable, m_labels)) {
// No stop_unwind(), the outer loop will handle that - we just need to break out of the switch/case.
Expand All @@ -2867,33 +2898,7 @@ Value SwitchStatement::execute(Interpreter& interpreter, GlobalObject& global_ob
}
}
}
return {};
};

bool falling_through = false;
SwitchCase const* default_switch_case = nullptr;
for (auto& switch_case : m_cases) {
if (!switch_case.test()) {
default_switch_case = &switch_case;
falling_through = true;
continue;
}
if (!falling_through) {
auto test_result = switch_case.test()->execute(interpreter, global_object);
if (interpreter.exception())
return {};
if (!is_strictly_equal(discriminant_result, test_result))
continue;
}
falling_through = true;
if (auto result = execute_switch_case(switch_case); result.has_value())
return *result;
}
if (default_switch_case) {
if (auto result = execute_switch_case(*default_switch_case); result.has_value())
return *result;
}

return last_value;
}

Expand Down
32 changes: 32 additions & 0 deletions Userland/Libraries/LibJS/Tests/switch-basic.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,43 @@ describe("basic switch tests", () => {
}
expect(i).toBe(5);
});

test("default branch is not taken if more exact branch exists", () => {
function switchTest(i) {
let result = 0;
switch (i) {
case 1:
result += 1;
break;
case 1:
expect().fail();
case 2:
result += 2;
default:
result += 4;
case 3:
result += 8;
break;
case 2:
expect().fail();
}
return result;
}

expect(switchTest(1)).toBe(1);
expect(switchTest(2)).toBe(14);
expect(switchTest(3)).toBe(8);
expect(switchTest(4)).toBe(12);
});
});

describe("errors", () => {
test("syntax errors", () => {
expect("switch () {}").not.toEval();
expect("switch () { case 1: continue; }").not.toEval();
expect("switch () { case 1: break doesnotexist; }").not.toEval();
expect("label: switch () { case 1: break not_the_right_label; }").not.toEval();
expect("label: switch () { case 1: continue label; }").not.toEval();
expect("switch (foo) { bar }").not.toEval();
expect("switch (foo) { default: default: }").not.toEval();
});
Expand Down

0 comments on commit e5d48ee

Please sign in to comment.