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
diff --git a/site/docs/skylark/concepts.md b/site/docs/skylark/concepts.md
index 703fac1..dcaebc9 100644
--- a/site/docs/skylark/concepts.md
+++ b/site/docs/skylark/concepts.md
@@ -183,6 +183,43 @@
* Flag: `--incompatible_disallow_toplevel_if_statement`
* Default: `false`
+### Comprehensions variables
+
+This change makes list and dict comprehensions follow Python 3's semantics
+instead of Python 2's. That is, comprehensions have their own local scopes, and
+variables bound by comprehensions are not accessible in the outer scope.
+
+As a temporary measure to help detect breakage, this change also causes
+variables defined in the immediate outer scope to become inaccessible if they
+are shadowed by any variables in a comprehension. This disallows any uses of the
+variable's name where its meaning would differ under the Python 2 and Python 3
+semantics. Variables above the immediate outer scope are not affected.
+
+``` python
+def fct():
+ x = 10
+ y = [x for x in range(3)]
+ return x
+```
+
+The meaning of this program depends on the flag:
+
+ * Under Skylark without this flag: `x` is 10 before the
+ comprehension and 2 afterwards. (2 is the last value assigned to `x` while
+ evaluating the comprehension.)
+
+ * Under Skylark with this flag: `x` becomes inaccessible after the
+ comprehension, so that `return x` is an error. If we moved the `x = 10` to
+ above the function, so that `x` became a global variable, then no error would
+ be raised, and the returned number would be 10.
+
+In other words, please do not refer to a loop variable outside the list or dict
+comprehension.
+
+* Flag: `--incompatible_comprehension_variables_do_not_leak`
+* Default: `false`
+
+
## Upcoming changes
The following items are upcoming changes.
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 c3dd10b..3ec6cd5 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 @@
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 1dc11eb..dcfefbd 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 @@
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 @@
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 6ce4801..3fd24c4 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 @@
+ "(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 f06c15f..cba8c34 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 @@
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 @@
}
}
- /** 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 @@
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 d00c1bd..81fa580 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 @@
// 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);
+ }
}
