Fix order dependent execution in assignment. (#2378)

Fixes #1917 (fixes the code example given with the hashes) Fixes the order dependent execution of assignment, so the following code works correctly: ```javascript function f(x) { console.log(x) } // used to check the order of execution let a = [[]] (f(1), a)[(f(2), 0)][(f(3), 0)] = (f(4), 123) // 🤮 ``` Prints out: ```bash 1 2 3 4 123 ``` As expected This introduces some opcodes: - ~~`Swap3`: currently used only to keep some previous code working that needs refactoring.~~ - ~~`RotateRight n`: Rotates the `n` top values from the top of the stack to the right by `1`.~~ Already added by #2390 ~~Besides the new opcodes,~~ Some opcodes pop and push order of values on the stack have been changed. To eliminate many swaps and to make this change easier. ~~This PR is still a WIP and needs more refactoring~~ This is now ready for review/merge :)
boa-dev · Oct 30, 2022 · bc2dd9c · bc2dd9c
1 parent 18824ba
commit bc2dd9c
Show file tree

Hide file tree

Showing 13 changed files with 225 additions and 122 deletions.
diff --git a/boa_engine/src/bytecompiler/mod.rs b/boa_engine/src/bytecompiler/mod.rs
diff --git a/boa_engine/src/vm/opcode/define/class/getter.rs b/boa_engine/src/vm/opcode/define/class/getter.rs
@@ -17,8 +17,8 @@ impl Operation for DefineClassGetterByName {
 
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
         let index = context.vm.read::<u32>();
-        let object = context.vm.pop();
         let value = context.vm.pop();
+        let object = context.vm.pop();
         let object = object.to_object(context)?;
         value
             .as_object()

diff --git a/boa_engine/src/vm/opcode/define/class/method.rs b/boa_engine/src/vm/opcode/define/class/method.rs
@@ -17,8 +17,8 @@ impl Operation for DefineClassMethodByName {
 
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
         let index = context.vm.read::<u32>();
-        let object = context.vm.pop();
         let value = context.vm.pop();
+        let object = context.vm.pop();
         let object = if let Some(object) = object.as_object() {
             object.clone()
         } else {

diff --git a/boa_engine/src/vm/opcode/define/class/setter.rs b/boa_engine/src/vm/opcode/define/class/setter.rs
@@ -17,8 +17,8 @@ impl Operation for DefineClassSetterByName {
 
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
         let index = context.vm.read::<u32>();
-        let object = context.vm.pop();
         let value = context.vm.pop();
+        let object = context.vm.pop();
         let object = object.to_object(context)?;
         value
             .as_object()

diff --git a/boa_engine/src/vm/opcode/define/own_property.rs b/boa_engine/src/vm/opcode/define/own_property.rs
@@ -17,8 +17,8 @@ impl Operation for DefineOwnPropertyByName {
 
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
         let index = context.vm.read::<u32>();
-        let object = context.vm.pop();
         let value = context.vm.pop();
+        let object = context.vm.pop();
         let object = if let Some(object) = object.as_object() {
             object.clone()
         } else {

diff --git a/boa_engine/src/vm/opcode/delete/mod.rs b/boa_engine/src/vm/opcode/delete/mod.rs
@@ -47,8 +47,8 @@ impl Operation for DeletePropertyByValue {
     const INSTRUCTION: &'static str = "INST - DeletePropertyByValue";
 
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
-        let object = context.vm.pop();
         let key = context.vm.pop();
+        let object = context.vm.pop();
         let result = object
             .to_object(context)?
             .__delete__(&key.to_property_key(context)?, context)?;

diff --git a/boa_engine/src/vm/opcode/get/property.rs b/boa_engine/src/vm/opcode/get/property.rs
@@ -50,8 +50,8 @@ impl Operation for GetPropertyByValue {
     const INSTRUCTION: &'static str = "INST - GetPropertyByValue";
 
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
-        let object = context.vm.pop();
         let key = context.vm.pop();
+        let object = context.vm.pop();
         let object = if let Some(object) = object.as_object() {
             object.clone()
         } else {
@@ -78,8 +78,8 @@ impl Operation for GetPropertyByValuePush {
     const INSTRUCTION: &'static str = "INST - GetPropertyByValuePush";
 
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
-        let object = context.vm.pop();
         let key = context.vm.pop();
+        let object = context.vm.pop();
         let object = if let Some(object) = object.as_object() {
             object.clone()
         } else {

diff --git a/boa_engine/src/vm/opcode/mod.rs b/boa_engine/src/vm/opcode/mod.rs
@@ -702,7 +702,7 @@ generate_impl! {
         ///
         /// Operands:
         ///
-        /// Stack: key, object **=>** value
+        /// Stack: object, key **=>** value
         GetPropertyByValue,
 
         /// Get a property by value from an object an push the key and value on the stack.
@@ -711,7 +711,7 @@ generate_impl! {
         ///
         /// Operands:
         ///
-        /// Stack: key, object **=>** key, value
+        /// Stack: object, key **=>** key, value
         GetPropertyByValuePush,
 
         /// Sets a property by name of an object.
@@ -720,21 +720,21 @@ generate_impl! {
         ///
         /// Operands: name_index: `u32`
         ///
-        /// Stack: value, object **=>**
+        /// Stack: object, value **=>** value
         SetPropertyByName,
 
         /// Defines a own property of an object by name.
         ///
         /// Operands: name_index: `u32`
         ///
-        /// Stack: value, object **=>**
+        /// Stack: object, value **=>**
         DefineOwnPropertyByName,
 
         /// Defines a class method by name.
         ///
         /// Operands: name_index: `u32`
         ///
-        /// Stack: value, object **=>**
+        /// Stack: object, value **=>**
         DefineClassMethodByName,
 
         /// Sets a property by value of an object.
@@ -743,7 +743,7 @@ generate_impl! {
         ///
         /// Operands:
         ///
-        /// Stack: value, key, object **=>**
+        /// Stack: object, key, value **=>** value
         SetPropertyByValue,
 
         /// Defines a own property of an object by value.
@@ -766,7 +766,7 @@ generate_impl! {
         ///
         /// Operands: name_index: `u32`
         ///
-        /// Stack: value, object **=>**
+        /// Stack: object, value **=>**
         SetPropertyGetterByName,
 
         /// Defines a getter class method by name.
@@ -775,7 +775,7 @@ generate_impl! {
         ///
         /// Operands: name_index: `u32`
         ///
-        /// Stack: value, object **=>**
+        /// Stack: object, value **=>**
         DefineClassGetterByName,
 
         /// Sets a getter property by value of an object.
@@ -802,7 +802,7 @@ generate_impl! {
         ///
         /// Operands: name_index: `u32`
         ///
-        /// Stack: value, object **=>**
+        /// Stack: object, value **=>**
         SetPropertySetterByName,
 
         /// Defines a setter class method by name.
@@ -811,7 +811,7 @@ generate_impl! {
         ///
         /// Operands: name_index: `u32`
         ///
-        /// Stack: value, object **=>**
+        /// Stack: object, value **=>**
         DefineClassSetterByName,
 
         /// Sets a setter property by value of an object.
@@ -838,7 +838,7 @@ generate_impl! {
         ///
         /// Operands: name_index: `u32`
         ///
-        /// Stack: object, value **=>**
+        /// Stack: object, value **=>** value
         AssignPrivateField,
 
         /// Set a private property of a class constructor by it's name.
@@ -936,7 +936,7 @@ generate_impl! {
         ///
         /// Operands:
         ///
-        /// Stack: key, object **=>**
+        /// Stack: object, key **=>**
         DeletePropertyByValue,
 
         /// Copy all properties of one object to another object.

diff --git a/boa_engine/src/vm/opcode/set/private.rs b/boa_engine/src/vm/opcode/set/private.rs
@@ -25,7 +25,8 @@ impl Operation for AssignPrivateField {
             let mut object_borrow_mut = object.borrow_mut();
             match object_borrow_mut.get_private_element(name.sym()) {
                 Some(PrivateElement::Field(_)) => {
-                    object_borrow_mut.set_private_element(name.sym(), PrivateElement::Field(value));
+                    object_borrow_mut
+                        .set_private_element(name.sym(), PrivateElement::Field(value.clone()));
                 }
                 Some(PrivateElement::Method(_)) => {
                     return Err(JsNativeError::typ()
@@ -38,7 +39,7 @@ impl Operation for AssignPrivateField {
                 }) => {
                     let setter = setter.clone();
                     drop(object_borrow_mut);
-                    setter.call(&object.clone().into(), &[value], context)?;
+                    setter.call(&object.clone().into(), &[value.clone()], context)?;
                 }
                 None => {
                     return Err(JsNativeError::typ()
@@ -56,6 +57,7 @@ impl Operation for AssignPrivateField {
                 .with_message("cannot set private property on non-object")
                 .into());
         }
+        context.vm.push(value);
         Ok(ShouldExit::False)
     }
 }

diff --git a/boa_engine/src/vm/opcode/set/property.rs b/boa_engine/src/vm/opcode/set/property.rs
@@ -18,8 +18,8 @@ impl Operation for SetPropertyByName {
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
         let index = context.vm.read::<u32>();
 
-        let object = context.vm.pop();
         let value = context.vm.pop();
+        let object = context.vm.pop();
         let object = if let Some(object) = object.as_object() {
             object.clone()
         } else {
@@ -33,7 +33,8 @@ impl Operation for SetPropertyByName {
             .into_common::<JsString>(false)
             .into();
 
-        object.set(name, value, context.vm.frame().code.strict, context)?;
+        object.set(name, value.clone(), context.vm.frame().code.strict, context)?;
+        context.vm.stack.push(value);
         Ok(ShouldExit::False)
     }
 }
@@ -50,17 +51,18 @@ impl Operation for SetPropertyByValue {
     const INSTRUCTION: &'static str = "INST - SetPropertyByValue";
 
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
-        let object = context.vm.pop();
-        let key = context.vm.pop();
         let value = context.vm.pop();
+        let key = context.vm.pop();
+        let object = context.vm.pop();
         let object = if let Some(object) = object.as_object() {
             object.clone()
         } else {
             object.to_object(context)?
         };
 
         let key = key.to_property_key(context)?;
-        object.set(key, value, context.vm.frame().code.strict, context)?;
+        object.set(key, value.clone(), context.vm.frame().code.strict, context)?;
+        context.vm.stack.push(value);
         Ok(ShouldExit::False)
     }
 }
@@ -78,8 +80,8 @@ impl Operation for SetPropertyGetterByName {
 
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
         let index = context.vm.read::<u32>();
-        let object = context.vm.pop();
         let value = context.vm.pop();
+        let object = context.vm.pop();
         let object = object.to_object(context)?;
         let name = context.vm.frame().code.names[index as usize];
         let name = context
@@ -155,8 +157,8 @@ impl Operation for SetPropertySetterByName {
 
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
         let index = context.vm.read::<u32>();
-        let object = context.vm.pop();
         let value = context.vm.pop();
+        let object = context.vm.pop();
         let object = object.to_object(context)?;
         let name = context.vm.frame().code.names[index as usize];
         let name = context

diff --git a/boa_engine/src/vm/opcode/unary_ops/decrement.rs b/boa_engine/src/vm/opcode/unary_ops/decrement.rs
@@ -41,13 +41,13 @@ impl Operation for DecPost {
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
         let value = context.vm.pop();
         let value = value.to_numeric(context)?;
-        context.vm.push(value.clone());
         match value {
             Numeric::Number(number) => context.vm.push(number - 1f64),
-            Numeric::BigInt(bigint) => {
-                context.vm.push(JsBigInt::sub(&bigint, &JsBigInt::one()));
+            Numeric::BigInt(ref bigint) => {
+                context.vm.push(JsBigInt::sub(bigint, &JsBigInt::one()));
             }
         }
+        context.vm.push(value);
         Ok(ShouldExit::False)
     }
 }
diff --git a/boa_engine/src/vm/opcode/unary_ops/increment.rs b/boa_engine/src/vm/opcode/unary_ops/increment.rs
@@ -41,13 +41,13 @@ impl Operation for IncPost {
     fn execute(context: &mut Context) -> JsResult<ShouldExit> {
         let value = context.vm.pop();
         let value = value.to_numeric(context)?;
-        context.vm.push(value.clone());
         match value {
             Numeric::Number(number) => context.vm.push(number + 1f64),
-            Numeric::BigInt(bigint) => {
-                context.vm.push(JsBigInt::add(&bigint, &JsBigInt::one()));
+            Numeric::BigInt(ref bigint) => {
+                context.vm.push(JsBigInt::add(bigint, &JsBigInt::one()));
             }
         }
+        context.vm.push(value);
         Ok(ShouldExit::False)
     }
 }
diff --git a/boa_engine/src/vm/tests.rs b/boa_engine/src/vm/tests.rs
@@ -1,4 +1,4 @@
-use crate::{exec, Context, JsValue};
+use crate::{check_output, exec, Context, JsValue, TestAction};
 
 #[test]
 fn typeof_string() {
@@ -189,3 +189,38 @@ fn get_reference_by_super() {
         JsValue::from("ab")
     );
 }
+
+#[test]
+fn order_of_execution_in_assigment() {
+    let scenario = r#"
+        let i = 0;
+        let array = [[]];
+
+        array[i++][i++] = i++;
+    "#;
+
+    check_output(&[
+        TestAction::Execute(scenario),
+        TestAction::TestEq("i", "3"),
+        TestAction::TestEq("array.length", "1"),
+        TestAction::TestEq("array[0].length", "2"),
+    ]);
+}
+
+#[test]
+fn order_of_execution_in_assigment_with_comma_expressions() {
+    let scenario = r#"
+        let result = "";
+        function f(i) {
+            result += i;
+        }
+        let a = [[]];
+    
+        (f(1), a)[(f(2), 0)][(f(3), 0)] = (f(4), 123);
+    "#;
+
+    check_output(&[
+        TestAction::Execute(scenario),
+        TestAction::TestEq("result", "\"1234\""),
+    ]);
+}