Make update operations reuse the last found binding locator (#2876)

* Make update operations reuse the last found binding locator

* Rename opcode

* Reword opcode comments

* Change capacity of `bindings_stack`

* Use the callframe stack to store the current binding

* Fix typo

* Reuse locators on assignment deletions

* Fix binding bug

* Remove leftover comment

boa_engine/src/builtins/json/mod.rs

@@ -123,7 +123,6 @@ impl Json {
                 context.realm().environment().compile_env(),
                 context,
             );
-            compiler.create_script_decls(&statement_list, false);
             compiler.compile_statement_list(&statement_list, true, false);
             Gc::new(compiler.finish())
         };

boa_engine/src/bytecompiler/class.rs

@@ -78,7 +78,6 @@ impl ByteCompiler<'_, '_> {
             } else {
                 None
             };
-            compiler.create_script_decls(expr.body(), false);
             compiler.compile_statement_list(expr.body(), false, false);
 
             let env_info = compiler.pop_compile_environment();
@@ -397,7 +396,7 @@ impl ByteCompiler<'_, '_> {
                     compiler.push_compile_environment(false);
                     compiler.create_immutable_binding(class_name.into(), true);
                     compiler.push_compile_environment(true);
-                    compiler.create_script_decls(statement_list, false);
+                    compiler.create_declarations(statement_list, false);
                     compiler.compile_statement_list(statement_list, false, false);
                     let env_info = compiler.pop_compile_environment();
                     compiler.pop_compile_environment();

boa_engine/src/bytecompiler/expression/assign.rs

@@ -56,7 +56,13 @@ impl ByteCompiler<'_, '_> {
                 Access::Variable { name } => {
                     let binding = self.get_binding_value(name);
                     let index = self.get_or_insert_binding(binding);
-                    self.emit(Opcode::GetName, &[index]);
+                    let lex = self.current_environment.borrow().is_lex_binding(name);
+
+                    if lex {
+                        self.emit(Opcode::GetName, &[index]);
+                    } else {
+                        self.emit(Opcode::GetNameAndLocator, &[index]);
+                    }
 
                     if short_circuit {
                         early_exit = Some(self.emit_opcode_with_operand(opcode));
@@ -68,10 +74,13 @@ impl ByteCompiler<'_, '_> {
                     if use_expr {
                         self.emit_opcode(Opcode::Dup);
                     }
-
+                    if lex {
-                    let binding = self.set_mutable_binding(name);
+                        let binding = self.set_mutable_binding(name);
-                    let index = self.get_or_insert_binding(binding);
+                        let index = self.get_or_insert_binding(binding);
-                    self.emit(Opcode::SetName, &[index]);
+                        self.emit(Opcode::SetName, &[index]);
+                    } else {
+                        self.emit_opcode(Opcode::SetNameByLocator);
+                    }
                 }
                 Access::Property { access } => match access {
                     PropertyAccess::Simple(access) => match access.field() {

boa_engine/src/bytecompiler/expression/update.rs

@@ -24,7 +24,13 @@ impl ByteCompiler<'_, '_> {
             Access::Variable { name } => {
                 let binding = self.get_binding_value(name);
                 let index = self.get_or_insert_binding(binding);
-                self.emit(Opcode::GetName, &[index]);
+                let lex = self.current_environment.borrow().is_lex_binding(name);
+
+                if lex {
+                    self.emit(Opcode::GetName, &[index]);
+                } else {
+                    self.emit(Opcode::GetNameAndLocator, &[index]);
+                }
 
                 self.emit_opcode(opcode);
                 if post {
@@ -33,9 +39,13 @@ impl ByteCompiler<'_, '_> {
                     self.emit_opcode(Opcode::Dup);
                 }
 
-                let binding = self.set_mutable_binding(name);
+                if lex {
-                let index = self.get_or_insert_binding(binding);
+                    let binding = self.set_mutable_binding(name);
-                self.emit(Opcode::SetName, &[index]);
+                    let index = self.get_or_insert_binding(binding);
+                    self.emit(Opcode::SetName, &[index]);
+                } else {
+                    self.emit_opcode(Opcode::SetNameByLocator);
+                }
             }
             Access::Property { access } => match access {
                 PropertyAccess::Simple(access) => match access.field() {

boa_engine/src/bytecompiler/function.rs

@@ -189,7 +189,6 @@ impl FunctionCompiler {
             compiler.emit_opcode(Opcode::Yield);
         }
 
-        compiler.create_script_decls(body, false);
         compiler.compile_statement_list(body, false, false);
 
         if let Some(env_label) = env_label {

boa_engine/src/bytecompiler/mod.rs

@@ -631,13 +631,26 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> {
     {
         match access {
             Access::Variable { name } => {
+                let binding = self.get_binding_value(name);
+                let index = self.get_or_insert_binding(binding);
+                let lex = self.current_environment.borrow().is_lex_binding(name);
+
+                if !lex {
+                    self.emit(Opcode::GetLocator, &[index]);
+                }
+
                 expr_fn(self, 0);
                 if use_expr {
                     self.emit(Opcode::Dup, &[]);
                 }
-                let binding = self.set_mutable_binding(name);
+
-                let index = self.get_or_insert_binding(binding);
+                if lex {
-                self.emit(Opcode::SetName, &[index]);
+                    let binding = self.set_mutable_binding(name);
+                    let index = self.get_or_insert_binding(binding);
+                    self.emit(Opcode::SetName, &[index]);
+                } else {
+                    self.emit_opcode(Opcode::SetNameByLocator);
+                }
             }
             Access::Property { access } => match access {
                 PropertyAccess::Simple(access) => match access.field() {
@@ -736,6 +749,7 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> {
         use_expr: bool,
         configurable_globals: bool,
     ) {
+        self.create_declarations(list, configurable_globals);
         if use_expr {
             let expr_index = list
                 .statements()
@@ -770,7 +784,7 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> {
         self.push_compile_environment(strict);
         let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment);
 
-        self.create_script_decls(list, true);
+        self.create_declarations(list, true);
 
         if use_expr {
             let expr_index = list
@@ -1349,7 +1363,7 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> {
     }
 
     /// Creates the declarations for a script.
-    pub(crate) fn create_script_decls(
+    pub(crate) fn create_declarations(
         &mut self,
         stmt_list: &StatementList,
         configurable_globals: bool,

boa_engine/src/bytecompiler/statement/block.rs

@@ -13,7 +13,6 @@ impl ByteCompiler<'_, '_> {
         self.push_compile_environment(false);
         let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment);
 
-        self.create_script_decls(block.statement_list(), configurable_globals);
         self.compile_statement_list(block.statement_list(), use_expr, configurable_globals);
 
         let env_info = self.pop_compile_environment();

boa_engine/src/bytecompiler/statement/switch.rs

@@ -4,10 +4,15 @@ use boa_ast::statement::Switch;
 impl ByteCompiler<'_, '_> {
     /// Compile a [`Switch`] `boa_ast` node
     pub(crate) fn compile_switch(&mut self, switch: &Switch, configurable_globals: bool) {
+        self.compile_expr(switch.val(), true);
+
         self.push_compile_environment(false);
         let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment);
         for case in switch.cases() {
-            self.create_script_decls(case.body(), configurable_globals);
+            self.create_declarations(case.body(), configurable_globals);
+        }
+        if let Some(body) = switch.default() {
+            self.create_declarations(body, configurable_globals);
         }
         let (start_label, end_label) = self.emit_opcode_with_two_operands(Opcode::LoopStart);
 
@@ -15,7 +20,6 @@ impl ByteCompiler<'_, '_> {
         self.push_switch_control_info(None, start_address);
         self.patch_jump_with_target(start_label, start_address);
 
-        self.compile_expr(switch.val(), true);
         let mut labels = Vec::with_capacity(switch.cases().len());
         for case in switch.cases() {
             self.compile_expr(case.condition(), true);
@@ -26,13 +30,16 @@ impl ByteCompiler<'_, '_> {
 
         for (label, case) in labels.into_iter().zip(switch.cases()) {
             self.patch_jump(label);
-            self.compile_statement_list(case.body(), false, configurable_globals);
+            for item in case.body().statements() {
+                self.compile_stmt_list_item(item, false, configurable_globals);
+            }
         }
 
         self.patch_jump(exit);
         if let Some(body) = switch.default() {
-            self.create_script_decls(body, configurable_globals);
+            for item in body.statements() {
-            self.compile_statement_list(body, false, configurable_globals);
+                self.compile_stmt_list_item(item, false, configurable_globals);
+            }
         }
 
         self.pop_switch_control_info();

boa_engine/src/bytecompiler/statement/try.rs

@@ -23,7 +23,6 @@ impl ByteCompiler<'_, '_> {
         self.push_compile_environment(false);
         let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment);
 
-        self.create_script_decls(t.block().statement_list(), configurable_globals);
         self.compile_statement_list(t.block().statement_list(), use_expr, configurable_globals);
 
         let env_info = self.pop_compile_environment();
@@ -89,7 +88,6 @@ impl ByteCompiler<'_, '_> {
             self.emit_opcode(Opcode::Pop);
         }
 
-        self.create_script_decls(catch.block().statement_list(), configurable_globals);
         self.compile_statement_list(
             catch.block().statement_list(),
             use_expr,
@@ -119,7 +117,6 @@ impl ByteCompiler<'_, '_> {
         self.push_compile_environment(false);
         let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment);
 
-        self.create_script_decls(finally.block().statement_list(), configurable_globals);
         self.compile_statement_list(
             finally.block().statement_list(),
             false,

boa_engine/src/context/mod.rs

@@ -253,7 +253,6 @@ impl<'host> Context<'host> {
             self.realm.environment().compile_env(),
             self,
         );
-        compiler.create_script_decls(statement_list, false);
         compiler.compile_statement_list(statement_list, true, false);
         Ok(Gc::new(compiler.finish()))
     }

boa_engine/src/environments/compile.rs

@@ -56,6 +56,17 @@ impl CompileTimeEnvironment {
             .map_or(false, |binding| binding.lex)
     }
 
+    /// Checks if `name` is a lexical binding.
+    pub(crate) fn is_lex_binding(&self, name: Identifier) -> bool {
+        if let Some(binding) = self.bindings.get(&name) {
+            binding.lex
+        } else if let Some(outer) = &self.outer {
+            outer.borrow().is_lex_binding(name)
+        } else {
+            false
+        }
+    }
+
     /// Returns the number of bindings in this environment.
     pub(crate) fn num_bindings(&self) -> usize {
         self.bindings.len()

boa_engine/src/environments/runtime.rs

@@ -3,7 +3,7 @@ use crate::{
     JsResult, JsString, JsSymbol, JsValue,
 };
 use boa_ast::expression::Identifier;
-use boa_gc::{Finalize, Gc, GcRefCell, Trace};
+use boa_gc::{empty_trace, Finalize, Gc, GcRefCell, Trace};
 use rustc_hash::FxHashSet;
 use std::cell::Cell;
 
@@ -663,7 +663,7 @@ impl DeclarativeEnvironmentStack {
 /// A binding locator contains all information about a binding that is needed to resolve it at runtime.
 ///
 /// Binding locators get created at bytecode compile time and are accessible at runtime via the [`crate::vm::CodeBlock`].
-#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, Finalize)]
 pub(crate) struct BindingLocator {
     name: Identifier,
     environment_index: usize,
@@ -673,6 +673,10 @@ pub(crate) struct BindingLocator {
     silent: bool,
 }
 
+unsafe impl Trace for BindingLocator {
+    empty_trace!();
+}
+
 impl BindingLocator {
     /// Creates a new declarative binding locator that has knows indices.
     pub(crate) const fn declarative(
@@ -691,7 +695,7 @@ impl BindingLocator {
     }
 
     /// Creates a binding locator that indicates that the binding is on the global object.
-    pub(in crate::environments) const fn global(name: Identifier) -> Self {
+    pub(super) const fn global(name: Identifier) -> Self {
         Self {
             name,
             environment_index: 0,
@@ -844,7 +848,13 @@ impl Context<'_> {
                 Environment::Declarative(env) => {
                     Ok(env.bindings.borrow()[locator.binding_index].is_some())
                 }
-                Environment::Object(_) => Ok(true),
+                Environment::Object(obj) => {
+                    let key: JsString = self
+                        .interner()
+                        .resolve_expect(locator.name.sym())
+                        .into_common(false);
+                    obj.clone().has_property(key, self)
+                }
             }
         }
     }
@@ -953,7 +963,7 @@ impl Context<'_> {
     }
 
     /// Return the environment at the given index. Panics if the index is out of range.
-    fn environment_expect(&self, index: usize) -> &Environment {
+    pub(crate) fn environment_expect(&self, index: usize) -> &Environment {
         self.vm
             .environments
             .stack

boa_engine/src/vm/call_frame/mod.rs

@@ -5,7 +5,10 @@
 mod abrupt_record;
 mod env_stack;
 
-use crate::{builtins::promise::PromiseCapability, object::JsObject, vm::CodeBlock};
+use crate::{
+    builtins::promise::PromiseCapability, environments::BindingLocator, object::JsObject,
+    vm::CodeBlock,
+};
 use boa_gc::{Finalize, Gc, Trace};
 use thin_vec::ThinVec;
 
@@ -39,6 +42,9 @@ pub struct CallFrame {
 
     // Iterators and their `[[Done]]` flags that must be closed when an abrupt completion is thrown.
     pub(crate) iterators: ThinVec<(JsObject, bool)>,
+
+    // The stack of bindings being updated.
+    pub(crate) binding_stack: Vec<BindingLocator>,
 }
 
 /// ---- `CallFrame` public API ----
@@ -69,6 +75,7 @@ impl CallFrame {
             promise_capability: None,
             async_generator: None,
             iterators: ThinVec::new(),
+            binding_stack: Vec::new(),
         }
     }
 

boa_engine/src/vm/code_block.rs

@@ -335,6 +335,8 @@ impl CodeBlock {
             | Opcode::DefInitLet
             | Opcode::DefInitConst
             | Opcode::GetName
+            | Opcode::GetLocator
+            | Opcode::GetNameAndLocator
             | Opcode::GetNameOrUndefined
             | Opcode::SetName
             | Opcode::DeleteName => {
@@ -495,6 +497,7 @@ impl CodeBlock {
             | Opcode::SetPrototype
             | Opcode::PushObjectEnvironment
             | Opcode::IsObject
+            | Opcode::SetNameByLocator
             | Opcode::Nop => String::new(),
         }
     }

boa_engine/src/vm/flowgraph/mod.rs

@@ -403,6 +403,8 @@ impl CodeBlock {
                 | Opcode::DefInitLet
                 | Opcode::DefInitConst
                 | Opcode::GetName
+                | Opcode::GetLocator
+                | Opcode::GetNameAndLocator
                 | Opcode::GetNameOrUndefined
                 | Opcode::SetName
                 | Opcode::DeleteName => {
@@ -566,6 +568,7 @@ impl CodeBlock {
                 | Opcode::SuperCallPrepare
                 | Opcode::SetPrototype
                 | Opcode::IsObject
+                | Opcode::SetNameByLocator
                 | Opcode::Nop
                 | Opcode::PushObjectEnvironment => {
                     graph.add_node(previous_pc, NodeShape::None, label.into(), Color::None);

boa_engine/src/vm/mod.rs

@@ -40,6 +40,7 @@ pub(crate) use {
 
 #[cfg(test)]
 mod tests;
+
 /// Virtual Machine.
 #[derive(Debug)]
 pub struct Vm {

boa_engine/src/vm/opcode/get/name.rs

@@ -33,6 +33,59 @@ impl Operation for GetName {
     }
 }
 
+/// `GetLocator` implements the Opcode Operation for `Opcode::GetLocator`
+///
+/// Operation:
+///  - Find a binding on the environment and set the `current_binding` of the current frame.
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct GetLocator;
+
+impl Operation for GetLocator {
+    const NAME: &'static str = "GetLocator";
+    const INSTRUCTION: &'static str = "INST - GetLocator";
+
+    fn execute(context: &mut Context<'_>) -> JsResult<CompletionType> {
+        let index = context.vm.read::<u32>();
+        let mut binding_locator = context.vm.frame().code_block.bindings[index as usize];
+        context.find_runtime_binding(&mut binding_locator)?;
+
+        context.vm.frame_mut().binding_stack.push(binding_locator);
+
+        Ok(CompletionType::Normal)
+    }
+}
+
+/// `GetNameAndLocator` implements the Opcode Operation for `Opcode::GetNameAndLocator`
+///
+/// Operation:
+///  - Find a binding on the environment chain and push its value to the stack, setting the
+/// `current_binding` of the current frame.
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct GetNameAndLocator;
+
+impl Operation for GetNameAndLocator {
+    const NAME: &'static str = "GetNameAndLocator";
+    const INSTRUCTION: &'static str = "INST - GetNameAndLocator";
+
+    fn execute(context: &mut Context<'_>) -> JsResult<CompletionType> {
+        let index = context.vm.read::<u32>();
+        let mut binding_locator = context.vm.frame().code_block.bindings[index as usize];
+        binding_locator.throw_mutate_immutable(context)?;
+        context.find_runtime_binding(&mut binding_locator)?;
+        let value = context.get_binding(binding_locator)?.ok_or_else(|| {
+            let name = context
+                .interner()
+                .resolve_expect(binding_locator.name().sym())
+                .to_string();
+            JsNativeError::reference().with_message(format!("{name} is not defined"))
+        })?;
+
+        context.vm.frame_mut().binding_stack.push(binding_locator);
+        context.vm.push(value);
+        Ok(CompletionType::Normal)
+    }
+}
+
 /// `GetNameOrUndefined` implements the Opcode Operation for `Opcode::GetNameOrUndefined`
 ///
 /// Operation:

boa_engine/src/vm/opcode/mod.rs

@@ -687,6 +687,21 @@ generate_impl! {
         /// Stack: **=>** value
         GetName,
 
+        /// Find a binding on the environment and set the `current_binding` of the current frame.
+        ///
+        /// Operands: name_index: `u32`
+        ///
+        /// Stack: **=>**
+        GetLocator,
+
+        ///  Find a binding on the environment chain and push its value to the stack and its
+        /// `BindingLocator` to the `bindings_stack`.
+        ///
+        /// Operands: name_index: `u32`
+        ///
+        /// Stack: **=>** value
+        GetNameAndLocator,
+
         /// Find a binding on the environment chain and push its value. If the binding does not exist push undefined.
         ///
         /// Operands: name_index: `u32`
@@ -701,6 +716,11 @@ generate_impl! {
         /// Stack: value **=>**
         SetName,
 
+        /// Assigns a value to the binding pointed by the top of the `bindings_stack`.
+        ///
+        /// Stack: value **=>**
+        SetNameByLocator,
+
         /// Deletes a property of the global object.
         ///
         /// Operands: name_index: `u32`

boa_engine/src/vm/opcode/set/name.rs

@@ -1,4 +1,5 @@
 use crate::{
+    environments::{BindingLocator, Environment},
     vm::{opcode::Operation, CompletionType},
     Context, JsNativeError, JsResult,
 };
@@ -25,34 +26,70 @@ impl Operation for SetName {
 
         context.find_runtime_binding(&mut binding_locator)?;
 
-        if !context.is_initialized_binding(&binding_locator)? {
+        verify_initialized(binding_locator, context)?;
-            if binding_locator.is_global() && context.vm.frame().code_block.strict {
-                let key = context
-                    .interner()
-                    .resolve_expect(binding_locator.name().sym())
-                    .to_string();
-
-                return Err(JsNativeError::reference()
-                    .with_message(format!(
-                        "cannot assign to uninitialized global property `{key}`"
-                    ))
-                    .into());
-            }
 
-            if !binding_locator.is_global() {
+        context.set_binding(binding_locator, value, context.vm.frame().code_block.strict)?;
-                let key = context
-                    .interner()
-                    .resolve_expect(binding_locator.name().sym())
-                    .to_string();
 
-                return Err(JsNativeError::reference()
+        Ok(CompletionType::Normal)
-                    .with_message(format!("cannot assign to uninitialized binding `{key}`"))
+    }
-                    .into());
+}
-            }
+
+/// `SetNameByLocator` implements the Opcode Operation for `Opcode::SetNameByLocator`
+///
+/// Operation:
+///  - Assigns a value to the binding pointed by the `current_binding` of the current frame.
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct SetNameByLocator;
+
+impl Operation for SetNameByLocator {
+    const NAME: &'static str = "SetNameByLocator";
+    const INSTRUCTION: &'static str = "INST - SetNameByLocator";
+
+    fn execute(context: &mut Context<'_>) -> JsResult<CompletionType> {
+        let binding_locator = context
+            .vm
+            .frame_mut()
+            .binding_stack
+            .pop()
+            .expect("locator should have been popped before");
+        let value = context.vm.pop();
+        if binding_locator.is_silent() {
+            return Ok(CompletionType::Normal);
         }
+        binding_locator.throw_mutate_immutable(context)?;
+
+        verify_initialized(binding_locator, context)?;
 
         context.set_binding(binding_locator, value, context.vm.frame().code_block.strict)?;
 
         Ok(CompletionType::Normal)
     }
 }
+
+/// Checks that the binding pointed by `locator` exists and is initialized.
+fn verify_initialized(locator: BindingLocator, context: &mut Context<'_>) -> JsResult<()> {
+    if !context.is_initialized_binding(&locator)? {
+        let key = context.interner().resolve_expect(locator.name().sym());
+        let strict = context.vm.frame().code_block.strict;
+
+        let message = if locator.is_global() {
+            strict.then(|| format!("cannot assign to uninitialized global property `{key}`"))
+        } else {
+            match context.environment_expect(locator.environment_index()) {
+                Environment::Declarative(_) => {
+                    Some(format!("cannot assign to uninitialized binding `{key}`"))
+                }
+                Environment::Object(_) if strict => {
+                    Some(format!("cannot assign to uninitialized property `{key}`"))
+                }
+                Environment::Object(_) => None,
+            }
+        };
+
+        if let Some(message) = message {
+            return Err(JsNativeError::reference().with_message(message).into());
+        }
+    }
+
+    Ok(())
+}