Remove `arguments_binding` field from `CodeBlock` (#2969)

* Ensure the "arguments" is the first argument

* Remove `arguments_binding` field from `CodeBlock`

boa_engine/src/builtins/function/arguments.rs

@@ -155,18 +155,22 @@ impl Arguments {
         // We use indices to access environment bindings at runtime.
         // To map to function parameters to binding indices, we use the fact, that bindings in a
         // function environment start with all of the arguments in order:
+        //
+        // Note: The first binding (binding 0) is where "arguments" is stored.
+        //
         // `function f (a,b,c)`
         // | binding index | `arguments` property key | identifier |
-        // | 0             | 0                        | a          |
+        // | 1             | 0                        | a          |
-        // | 1             | 1                        | b          |
+        // | 2             | 1                        | b          |
-        // | 2             | 2                        | c          |
+        // | 3             | 2                        | c          |
         //
         // Notice that the binding index does not correspond to the argument index:
         // `function f (a,a,b)` => binding indices 0 (a), 1 (b), 2 (c)
         // | binding index | `arguments` property key | identifier |
         // | -             | 0                        | -          |
-        // | 0             | 1                        | a          |
+        // | 1             | 1                        | a          |
-        // | 1             | 2                        | b          |
+        // | 2             | 2                        | b          |
+        //
         // While the `arguments` object contains all arguments, they must not be all bound.
         // In the case of duplicate parameter names, the last one is bound as the environment binding.
         //
@@ -178,10 +182,14 @@ impl Arguments {
             if property_index >= len {
                 break;
             }
-            let binding_index = bindings.len() as u32;
+
+            // NOTE(HalidOdat): Offset by +1 to account for the first binding ("argument").
+            let binding_index = bindings.len() as u32 + 1;
+
             let entry = bindings
                 .entry(name)
                 .or_insert((binding_index, property_index));
+
             entry.1 = property_index;
             property_index += 1;
         }

boa_engine/src/bytecompiler/declarations.rs

@@ -1,6 +1,9 @@
 use crate::{
     bytecompiler::{ByteCompiler, FunctionCompiler, FunctionSpec, Label, NodeKind},
-    vm::{create_function_object_fast, create_generator_function_object, BindingOpcode, Opcode},
+    vm::{
+        create_function_object_fast, create_generator_function_object, BindingOpcode,
+        CodeBlockFlags, Opcode,
+    },
     JsNativeError, JsResult,
 };
 use boa_ast::{
@@ -844,7 +847,7 @@ impl ByteCompiler<'_, '_> {
         function_names.reverse();
         functions_to_initialize.reverse();
 
-        //15. Let argumentsObjectNeeded be true.
+        // 15. Let argumentsObjectNeeded be true.
         let mut arguments_object_needed = true;
 
         let arguments = Sym::ARGUMENTS.into();
@@ -882,26 +885,10 @@ impl ByteCompiler<'_, '_> {
             additional_env = true;
         }
 
-        // 21. For each String paramName of parameterNames, do
-        for param_name in &parameter_names {
-            // a. Let alreadyDeclared be ! env.HasBinding(paramName).
-            let already_declared = self.has_binding(*param_name);
-
-            // b. NOTE: Early errors ensure that duplicate parameter names can only occur in non-strict
-            //    functions that do not have parameter default values or rest parameters.
-
-            // c. If alreadyDeclared is false, then
-            if !already_declared {
-                // i. Perform ! env.CreateMutableBinding(paramName, false).
-                self.create_mutable_binding(*param_name, false);
-
-                // Note: These steps are not necessary in our implementation.
-                // ii. If hasDuplicates is true, then
-                //     1. Perform ! env.InitializeBinding(paramName, undefined).
-            }
-        }
-
         // 22. If argumentsObjectNeeded is true, then
+        //
+        // NOTE(HalidOdat): Has been moved up, so "arguments" gets registed as
+        //     the first binding in the environment with index 0.
         if arguments_object_needed {
             // Note: This happens at runtime.
             // a. If strict is true or simpleParameterList is false, then
@@ -918,21 +905,48 @@ impl ByteCompiler<'_, '_> {
                 // ii. NOTE: In strict mode code early errors prevent attempting to assign
                 //           to this binding, so its mutability is not observable.
                 self.create_immutable_binding(arguments, false);
-                self.arguments_binding = Some(self.initialize_immutable_binding(arguments));
             }
             // d. Else,
             else {
                 // i. Perform ! env.CreateMutableBinding("arguments", false).
                 self.create_mutable_binding(arguments, false);
-                self.arguments_binding = Some(self.initialize_mutable_binding(arguments, false));
             }
 
+            self.code_block_flags |= CodeBlockFlags::NEEDS_ARGUMENTS_OBJECT;
+        }
+
+        // 21. For each String paramName of parameterNames, do
+        for param_name in &parameter_names {
+            // a. Let alreadyDeclared be ! env.HasBinding(paramName).
+            let already_declared = self.has_binding(*param_name);
+
+            // b. NOTE: Early errors ensure that duplicate parameter names can only occur in non-strict
+            //    functions that do not have parameter default values or rest parameters.
+
+            // c. If alreadyDeclared is false, then
+            if !already_declared {
+                // i. Perform ! env.CreateMutableBinding(paramName, false).
+                self.create_mutable_binding(*param_name, false);
+
+                // Note: These steps are not necessary in our implementation.
+                // ii. If hasDuplicates is true, then
+                //     1. Perform ! env.InitializeBinding(paramName, undefined).
+            }
+        }
+
+        // 22. If argumentsObjectNeeded is true, then
+        if arguments_object_needed {
+            // MOVED: a-d.
+            //
+            // NOTE(HalidOdat): Has been moved up, see comment above.
+
             // Note: This happens at runtime.
             // e. Perform ! env.InitializeBinding("arguments", ao).
 
             // f. Let parameterBindings be the list-concatenation of parameterNames and « "arguments" ».
             parameter_names.push(arguments);
         }
+
         // 23. Else,
         //     a. Let parameterBindings be parameterNames.
         let parameter_bindings = parameter_names.clone();

boa_engine/src/bytecompiler/mod.rs

@@ -242,9 +242,6 @@ pub struct ByteCompiler<'ctx, 'host> {
     /// Functions inside this function
     pub(crate) functions: Vec<Gc<CodeBlock>>,
 
-    /// The `arguments` binding location of the function, if set.
-    pub(crate) arguments_binding: Option<BindingLocator>,
-
     /// Compile time environments in this function.
     pub(crate) compile_environments: Vec<Gc<GcRefCell<CompileTimeEnvironment>>>,
 
@@ -299,7 +296,6 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> {
             functions: Vec::default(),
             this_mode: ThisMode::Global,
             params: FormalParameterList::default(),
-            arguments_binding: None,
             compile_environments: Vec::default(),
             class_field_initializer_name: None,
             code_block_flags,
@@ -1349,7 +1345,6 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> {
             private_names: self.private_names.into_boxed_slice(),
             bindings: self.bindings.into_boxed_slice(),
             functions: self.functions.into_boxed_slice(),
-            arguments_binding: self.arguments_binding,
             compile_environments: self.compile_environments.into_boxed_slice(),
             class_field_initializer_name: self.class_field_initializer_name,
             flags: Cell::new(self.code_block_flags),

boa_engine/src/vm/code_block.rs

@@ -69,6 +69,11 @@ bitflags! {
         /// Does this function have a parameters environment.
         const PARAMETERS_ENV_BINDINGS = 0b0000_1000;
 
+        /// Does this function need a `"arguments"` object.
+        ///
+        /// The `"arguments"` binding is the first binding.
+        const NEEDS_ARGUMENTS_OBJECT = 0b0001_0000;
+
         /// Trace instruction execution to `stdout`.
         #[cfg(feature = "trace")]
         const TRACEABLE = 0b1000_0000;
@@ -124,10 +129,6 @@ pub struct CodeBlock {
     /// Functions inside this function
     pub(crate) functions: Box<[Gc<Self>]>,
 
-    /// The `arguments` binding location of the function, if set.
-    #[unsafe_ignore_trace]
-    pub(crate) arguments_binding: Option<BindingLocator>,
-
     /// Compile time environments in this function.
     pub(crate) compile_environments: Box<[Gc<GcRefCell<CompileTimeEnvironment>>]>,
 
@@ -155,7 +156,6 @@ impl CodeBlock {
             length,
             this_mode: ThisMode::Global,
             params: FormalParameterList::default(),
-            arguments_binding: None,
             compile_environments: Box::default(),
             class_field_initializer_name: None,
         }
@@ -206,6 +206,13 @@ impl CodeBlock {
             .get()
             .contains(CodeBlockFlags::PARAMETERS_ENV_BINDINGS)
     }
+
+    /// Does this function need a `"arguments"` object.
+    pub(crate) fn needs_arguments_object(&self) -> bool {
+        self.flags
+            .get()
+            .contains(CodeBlockFlags::NEEDS_ARGUMENTS_OBJECT)
+    }
 }
 
 /// ---- `CodeBlock` private API ----
@@ -1078,7 +1085,19 @@ impl JsObject {
                 .push_lexical(code.compile_environments[last_env].clone());
         }
 
-        if let Some(binding) = code.arguments_binding {
+        // Taken from: `FunctionDeclarationInstantiation` abstract function.
+        //
+        // Spec: https://tc39.es/ecma262/#sec-functiondeclarationinstantiation
+        //
+        // 22. If argumentsObjectNeeded is true, then
+        if code.needs_arguments_object() {
+            // a. If strict is true or simpleParameterList is false, then
+            //     i. Let ao be CreateUnmappedArgumentsObject(argumentsList).
+            // b. Else,
+            //     i. NOTE: A mapped argument object is only provided for non-strict functions
+            //              that don't have a rest parameter, any parameter
+            //              default value initializers, or any destructured parameters.
+            //     ii. Let ao be CreateMappedArgumentsObject(func, formals, argumentsList, env).
             let arguments_obj = if code.strict() || !code.params.is_simple() {
                 Arguments::create_unmapped_arguments_object(args, context)
             } else {
@@ -1091,11 +1110,11 @@ impl JsObject {
                     context,
                 )
             };
-            context.vm.environments.put_lexical_value(
+            let env_index = context.vm.environments.len() as u32 - 1;
-                binding.environment_index(),
+            context
-                binding.binding_index(),
+                .vm
-                arguments_obj.into(),
+                .environments
-            );
+                .put_lexical_value(env_index, 0, arguments_obj.into());
         }
 
         let argument_count = args.len();
@@ -1336,7 +1355,19 @@ impl JsObject {
                         .push_lexical(code.compile_environments[0].clone());
                 }
 
-                if let Some(binding) = code.arguments_binding {
+                // Taken from: `FunctionDeclarationInstantiation` abstract function.
+                //
+                // Spec: https://tc39.es/ecma262/#sec-functiondeclarationinstantiation
+                //
+                // 22. If argumentsObjectNeeded is true, then
+                if code.needs_arguments_object() {
+                    // a. If strict is true or simpleParameterList is false, then
+                    //     i. Let ao be CreateUnmappedArgumentsObject(argumentsList).
+                    // b. Else,
+                    //     i. NOTE: A mapped argument object is only provided for non-strict functions
+                    //              that don't have a rest parameter, any parameter
+                    //              default value initializers, or any destructured parameters.
+                    //     ii. Let ao be CreateMappedArgumentsObject(func, formals, argumentsList, env).
                     let arguments_obj = if code.strict() || !code.params.is_simple() {
                         Arguments::create_unmapped_arguments_object(args, context)
                     } else {
@@ -1349,11 +1380,12 @@ impl JsObject {
                             context,
                         )
                     };
-                    context.vm.environments.put_lexical_value(
+
-                        binding.environment_index(),
+                    let env_index = context.vm.environments.len() as u32 - 1;
-                        binding.binding_index(),
+                    context
-                        arguments_obj.into(),
+                        .vm
-                    );
+                        .environments
+                        .put_lexical_value(env_index, 0, arguments_obj.into());
                 }
 
                 let argument_count = args.len();