From cf85843dc838c7c36e6b635b34f31d100e64c63b Mon Sep 17 00:00:00 2001 From: raskad <32105367+raskad@users.noreply.github.com> Date: Wed, 29 Mar 2023 20:05:51 +0000 Subject: [PATCH] Fix super call execution order (#2724) This Pull Request fixes/closes #2672. It changes the following: - Get the super constructor and the new target before executing arguments in super calls. --- boa_engine/src/bytecompiler/expression/mod.rs | 2 + boa_engine/src/vm/code_block.rs | 1 + boa_engine/src/vm/flowgraph/mod.rs | 1 + boa_engine/src/vm/opcode/environment/mod.rs | 106 ++++++++++-------- boa_engine/src/vm/opcode/mod.rs | 11 +- boa_engine/src/vm/tests.rs | 31 ++++- 6 files changed, 105 insertions(+), 47 deletions(-) diff --git a/boa_engine/src/bytecompiler/expression/mod.rs b/boa_engine/src/bytecompiler/expression/mod.rs index 777fc0e61f..9aa1d262f7 100644 --- a/boa_engine/src/bytecompiler/expression/mod.rs +++ b/boa_engine/src/bytecompiler/expression/mod.rs @@ -255,6 +255,8 @@ impl ByteCompiler<'_, '_> { } Expression::Class(class) => self.class(class, true), Expression::SuperCall(super_call) => { + self.emit_opcode(Opcode::SuperCallPrepare); + let contains_spread = super_call .arguments() .iter() diff --git a/boa_engine/src/vm/code_block.rs b/boa_engine/src/vm/code_block.rs index adf42be389..1eb1875e75 100644 --- a/boa_engine/src/vm/code_block.rs +++ b/boa_engine/src/vm/code_block.rs @@ -448,6 +448,7 @@ impl CodeBlock { | Opcode::SuperCallDerived | Opcode::Await | Opcode::PushNewTarget + | Opcode::SuperCallPrepare | Opcode::CallEvalSpread | Opcode::CallSpread | Opcode::NewSpread diff --git a/boa_engine/src/vm/flowgraph/mod.rs b/boa_engine/src/vm/flowgraph/mod.rs index 8bfe8b66d1..bbeb1038a0 100644 --- a/boa_engine/src/vm/flowgraph/mod.rs +++ b/boa_engine/src/vm/flowgraph/mod.rs @@ -525,6 +525,7 @@ impl CodeBlock { | Opcode::CallSpread | Opcode::NewSpread | Opcode::SuperCallSpread + | Opcode::SuperCallPrepare | Opcode::SetPrototype | Opcode::IsObject | Opcode::Nop diff --git a/boa_engine/src/vm/opcode/environment/mod.rs b/boa_engine/src/vm/opcode/environment/mod.rs index 7cb5d29475..e01aed987b 100644 --- a/boa_engine/src/vm/opcode/environment/mod.rs +++ b/boa_engine/src/vm/opcode/environment/mod.rs @@ -84,6 +84,46 @@ impl Operation for Super { } } +/// `SuperCallPrepare` implements the Opcode Operation for `Opcode::SuperCallPrepare` +/// +/// Operation: +/// - Get the super constructor and the new target of the current environment. +#[derive(Debug, Clone, Copy)] +pub(crate) struct SuperCallPrepare; + +impl Operation for SuperCallPrepare { + const NAME: &'static str = "SuperCallPrepare"; + const INSTRUCTION: &'static str = "INST - SuperCallPrepare"; + + fn execute(context: &mut Context<'_>) -> JsResult { + let this_env = context + .realm + .environments + .get_this_environment() + .as_function_slots() + .expect("super call must be in function environment"); + let this_env_borrow = this_env.borrow(); + let new_target = this_env_borrow + .new_target() + .expect("must have new target") + .clone(); + let active_function = this_env_borrow.function_object().clone(); + drop(this_env_borrow); + let super_constructor = active_function + .__get_prototype_of__(context) + .expect("function object must have prototype"); + + if let Some(constructor) = super_constructor { + context.vm.push(constructor); + } else { + context.vm.push(JsValue::Null); + } + context.vm.push(new_target); + + Ok(CompletionType::Normal) + } +} + /// `SuperCall` implements the Opcode Operation for `Opcode::SuperCall` /// /// Operation: @@ -103,33 +143,20 @@ impl Operation for SuperCall { } arguments.reverse(); - let (new_target, active_function) = { - let this_env = context - .realm - .environments - .get_this_environment() - .as_function_slots() - .expect("super call must be in function environment"); - let this_env_borrow = this_env.borrow(); - let new_target = this_env_borrow - .new_target() - .expect("must have new target") - .clone(); - let active_function = this_env.borrow().function_object().clone(); - (new_target, active_function) - }; - let super_constructor = active_function - .__get_prototype_of__(context) - .expect("function object must have prototype") - .expect("function object must have prototype"); + let new_target_value = context.vm.pop(); + let super_constructor = context.vm.pop(); - if !super_constructor.is_constructor() { + let new_target = new_target_value + .as_object() + .expect("new target must be object"); + + let Some(super_constructor) = super_constructor.as_constructor() else { return Err(JsNativeError::typ() .with_message("super constructor object must be constructor") .into()); - } + }; - let result = super_constructor.__construct__(&arguments, &new_target, context)?; + let result = super_constructor.__construct__(&arguments, new_target, context)?; let this_env = context .realm @@ -144,6 +171,8 @@ impl Operation for SuperCall { .into()); } + let active_function = this_env.borrow().function_object().clone(); + result.initialize_instance_elements(&active_function, context)?; context.vm.push(result); @@ -175,33 +204,20 @@ impl Operation for SuperCallSpread { .expect("arguments array in call spread function must be dense") .clone(); - let (new_target, active_function) = { - let this_env = context - .realm - .environments - .get_this_environment() - .as_function_slots() - .expect("super call must be in function environment"); - let this_env_borrow = this_env.borrow(); - let new_target = this_env_borrow - .new_target() - .expect("must have new target") - .clone(); - let active_function = this_env.borrow().function_object().clone(); - (new_target, active_function) - }; - let super_constructor = active_function - .__get_prototype_of__(context) - .expect("function object must have prototype") - .expect("function object must have prototype"); + let new_target_value = context.vm.pop(); + let super_constructor = context.vm.pop(); - if !super_constructor.is_constructor() { + let new_target = new_target_value + .as_object() + .expect("new target must be object"); + + let Some(super_constructor) = super_constructor.as_constructor() else { return Err(JsNativeError::typ() .with_message("super constructor object must be constructor") .into()); - } + }; - let result = super_constructor.__construct__(&arguments, &new_target, context)?; + let result = super_constructor.__construct__(&arguments, new_target, context)?; let this_env = context .realm @@ -216,6 +232,8 @@ impl Operation for SuperCallSpread { .into()); } + let active_function = this_env.borrow().function_object().clone(); + result.initialize_instance_elements(&active_function, context)?; context.vm.push(result); diff --git a/boa_engine/src/vm/opcode/mod.rs b/boa_engine/src/vm/opcode/mod.rs index e9c4a6a50b..f1cdded9f5 100644 --- a/boa_engine/src/vm/opcode/mod.rs +++ b/boa_engine/src/vm/opcode/mod.rs @@ -1200,18 +1200,25 @@ generate_impl! { /// Stack: **=>** super Super, + /// Get the super constructor and the new target of the current environment. + /// + /// Operands: + /// + /// Stack: **=>** super_constructor, new_target + SuperCallPrepare, + /// Execute the `super()` method. /// /// Operands: argument_count: `u32` /// - /// Stack: argument_1, ... argument_n **=>** + /// Stack: super_constructor, new_target, argument_1, ... argument_n **=>** SuperCall, /// Execute the `super()` method where the arguments contain spreads. /// /// Operands: /// - /// Stack: arguments_array **=>** + /// Stack: super_constructor, new_target, arguments_array **=>** SuperCallSpread, /// Execute the `super()` method when no constructor of the class is defined. diff --git a/boa_engine/src/vm/tests.rs b/boa_engine/src/vm/tests.rs index af25d402fc..929e3d9a82 100644 --- a/boa_engine/src/vm/tests.rs +++ b/boa_engine/src/vm/tests.rs @@ -1,4 +1,4 @@ -use crate::{run_test_actions, JsValue, TestAction}; +use crate::{builtins::error::ErrorKind, run_test_actions, JsValue, TestAction}; use indoc::indoc; #[test] @@ -178,6 +178,35 @@ fn get_reference_by_super() { )]); } +#[test] +fn super_call_constructor_null() { + run_test_actions([TestAction::assert_native_error( + indoc! {r#" + class A extends Object { + constructor() { + Object.setPrototypeOf(A, null); + super(A); + } + } + new A(); + "#}, + ErrorKind::Type, + "super constructor object must be constructor", + )]); +} + +#[test] +fn super_call_get_constructor_before_arguments_execution() { + run_test_actions([TestAction::assert(indoc! {r#" + class A extends Object { + constructor() { + super(Object.setPrototypeOf(A, null)); + } + } + new A() instanceof A; + "#})]); +} + #[test] fn order_of_execution_in_assigment() { run_test_actions([