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-dev · Mar 29, 2023 · cf85843 · cf85843
1 parent bf47815
commit cf85843
Show file tree

Hide file tree

Showing 6 changed files with 105 additions and 47 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<CompletionType> {
+        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
@@ -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
@@ -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([