Functions seem to have access to their caller's scope

[0x7D2B]

When calling a function, it appears to be able to access variables defined in the context it was called in, even if it's not supposed to be able to.

To Reproduce

function f1() {
    return a
}
function f2() {
    let a = 1
    return f1()
}

f2()

f2() appears to return 1 instead of raising a ReferenceError.

Expected behavior

ReferenceError is raised

Build environment (please complete the following information):

• OS: Linux
• Target triple: aarch64-unknown-linux-gnu
• Rustc version: rustc 1.47.0 (18bf6b4f0 2020-10-07), rustc 1.48.0 (7eac88abb 2020-11-16)

I also observed this issue in Boa Playground.

[RageKnify]

I think you're mixing 2 different bugs together, the expeted behaviour would be for a ReferenceError to be thrown when executing f1.
f2 returning 1 is caused by #672 and is a separate issue.
As an example, executing the example you provided in a Chrome console results in:

VM32:2 Uncaught ReferenceError: a is not defined
    at f1 (<anonymous>:2:5)
    at f2 (<anonymous>:6:5)
    at <anonymous>:1:1

[0x7D2B]

Hmm, maybe I had an a declared somewhere earlier while testing in the browser which is why it was returning undefined. I updated the test case to avoid #672 but it still returns 1 instead of raising ReferenceError.

[0x7D2B]

I started digging deeper into the problem and I think the root cause is how lexical environments are implemented. When function environment records are created, they seem to be using the right outer environment, but when they're pushed to the LexicalEnvironment stack, that environment gets overwritten. Then, even if that's fixed, the issue will still persist because of the way the environment stack is used in the rest of the lexical environment implementation, for example in get_binding_value. Because of this, the outer environment for function calls will always end up being the environment the function was called in, rather than where it was declared.

Here's a proof of concept (so excuse the excessive cloning) that fixes the first problem and changes get_binding_value to traverse outer environments rather than using the stack.

--- a/boa/src/environment/lexical_environment.rs
+++ b/boa/src/environment/lexical_environment.rs
@@ -94,7 +94,7 @@ 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);
+        //env.borrow_mut().set_outer_environment(current_env);
         self.environment_stack.push_back(env);
     }

@@ -239,15 +239,21 @@ impl LexicalEnvironment {
     }

     pub fn get_binding_value(&self, name: &str) -> Result<Value, ErrorKind> {
-        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
-                )))
-            })
+        println!("!! {}", name);
+        let mut env: Environment = self.get_current_environment_ref().clone();
+        while !env.borrow().has_binding(name) {
+            println!("env: {:?}", env.borrow().get_environment_type());
+            env = env
+                .clone()
+                .borrow()
+                .get_outer_environment()
+                .ok_or_else(|| {
+                    ErrorKind::new_reference_error(format!("{} is not defined", name))
+                })?;
+        }
+        println!("last env: {:?}", env.borrow().get_environment_type());
+
+        return env.clone().borrow().get_binding_value(name, false);
     }

With this change, the snippet in my initial report starts to correctly raise a ReferenceError.

[0x7D2B]

Alternate implementation which replaces the environments iterator to traverse over the current environment's ancestors:

diff --git a/boa/src/environment/lexical_environment.rs b/boa/src/environment/lexical_environment.rs
index 8bdd9cd3f..46fdb8ae5 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,8 +100,10 @@ impl LexicalEnvironment {
         self.environment_stack.pop_back()
     }
 
-    pub fn environments(&self) -> impl Iterator<Item = &Environment> {
-        self.environment_stack.iter().rev()
+    fn environments(&self) -> impl Iterator<Item = Environment> {
+        std::iter::successors(Some(self.get_current_environment_ref().clone()), |env| {
+            env.borrow().get_outer_environment()
+        })
     }
 
     pub fn get_global_object(&self) -> Option<Value> {
@@ -134,17 +134,15 @@ impl LexicalEnvironment {
                 .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()
+                self.environments()
                     .find(|env| {
                         matches!(
                             env.borrow().get_environment_type(),
                             EnvironmentType::Function | EnvironmentType::Global
                         )
                     })
-                    .expect("No function or global environment");
-
-                env.borrow_mut()
+                    .expect("No function or global environment")
+                    .borrow_mut()
                     .create_mutable_binding(name, deletion, false)
             }
         }
@@ -163,17 +161,16 @@ impl LexicalEnvironment {
                 .create_immutable_binding(name, deletion),
             VariableScope::Function => {
                 // Find the first function or global environment (from the top of the stack)
-                let env = self
-                    .environments()
+                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)
+                    .expect("No function or global environment")
+                    .borrow_mut()
+                    .create_immutable_binding(name, deletion)
             }
         }
     }
@@ -189,15 +186,17 @@ impl LexicalEnvironment {
             .environments()
             .find(|env| env.borrow().has_binding(name));
 
-        let env = if let Some(env) = env {
+        if let Some(ref 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)
+        }
+        .borrow_mut()
+        .set_mutable_binding(name, value, strict)?;
+        Ok(())
     }
 
     pub fn initialize_binding(&mut self, name: &str, value: Value) -> Result<(), ErrorKind> {
@@ -206,15 +205,17 @@ impl LexicalEnvironment {
             .environments()
             .find(|env| env.borrow().has_binding(name));
 
-        let env = if let Some(env) = env {
+        if let Some(ref 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)
+        }
+        .borrow_mut()
+        .initialize_binding(name, value)?;
+        Ok(())
     }
 
     /// get_current_environment_ref is used when you only need to borrow the environment

Not a fan of doing it this way though. I think I want to modify EnvironmentRecordTrait to add recursive implementations of most LexicalEnvironment methods to avoid cloning altogether.