Default cases on switch statements not cascading to below cases

[jedel1043]

Describe the bug
This is mainly a bug divided in possibly two separate, unintended behaviours:

• default is assumed to always be the last case of a switch (which is not true), and automatically breaks out of the case statement. The correct behaviour should be to cascade to subsequent cases.
• Cases that come after the default case are not tested for.

To Reproduce

let foo = 1;
switch (foo) {
  case 2:
    console.log(2);
    break;
  default:
    console.log('default');
  case 1:
    console.log(1);
}

Expected behaviour
When foo is 1, it should only print 1. When foo is neither 1 nor 2, it should print default and then 1.

Current behaviour
When foo is not 2, it always prints default, even if foo is 1.

[karol-janik]

Hello!

I would love to try to solve this problem!

[jedel1043]

@karol-janik Thank you very much! Let me give you some pointers on what it needs to be done.

The AST corresponding to a switch statement lives here:

boa/boa_ast/src/statement/switch.rs

Lines 89 to 93 in c4eabd2

	pub struct Switch {
	val: Expression,
	cases: Box<[Case]>,
	default: Option<StatementList>,
	}

Right now, the structure assumes the default variant is the last Case of every Switch, which is not true. This should be changed to also store the position of default in the list of Case'; this can be accomplished either by removing default and adding a is_default: bool in Case or by somehow storing the relative position of the default with respect to the cases array. Both options are good, but the latter option would be better to ensure there are no duplicate default Cases in the case array. Just an observation though, it could be that there are better solutions I'm not seeing right now.

Next, I'm seeing no problems on the parser side:

boa/boa_parser/src/parser/statement/switch/mod.rs

Lines 75 to 79 in c4eabd2

	let (cases, default) =
	CaseBlock::new(self.allow_yield, self.allow_await, self.allow_return)
	.parse(cursor, interner)?;
	let switch = Switch::new(condition, cases, default);

Apparently the parser already tries to parse all Cases even if it already parsed the default case, so it will need just small adjustments on the construction of the AST node.

Finally, the VM byte compiler:

boa/boa_engine/src/bytecompiler/statement/mod.rs

Lines 152 to 198 in c4eabd2

	pub(crate) fn compile_switch(
	&mut self,
	switch: &Switch,
	configurable_globals: bool,
	) -> JsResult<()> {
	self.context.push_compile_time_environment(false);
	let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment);
	for case in switch.cases() {
	self.create_decls(case.body(), configurable_globals);
	}
	self.emit_opcode(Opcode::LoopStart);
	let start_address = self.next_opcode_location();
	self.push_switch_control_info(None, start_address);
	self.compile_expr(switch.val(), true)?;
	let mut labels = Vec::with_capacity(switch.cases().len());
	for case in switch.cases() {
	self.compile_expr(case.condition(), true)?;
	labels.push(self.emit_opcode_with_operand(Opcode::Case));
	}
	let exit = self.emit_opcode_with_operand(Opcode::Default);
	for (label, case) in labels.into_iter().zip(switch.cases()) {
	self.patch_jump(label);
	self.compile_statement_list(case.body(), false, configurable_globals)?;
	}
	self.patch_jump(exit);
	if let Some(body) = switch.default() {
	self.create_decls(body, configurable_globals);
	self.compile_statement_list(body, false, configurable_globals)?;
	}
	self.pop_switch_control_info();
	self.emit_opcode(Opcode::LoopEnd);
	let (num_bindings, compile_environment) = self.context.pop_compile_time_environment();
	let index_compile_environment = self.push_compile_environment(compile_environment);
	self.patch_jump_with_target(push_env.0, num_bindings as u32);
	self.patch_jump_with_target(push_env.1, index_compile_environment as u32);
	self.emit_opcode(Opcode::PopEnvironment);
	Ok(())
	}

This needs to be rewritten in order to make the default case cascade to lower cases. I think this is the most difficult part of this problem, since you'll need to preserve the branch order of the switch case while testing for values after the default case.

Let me know if you have some questions about the codebase! 😁

[HalidOdat]

#2907 Seems to have fixed this, so closing this :)