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'");
}
