Don't panic when function parameters share names (#1017)

* Don't panic when function parameters share names * rustfmt
boa-dev · Jan 2, 2021 · 90a8587 · 90a8587
1 parent e0a135e
commit 90a8587
Show file tree

Hide file tree

Showing 9 changed files with 64 additions and 28 deletions.
diff --git a/boa/src/builtins/function/mod.rs b/boa/src/builtins/function/mod.rs
@@ -124,7 +124,8 @@ impl Function {
         // Create binding
         local_env
             .borrow_mut()
-            .create_mutable_binding(param.name().to_owned(), false)
+            // Function parameters can share names in JavaScript...
+            .create_mutable_binding(param.name().to_owned(), false, true)
             .expect("Failed to create binding for rest param");
 
         // Set Binding to value
@@ -144,7 +145,7 @@ impl Function {
         // Create binding
         local_env
             .borrow_mut()
-            .create_mutable_binding(param.name().to_owned(), false)
+            .create_mutable_binding(param.name().to_owned(), false, true)
             .expect("Failed to create binding");
 
         // Set Binding to value

diff --git a/boa/src/environment/declarative_environment_record.rs b/boa/src/environment/declarative_environment_record.rs
@@ -42,12 +42,19 @@ impl EnvironmentRecordTrait for DeclarativeEnvironmentRecord {
         self.env_rec.contains_key(name)
     }
 
-    fn create_mutable_binding(&mut self, name: String, deletion: bool) -> Result<(), ErrorKind> {
-        assert!(
-            !self.env_rec.contains_key(&name),
-            "Identifier {} has already been declared",
-            name
-        );
+    fn create_mutable_binding(
+        &mut self,
+        name: String,
+        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,
@@ -105,7 +112,7 @@ impl EnvironmentRecordTrait for DeclarativeEnvironmentRecord {
                 )));
             }
 
-            self.create_mutable_binding(name.to_owned(), true)?;
+            self.create_mutable_binding(name.to_owned(), true, false)?;
             self.initialize_binding(name, value)?;
             return Ok(());
         }

diff --git a/boa/src/environment/environment_record_trait.rs b/boa/src/environment/environment_record_trait.rs
@@ -26,7 +26,17 @@ pub trait EnvironmentRecordTrait: Debug + Trace + Finalize {
 
     /// Create a new but uninitialized mutable binding in an Environment Record. The String value N is the text of the bound name.
     /// If the Boolean argument deletion is true the binding may be subsequently deleted.
-    fn create_mutable_binding(&mut self, name: String, deletion: bool) -> Result<(), ErrorKind>;
+    ///
+    /// * `allow_name_reuse` - specifies whether or not reusing binding names is allowed.
+    ///
+    /// Most variable names cannot be reused, but functions in JavaScript are allowed to have multiple
+    /// paraments with the same name.
+    fn create_mutable_binding(
+        &mut self,
+        name: String,
+        deletion: bool,
+        allow_name_reuse: bool,
+    ) -> Result<(), ErrorKind>;
 
     /// Create a new but uninitialized immutable binding in an Environment Record.
     /// The String value N is the text of the bound name.

diff --git a/boa/src/environment/function_environment_record.rs b/boa/src/environment/function_environment_record.rs
@@ -98,12 +98,19 @@ impl EnvironmentRecordTrait for FunctionEnvironmentRecord {
         self.env_rec.contains_key(name)
     }
 
-    fn create_mutable_binding(&mut self, name: String, deletion: bool) -> Result<(), ErrorKind> {
-        assert!(
-            !self.env_rec.contains_key(&name),
-            "Identifier {} has already been declared",
-            name
-        );
+    fn create_mutable_binding(
+        &mut self,
+        name: String,
+        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,
@@ -174,7 +181,7 @@ impl EnvironmentRecordTrait for FunctionEnvironmentRecord {
                 )));
             }
 
-            self.create_mutable_binding(name.to_owned(), true)?;
+            self.create_mutable_binding(name.to_owned(), true, false)?;
             self.initialize_binding(name, value)?;
             return Ok(());
         }

diff --git a/boa/src/environment/global_environment_record.rs b/boa/src/environment/global_environment_record.rs
@@ -86,7 +86,7 @@ impl GlobalEnvironmentRecord {
         let has_property = global_object.has_field(name.as_str());
         let extensible = global_object.is_extensible();
         if !has_property && extensible {
-            obj_rec.create_mutable_binding(name.clone(), deletion)?;
+            obj_rec.create_mutable_binding(name.clone(), deletion, false)?;
             obj_rec.initialize_binding(&name, Value::undefined())?;
         }
 
@@ -131,16 +131,21 @@ impl EnvironmentRecordTrait for GlobalEnvironmentRecord {
         self.object_record.has_binding(name)
     }
 
-    fn create_mutable_binding(&mut self, name: String, deletion: bool) -> Result<(), ErrorKind> {
-        if self.declarative_record.has_binding(&name) {
+    fn create_mutable_binding(
+        &mut self,
+        name: String,
+        deletion: bool,
+        allow_name_reuse: bool,
+    ) -> Result<(), ErrorKind> {
+        if !allow_name_reuse && self.declarative_record.has_binding(&name) {
             return Err(ErrorKind::new_type_error(format!(
                 "Binding already exists for {}",
                 name
             )));
         }
 
         self.declarative_record
-            .create_mutable_binding(name, deletion)
+            .create_mutable_binding(name, deletion, allow_name_reuse)
     }
 
     fn create_immutable_binding(&mut self, name: String, strict: bool) -> Result<(), ErrorKind> {

diff --git a/boa/src/environment/lexical_environment.rs b/boa/src/environment/lexical_environment.rs
@@ -131,7 +131,7 @@ impl LexicalEnvironment {
             VariableScope::Block => self
                 .get_current_environment()
                 .borrow_mut()
-                .create_mutable_binding(name, deletion),
+                .create_mutable_binding(name, deletion, false),
             VariableScope::Function => {
                 // Find the first function or global environment (from the top of the stack)
                 let env = self
@@ -144,7 +144,8 @@ impl LexicalEnvironment {
                     })
                     .expect("No function or global environment");
 
-                env.borrow_mut().create_mutable_binding(name, deletion)
+                env.borrow_mut()
+                    .create_mutable_binding(name, deletion, false)
             }
         }
     }

diff --git a/boa/src/environment/object_environment_record.rs b/boa/src/environment/object_environment_record.rs
@@ -37,7 +37,12 @@ impl EnvironmentRecordTrait for ObjectEnvironmentRecord {
         }
     }
 
-    fn create_mutable_binding(&mut self, name: String, deletion: bool) -> Result<(), ErrorKind> {
+    fn create_mutable_binding(
+        &mut self,
+        name: String,
+        deletion: bool,
+        _allow_name_reuse: bool,
+    ) -> Result<(), ErrorKind> {
         // TODO: could save time here and not bother generating a new undefined object,
         // only for it to be replace with the real value later. We could just add the name to a Vector instead
         let bindings = &mut self.bindings;

diff --git a/boa/src/gc.rs b/boa/src/gc.rs
@@ -5,7 +5,7 @@
 #![allow(clippy::unsafe_removed_from_name)]
 
 pub use crate::object::GcObject;
-pub use ::gc::{
+pub use gc::{
     custom_trace, force_collect, unsafe_empty_trace as empty_trace, Finalize, GcCellRef as Ref,
     GcCellRefMut as RefMut, Trace,
 };
diff --git a/boa/src/object/gcobject.rs b/boa/src/object/gcobject.rs
@@ -128,7 +128,7 @@ impl GcObject {
                         environment,
                         flags,
                     } => {
-                        // Create a new Function environment who's parent is set to the scope of the function declaration (self.environment)
+                        // Create a new Function environment whose parent is set to the scope of the function declaration (self.environment)
                         // <https://tc39.es/ecma262/#sec-prepareforordinarycall>
                         let local_env = new_function_environment(
                             this_function_object,
@@ -162,7 +162,7 @@ impl GcObject {
                         let arguments_obj = create_unmapped_arguments_object(args);
                         local_env
                             .borrow_mut()
-                            .create_mutable_binding("arguments".to_string(), false)
+                            .create_mutable_binding("arguments".to_string(), false, false)
                             .map_err(|e| e.to_error(context))?;
                         local_env
                             .borrow_mut()
@@ -259,7 +259,7 @@ impl GcObject {
                         let arguments_obj = create_unmapped_arguments_object(args);
                         local_env
                             .borrow_mut()
-                            .create_mutable_binding("arguments".to_string(), false)
+                            .create_mutable_binding("arguments".to_string(), false, false)
                             .map_err(|e| e.to_error(context))?;
                         local_env
                             .borrow_mut()