From 57bd79ae242630ab72c313c84416f156c13e8c71 Mon Sep 17 00:00:00 2001 From: 0x7D2B <72297086+0x7D2B@users.noreply.github.com> Date: Mon, 5 Apr 2021 17:08:54 +0000 Subject: [PATCH] Rework environment records (#1156) * Add test for function scope * Make the environment iterator follow outer environments * Make FunctionEnvironmentRecord reuse code * Refactor environment records * Override default methods instead of checking environment types * Fix various environment cleanup issues * Fix lint issues --- .../declarative_environment_record.rs | 4 +- .../environment/environment_record_trait.rs | 105 +++++++++- .../function_environment_record.rs | 183 +++++------------- .../environment/global_environment_record.rs | 45 ++++- boa/src/environment/lexical_environment.rs | 140 +++++--------- .../environment/object_environment_record.rs | 7 +- boa/src/object/gcobject.rs | 9 +- boa/src/syntax/ast/node/block/mod.rs | 7 +- .../ast/node/iteration/for_in_loop/mod.rs | 1 + .../ast/node/iteration/for_of_loop/mod.rs | 1 + 10 files changed, 259 insertions(+), 243 deletions(-) diff --git a/boa/src/environment/declarative_environment_record.rs b/boa/src/environment/declarative_environment_record.rs index 7d585b739a4..0ba2e4f6153 100644 --- a/boa/src/environment/declarative_environment_record.rs +++ b/boa/src/environment/declarative_environment_record.rs @@ -184,8 +184,8 @@ impl EnvironmentRecordTrait for DeclarativeEnvironmentRecord { Value::undefined() } - fn get_outer_environment(&self) -> Option { - self.outer_env.as_ref().cloned() + fn get_outer_environment_ref(&self) -> Option<&Environment> { + self.outer_env.as_ref() } fn set_outer_environment(&mut self, env: Environment) { diff --git a/boa/src/environment/environment_record_trait.rs b/boa/src/environment/environment_record_trait.rs index 1cf0243121b..7cc0439a0a8 100644 --- a/boa/src/environment/environment_record_trait.rs +++ b/boa/src/environment/environment_record_trait.rs @@ -9,6 +9,7 @@ //! There are 5 Environment record kinds. They all have methods in common, these are implemented as a the `EnvironmentRecordTrait` //! use super::ErrorKind; +use crate::environment::lexical_environment::VariableScope; use crate::{ environment::lexical_environment::{Environment, EnvironmentType}, gc::{Finalize, Trace}, @@ -88,7 +89,10 @@ pub trait EnvironmentRecordTrait: Debug + Trace + Finalize { fn with_base_object(&self) -> Value; /// Get the next environment up - fn get_outer_environment(&self) -> Option; + fn get_outer_environment_ref(&self) -> Option<&Environment>; + fn get_outer_environment(&self) -> Option { + self.get_outer_environment_ref().cloned() + } /// Set the next environment up fn set_outer_environment(&mut self, env: Environment); @@ -98,4 +102,103 @@ pub trait EnvironmentRecordTrait: Debug + Trace + Finalize { /// Fetch global variable fn get_global_object(&self) -> Option; + + /// Return the `this` binding from the environment or try to get it from outer environments + fn recursive_get_this_binding(&self) -> Result { + if self.has_this_binding() { + self.get_this_binding() + } else { + match self.get_outer_environment_ref() { + Some(outer) => outer.borrow().recursive_get_this_binding(), + None => Ok(Value::Undefined), + } + } + } + + /// Create mutable binding while handling outer environments + fn recursive_create_mutable_binding( + &mut self, + name: String, + deletion: bool, + scope: VariableScope, + ) -> Result<(), ErrorKind> { + match scope { + VariableScope::Block => self.create_mutable_binding(name, deletion, false), + VariableScope::Function => self + .get_outer_environment_ref() + .expect("No function or global environment") + .borrow_mut() + .recursive_create_mutable_binding(name, deletion, scope), + } + } + + /// Create immutable binding while handling outer environments + fn recursive_create_immutable_binding( + &mut self, + name: String, + deletion: bool, + scope: VariableScope, + ) -> Result<(), ErrorKind> { + match scope { + VariableScope::Block => self.create_immutable_binding(name, deletion), + VariableScope::Function => self + .get_outer_environment_ref() + .expect("No function or global environment") + .borrow_mut() + .recursive_create_immutable_binding(name, deletion, scope), + } + } + + /// Set mutable binding while handling outer environments + fn recursive_set_mutable_binding( + &mut self, + name: &str, + value: Value, + strict: bool, + ) -> Result<(), ErrorKind> { + if self.has_binding(name) { + self.set_mutable_binding(name, value, strict) + } else { + self.get_outer_environment_ref() + .expect("Environment stack underflow") + .borrow_mut() + .recursive_set_mutable_binding(name, value, strict) + } + } + + /// Initialize binding while handling outer environments + fn recursive_initialize_binding(&mut self, name: &str, value: Value) -> Result<(), ErrorKind> { + if self.has_binding(name) { + self.initialize_binding(name, value) + } else { + self.get_outer_environment_ref() + .expect("Environment stack underflow") + .borrow_mut() + .recursive_initialize_binding(name, value) + } + } + + /// Check if a binding exists in current or any outer environment + fn recursive_has_binding(&self, name: &str) -> bool { + self.has_binding(name) + || match self.get_outer_environment_ref() { + Some(outer) => outer.borrow().recursive_has_binding(name), + None => false, + } + } + + /// Retrieve binding from current or any outer environment + fn recursive_get_binding_value(&self, name: &str) -> Result { + if self.has_binding(name) { + self.get_binding_value(name, false) + } else { + match self.get_outer_environment_ref() { + Some(outer) => outer.borrow().recursive_get_binding_value(name), + None => Err(ErrorKind::new_reference_error(format!( + "{} is not defined", + name + ))), + } + } + } } diff --git a/boa/src/environment/function_environment_record.rs b/boa/src/environment/function_environment_record.rs index 441c15c1844..d0f8228b94e 100644 --- a/boa/src/environment/function_environment_record.rs +++ b/boa/src/environment/function_environment_record.rs @@ -11,15 +11,14 @@ use super::ErrorKind; use crate::{ environment::{ - declarative_environment_record::DeclarativeEnvironmentRecordBinding, + declarative_environment_record::DeclarativeEnvironmentRecord, environment_record_trait::EnvironmentRecordTrait, - lexical_environment::{Environment, EnvironmentType}, + lexical_environment::{Environment, EnvironmentType, VariableScope}, }, gc::{empty_trace, Finalize, Trace}, object::GcObject, Value, }; -use rustc_hash::FxHashMap; /// Different binding status for `this`. /// Usually set on a function environment record @@ -40,7 +39,7 @@ unsafe impl Trace for BindingStatus { /// #[derive(Debug, Trace, Finalize, Clone)] pub struct FunctionEnvironmentRecord { - pub env_rec: FxHashMap, + pub declarative_record: DeclarativeEnvironmentRecord, /// This is the this value used for this invocation of the function. pub this_value: Value, /// If the value is "lexical", this is an ArrowFunction and does not have a local this value. @@ -55,9 +54,6 @@ pub struct FunctionEnvironmentRecord { /// `[[NewTarget]]` is the value of the `[[Construct]]` newTarget parameter. /// Otherwise, its value is undefined. pub new_target: Value, - /// Reference to the outer environment to help with the scope chain - /// Option type is needed as some environments can be created before we know what the outer env is - pub outer_env: Option, } impl FunctionEnvironmentRecord { @@ -95,7 +91,7 @@ impl FunctionEnvironmentRecord { impl EnvironmentRecordTrait for FunctionEnvironmentRecord { fn has_binding(&self, name: &str) -> bool { - self.env_rec.contains_key(name) + self.declarative_record.has_binding(name) } fn create_mutable_binding( @@ -104,136 +100,51 @@ impl EnvironmentRecordTrait for FunctionEnvironmentRecord { deletion: bool, allow_name_reuse: bool, ) -> Result<(), ErrorKind> { - if !allow_name_reuse { - assert!( - !self.env_rec.contains_key(&name), - "Identifier {} has already been declared", - name - ); - } - - self.env_rec.insert( - name, - DeclarativeEnvironmentRecordBinding { - value: None, - can_delete: deletion, - mutable: true, - strict: false, - }, - ); - Ok(()) - } - - fn get_this_binding(&self) -> Result { - match self.this_binding_status { - BindingStatus::Lexical => { - panic!("There is no this for a lexical function record"); - } - BindingStatus::Uninitialized => Err(ErrorKind::new_reference_error( - "Uninitialised binding for this function", - )), - - BindingStatus::Initialized => Ok(self.this_value.clone()), - } + self.declarative_record + .create_mutable_binding(name, deletion, allow_name_reuse) } fn create_immutable_binding(&mut self, name: String, strict: bool) -> Result<(), ErrorKind> { - assert!( - !self.env_rec.contains_key(&name), - "Identifier {} has already been declared", - name - ); - - self.env_rec.insert( - name, - DeclarativeEnvironmentRecordBinding { - value: None, - can_delete: true, - mutable: false, - strict, - }, - ); - Ok(()) + self.declarative_record + .create_immutable_binding(name, strict) } fn initialize_binding(&mut self, name: &str, value: Value) -> Result<(), ErrorKind> { - if let Some(ref mut record) = self.env_rec.get_mut(name) { - if record.value.is_none() { - record.value = Some(value); - return Ok(()); - } - } - panic!("record must have binding for {}", name) + self.declarative_record.initialize_binding(name, value) } - #[allow(clippy::else_if_without_else)] fn set_mutable_binding( &mut self, name: &str, value: Value, - mut strict: bool, + strict: bool, ) -> Result<(), ErrorKind> { - if self.env_rec.get(name).is_none() { - if strict { - return Err(ErrorKind::new_reference_error(format!( - "{} not found", - name - ))); - } - - self.create_mutable_binding(name.to_owned(), true, false)?; - self.initialize_binding(name, value)?; - return Ok(()); - } - - let record: &mut DeclarativeEnvironmentRecordBinding = self.env_rec.get_mut(name).unwrap(); - if record.strict { - strict = true - } - if record.value.is_none() { - return Err(ErrorKind::new_reference_error(format!( - "{} has not been initialized", - name - ))); - } - if record.mutable { - record.value = Some(value); - } else if strict { - return Err(ErrorKind::new_type_error(format!( - "Cannot mutate an immutable binding {}", - name - ))); - } - - Ok(()) + self.declarative_record + .set_mutable_binding(name, value, strict) } fn get_binding_value(&self, name: &str, _strict: bool) -> Result { - if let Some(binding) = self.env_rec.get(name) { - if let Some(ref val) = binding.value { - Ok(val.clone()) - } else { - Err(ErrorKind::new_reference_error(format!( - "{} is an uninitialized binding", - name - ))) - } - } else { - panic!("Cannot get binding value for {}", name); - } + self.declarative_record.get_binding_value(name, _strict) } fn delete_binding(&mut self, name: &str) -> bool { - match self.env_rec.get(name) { - Some(binding) => { - if binding.can_delete { - self.env_rec.remove(name); - true - } else { - false - } + self.declarative_record.delete_binding(name) + } + + fn has_this_binding(&self) -> bool { + !matches!(self.this_binding_status, BindingStatus::Lexical) + } + + fn get_this_binding(&self) -> Result { + match self.this_binding_status { + BindingStatus::Lexical => { + panic!("There is no this for a lexical function record"); } - None => panic!("env_rec has no binding for {}", name), + BindingStatus::Uninitialized => Err(ErrorKind::new_reference_error( + "Uninitialised binding for this function", + )), + + BindingStatus::Initialized => Ok(self.this_value.clone()), } } @@ -245,23 +156,16 @@ impl EnvironmentRecordTrait for FunctionEnvironmentRecord { } } - fn has_this_binding(&self) -> bool { - !matches!(self.this_binding_status, BindingStatus::Lexical) - } - fn with_base_object(&self) -> Value { Value::undefined() } - fn get_outer_environment(&self) -> Option { - match &self.outer_env { - Some(outer) => Some(outer.clone()), - None => None, - } + fn get_outer_environment_ref(&self) -> Option<&Environment> { + self.declarative_record.get_outer_environment_ref() } fn set_outer_environment(&mut self, env: Environment) { - self.outer_env = Some(env); + self.declarative_record.set_outer_environment(env) } fn get_environment_type(&self) -> EnvironmentType { @@ -269,9 +173,24 @@ impl EnvironmentRecordTrait for FunctionEnvironmentRecord { } fn get_global_object(&self) -> Option { - match &self.outer_env { - Some(ref outer) => outer.borrow().get_global_object(), - None => None, - } + self.declarative_record.get_global_object() + } + + fn recursive_create_mutable_binding( + &mut self, + name: String, + deletion: bool, + _scope: VariableScope, + ) -> Result<(), ErrorKind> { + self.create_mutable_binding(name, deletion, false) + } + + fn recursive_create_immutable_binding( + &mut self, + name: String, + deletion: bool, + _scope: VariableScope, + ) -> Result<(), ErrorKind> { + self.create_immutable_binding(name, deletion) } } diff --git a/boa/src/environment/global_environment_record.rs b/boa/src/environment/global_environment_record.rs index e9da576e8b3..d5443d29b74 100644 --- a/boa/src/environment/global_environment_record.rs +++ b/boa/src/environment/global_environment_record.rs @@ -12,7 +12,7 @@ use crate::{ environment::{ declarative_environment_record::DeclarativeEnvironmentRecord, environment_record_trait::EnvironmentRecordTrait, - lexical_environment::{Environment, EnvironmentType}, + lexical_environment::{Environment, EnvironmentType, VariableScope}, object_environment_record::ObjectEnvironmentRecord, }, gc::{Finalize, Trace}, @@ -120,10 +120,6 @@ impl GlobalEnvironmentRecord { } impl EnvironmentRecordTrait for GlobalEnvironmentRecord { - fn get_this_binding(&self) -> Result { - Ok(self.global_this_binding.clone()) - } - fn has_binding(&self, name: &str) -> bool { if self.declarative_record.has_binding(name) { return true; @@ -216,6 +212,10 @@ impl EnvironmentRecordTrait for GlobalEnvironmentRecord { true } + fn get_this_binding(&self) -> Result { + Ok(self.global_this_binding.clone()) + } + fn has_super_binding(&self) -> bool { false } @@ -228,6 +228,10 @@ impl EnvironmentRecordTrait for GlobalEnvironmentRecord { None } + fn get_outer_environment_ref(&self) -> Option<&Environment> { + None + } + fn set_outer_environment(&mut self, _env: Environment) { // TODO: Implement panic!("Not implemented yet") @@ -240,4 +244,35 @@ impl EnvironmentRecordTrait for GlobalEnvironmentRecord { fn get_global_object(&self) -> Option { Some(self.global_this_binding.clone()) } + + fn recursive_create_mutable_binding( + &mut self, + name: String, + deletion: bool, + _scope: VariableScope, + ) -> Result<(), ErrorKind> { + self.create_mutable_binding(name, deletion, false) + } + + fn recursive_create_immutable_binding( + &mut self, + name: String, + deletion: bool, + _scope: VariableScope, + ) -> Result<(), ErrorKind> { + self.create_immutable_binding(name, deletion) + } + + fn recursive_set_mutable_binding( + &mut self, + name: &str, + value: Value, + strict: bool, + ) -> Result<(), ErrorKind> { + self.set_mutable_binding(name, value, strict) + } + + fn recursive_initialize_binding(&mut self, name: &str, value: Value) -> Result<(), ErrorKind> { + self.initialize_binding(name, value) + } } diff --git a/boa/src/environment/lexical_environment.rs b/boa/src/environment/lexical_environment.rs index 07427ba9767..cd758ac5b66 100644 --- a/boa/src/environment/lexical_environment.rs +++ b/boa/src/environment/lexical_environment.rs @@ -93,8 +93,6 @@ impl LexicalEnvironment { } pub fn push(&mut self, env: Environment) { - let current_env: Environment = self.get_current_environment().clone(); - env.borrow_mut().set_outer_environment(current_env); self.environment_stack.push_back(env); } @@ -102,23 +100,16 @@ impl LexicalEnvironment { self.environment_stack.pop_back() } - pub fn environments(&self) -> impl Iterator { - self.environment_stack.iter().rev() - } - pub fn get_global_object(&self) -> Option { - self.environment_stack - .get(0) - .expect("") + self.get_current_environment_ref() .borrow() .get_global_object() } pub fn get_this_binding(&self) -> Result { - self.environments() - .find(|env| env.borrow().has_this_binding()) - .map(|env| env.borrow().get_this_binding()) - .unwrap_or_else(|| Ok(Value::Undefined)) + self.get_current_environment_ref() + .borrow() + .recursive_get_this_binding() } pub fn create_mutable_binding( @@ -127,27 +118,9 @@ impl LexicalEnvironment { deletion: bool, scope: VariableScope, ) -> Result<(), ErrorKind> { - match scope { - VariableScope::Block => self - .get_current_environment() - .borrow_mut() - .create_mutable_binding(name, deletion, false), - VariableScope::Function => { - // Find the first function or global environment (from the top of the stack) - let env = self - .environments() - .find(|env| { - matches!( - env.borrow().get_environment_type(), - EnvironmentType::Function | EnvironmentType::Global - ) - }) - .expect("No function or global environment"); - - env.borrow_mut() - .create_mutable_binding(name, deletion, false) - } - } + self.get_current_environment() + .borrow_mut() + .recursive_create_mutable_binding(name, deletion, scope) } pub fn create_immutable_binding( @@ -156,26 +129,9 @@ impl LexicalEnvironment { deletion: bool, scope: VariableScope, ) -> Result<(), ErrorKind> { - match scope { - VariableScope::Block => self - .get_current_environment() - .borrow_mut() - .create_immutable_binding(name, deletion), - VariableScope::Function => { - // Find the first function or global environment (from the top of the stack) - let env = self - .environments() - .find(|env| { - matches!( - env.borrow().get_environment_type(), - EnvironmentType::Function | EnvironmentType::Global - ) - }) - .expect("No function or global environment"); - - env.borrow_mut().create_immutable_binding(name, deletion) - } - } + self.get_current_environment() + .borrow_mut() + .recursive_create_immutable_binding(name, deletion, scope) } pub fn set_mutable_binding( @@ -184,37 +140,15 @@ impl LexicalEnvironment { value: Value, strict: bool, ) -> Result<(), ErrorKind> { - // Find the first environment which has the given binding - let env = self - .environments() - .find(|env| env.borrow().has_binding(name)); - - let env = if let Some(env) = env { - env - } else { - // global_env doesn't need has_binding to be satisfied in non strict mode - self.environment_stack - .get(0) - .expect("Environment stack underflow") - }; - env.borrow_mut().set_mutable_binding(name, value, strict) + self.get_current_environment() + .borrow_mut() + .recursive_set_mutable_binding(name, value, strict) } pub fn initialize_binding(&mut self, name: &str, value: Value) -> Result<(), ErrorKind> { - // Find the first environment which has the given binding - let env = self - .environments() - .find(|env| env.borrow().has_binding(name)); - - let env = if let Some(env) = env { - env - } else { - // global_env doesn't need has_binding to be satisfied in non strict mode - self.environment_stack - .get(0) - .expect("Environment stack underflow") - }; - env.borrow_mut().initialize_binding(name, value) + self.get_current_environment() + .borrow_mut() + .recursive_initialize_binding(name, value) } /// get_current_environment_ref is used when you only need to borrow the environment @@ -234,20 +168,15 @@ impl LexicalEnvironment { } pub fn has_binding(&self, name: &str) -> bool { - self.environments() - .any(|env| env.borrow().has_binding(name)) + self.get_current_environment_ref() + .borrow() + .recursive_has_binding(name) } pub fn get_binding_value(&self, name: &str) -> Result { - self.environments() - .find(|env| env.borrow().has_binding(name)) - .map(|env| env.borrow().get_binding_value(name, false)) - .unwrap_or_else(|| { - Err(ErrorKind::new_reference_error(format!( - "{} is not defined", - name - ))) - }) + self.get_current_environment_ref() + .borrow() + .recursive_get_binding_value(name) } } @@ -269,12 +198,14 @@ pub fn new_function_environment( new_target: Value, ) -> Environment { let mut func_env = FunctionEnvironmentRecord { - env_rec: FxHashMap::default(), + declarative_record: DeclarativeEnvironmentRecord { + env_rec: FxHashMap::default(), + outer_env: outer, // this will come from Environment set as a private property of F - https://tc39.es/ecma262/#sec-ecmascript-function-objects + }, function: f, this_binding_status: binding_status, home_object: Value::undefined(), new_target, - outer_env: outer, // this will come from Environment set as a private property of F - https://tc39.es/ecma262/#sec-ecmascript-function-objects this_value: Value::undefined(), }; // If a `this` value has been passed, bind it to the environment @@ -372,6 +303,25 @@ mod tests { assert_eq!(&exec(scenario), "true"); } + #[test] + fn functions_use_declaration_scope() { + let scenario = r#" + function foo() { + try { + bar; + } catch (err) { + return err.message; + } + } + { + let bar = "bar"; + foo(); + } + "#; + + assert_eq!(&exec(scenario), "\"bar is not defined\""); + } + #[test] fn set_outer_var_in_blockscope() { let scenario = r#" diff --git a/boa/src/environment/object_environment_record.rs b/boa/src/environment/object_environment_record.rs index 1c5639fc737..b2e83ee0318 100644 --- a/boa/src/environment/object_environment_record.rs +++ b/boa/src/environment/object_environment_record.rs @@ -127,11 +127,8 @@ impl EnvironmentRecordTrait for ObjectEnvironmentRecord { Value::undefined() } - fn get_outer_environment(&self) -> Option { - match &self.outer_env { - Some(outer) => Some(outer.clone()), - None => None, - } + fn get_outer_environment_ref(&self) -> Option<&Environment> { + self.outer_env.as_ref() } fn set_outer_environment(&mut self, env: Environment) { diff --git a/boa/src/object/gcobject.rs b/boa/src/object/gcobject.rs index 4ce3d705e6f..234bb23d359 100644 --- a/boa/src/object/gcobject.rs +++ b/boa/src/object/gcobject.rs @@ -305,8 +305,13 @@ impl GcObject { let _ = body.run(context); // local_env gets dropped here, its no longer needed - let binding = context.realm_mut().environment.get_this_binding(); - binding.map_err(|e| e.to_error(context)) + let result = context + .realm_mut() + .environment + .get_this_binding() + .map_err(|e| e.to_error(context)); + context.realm_mut().environment.pop(); + result } FunctionBody::BuiltInFunction(_) => unreachable!("Cannot have a function in construct"), } diff --git a/boa/src/syntax/ast/node/block/mod.rs b/boa/src/syntax/ast/node/block/mod.rs index 93bedf48161..c87fbbb35c8 100644 --- a/boa/src/syntax/ast/node/block/mod.rs +++ b/boa/src/syntax/ast/node/block/mod.rs @@ -64,7 +64,12 @@ impl Executable for Block { // The return value is uninitialized, which means it defaults to Value::Undefined let mut obj = Value::default(); for statement in self.items() { - obj = statement.run(context)?; + obj = statement.run(context).map_err(|e| { + // No matter how control leaves the Block the LexicalEnvironment is always + // restored to its former state. + context.realm_mut().environment.pop(); + e + })?; match context.executor().get_current_state() { InterpreterState::Return => { diff --git a/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs b/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs index faf1124be52..22caa67fc45 100644 --- a/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs +++ b/boa/src/syntax/ast/node/iteration/for_in_loop/mod.rs @@ -100,6 +100,7 @@ impl Executable for ForInLoop { } let iterator_result = iterator.next(context)?; if iterator_result.is_done() { + context.realm_mut().environment.pop(); break; } let next_result = iterator_result.value(); diff --git a/boa/src/syntax/ast/node/iteration/for_of_loop/mod.rs b/boa/src/syntax/ast/node/iteration/for_of_loop/mod.rs index f0611c90ea0..eea1aad38bc 100644 --- a/boa/src/syntax/ast/node/iteration/for_of_loop/mod.rs +++ b/boa/src/syntax/ast/node/iteration/for_of_loop/mod.rs @@ -90,6 +90,7 @@ impl Executable for ForOfLoop { } let iterator_result = iterator.next(context)?; if iterator_result.is_done() { + context.realm_mut().environment.pop(); break; } let next_result = iterator_result.value();