Introduce --incompatible_comprehension_variables_do_not_leak

When the flag is activated, variables in list comprehensions do not leak anymore.
Even if a variable was defined before the loop, it's not accessible after the loop.
This change allows us to detect any breakage and ensures that no user is accessing
loop variables after the loop.

This will make possible for us to change the behavior and follow Python 3 semantics
in the future.

RELNOTES: None.
PiperOrigin-RevId: 158895514

5 files changed, 93 insertions, 8 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
index c3dd10bd8c..3ec6cd5c1b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
@@ -253,14 +253,40 @@ public abstract class AbstractComprehension extends Expression {
   Object doEval(Environment env) throws EvalException, InterruptedException {
     OutputCollector collector = createCollector(env);
     evalStep(env, collector, 0);
-    return collector.getResult(env);
+    Object result = collector.getResult(env);
+
+    if (!env.getSemantics().incompatibleComprehensionVariablesDoNotLeak) {
+      return result;
+    }
+
+    // Undefine loop variables (remove them from the environment).
+    // This code is useful for the transition, to make sure no one relies on the old behavior
+    // (where loop variables were leaking).
+    // TODO(laurentlb): Instead of removing variables, we should create them in a nested scope.
+    for (Clause clause : clauses) {
+      // Check if a loop variable conflicts with another local variable.
+      LValue lvalue = clause.getLValue();
+      if (lvalue != null) {
+        for (String name : lvalue.boundNames()) {
+          env.removeLocalBinding(name);
+        }
+      }
+    }
+    return result;
   }
 
   @Override
-  void validate(ValidationEnvironment env) throws EvalException {
+  void validate(ValidationEnvironment parentEnv) throws EvalException {
+    // Create a new scope so that loop variables do not leak outside the comprehension.
+    ValidationEnvironment env =
+        parentEnv.getSemantics().incompatibleComprehensionVariablesDoNotLeak
+            ? new ValidationEnvironment(parentEnv)
+            : parentEnv;
+
     for (Clause clause : clauses) {
       clause.validate(env, getLocation());
     }
+
     // Clauses have to be validated before expressions in order to introduce the variable names.
     for (Expression expr : outputExpressions) {
       expr.validate(env);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index 1dc11eb248..dcfefbd103 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -242,6 +242,15 @@ public final class Environment implements Freezable {
       bindings.put(varname, value);
     }
 
+    /**
+     * TODO(laurentlb): Remove this method when possible. It should probably not
+     * be part of the public interface.
+     */
+    void remove(Environment env, String varname) throws MutabilityException {
+      Mutability.checkMutable(this, env);
+      bindings.remove(varname);
+    }
+
     @Override
     public String toString() {
       return String.format("<Frame%s>", mutability());
@@ -696,6 +705,14 @@ public final class Environment implements Freezable {
     return this;
   }
 
+  /** Remove variable from local bindings. */
+  void removeLocalBinding(String varname) {
+    try {
+      currentFrame().remove(this, varname);
+    } catch (MutabilityException e) {
+      throw new AssertionError(e);
+    }
+  }
 
   /**
    * Modifies a binding in the current Frame of this Environment, as would an
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
index 6ce4801f8a..3fd24c45fd 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
@@ -96,4 +96,16 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
             + "(outside a function definition)"
   )
   public boolean incompatibleDisallowToplevelIfStatement;
+
+  @Option(
+    name = "incompatible_comprehension_variables_do_not_leak",
+    defaultValue = "false",
+    category = "incompatible changes",
+    help =
+        "If set to true, loop variables in a comprehension shadow any existing variable by "
+            + "the same name. If the existing variable was declared in the same scope that "
+            + "contains the comprehension, then it also becomes inaccessible after the "
+            + " comprehension executes."
+  )
+  public boolean incompatibleComprehensionVariablesDoNotLeak;
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
index f06c15fa6c..cba8c34884 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -69,6 +69,10 @@ public final class ValidationEnvironment {
     return parent == null;
   }
 
+  SkylarkSemanticsOptions getSemantics() {
+    return semantics;
+  }
+
   /** Declare a variable and add it to the environment. */
   void declare(String varname, Location location) throws EvalException {
     checkReadonly(varname, location);
@@ -92,10 +96,10 @@ public final class ValidationEnvironment {
     }
   }
 
-  /** Returns true if the symbol exists in the validation environment. */
+  /** Returns true if the symbol exists in the validation environment (or a parent). */
   boolean hasSymbolInEnvironment(String varname) {
     return variables.contains(varname)
-        || (parent != null && topLevel().variables.contains(varname));
+        || (parent != null && parent.hasSymbolInEnvironment(varname));
   }
 
   /** Returns the set of all accessible symbols (both local and global) */
@@ -108,10 +112,6 @@ public final class ValidationEnvironment {
     return all;
   }
 
-  private ValidationEnvironment topLevel() {
-    return Preconditions.checkNotNull(parent == null ? this : parent);
-  }
-
   /**
    * Starts a session with temporarily disabled readonly checking for variables between branches.
    * This is useful to validate control flows like if-else when we know that certain parts of the
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index d00c1bd50a..81fa580ecb 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -1490,4 +1490,34 @@ public class SkylarkEvaluationTest extends EvaluationTest {
             // TODO(bazel-team): This should probably match the error above better.
             "struct has no method 'nonexistent_method'", "v = val.nonexistent_method()");
   }
+
+  @Test
+  public void testListComprehensionsDoNotLeakVariables() throws Exception {
+    env =
+        newEnvironmentWithSkylarkOptions("--incompatible_comprehension_variables_do_not_leak=true");
+    checkEvalErrorContains(
+        "name 'a' is not defined",
+        "def foo():",
+        "  a = 10",
+        "  b = [a for a in range(3)]",
+        "  return a",
+        "x = foo()");
+  }
+
+  @Test
+  public void testListComprehensionsShadowGlobalVariable() throws Exception {
+    env =
+        newEnvironmentWithSkylarkOptions("--incompatible_comprehension_variables_do_not_leak=true");
+    eval("a = 18", "def foo():", "  b = [a for a in range(3)]", "  return a", "x = foo()");
+    assertThat(lookup("x")).isEqualTo(18);
+  }
+
+  @Test
+  public void testListComprehensionsLeakVariables() throws Exception {
+    env =
+        newEnvironmentWithSkylarkOptions(
+            "--incompatible_comprehension_variables_do_not_leak=false");
+    eval("def foo():", "  a = 10", "  b = [a for a in range(3)]", "  return a", "x = foo()");
+    assertThat(lookup("x")).isEqualTo(2);
+  }
 }