Introduce --incompatible_static_name_resolution This implements a part of the name resolution document: https://github.com/bazelbuild/proposals/blob/master/docs/2018-06-18-name-resolution.md When the flag is set: - Builtins may be shadowed (e.g. `len = 2`) on top-level - Symbols may be used before their definition point, e.g. def fct(): return x x = 2 In a followup change, we'll need to update the dynamic Environment too (to have truly lexical binding). https://github.com/bazelbuild/bazel/issues/5637 RELNOTES: None. PiperOrigin-RevId: 210412609
diff --git a/site/docs/skylark/backward-compatibility.md b/site/docs/skylark/backward-compatibility.md index 3e2d509..a030dfe 100644 --- a/site/docs/skylark/backward-compatibility.md +++ b/site/docs/skylark/backward-compatibility.md
@@ -46,6 +46,7 @@ * [New-style JavaInfo constructor](#new-style-java_info) * [Disallow tools in action inputs](#disallow-tools-in-action-inputs) * [Expand directories in Args](#expand-directories-in-args) +* [Static Name Resolution](#static-name-resolution) ### Dictionary concatenation @@ -416,4 +417,18 @@ * Flag: `--incompatible_expand_directories` * Default: `false` + +### Static Name Resolution + +When the flag is set, use a saner way to resolve variables. The previous +behavior was buggy in a number of subtle ways. See [the +proposal](https://github.com/bazelbuild/proposals/blob/master/docs/2018-06-18-name-resolution.md) +for background and examples. + +The proposal is not fully implemented yet. + +* Flag: `--incompatible_static_name_resolution` +* Default: `false` + + !-- Add new options here -->
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java index 076f156..e3f0298 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
@@ -373,6 +373,21 @@ public boolean incompatibleRemoveNativeHttpArchive; @Option( + name = "incompatible_static_name_resolution", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS, + effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = + "If set to true, the interpreter follows the semantics related to name resolution, " + + "scoping, and shadowing, as defined in " + + "https://github.com/bazelbuild/proposals/blob/master/docs/2018-06-18-name-resolution.md") + public boolean incompatibleStaticNameResolution; + + @Option( name = "incompatible_string_is_not_iterable", defaultValue = "false", documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS, @@ -423,6 +438,7 @@ .incompatibleRangeType(incompatibleRangeType) .incompatibleRemoveNativeGitRepository(incompatibleRemoveNativeGitRepository) .incompatibleRemoveNativeHttpArchive(incompatibleRemoveNativeHttpArchive) + .incompatibleStaticNameResolution(incompatibleStaticNameResolution) .incompatibleStringIsNotIterable(incompatibleStringIsNotIterable) .internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary) .build();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java index 187bf0f..2062ae1 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
@@ -87,6 +87,8 @@ public abstract boolean incompatibleRemoveNativeHttpArchive(); + public abstract boolean incompatibleStaticNameResolution(); + public abstract boolean incompatibleStringIsNotIterable(); public abstract boolean internalSkylarkFlagTestCanary(); @@ -129,6 +131,7 @@ .incompatibleRangeType(false) .incompatibleRemoveNativeGitRepository(false) .incompatibleRemoveNativeHttpArchive(false) + .incompatibleStaticNameResolution(false) .incompatibleStringIsNotIterable(false) .internalSkylarkFlagTestCanary(false) .build(); @@ -184,6 +187,8 @@ public abstract Builder incompatibleRemoveNativeHttpArchive(boolean value); + public abstract Builder incompatibleStaticNameResolution(boolean value); + public abstract Builder incompatibleStringIsNotIterable(boolean value); public abstract Builder internalSkylarkFlagTestCanary(boolean value);
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 56eede9..14d55ac 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
@@ -23,7 +23,21 @@ import java.util.Set; import javax.annotation.Nullable; -/** A class for doing static checks on files, before evaluating them. */ +/** + * A class for doing static checks on files, before evaluating them. + * + * <p>The behavior is affected by semantics.incompatibleStaticNameResolution(). When it is set to + * true, we implement the semantics discussed in + * https://github.com/bazelbuild/proposals/blob/master/docs/2018-06-18-name-resolution.md + * + * <p>When a variable is defined, it is visible in the entire block. For example, a global variable + * is visible in the entire file; a variable in a function is visible in the entire function block + * (even on the lines before its first assignment). + * + * <p>The legacy behavior is kept during the transition and will be removed in the future. In the + * legacy behavior, there is no clear separation between the first pass (collect all definitions) + * and the second pass (ensure the symbols can be resolved). + */ public final class ValidationEnvironment extends SyntaxTreeVisitor { private enum Scope { @@ -74,15 +88,78 @@ /** Create a ValidationEnvironment for a given global Environment (containing builtins). */ ValidationEnvironment(Environment env) { Preconditions.checkArgument(env.isGlobal()); + semantics = env.getSemantics(); block = new Block(Scope.Universe, null); Set<String> builtinVariables = env.getVariableNames(); block.variables.addAll(builtinVariables); - block.readOnlyVariables.addAll(builtinVariables); - semantics = env.getSemantics(); + if (!semantics.incompatibleStaticNameResolution()) { + block.readOnlyVariables.addAll(builtinVariables); + } + } + + /** + * Add all definitions to the current block. This is done because symbols are sometimes used + * before their definition point (e.g. a functions are not necessarily declared in order). + * + * <p>The old behavior (when incompatibleStaticNameResolution is false) doesn't have this first + * pass. + */ + private void collectDefinitions(Iterable<Statement> stmts) { + for (Statement stmt : stmts) { + collectDefinitions(stmt); + } + } + + private void collectDefinitions(Statement stmt) { + switch (stmt.kind()) { + case ASSIGNMENT: + collectDefinitions(((AssignmentStatement) stmt).getLValue()); + break; + case AUGMENTED_ASSIGNMENT: + collectDefinitions(((AugmentedAssignmentStatement) stmt).getLValue()); + break; + case IF: + IfStatement ifStmt = (IfStatement) stmt; + for (IfStatement.ConditionalStatements cond : ifStmt.getThenBlocks()) { + collectDefinitions(cond.getStatements()); + } + collectDefinitions(ifStmt.getElseBlock()); + break; + case FOR: + ForStatement forStmt = (ForStatement) stmt; + collectDefinitions(forStmt.getVariable()); + collectDefinitions(forStmt.getBlock()); + break; + case FUNCTION_DEF: + Identifier fctName = ((FunctionDefStatement) stmt).getIdentifier(); + declare(fctName.getName(), fctName.getLocation()); + break; + case LOAD: + for (Identifier id : ((LoadStatement) stmt).getSymbols()) { + declare(id.getName(), id.getLocation()); + } + break; + case CONDITIONAL: + case EXPRESSION: + case FLOW: + case PASS: + case RETURN: + // nothing to declare + } + } + + private void collectDefinitions(LValue left) { + for (Identifier id : left.boundIdentifiers()) { + declare(id.getName(), id.getLocation()); + } } @Override public void visit(LoadStatement node) { + if (semantics.incompatibleStaticNameResolution()) { + return; + } + for (Identifier symbol : node.getSymbols()) { declare(symbol.getName(), node.getLocation()); } @@ -97,7 +174,9 @@ private void validateLValue(Location loc, Expression expr) { if (expr instanceof Identifier) { - declare(((Identifier) expr).getName(), loc); + if (!semantics.incompatibleStaticNameResolution()) { + declare(((Identifier) expr).getName(), loc); + } } else if (expr instanceof IndexExpression) { visit(expr); } else if (expr instanceof ListLiteral) { @@ -149,6 +228,13 @@ @Override public void visit(AbstractComprehension node) { openBlock(Scope.Local); + if (semantics.incompatibleStaticNameResolution()) { + for (AbstractComprehension.Clause clause : node.getClauses()) { + if (clause.getLValue() != null) { + collectDefinitions(clause.getLValue()); + } + } + } super.visit(node); closeBlock(); } @@ -166,6 +252,9 @@ declare(param.getName(), param.getLocation()); } } + if (semantics.incompatibleStaticNameResolution()) { + collectDefinitions(node.getStatements()); + } visitAll(node.getStatements()); closeBlock(); } @@ -176,7 +265,7 @@ throw new ValidationException( node.getLocation(), "if statements are not allowed at the top level. You may move it inside a function " - + "or use an if expression (x if condition else y)."); + + "or use an if expression (x if condition else y)."); } super.visit(node); } @@ -273,17 +362,23 @@ openBlock(Scope.Module); - // Add every function in the environment before validating. This is - // necessary because functions may call other functions defined - // later in the file. - for (Statement statement : statements) { - if (statement instanceof FunctionDefStatement) { - FunctionDefStatement fct = (FunctionDefStatement) statement; - declare(fct.getIdentifier().getName(), fct.getLocation()); + if (semantics.incompatibleStaticNameResolution()) { + // Add each variable defined by statements, not including definitions that appear in + // sub-scopes of the given statements (function bodies and comprehensions). + collectDefinitions(statements); + } else { + // Legacy behavior, to be removed. Add only the functions in the environment before + // validating. + for (Statement statement : statements) { + if (statement instanceof FunctionDefStatement) { + FunctionDefStatement fct = (FunctionDefStatement) statement; + declare(fct.getIdentifier().getName(), fct.getLocation()); + } } } - this.visitAll(statements); + // Second pass: ensure that all symbols have been defined. + visitAll(statements); closeBlock(); }
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index eff7ba3..90588f9 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -144,6 +144,7 @@ "--incompatible_range_type=" + rand.nextBoolean(), "--incompatible_remove_native_git_repository=" + rand.nextBoolean(), "--incompatible_remove_native_http_archive=" + rand.nextBoolean(), + "--incompatible_static_name_resolution=" + rand.nextBoolean(), "--incompatible_string_is_not_iterable=" + rand.nextBoolean(), "--internal_skylark_flag_test_canary=" + rand.nextBoolean()); } @@ -179,6 +180,7 @@ .incompatibleRangeType(rand.nextBoolean()) .incompatibleRemoveNativeGitRepository(rand.nextBoolean()) .incompatibleRemoveNativeHttpArchive(rand.nextBoolean()) + .incompatibleStaticNameResolution(rand.nextBoolean()) .incompatibleStringIsNotIterable(rand.nextBoolean()) .internalSkylarkFlagTestCanary(rand.nextBoolean()) .build();
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java index 73f66fb..f11cfe7 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java
@@ -245,19 +245,17 @@ } @Test - public void testReadOnly() throws Exception { - Environment env = newSkylarkEnvironment() - .setup("special_var", 42) - .update("global_var", 666); + public void testBuiltinsCanBeShadowed() throws Exception { + Environment env = + newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true") + .setup("special_var", 42); + BuildFileAST.eval(env, "special_var = 41"); + assertThat(env.lookup("special_var")).isEqualTo(41); + } - // We don't even get a runtime exception trying to modify these, - // because we get compile-time exceptions even before we reach runtime! - try { - BuildFileAST.eval(env, "special_var = 41"); - throw new AssertionError("failed to fail"); - } catch (EvalException e) { - assertThat(e).hasMessageThat().contains("Variable special_var is read only"); - } + @Test + public void testVariableIsReferencedBeforeAssignment() throws Exception { + Environment env = newSkylarkEnvironment().update("global_var", 666); try { BuildFileAST.eval(env, "def foo(x): x += global_var; global_var = 36; return x", "foo(1)");
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java index 3a003b1..403bfa3 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
@@ -93,6 +93,18 @@ @Test public void testDefinitionByItself() throws Exception { + env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true"); + // Variables are assumed to be statically visible in the block (even if they might not be + // initialized). + parse("a = a"); + parse("a += a"); + parse("[[] for a in a]"); + parse("def f():", " for a in a: pass"); + } + + @Test + public void testDefinitionByItselfLegacy() throws Exception { + env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=false"); checkError("name 'a' is not defined", "a = a"); checkError("name 'a' is not defined", "a += a"); checkError("name 'a' is not defined", "[[] for a in a]"); @@ -105,11 +117,18 @@ } @Test - public void testBuiltinSymbolsAreReadOnly() throws Exception { + public void testBuiltinSymbolsAreReadOnlyLegacy() throws Exception { + env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=false"); checkError("Variable repr is read only", "repr = 1"); } @Test + public void testBuiltinsCanBeShadowed() throws Exception { + env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true"); + parse("repr = 1"); + } + + @Test public void testSkylarkGlobalVariablesAreReadonly() throws Exception { checkError("Variable a is read only", "a = 1", "a = 2"); } @@ -130,6 +149,23 @@ } @Test + public void testGlobalDefinedBelow() throws Exception { + env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true"); + parse("def bar(): return x", "x = 5\n"); + } + + @Test + public void testLocalVariableDefinedBelow() throws Exception { + env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true"); + parse( + "def bar():", + " for i in range(5):", + " if i > 2: return x", + " x = i" // x is visible in the entire function block + ); + } + + @Test public void testFunctionDoesNotExist() { checkError("name 'foo' is not defined", "def bar(): a = foo() + 'a'"); }