From e7c689c0d543002202dc11509b1d4511d17b1029 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Juli=C3=A1n=20Espina?= Date: Wed, 26 Apr 2023 04:29:51 +0000 Subject: [PATCH] Refactor binding handling APIs (#2870) We have currently some bugs related to binding assignments on arithmetic operations (`+=`, `++`, `?=`, etc.), but fixing those was getting a bit complex with our current bindings APIs. This PR refactors our binding handling APIs to be a bit easier to use. --- boa_engine/src/builtins/eval/mod.rs | 2 +- boa_engine/src/environments/runtime.rs | 421 ++++++++----------------- boa_engine/src/vm/code_block.rs | 14 +- boa_engine/src/vm/opcode/define/mod.rs | 36 +-- boa_engine/src/vm/opcode/delete/mod.rs | 44 +-- boa_engine/src/vm/opcode/get/name.rs | 91 +----- boa_engine/src/vm/opcode/set/name.rs | 69 ++-- 7 files changed, 185 insertions(+), 492 deletions(-) diff --git a/boa_engine/src/builtins/eval/mod.rs b/boa_engine/src/builtins/eval/mod.rs index 080712b471..070459e9d2 100644 --- a/boa_engine/src/builtins/eval/mod.rs +++ b/boa_engine/src/builtins/eval/mod.rs @@ -213,7 +213,7 @@ impl Eval { // Poison the last parent function environment, because it may contain new declarations after/during eval. if !strict { - context.vm.environments.poison_last_function(); + context.vm.environments.poison_until_last_function(); } // Set the compile time environment to the current running environment and save the number of current environments. diff --git a/boa_engine/src/environments/runtime.rs b/boa_engine/src/environments/runtime.rs index ecc536d9a7..108c89033e 100644 --- a/boa_engine/src/environments/runtime.rs +++ b/boa_engine/src/environments/runtime.rs @@ -266,6 +266,7 @@ impl Environment { } /// Returns the declarative environment and panic if it is not one. + #[track_caller] pub(crate) fn declarative_expect(&self) -> &Gc { self.as_declarative() .expect("environment must be declarative") @@ -418,8 +419,8 @@ impl DeclarativeEnvironmentStack { let environment = self .stack .iter() - .filter_map(Environment::as_declarative) - .last() + .rev() + .find_map(Environment::as_declarative) .expect("global environment must always exist"); (environment.poisoned.get(), with || environment.with.get()) }; @@ -464,8 +465,8 @@ impl DeclarativeEnvironmentStack { let environment = self .stack .iter() - .filter_map(Environment::as_declarative) - .last() + .rev() + .find_map(Environment::as_declarative) .expect("global environment must always exist"); (environment.poisoned.get(), with || environment.with.get()) }; @@ -523,9 +524,8 @@ impl DeclarativeEnvironmentStack { let environment = self .stack .iter() - .filter_map(Environment::as_declarative) - .filter(|e| e.slots().is_some()) - .last() + .rev() + .find_map(|env| env.as_declarative().filter(|decl| decl.slots().is_some())) .expect("global environment must always exist"); ( environment.poisoned.get(), @@ -545,6 +545,7 @@ impl DeclarativeEnvironmentStack { } /// Pop environment from the environments stack. + #[track_caller] pub(crate) fn pop(&mut self) -> Environment { debug_assert!(self.stack.len() > 1); self.stack @@ -557,11 +558,11 @@ impl DeclarativeEnvironmentStack { /// # Panics /// /// Panics if no environment exists on the stack. - pub(crate) fn current(&mut self) -> Gc { + #[track_caller] + pub(crate) fn current(&mut self) -> Environment { self.stack .last() .expect("global environment must always exist") - .declarative_expect() .clone() } @@ -582,68 +583,27 @@ impl DeclarativeEnvironmentStack { /// Mark that there may be added bindings from the current environment to the next function /// environment. - pub(crate) fn poison_last_function(&mut self) { - for env in self.stack.iter_mut().rev() { - if let Some(env) = env.as_declarative() { - if env.compile_env().borrow().is_function() { - env.poisoned.set(true); - return; - } - } - } - } - - /// Get the value of a binding. Ignores object environments. - /// - /// # Panics - /// - /// Panics if the environment or binding index are out of range. - pub(crate) fn get_value_optional( - &self, - mut environment_index: usize, - mut binding_index: usize, - name: Identifier, - ) -> Option { - if environment_index != self.stack.len() - 1 { - for env_index in (environment_index + 1..self.stack.len()).rev() { - let Environment::Declarative(env) = self - .stack - .get(env_index) - .expect("environment index must be in range") else { - continue; - }; - if !env.poisoned.get() { - break; - } - let compile = env.compile.borrow(); - if compile.is_function() { - if let Some(b) = compile.get_binding(name) { - environment_index = b.environment_index; - binding_index = b.binding_index; - break; - } - } + pub(crate) fn poison_until_last_function(&mut self) { + for env in self + .stack + .iter() + .rev() + .filter_map(Environment::as_declarative) + { + env.poisoned.set(true); + if env.compile_env().borrow().is_function() { + return; } } - - self.stack - .get(environment_index) - .expect("environment index must be in range") - .declarative_expect() - .bindings - .borrow() - .get(binding_index) - .expect("binding index must be in range") - .clone() } - /// Set the value of a binding. + /// Set the value of a declarative binding. /// /// # Panics /// /// Panics if the environment or binding index are out of range. #[track_caller] - pub(crate) fn put_value( + pub(crate) fn put_declarative_value( &mut self, environment_index: usize, binding_index: usize, @@ -688,37 +648,6 @@ impl DeclarativeEnvironmentStack { *binding = Some(value); } } - - /// Check if a binding name does exist in a poisoned environment. - /// - /// A binding could be marked as `global`, and at the same time, exist in a deeper environment - /// context; if the global context is poisoned, an `eval` call could have added a binding that is - /// not global with the same name as the global binding. This double checks that the binding name - /// is truly a global property. - /// - /// # Panics - /// - /// Panics if the global environment does not exist. - pub(crate) fn binding_in_poisoned_environment(&mut self, name: Identifier) -> bool { - for env in self - .stack - .split_first() - .expect("global environment must exist") - .1 - .iter() - .filter_map(Environment::as_declarative) - .rev() - { - if !env.poisoned.get() { - return false; - } - let compile = env.compile.borrow(); - if compile.is_function() && compile.get_binding(name).is_some() { - return true; - } - } - false - } } /// A binding locator contains all information about a binding that is needed to resolve it at runtime. @@ -736,7 +665,7 @@ pub(crate) struct BindingLocator { impl BindingLocator { /// Creates a new declarative binding locator that has knows indices. - pub(in crate::environments) const fn declarative( + pub(crate) const fn declarative( name: Identifier, environment_index: usize, binding_index: usize, @@ -830,26 +759,33 @@ impl BindingLocator { } impl Context<'_> { - /// Get the value of a binding. - /// - /// # Panics - /// - /// Panics if the environment or binding index are out of range. - pub(crate) fn get_value_optional( - &mut self, - mut environment_index: usize, - mut binding_index: usize, - name: Identifier, - ) -> JsResult> { - for env_index in (environment_index + 1..self.vm.environments.stack.len()).rev() { + /// Gets the corresponding runtime binding of the provided `BindingLocator`, modifying + /// its indexes in place. + /// + /// This readjusts a `BindingLocator` to the correct binding if a `with` environment or + /// `eval` call modified the compile-time bindings. + /// + /// Only use if the binding origin is unknown or comes from a `var` declaration. Lexical bindings + /// are completely removed of runtime checks because the specification guarantees that runtime + /// semantics cannot add or remove lexical bindings. + pub(crate) fn find_runtime_binding(&mut self, locator: &mut BindingLocator) -> JsResult<()> { + let current = self.vm.environments.current(); + if let Some(env) = current.as_declarative() { + if !env.with.get() && !env.poisoned.get() { + return Ok(()); + } + } + + for env_index in (locator.environment_index..self.vm.environments.stack.len()).rev() { match self.environment_expect(env_index) { Environment::Declarative(env) => { if env.poisoned.get() { let compile = env.compile.borrow(); if compile.is_function() { - if let Some(b) = compile.get_binding(name) { - environment_index = b.environment_index; - binding_index = b.binding_index; + if let Some(b) = compile.get_binding(locator.name) { + locator.environment_index = b.environment_index; + locator.binding_index = b.binding_index; + locator.global = false; break; } } @@ -861,7 +797,7 @@ impl Context<'_> { let o = o.clone(); let key: JsString = self .interner() - .resolve_expect(name.sym()) + .resolve_expect(locator.name.sym()) .into_common(false); if o.has_property(key.clone(), self)? { if let Some(unscopables) = o.get(JsSymbol::unscopables(), self)?.as_object() @@ -870,239 +806,140 @@ impl Context<'_> { continue; } } - return o.get(key, self).map(Some); + locator.environment_index = env_index; + locator.global = false; + break; } } } } - Ok(self - .environment_expect(environment_index) - .declarative_expect() - .bindings - .borrow() - .get(binding_index) - .expect("binding index must be in range") - .clone()) + Ok(()) } - /// Get the value of a binding by it's name. + /// Checks if the binding pointed by `locator` is initialized. /// - /// This only considers function environments that are poisoned. - /// All other bindings are accessed via indices. - pub(crate) fn get_value_if_global_poisoned( - &mut self, - name: Identifier, - ) -> JsResult> { - for env_index in (0..self.vm.environments.stack.len()).rev() { - let env = self.environment_expect(env_index); - - match env { + /// # Panics + /// + /// Panics if the environment or binding index are out of range. + pub(crate) fn is_initialized_binding(&mut self, locator: &BindingLocator) -> JsResult { + if locator.global { + let key: JsString = self + .interner() + .resolve_expect(locator.name.sym()) + .into_common(false); + self.global_object().has_property(key, self) + } else { + match self.environment_expect(locator.environment_index) { Environment::Declarative(env) => { - if env.poisoned.get() { - let compile = env.compile.borrow(); - if compile.is_function() { - if let Some(b) = compile.get_binding(name) { - return Ok(self - .environment_expect(b.environment_index) - .declarative_expect() - .bindings - .borrow() - .get(b.binding_index) - .expect("binding index must be in range") - .clone()); - } - } else if !env.with.get() { - return Ok(None); - } - } - } - Environment::Object(o) => { - let o = o.clone(); - let key: JsString = self - .interner() - .resolve_expect(name.sym()) - .into_common(false); - if o.has_property(key.clone(), self)? { - if let Some(unscopables) = o.get(JsSymbol::unscopables(), self)?.as_object() - { - if unscopables.get(key.clone(), self)?.to_boolean() { - continue; - } - } - return o.get(key, self).map(Some); - } + Ok(env.bindings.borrow()[locator.binding_index].is_some()) } + Environment::Object(_) => Ok(true), } } - - Ok(None) } - /// Set the value of a binding if it is initialized. - /// Return `true` if the value has been set. + /// Get the value of a binding. /// /// # Panics /// /// Panics if the environment or binding index are out of range. - pub(crate) fn put_value_if_initialized( - &mut self, - mut environment_index: usize, - mut binding_index: usize, - name: Identifier, - value: JsValue, - strict: bool, - ) -> JsResult { - for env_index in (environment_index + 1..self.vm.environments.stack.len()).rev() { - let env = self.environment_expect(env_index); - - match env { + pub(crate) fn get_binding(&mut self, locator: BindingLocator) -> JsResult> { + if locator.global { + let global = self.global_object(); + let key: JsString = self + .interner() + .resolve_expect(locator.name.sym()) + .into_common(false); + if global.has_property(key.clone(), self)? { + global.get(key, self).map(Some) + } else { + Ok(None) + } + } else { + match self.environment_expect(locator.environment_index) { Environment::Declarative(env) => { - if env.poisoned.get() { - let compile = env.compile.borrow(); - if compile.is_function() { - if let Some(b) = compile.get_binding(name) { - environment_index = b.environment_index; - binding_index = b.binding_index; - break; - } - } - } else if !env.with.get() { - break; - } + Ok(env.bindings.borrow()[locator.binding_index].clone()) } - Environment::Object(o) => { - let o = o.clone(); + Environment::Object(obj) => { + let obj = obj.clone(); let key: JsString = self .interner() - .resolve_expect(name.sym()) + .resolve_expect(locator.name.sym()) .into_common(false); - if o.has_property(key.clone(), self)? { - if let Some(unscopables) = o.get(JsSymbol::unscopables(), self)?.as_object() - { - if unscopables.get(key.clone(), self)?.to_boolean() { - continue; - } - } - return o.set(key, value, strict, self); - } + obj.get(key, self).map(Some) } } } - - let mut bindings = self - .environment_expect(environment_index) - .declarative_expect() - .bindings - .borrow_mut(); - let binding = bindings - .get_mut(binding_index) - .expect("binding index must be in range"); - if binding.is_none() { - Ok(false) - } else { - *binding = Some(value); - Ok(true) - } } - /// Set the value of a binding by it's name. - /// - /// This only considers function environments that are poisoned. - /// All other bindings are set via indices. + /// Sets the value of a binding. /// /// # Panics /// /// Panics if the environment or binding index are out of range. - pub(crate) fn put_value_if_global_poisoned( + #[track_caller] + pub(crate) fn set_binding( &mut self, - name: Identifier, - value: &JsValue, + locator: BindingLocator, + value: JsValue, strict: bool, - ) -> JsResult { - for env_index in (0..self.vm.environments.stack.len()).rev() { - let env = self.environment_expect(env_index); - - match env { - Environment::Declarative(env) => { - if env.poisoned.get() { - let compile = env.compile.borrow(); - if compile.is_function() { - if let Some(b) = compile.get_binding(name) { - let mut bindings = self - .environment_expect(b.environment_index) - .declarative_expect() - .bindings - .borrow_mut(); - let binding = bindings - .get_mut(b.binding_index) - .expect("binding index must be in range"); - *binding = Some(value.clone()); - return Ok(true); - } - } - } else if !env.with.get() { - return Ok(false); - } + ) -> JsResult<()> { + if locator.global { + let key = self + .interner() + .resolve_expect(locator.name().sym()) + .into_common::(false); + + self.global_object().set(key, value, strict, self)?; + } else { + match self.environment_expect(locator.environment_index) { + Environment::Declarative(decl) => { + decl.bindings.borrow_mut()[locator.binding_index] = Some(value); } - Environment::Object(o) => { - let o = o.clone(); + Environment::Object(obj) => { + let obj = obj.clone(); let key: JsString = self .interner() - .resolve_expect(name.sym()) + .resolve_expect(locator.name.sym()) .into_common(false); - if o.has_property(key.clone(), self)? { - if let Some(unscopables) = o.get(JsSymbol::unscopables(), self)?.as_object() - { - if unscopables.get(key.clone(), self)?.to_boolean() { - continue; - } - } - return o.set(key, value.clone(), strict, self); - } + + obj.set(key, value, strict, self)?; } } } - Ok(false) + Ok(()) } - /// Delete a binding form an object environment if it exists. + /// Deletes a binding if it exists. /// - /// Returns a tuple of `(found, deleted)`. - pub(crate) fn delete_binding_from_object_environment( - &mut self, - name: Identifier, - ) -> JsResult<(bool, bool)> { - for env_index in (0..self.vm.environments.stack.len()).rev() { - let env = self.environment_expect(env_index); - - match env { - Environment::Object(o) => { - let o = o.clone(); + /// Returns `true` if the binding was deleted. + /// + /// # Panics + /// + /// Panics if the environment or binding index are out of range. + pub(crate) fn delete_binding(&mut self, locator: BindingLocator) -> JsResult { + if locator.is_global() { + let key: JsString = self + .interner() + .resolve_expect(locator.name().sym()) + .into_common::(false); + self.global_object().__delete__(&key.into(), self) + } else { + match self.environment_expect(locator.environment_index) { + Environment::Declarative(_) => Ok(false), + Environment::Object(obj) => { + let obj = obj.clone(); let key: JsString = self .interner() - .resolve_expect(name.sym()) + .resolve_expect(locator.name.sym()) .into_common(false); - if o.has_property(key.clone(), self)? { - if let Some(unscopables) = o.get(JsSymbol::unscopables(), self)?.as_object() - { - if unscopables.get(key.clone(), self)?.to_boolean() { - continue; - } - } - return Ok((true, o.__delete__(&key.into(), self)?)); - } - } - Environment::Declarative(env) => { - if !env.with.get() { - return Ok((false, false)); - } + + obj.__delete__(&key.into(), self) } } } - - Ok((false, false)) } /// Return the environment at the given index. Panics if the index is out of range. diff --git a/boa_engine/src/vm/code_block.rs b/boa_engine/src/vm/code_block.rs index 02d3247b9c..a33daa30e7 100644 --- a/boa_engine/src/vm/code_block.rs +++ b/boa_engine/src/vm/code_block.rs @@ -952,7 +952,7 @@ impl JsObject { context .vm .environments - .put_value(index, 0, class_object.into()); + .put_declarative_value(index, 0, class_object.into()); last_env -= 1; } @@ -964,7 +964,7 @@ impl JsObject { context .vm .environments - .put_value(index, 0, self.clone().into()); + .put_declarative_value(index, 0, self.clone().into()); last_env -= 1; } @@ -994,11 +994,11 @@ impl JsObject { &this_function_object, &code.params, args, - &env, + env.declarative_expect(), context, ) }; - context.vm.environments.put_value( + context.vm.environments.put_declarative_value( binding.environment_index(), binding.binding_index(), arguments_obj.into(), @@ -1202,7 +1202,7 @@ impl JsObject { context .vm .environments - .put_value(index, 0, self.clone().into()); + .put_declarative_value(index, 0, self.clone().into()); last_env -= 1; } @@ -1231,11 +1231,11 @@ impl JsObject { &this_function_object, &code.params, args, - &env, + env.declarative_expect(), context, ) }; - context.vm.environments.put_value( + context.vm.environments.put_declarative_value( binding.environment_index(), binding.binding_index(), arguments_obj.into(), diff --git a/boa_engine/src/vm/opcode/define/mod.rs b/boa_engine/src/vm/opcode/define/mod.rs index 9f04f782cd..4a9bec904e 100644 --- a/boa_engine/src/vm/opcode/define/mod.rs +++ b/boa_engine/src/vm/opcode/define/mod.rs @@ -1,6 +1,6 @@ use crate::{ vm::{opcode::Operation, CompletionType}, - Context, JsResult, JsString, JsValue, + Context, JsResult, JsValue, }; pub(crate) mod class; @@ -52,36 +52,16 @@ impl Operation for DefInitVar { fn execute(context: &mut Context<'_>) -> JsResult { let index = context.vm.read::(); let value = context.vm.pop(); - let binding_locator = context.vm.frame().code_block.bindings[index as usize]; + let mut binding_locator = context.vm.frame().code_block.bindings[index as usize]; if binding_locator.is_silent() { return Ok(CompletionType::Normal); } binding_locator.throw_mutate_immutable(context)?; - if binding_locator.is_global() { - if !context.put_value_if_global_poisoned( - binding_locator.name(), - &value, - context.vm.frame().code_block.strict, - )? { - let key = context - .interner() - .resolve_expect(binding_locator.name().sym()) - .into_common::(false); - context.global_object().set( - key, - value, - context.vm.frame().code_block.strict, - context, - )?; - } - } else { - context.vm.environments.put_value( - binding_locator.environment_index(), - binding_locator.binding_index(), - value, - ); - } + context.find_runtime_binding(&mut binding_locator)?; + + context.set_binding(binding_locator, value, context.vm.frame().code_block.strict)?; + Ok(CompletionType::Normal) } } @@ -100,7 +80,7 @@ impl Operation for DefLet { fn execute(context: &mut Context<'_>) -> JsResult { let index = context.vm.read::(); let binding_locator = context.vm.frame().code_block.bindings[index as usize]; - context.vm.environments.put_value( + context.vm.environments.put_declarative_value( binding_locator.environment_index(), binding_locator.binding_index(), JsValue::Undefined, @@ -126,7 +106,7 @@ macro_rules! implement_declaratives { let index = context.vm.read::(); let value = context.vm.pop(); let binding_locator = context.vm.frame().code_block.bindings[index as usize]; - context.vm.environments.put_value( + context.vm.environments.put_declarative_value( binding_locator.environment_index(), binding_locator.binding_index(), value, diff --git a/boa_engine/src/vm/opcode/delete/mod.rs b/boa_engine/src/vm/opcode/delete/mod.rs index cb0cc539d5..5f6fefada4 100644 --- a/boa_engine/src/vm/opcode/delete/mod.rs +++ b/boa_engine/src/vm/opcode/delete/mod.rs @@ -76,50 +76,12 @@ impl Operation for DeleteName { fn execute(context: &mut Context<'_>) -> JsResult { let index = context.vm.read::(); - let binding_locator = context.vm.frame().code_block.bindings[index as usize]; + let mut binding_locator = context.vm.frame().code_block.bindings[index as usize]; binding_locator.throw_mutate_immutable(context)?; - let deleted = if binding_locator.is_global() - && !context - .vm - .environments - .binding_in_poisoned_environment(binding_locator.name()) - { - let (found, deleted) = - context.delete_binding_from_object_environment(binding_locator.name())?; - if found { - context.vm.push(deleted); - return Ok(CompletionType::Normal); - } + context.find_runtime_binding(&mut binding_locator)?; - let key: JsString = context - .interner() - .resolve_expect(binding_locator.name().sym()) - .into_common::(false); - let deleted = context - .global_object() - .__delete__(&key.clone().into(), context)?; - - if !deleted && context.vm.frame().code_block.strict { - return Err(JsNativeError::typ() - .with_message(format!( - "property `{}` is non-configurable and cannot be deleted", - key.to_std_string_escaped() - )) - .into()); - } - deleted - } else { - context - .vm - .environments - .get_value_optional( - binding_locator.environment_index(), - binding_locator.binding_index(), - binding_locator.name(), - ) - .is_none() - }; + let deleted = context.delete_binding(binding_locator)?; context.vm.push(deleted); Ok(CompletionType::Normal) diff --git a/boa_engine/src/vm/opcode/get/name.rs b/boa_engine/src/vm/opcode/get/name.rs index 38b4f1bbba..704731c262 100644 --- a/boa_engine/src/vm/opcode/get/name.rs +++ b/boa_engine/src/vm/opcode/get/name.rs @@ -1,8 +1,7 @@ use crate::{ error::JsNativeError, - property::DescriptorKind, vm::{opcode::Operation, CompletionType}, - Context, JsResult, JsString, JsValue, + Context, JsResult, }; /// `GetName` implements the Opcode Operation for `Opcode::GetName` @@ -18,57 +17,16 @@ impl Operation for GetName { fn execute(context: &mut Context<'_>) -> JsResult { let index = context.vm.read::(); - let binding_locator = context.vm.frame().code_block.bindings[index as usize]; + let mut binding_locator = context.vm.frame().code_block.bindings[index as usize]; binding_locator.throw_mutate_immutable(context)?; - - let value = if binding_locator.is_global() { - if let Some(value) = context.get_value_if_global_poisoned(binding_locator.name())? { - value - } else { - let key: JsString = context - .interner() - .resolve_expect(binding_locator.name().sym()) - .into_common(false); - match context.global_object().get_property(&key.clone().into()) { - Some(desc) => match desc.kind() { - DescriptorKind::Data { - value: Some(value), .. - } => value.clone(), - DescriptorKind::Accessor { get: Some(get), .. } if !get.is_undefined() => { - let get = get.clone(); - get.call(&context.global_object().into(), &[], context)? - } - _ => { - return Err(JsNativeError::reference() - .with_message(format!( - "{} is not defined", - key.to_std_string_escaped() - )) - .into()) - } - }, - _ => { - return Err(JsNativeError::reference() - .with_message(format!("{} is not defined", key.to_std_string_escaped())) - .into()) - } - } - } - } else if let Some(value) = context.get_value_optional( - binding_locator.environment_index(), - binding_locator.binding_index(), - binding_locator.name(), - )? { - value - } else { + 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(); - return Err(JsNativeError::reference() - .with_message(format!("{name} is not initialized")) - .into()); - }; + JsNativeError::reference().with_message(format!("{name} is not defined")) + })?; context.vm.push(value); Ok(CompletionType::Normal) @@ -88,39 +46,12 @@ impl Operation for GetNameOrUndefined { fn execute(context: &mut Context<'_>) -> JsResult { let index = context.vm.read::(); - let binding_locator = context.vm.frame().code_block.bindings[index as usize]; + let mut binding_locator = context.vm.frame().code_block.bindings[index as usize]; binding_locator.throw_mutate_immutable(context)?; - let value = if binding_locator.is_global() { - if let Some(value) = context.get_value_if_global_poisoned(binding_locator.name())? { - value - } else { - let key: JsString = context - .interner() - .resolve_expect(binding_locator.name().sym()) - .into_common(false); - match context.global_object().get_property(&key.into()) { - Some(desc) => match desc.kind() { - DescriptorKind::Data { - value: Some(value), .. - } => value.clone(), - DescriptorKind::Accessor { get: Some(get), .. } if !get.is_undefined() => { - let get = get.clone(); - get.call(&context.global_object().into(), &[], context)? - } - _ => JsValue::undefined(), - }, - _ => JsValue::undefined(), - } - } - } else if let Some(value) = context.get_value_optional( - binding_locator.environment_index(), - binding_locator.binding_index(), - binding_locator.name(), - )? { - value - } else { - JsValue::undefined() - }; + + context.find_runtime_binding(&mut binding_locator)?; + + let value = context.get_binding(binding_locator)?.unwrap_or_default(); context.vm.push(value); Ok(CompletionType::Normal) diff --git a/boa_engine/src/vm/opcode/set/name.rs b/boa_engine/src/vm/opcode/set/name.rs index f764e82ff5..1cd843f990 100644 --- a/boa_engine/src/vm/opcode/set/name.rs +++ b/boa_engine/src/vm/opcode/set/name.rs @@ -1,7 +1,6 @@ use crate::{ - error::JsNativeError, vm::{opcode::Operation, CompletionType}, - Context, JsResult, JsString, + Context, JsNativeError, JsResult, }; /// `SetName` implements the Opcode Operation for `Opcode::SetName` @@ -17,59 +16,43 @@ impl Operation for SetName { fn execute(context: &mut Context<'_>) -> JsResult { let index = context.vm.read::(); - let binding_locator = context.vm.frame().code_block.bindings[index as usize]; + let mut binding_locator = context.vm.frame().code_block.bindings[index as usize]; let value = context.vm.pop(); if binding_locator.is_silent() { return Ok(CompletionType::Normal); } binding_locator.throw_mutate_immutable(context)?; - if binding_locator.is_global() { - if !context.put_value_if_global_poisoned( - binding_locator.name(), - &value, - context.vm.frame().code_block.strict, - )? { - let key: JsString = context + context.find_runtime_binding(&mut binding_locator)?; + + if !context.is_initialized_binding(&binding_locator)? { + if binding_locator.is_global() && context.vm.frame().code_block.strict { + let key = context .interner() .resolve_expect(binding_locator.name().sym()) - .into_common(false); - let exists = context - .global_object() - .has_own_property(key.clone(), context)?; + .to_string(); - if !exists && context.vm.frame().code_block.strict { - return Err(JsNativeError::reference() - .with_message(format!( - "assignment to undeclared variable {}", - key.to_std_string_escaped() - )) - .into()); - } + return Err(JsNativeError::reference() + .with_message(format!( + "cannot assign to uninitialized global property `{key}`" + )) + .into()); + } - context.global_object().set( - key, - value, - context.vm.frame().code_block.strict, - context, - )?; + if !binding_locator.is_global() { + let key = context + .interner() + .resolve_expect(binding_locator.name().sym()) + .to_string(); + + return Err(JsNativeError::reference() + .with_message(format!("cannot assign to uninitialized binding `{key}`")) + .into()); } - } else if !context.put_value_if_initialized( - binding_locator.environment_index(), - binding_locator.binding_index(), - binding_locator.name(), - value, - context.vm.frame().code_block.strict, - )? { - return Err(JsNativeError::reference() - .with_message(format!( - "cannot access '{}' before initialization", - context - .interner() - .resolve_expect(binding_locator.name().sym()) - )) - .into()); } + + context.set_binding(binding_locator, value, context.vm.frame().code_block.strict)?; + Ok(CompletionType::Normal) } }