diff options
6 files changed, 130 insertions, 8 deletions
diff --git a/site/docs/skylark/concepts.md b/site/docs/skylark/concepts.md index 703fac1029..dcaebc9012 100644 --- a/site/docs/skylark/concepts.md +++ b/site/docs/skylark/concepts.md @@ -183,6 +183,43 @@ that global values cannot be redefined. * 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 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); + } } |