Fix spread arguments in function calls (#2216)

Currently we only spread spread-expressions if they are the last argument in the function call. With this fix all arguments are spread if needed. The downside is that an array object is allocated to store all arguments if the arguments contain a spread-expression. But with dense indexed properties inplemented in #2167 this should be reasonably fast.

boa_engine/src/bytecompiler/mod.rs

@@ -1234,15 +1234,30 @@ impl<'b> ByteCompiler<'b> {
             }
             Node::ClassExpr(class) => self.class(class, true)?,
             Node::SuperCall(super_call) => {
-                for arg in super_call.args() {
+                let contains_spread = super_call
-                    self.compile_expr(arg, true)?;
+                    .args()
+                    .iter()
+                    .any(|arg| matches!(arg, Node::Spread(_)));
+
+                if contains_spread {
+                    self.emit_opcode(Opcode::PushNewArray);
+                    for arg in super_call.args() {
+                        self.compile_expr(arg, true)?;
+                        if let Node::Spread(_) = arg {
+                            self.emit_opcode(Opcode::InitIterator);
+                            self.emit_opcode(Opcode::PushIteratorToArray);
+                        } else {
+                            self.emit_opcode(Opcode::PushValueToArray);
+                        }
+                    }
+                } else {
+                    for arg in super_call.args() {
+                        self.compile_expr(arg, true)?;
+                    }
                 }
 
-                let last_is_rest_parameter =
+                if contains_spread {
-                    matches!(super_call.args().last(), Some(Node::Spread(_)));
+                    self.emit_opcode(Opcode::SuperCallSpread);
-
-                if last_is_rest_parameter {
-                    self.emit(Opcode::SuperCallWithRest, &[super_call.args().len() as u32]);
                 } else {
                     self.emit(Opcode::SuperCall, &[super_call.args().len() as u32]);
                 }
@@ -2144,24 +2159,31 @@ impl<'b> ByteCompiler<'b> {
             }
         }
 
-        for arg in call.args().iter() {
+        let contains_spread = call.args().iter().any(|arg| matches!(arg, Node::Spread(_)));
-            self.compile_expr(arg, true)?;
-        }
 
-        let last_is_rest_parameter = matches!(call.args().last(), Some(Node::Spread(_)));
+        if contains_spread {
+            self.emit_opcode(Opcode::PushNewArray);
+            for arg in call.args() {
+                self.compile_expr(arg, true)?;
+                if let Node::Spread(_) = arg {
+                    self.emit_opcode(Opcode::InitIterator);
+                    self.emit_opcode(Opcode::PushIteratorToArray);
+                } else {
+                    self.emit_opcode(Opcode::PushValueToArray);
+                }
+            }
+        } else {
+            for arg in call.args() {
+                self.compile_expr(arg, true)?;
+            }
+        }
 
         match kind {
-            CallKind::CallEval if last_is_rest_parameter => {
+            CallKind::CallEval if contains_spread => self.emit_opcode(Opcode::CallEvalSpread),
-                self.emit(Opcode::CallEvalWithRest, &[call.args().len() as u32]);
-            }
             CallKind::CallEval => self.emit(Opcode::CallEval, &[call.args().len() as u32]),
-            CallKind::Call if last_is_rest_parameter => {
+            CallKind::Call if contains_spread => self.emit_opcode(Opcode::CallSpread),
-                self.emit(Opcode::CallWithRest, &[call.args().len() as u32]);
-            }
             CallKind::Call => self.emit(Opcode::Call, &[call.args().len() as u32]),
-            CallKind::New if last_is_rest_parameter => {
+            CallKind::New if contains_spread => self.emit_opcode(Opcode::NewSpread),
-                self.emit(Opcode::NewWithRest, &[call.args().len() as u32]);
-            }
             CallKind::New => self.emit(Opcode::New, &[call.args().len() as u32]),
         }
 

boa_engine/src/object/property_map.rs

@@ -272,6 +272,15 @@ impl PropertyMap {
         self.indexed_properties = IndexedProperties::Dense(properties);
     }
 
+    /// Returns the vec of dense indexed properties if they exist.
+    pub(crate) fn dense_indexed_properties(&self) -> Option<&Vec<JsValue>> {
+        if let IndexedProperties::Dense(properties) = &self.indexed_properties {
+            Some(properties)
+        } else {
+            None
+        }
+    }
+
     /// An iterator visiting all key-value pairs in arbitrary order. The iterator element type is `(PropertyKey, &'a Property)`.
     ///
     /// This iterator does not recurse down the prototype chain.

boa_engine/src/vm/code_block.rs

@@ -199,13 +199,9 @@ impl CodeBlock {
             | Opcode::LogicalOr
             | Opcode::Coalesce
             | Opcode::CallEval
-            | Opcode::CallEvalWithRest
             | Opcode::Call
-            | Opcode::CallWithRest
             | Opcode::New
-            | Opcode::NewWithRest
             | Opcode::SuperCall
-            | Opcode::SuperCallWithRest
             | Opcode::ForInLoopInitIterator
             | Opcode::ForInLoopNext
             | Opcode::ConcatToString
@@ -373,6 +369,10 @@ impl CodeBlock {
             | Opcode::PushClassField
             | Opcode::SuperCallDerived
             | Opcode::Await
+            | Opcode::CallEvalSpread
+            | Opcode::CallSpread
+            | Opcode::NewSpread
+            | Opcode::SuperCallSpread
             | Opcode::Nop => String::new(),
         }
     }

boa_engine/src/vm/mod.rs

@@ -1584,21 +1584,18 @@ impl Context {
                 }
                 self.vm.push(result);
             }
-            Opcode::SuperCallWithRest => {
+            Opcode::SuperCallSpread => {
-                let argument_count = self.vm.read::<u32>();
+                // Get the arguments that are stored as an array object on the stack.
-                let rest_argument = self.vm.pop();
+                let arguments_array = self.vm.pop();
-                let mut arguments = Vec::with_capacity(argument_count as usize);
+                let arguments_array_object = arguments_array
-                for _ in 0..(argument_count - 1) {
+                    .as_object()
-                    arguments.push(self.vm.pop());
+                    .expect("arguments array in call spread function must be an object");
-                }
+                let arguments = arguments_array_object
-                arguments.reverse();
+                    .borrow()
-
+                    .properties()
-                let iterator_record = rest_argument.get_iterator(self, None, None)?;
+                    .dense_indexed_properties()
-                let mut rest_arguments = Vec::new();
+                    .expect("arguments array in call spread function must be dense")
-                while let Some(next) = iterator_record.step(self)? {
+                    .clone();
-                    rest_arguments.push(next.value(self)?);
-                }
-                arguments.append(&mut rest_arguments);
 
                 let (new_target, active_function) = {
                     let this_env = self
@@ -1765,27 +1762,26 @@ impl Context {
                     self.vm.push(result);
                 }
             }
-            Opcode::CallEvalWithRest => {
+            Opcode::CallEvalSpread => {
                 if self.vm.stack_size_limit <= self.vm.stack.len() {
                     return self.throw_range_error("Maximum call stack size exceeded");
                 }
-                let argument_count = self.vm.read::<u32>();
+
-                let rest_argument = self.vm.pop();
+                // Get the arguments that are stored as an array object on the stack.
-                let mut arguments = Vec::with_capacity(argument_count as usize);
+                let arguments_array = self.vm.pop();
-                for _ in 0..(argument_count - 1) {
+                let arguments_array_object = arguments_array
-                    arguments.push(self.vm.pop());
+                    .as_object()
-                }
+                    .expect("arguments array in call spread function must be an object");
-                arguments.reverse();
+                let arguments = arguments_array_object
+                    .borrow()
+                    .properties()
+                    .dense_indexed_properties()
+                    .expect("arguments array in call spread function must be dense")
+                    .clone();
+
                 let func = self.vm.pop();
                 let this = self.vm.pop();
 
-                let iterator_record = rest_argument.get_iterator(self, None, None)?;
-                let mut rest_arguments = Vec::new();
-                while let Some(next) = iterator_record.step(self)? {
-                    rest_arguments.push(next.value(self)?);
-                }
-                arguments.append(&mut rest_arguments);
-
                 let object = match func {
                     JsValue::Object(ref object) if object.is_callable() => object.clone(),
                     _ => return self.throw_type_error("not a callable function"),
@@ -1832,27 +1828,26 @@ impl Context {
 
                 self.vm.push(result);
             }
-            Opcode::CallWithRest => {
+            Opcode::CallSpread => {
                 if self.vm.stack_size_limit <= self.vm.stack.len() {
                     return self.throw_range_error("Maximum call stack size exceeded");
                 }
-                let argument_count = self.vm.read::<u32>();
+
-                let rest_argument = self.vm.pop();
+                // Get the arguments that are stored as an array object on the stack.
-                let mut arguments = Vec::with_capacity(argument_count as usize);
+                let arguments_array = self.vm.pop();
-                for _ in 0..(argument_count - 1) {
+                let arguments_array_object = arguments_array
-                    arguments.push(self.vm.pop());
+                    .as_object()
-                }
+                    .expect("arguments array in call spread function must be an object");
-                arguments.reverse();
+                let arguments = arguments_array_object
+                    .borrow()
+                    .properties()
+                    .dense_indexed_properties()
+                    .expect("arguments array in call spread function must be dense")
+                    .clone();
+
                 let func = self.vm.pop();
                 let this = self.vm.pop();
 
-                let iterator_record = rest_argument.get_iterator(self, None, None)?;
-                let mut rest_arguments = Vec::new();
-                while let Some(next) = iterator_record.step(self)? {
-                    rest_arguments.push(next.value(self)?);
-                }
-                arguments.append(&mut rest_arguments);
-
                 let object = match func {
                     JsValue::Object(ref object) if object.is_callable() => object.clone(),
                     _ => return self.throw_type_error("not a callable function"),
@@ -1881,25 +1876,23 @@ impl Context {
 
                 self.vm.push(result);
             }
-            Opcode::NewWithRest => {
+            Opcode::NewSpread => {
                 if self.vm.stack_size_limit <= self.vm.stack.len() {
                     return self.throw_range_error("Maximum call stack size exceeded");
                 }
-                let argument_count = self.vm.read::<u32>();
+                // Get the arguments that are stored as an array object on the stack.
-                let rest_argument = self.vm.pop();
+                let arguments_array = self.vm.pop();
-                let mut arguments = Vec::with_capacity(argument_count as usize);
+                let arguments_array_object = arguments_array
-                for _ in 0..(argument_count - 1) {
+                    .as_object()
-                    arguments.push(self.vm.pop());
+                    .expect("arguments array in call spread function must be an object");
-                }
+                let arguments = arguments_array_object
-                arguments.reverse();
+                    .borrow()
-                let func = self.vm.pop();
+                    .properties()
+                    .dense_indexed_properties()
+                    .expect("arguments array in call spread function must be dense")
+                    .clone();
 
-                let iterator_record = rest_argument.get_iterator(self, None, None)?;
+                let func = self.vm.pop();
-                let mut rest_arguments = Vec::new();
-                while let Some(next) = iterator_record.step(self)? {
-                    rest_arguments.push(next.value(self)?);
-                }
-                arguments.append(&mut rest_arguments);
 
                 let result = func
                     .as_constructor()

boa_engine/src/vm/opcode.rs

@@ -886,12 +886,12 @@ pub enum Opcode {
     /// Stack: argument_1, ... argument_n **=>**
     SuperCall,
 
-    /// Execute the `super()` method where the last argument is a rest parameter.
+    /// Execute the `super()` method where the arguments contain spreads.
     ///
-    /// Operands: argument_count: `u32`
+    /// Operands:
     ///
-    /// Stack: argument_1, ... argument_n **=>**
+    /// Stack: arguments_array **=>**
-    SuperCallWithRest,
+    SuperCallSpread,
 
     /// Execute the `super()` method when no constructor of the class is defined.
     ///
@@ -950,12 +950,12 @@ pub enum Opcode {
     /// Stack: func, this, argument_1, ... argument_n **=>** result
     CallEval,
 
-    /// Call a function named "eval" where the last argument is a rest parameter.
+    /// Call a function named "eval" where the arguments contain spreads.
     ///
-    /// Operands: argument_count: `u32`
+    /// Operands:
     ///
-    /// Stack: func, this, argument_1, ... argument_n **=>** result
+    /// Stack: arguments_array, func, this **=>** result
-    CallEvalWithRest,
+    CallEvalSpread,
 
     /// Call a function.
     ///
@@ -964,12 +964,12 @@ pub enum Opcode {
     /// Stack: func, this, argument_1, ... argument_n **=>** result
     Call,
 
-    /// Call a function where the last argument is a rest parameter.
+    /// Call a function where the arguments contain spreads.
     ///
-    /// Operands: argument_count: `u32`
+    /// Operands:
     ///
-    /// Stack: func, this, argument_1, ... argument_n **=>** result
+    /// Stack: arguments_array, func, this **=>** result
-    CallWithRest,
+    CallSpread,
 
     /// Call construct on a function.
     ///
@@ -978,12 +978,12 @@ pub enum Opcode {
     /// Stack: func, argument_1, ... argument_n **=>** result
     New,
 
-    /// Call construct on a function where the last argument is a rest parameter.
+    /// Call construct on a function where the arguments contain spreads.
     ///
-    /// Operands: argument_count: `u32`
+    /// Operands:
     ///
-    /// Stack: func, argument_1, ... argument_n **=>** result
+    /// Stack: arguments_array, func **=>** result
-    NewWithRest,
+    NewSpread,
 
     /// Return from a function.
     ///
@@ -1302,7 +1302,7 @@ impl Opcode {
             Self::This => "This",
             Self::Super => "Super",
             Self::SuperCall => "SuperCall",
-            Self::SuperCallWithRest => "SuperCallWithRest",
+            Self::SuperCallSpread => "SuperCallWithRest",
             Self::SuperCallDerived => "SuperCallDerived",
             Self::Case => "Case",
             Self::Default => "Default",
@@ -1311,11 +1311,11 @@ impl Opcode {
             Self::GetGenerator => "GetGenerator",
             Self::GetGeneratorAsync => "GetGeneratorAsync",
             Self::CallEval => "CallEval",
-            Self::CallEvalWithRest => "CallEvalWithRest",
+            Self::CallEvalSpread => "CallEvalSpread",
             Self::Call => "Call",
-            Self::CallWithRest => "CallWithRest",
+            Self::CallSpread => "CallSpread",
             Self::New => "New",
-            Self::NewWithRest => "NewWithRest",
+            Self::NewSpread => "NewSpread",
             Self::Return => "Return",
             Self::PushDeclarativeEnvironment => "PushDeclarativeEnvironment",
             Self::PushFunctionEnvironment => "PushFunctionEnvironment",
@@ -1445,7 +1445,7 @@ impl Opcode {
             Self::This => "INST - This",
             Self::Super => "INST - Super",
             Self::SuperCall => "INST - SuperCall",
-            Self::SuperCallWithRest => "INST - SuperCallWithRest",
+            Self::SuperCallSpread => "INST - SuperCallWithRest",
             Self::SuperCallDerived => "INST - SuperCallDerived",
             Self::Case => "INST - Case",
             Self::Default => "INST - Default",
@@ -1454,11 +1454,11 @@ impl Opcode {
             Self::GetGenerator => "INST - GetGenerator",
             Self::GetGeneratorAsync => "INST - GetGeneratorAsync",
             Self::CallEval => "INST - CallEval",
-            Self::CallEvalWithRest => "INST - CallEvalWithRest",
+            Self::CallEvalSpread => "INST - CallEvalSpread",
             Self::Call => "INST - Call",
-            Self::CallWithRest => "INST - CallWithRest",
+            Self::CallSpread => "INST - CallSpread",
             Self::New => "INST - New",
-            Self::NewWithRest => "INST - NewWithRest",
+            Self::NewSpread => "INST - NewSpread",
             Self::Return => "INST - Return",
             Self::PushDeclarativeEnvironment => "INST - PushDeclarativeEnvironment",
             Self::PushFunctionEnvironment => "INST - PushFunctionEnvironment",