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

boa/src/environment/declarative_environment_record.rs

@@ -184,8 +184,8 @@ impl EnvironmentRecordTrait for DeclarativeEnvironmentRecord {
         Value::undefined()
     }
 
-    fn get_outer_environment(&self) -> Option<Environment> {
-        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) {

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<Environment>;
+    fn get_outer_environment_ref(&self) -> Option<&Environment>;
+    fn get_outer_environment(&self) -> Option<Environment> {
+        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<Value>;
+
+    /// Return the `this` binding from the environment or try to get it from outer environments
+    fn recursive_get_this_binding(&self) -> Result<Value, ErrorKind> {
+        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<Value, ErrorKind> {
+        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
+                ))),
+            }
+        }
+    }
 }

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 {
 /// <https://tc39.es/ecma262/#table-16>
 #[derive(Debug, Trace, Finalize, Clone)]
 pub struct FunctionEnvironmentRecord {
-    pub env_rec: FxHashMap<String, DeclarativeEnvironmentRecordBinding>,
+    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<Environment>,
 }
 
 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<Value, ErrorKind> {
-        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<Value, ErrorKind> {
-        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<Value, ErrorKind> {
+        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,33 +156,41 @@ 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<Environment> {
-        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 {
         EnvironmentType::Function
     }
 
     fn get_global_object(&self) -> Option<Value> {
-        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)
     }
 }