From 908015f9fd2dd6debd7bed126c6571640b144cf4 Mon Sep 17 00:00:00 2001 From: Haled Odat <8566042+HalidOdat@users.noreply.github.com> Date: Sun, 7 May 2023 22:35:15 +0200 Subject: [PATCH] Fix switch `default` execution (#2907) * Fix switch `default` execution * Fix typo * Update switch.rs --- boa_ast/src/statement/switch.rs | 90 ++++++++++++------- boa_engine/src/bytecompiler/mod.rs | 3 +- .../src/bytecompiler/statement/switch.rs | 27 ++++-- boa_engine/src/vm/opcode/mod.rs | 2 +- boa_parser/src/parser/statement/switch/mod.rs | 19 ++-- .../src/parser/statement/switch/tests.rs | 29 +++--- 6 files changed, 105 insertions(+), 65 deletions(-) diff --git a/boa_ast/src/statement/switch.rs b/boa_ast/src/statement/switch.rs index d1a60096257..8f300389117 100644 --- a/boa_ast/src/statement/switch.rs +++ b/boa_ast/src/statement/switch.rs @@ -22,23 +22,39 @@ use core::ops::ControlFlow; #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] #[derive(Clone, Debug, PartialEq)] pub struct Case { - condition: Expression, + condition: Option, body: StatementList, } impl Case { - /// Creates a `Case` AST node. + /// Creates a regular `Case` AST node. #[inline] #[must_use] pub const fn new(condition: Expression, body: StatementList) -> Self { - Self { condition, body } + Self { + condition: Some(condition), + body, + } + } + + /// Creates a default `Case` AST node. + #[inline] + #[must_use] + pub const fn default(body: StatementList) -> Self { + Self { + condition: None, + body, + } } /// Gets the condition of the case. + /// + /// If it's a regular case returns [`Some`] with the [`Expression`], + /// otherwise [`None`] is returned if it's the default case. #[inline] #[must_use] - pub const fn condition(&self) -> &Expression { - &self.condition + pub const fn condition(&self) -> Option<&Expression> { + self.condition.as_ref() } /// Gets the statement listin the body of the case. @@ -47,6 +63,13 @@ impl Case { pub const fn body(&self) -> &StatementList { &self.body } + + /// Check if the case is the `default` case. + #[inline] + #[must_use] + pub const fn is_default(&self) -> bool { + self.condition.is_none() + } } impl VisitWith for Case { @@ -54,7 +77,10 @@ impl VisitWith for Case { where V: Visitor<'a>, { - try_break!(visitor.visit_expression(&self.condition)); + if let Some(condition) = &self.condition { + try_break!(visitor.visit_expression(condition)); + } + visitor.visit_statement_list(&self.body) } @@ -62,7 +88,9 @@ impl VisitWith for Case { where V: VisitorMut<'a>, { - try_break!(visitor.visit_expression_mut(&mut self.condition)); + if let Some(condition) = &mut self.condition { + try_break!(visitor.visit_expression_mut(condition)); + } visitor.visit_statement_list_mut(&mut self.body) } } @@ -89,19 +117,14 @@ impl VisitWith for Case { pub struct Switch { val: Expression, cases: Box<[Case]>, - default: Option, } impl Switch { /// Creates a `Switch` AST node. #[inline] #[must_use] - pub fn new(val: Expression, cases: Box<[Case]>, default: Option) -> Self { - Self { - val, - cases, - default, - } + pub fn new(val: Expression, cases: Box<[Case]>) -> Self { + Self { val, cases } } /// Gets the value to switch. @@ -121,8 +144,13 @@ impl Switch { /// Gets the default statement list, if any. #[inline] #[must_use] - pub const fn default(&self) -> Option<&StatementList> { - self.default.as_ref() + pub fn default(&self) -> Option<&StatementList> { + for case in self.cases.as_ref() { + if case.is_default() { + return Some(case.body()); + } + } + None } } @@ -131,20 +159,20 @@ impl ToIndentedString for Switch { let indent = " ".repeat(indentation); let mut buf = format!("switch ({}) {{\n", self.val().to_interned_string(interner)); for e in self.cases().iter() { - buf.push_str(&format!( - "{} case {}:\n{}", - indent, - e.condition().to_interned_string(interner), - e.body().to_indented_string(interner, indentation + 2) - )); + if let Some(condition) = e.condition() { + buf.push_str(&format!( + "{indent} case {}:\n{}", + condition.to_interned_string(interner), + e.body().to_indented_string(interner, indentation + 2) + )); + } else { + buf.push_str(&format!( + "{indent} default:\n{}", + e.body().to_indented_string(interner, indentation + 2) + )); + } } - if let Some(ref default) = self.default { - buf.push_str(&format!( - "{indent} default:\n{}", - default.to_indented_string(interner, indentation + 2) - )); - } buf.push_str(&format!("{indent}}}")); buf @@ -167,9 +195,6 @@ impl VisitWith for Switch { for case in self.cases.iter() { try_break!(visitor.visit_case(case)); } - if let Some(sl) = &self.default { - try_break!(visitor.visit_statement_list(sl)); - } ControlFlow::Continue(()) } @@ -181,9 +206,6 @@ impl VisitWith for Switch { for case in self.cases.iter_mut() { try_break!(visitor.visit_case_mut(case)); } - if let Some(sl) = &mut self.default { - try_break!(visitor.visit_statement_list_mut(sl)); - } ControlFlow::Continue(()) } } diff --git a/boa_engine/src/bytecompiler/mod.rs b/boa_engine/src/bytecompiler/mod.rs index a9d31fe6945..5b488a19a3e 100644 --- a/boa_engine/src/bytecompiler/mod.rs +++ b/boa_engine/src/bytecompiler/mod.rs @@ -174,7 +174,7 @@ enum Literal { } #[must_use] -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) struct Label { index: u32, } @@ -295,6 +295,7 @@ pub struct ByteCompiler<'ctx, 'host> { impl<'ctx, 'host> ByteCompiler<'ctx, 'host> { /// Represents a placeholder address that will be patched later. const DUMMY_ADDRESS: u32 = u32::MAX; + const DUMMY_LABEL: Label = Label { index: u32::MAX }; /// Creates a new [`ByteCompiler`]. #[inline] diff --git a/boa_engine/src/bytecompiler/statement/switch.rs b/boa_engine/src/bytecompiler/statement/switch.rs index 93b16f7f069..08c986ec5a8 100644 --- a/boa_engine/src/bytecompiler/statement/switch.rs +++ b/boa_engine/src/bytecompiler/statement/switch.rs @@ -19,20 +19,35 @@ impl ByteCompiler<'_, '_> { 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)); + // If it does not have a condition it is the default case. + let label = if let Some(condition) = case.condition() { + self.compile_expr(condition, true); + + self.emit_opcode_with_operand(Opcode::Case) + } else { + Self::DUMMY_LABEL + }; + + labels.push(label); } - let exit = self.emit_opcode_with_operand(Opcode::Default); + let default_label = self.emit_opcode_with_operand(Opcode::Default); + let mut default_label_set = false; for (label, case) in labels.into_iter().zip(switch.cases()) { + // Check if it's the default case. + let label = if label == Self::DUMMY_LABEL { + default_label_set = true; + default_label + } else { + label + }; self.patch_jump(label); self.compile_statement_list(case.body(), false); } - self.patch_jump(exit); - if let Some(body) = switch.default() { - self.compile_statement_list(body, false); + if !default_label_set { + self.patch_jump(default_label); } self.pop_switch_control_info(); diff --git a/boa_engine/src/vm/opcode/mod.rs b/boa_engine/src/vm/opcode/mod.rs index 0087bff176f..e4611ecc35c 100644 --- a/boa_engine/src/vm/opcode/mod.rs +++ b/boa_engine/src/vm/opcode/mod.rs @@ -164,7 +164,7 @@ pub(crate) trait Operation { } generate_impl! { - #[derive(Debug, Clone, Copy, TryFromPrimitive)] + #[derive(Debug, Clone, Copy, PartialEq, Eq, TryFromPrimitive)] #[repr(u8)] pub enum Opcode { /// Pop the top value from the stack. diff --git a/boa_parser/src/parser/statement/switch/mod.rs b/boa_parser/src/parser/statement/switch/mod.rs index d7ccd60e79a..0d4f9a33439 100644 --- a/boa_parser/src/parser/statement/switch/mod.rs +++ b/boa_parser/src/parser/statement/switch/mod.rs @@ -72,11 +72,10 @@ where let position = cursor.peek(0, interner).or_abrupt()?.span().start(); - let (cases, default) = - CaseBlock::new(self.allow_yield, self.allow_await, self.allow_return) - .parse(cursor, interner)?; + let cases = CaseBlock::new(self.allow_yield, self.allow_await, self.allow_return) + .parse(cursor, interner)?; - let switch = Switch::new(condition, cases, default); + let switch = Switch::new(condition, cases); // It is a Syntax Error if the LexicallyDeclaredNames of CaseBlock contains any duplicate // entries, unless the source text matched by this production is not strict mode code and the @@ -144,13 +143,13 @@ impl TokenParser for CaseBlock where R: Read, { - type Output = (Box<[statement::Case]>, Option); + type Output = Box<[statement::Case]>; fn parse(self, cursor: &mut Cursor, interner: &mut Interner) -> ParseResult { cursor.expect(Punctuator::OpenBlock, "switch case block", interner)?; let mut cases = Vec::new(); - let mut default = None; + let mut has_default_case = false; loop { let token = cursor.next(interner).or_abrupt()?; @@ -181,7 +180,7 @@ where cases.push(statement::Case::new(cond, statement_list)); } TokenKind::Keyword((Keyword::Default, false)) => { - if default.is_some() { + if has_default_case { // If default has already been defined then it cannot be defined again and to do so is an error. return Err(Error::unexpected( token.to_string(interner), @@ -202,7 +201,9 @@ where ) .parse(cursor, interner)?; - default = Some(statement_list); + cases.push(statement::Case::default(statement_list)); + + has_default_case = true; } TokenKind::Punctuator(Punctuator::CloseBlock) => break, _ => { @@ -216,6 +217,6 @@ where } } - Ok((cases.into_boxed_slice(), default)) + Ok(cases.into_boxed_slice()) } } diff --git a/boa_parser/src/parser/statement/switch/tests.rs b/boa_parser/src/parser/statement/switch/tests.rs index c5c04ce6b69..b1de2ff8392 100644 --- a/boa_parser/src/parser/statement/switch/tests.rs +++ b/boa_parser/src/parser/statement/switch/tests.rs @@ -197,22 +197,23 @@ fn check_separated_switch() { ] .into(), ), - ] - .into(), - Some( - vec![Statement::Expression(Expression::from(Call::new( - Expression::PropertyAccess( - SimplePropertyAccess::new(Identifier::new(console).into(), log).into(), - ), - vec![Literal::from( - interner.get_or_intern_static("Default", utf16!("Default")), - ) + Case::default( + vec![Statement::Expression(Expression::from(Call::new( + Expression::PropertyAccess( + SimplePropertyAccess::new(Identifier::new(console).into(), log) + .into(), + ), + vec![Literal::from( + interner.get_or_intern_static("Default", utf16!("Default")), + ) + .into()] + .into(), + ))) .into()] .into(), - ))) - .into()] - .into(), - ), + ), + ] + .into(), )) .into(), ],