Allow shadowing of builtins in bzl files
This was previously allowed only with --incompatible_static_name_resolution (although it was a backward-compatible change). Removing the code path simplifies the code and makes easier to implement the static name resolution proposal.
The downside is that it will temporarily allow this corner-case:
```
x = len(...) # reference to the builtin
len = ... # shadow the builtin with a new global
```
RELNOTES: None.
PiperOrigin-RevId: 211987952
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 a8158e3..c32ab06 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
@@ -1112,7 +1112,14 @@
/** Returns the value of a variable defined in the Universe scope (builtins). */
public Object universeLookup(String varname) {
// TODO(laurentlb): look only at globalFrame.parent.
- return globalFrame.get(varname);
+ Object result = globalFrame.get(varname);
+
+ if (result == null) {
+ // TODO(laurentlb): Remove once PACKAGE_NAME and REPOSITOYRY_NAME are removed (they are the
+ // only two user-visible values that use the dynamicFrame).
+ return dynamicLookup(varname);
+ }
+ return result;
}
/** Returns the value of a variable defined with setupDynamic. */
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index d212c67..84de492 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -107,9 +107,12 @@
if (result == null) {
// Since Scope was set, we know that the variable is defined in the scope.
// However, the assignment was not yet executed.
- throw new EvalException(
- getLocation(),
- scope.getQualifier() + " variable '" + name + "' is referenced before assignment.");
+ EvalException e = getSpecialException();
+ throw e != null
+ ? e
+ : new EvalException(
+ getLocation(),
+ scope.getQualifier() + " variable '" + name + "' is referenced before assignment.");
}
return result;
@@ -125,11 +128,8 @@
return Kind.IDENTIFIER;
}
- EvalException createInvalidIdentifierException(Set<String> symbols) {
- if (name.equals("$error$")) {
- return new EvalException(getLocation(), "contains syntax error(s)", true);
- }
-
+ /** Exception to provide a better error message for using PACKAGE_NAME or REPOSITORY_NAME. */
+ private EvalException getSpecialException() {
if (name.equals("PACKAGE_NAME")) {
return new EvalException(
getLocation(),
@@ -148,6 +148,18 @@
+ " You can temporarily allow the old name "
+ "by using --incompatible_package_name_is_a_function=false");
}
+ return null;
+ }
+
+ EvalException createInvalidIdentifierException(Set<String> symbols) {
+ if (name.equals("$error$")) {
+ return new EvalException(getLocation(), "contains syntax error(s)", true);
+ }
+
+ EvalException e = getSpecialException();
+ if (e != null) {
+ return e;
+ }
String suggestion = SpellChecker.didYouMean(name, symbols);
return new EvalException(getLocation(), "name '" + name + "' is not defined" + suggestion);
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 ec7a900..8900e5f 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
@@ -26,17 +26,12 @@
/**
* 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
+ * <p>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 {
@@ -61,7 +56,6 @@
private static class Block {
private final Set<String> variables = new HashSet<>();
- private final Set<String> readOnlyVariables = new HashSet<>();
private final Scope scope;
@Nullable private final Block parent;
@@ -102,18 +96,12 @@
block = new Block(Scope.Universe, null);
Set<String> builtinVariables = env.getVariableNames();
block.variables.addAll(builtinVariables);
- if (!semantics.incompatibleStaticNameResolution()) {
- block.readOnlyVariables.addAll(builtinVariables);
- }
}
/**
* First pass: 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) {
@@ -166,40 +154,22 @@
}
@Override
- public void visit(LoadStatement node) {
- if (semantics.incompatibleStaticNameResolution()) {
- return;
- }
-
- for (Identifier symbol : node.getSymbols()) {
- declare(symbol.getName(), node.getLocation());
- }
- }
-
- @Override
public void visit(Identifier node) {
@Nullable Block b = blockThatDefines(node.getName());
if (b == null) {
throw new ValidationException(node.createInvalidIdentifierException(getAllSymbols()));
}
- if (semantics.incompatibleStaticNameResolution()) {
- // The scoping information is reliable only with the new behavior.
- node.setScope(b.scope);
- }
+ node.setScope(b.scope);
}
private void validateLValue(Location loc, Expression expr) {
- if (expr instanceof Identifier) {
- if (!semantics.incompatibleStaticNameResolution()) {
- declare(((Identifier) expr).getName(), loc);
- }
- } else if (expr instanceof IndexExpression) {
+ if (expr instanceof IndexExpression) {
visit(expr);
} else if (expr instanceof ListLiteral) {
for (Expression e : ((ListLiteral) expr).getElements()) {
validateLValue(loc, e);
}
- } else {
+ } else if (!(expr instanceof Identifier)) {
throw new ValidationException(loc, "cannot assign to '" + expr + "'");
}
}
@@ -244,11 +214,9 @@
@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());
- }
+ for (AbstractComprehension.Clause clause : node.getClauses()) {
+ if (clause.getLValue() != null) {
+ collectDefinitions(clause.getLValue());
}
}
super.visit(node);
@@ -268,9 +236,7 @@
declare(param.getName(), param.getLocation());
}
}
- if (semantics.incompatibleStaticNameResolution()) {
- collectDefinitions(node.getStatements());
- }
+ collectDefinitions(node.getStatements());
visitAll(node.getStatements());
closeBlock();
}
@@ -298,25 +264,13 @@
/** Declare a variable and add it to the environment. */
private void declare(String varname, Location location) {
- boolean readOnlyViolation = false;
- if (block.readOnlyVariables.contains(varname)) {
- readOnlyViolation = true;
- }
- if (block.scope == Scope.Module && block.parent.readOnlyVariables.contains(varname)) {
- // TODO(laurentlb): This behavior is buggy. Symbols in the module scope should shadow symbols
- // from the universe. https://github.com/bazelbuild/bazel/issues/5637
- readOnlyViolation = true;
- }
- if (readOnlyViolation) {
+ if (block.scope == Scope.Module && block.variables.contains(varname)) {
+ // Symbols defined in the module scope cannot be reassigned.
throw new ValidationException(
location,
String.format("Variable %s is read only", varname),
"https://bazel.build/versions/master/docs/skylark/errors/read-only-variable.html");
}
- if (block.scope == Scope.Module) {
- // Symbols defined in the module scope cannot be reassigned.
- block.readOnlyVariables.add(varname);
- }
block.variables.add(varname);
}
@@ -378,20 +332,9 @@
openBlock(Scope.Module);
- 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());
- }
- }
- }
+ // Add each variable defined by statements, not including definitions that appear in
+ // sub-scopes of the given statements (function bodies and comprehensions).
+ collectDefinitions(statements);
// Second pass: ensure that all symbols have been defined.
visitAll(statements);
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 1b53776..bbad014 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
@@ -217,7 +217,7 @@
} catch (EvalExceptionWithStackTrace e) {
assertThat(e)
.hasMessageThat()
- .contains("Variable 'global_var' is referenced before assignment.");
+ .contains("local variable 'global_var' is referenced before assignment.");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index 1ee0dff..c2aa865 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -357,8 +357,15 @@
@Test
public void testListComprehensionDefinitionOrder() throws Exception {
- newTest().testIfErrorContains("name 'y' is not defined",
- "[x for x in (1, 2) if y for y in (3, 4)]");
+ new BuildTest()
+ .testIfErrorContains("name 'y' is not defined", "[x for x in (1, 2) if y for y in (3, 4)]");
+
+ // We provide a better error message when we use the ValidationEnvironment. It should be used
+ // for BUILD files too in the future.
+ new SkylarkTest()
+ .testIfErrorContains(
+ "local variable 'y' is referenced before assignment",
+ "[x for x in (1, 2) if y for y in (3, 4)]");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
index f91e2a8..95fd030 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
@@ -105,7 +105,8 @@
@Test
public void testFunctionDefLocalVariableReferencedBeforeAssignment() throws Exception {
- checkEvalErrorContains("Variable 'a' is referenced before assignment.",
+ checkEvalErrorContains(
+ "local variable 'a' is referenced before assignment.",
"a = 1",
"def func():",
" b = a",
@@ -116,7 +117,8 @@
@Test
public void testFunctionDefLocalVariableReferencedInCallBeforeAssignment() throws Exception {
- checkEvalErrorContains("Variable 'a' is referenced before assignment.",
+ checkEvalErrorContains(
+ "local variable 'a' is referenced before assignment.",
"def dummy(x):",
" pass",
"a = 1",
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 8333245..2f5a19a 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
@@ -1789,15 +1789,17 @@
@Test
public void testFunctionCallBadOrdering() throws Exception {
- new SkylarkTest().testIfErrorContains("name 'foo' is not defined",
- "def func(): return foo() * 2",
- "x = func()",
- "def foo(): return 2");
+ new SkylarkTest()
+ .testIfErrorContains(
+ "global variable 'foo' is referenced before assignment",
+ "def func(): return foo() * 2",
+ "x = func()",
+ "def foo(): return 2");
}
@Test
public void testLocalVariableDefinedBelow() throws Exception {
- new SkylarkTest("--incompatible_static_name_resolution=true")
+ new SkylarkTest()
.setUp(
"def beforeEven(li):", // returns the value before the first even number
" for i in li:",
@@ -1822,10 +1824,11 @@
}
@Test
- public void testLegacyGlobalIsNotInitialized() throws Exception {
+ public void testShadowBuiltin() throws Exception {
+ // TODO(laurentlb): Forbid this.
new SkylarkTest("--incompatible_static_name_resolution=false")
- .setUp("a = len")
- .testIfErrorContains("Variable len is read only", "len = 2");
+ .setUp("x = len('abc')", "len = 2", "y = x + len")
+ .testLookup("y", 5);
}
@Test
@@ -2194,7 +2197,7 @@
@Test
public void testListComprehensionsDoNotLeakVariables() throws Exception {
checkEvalErrorContains(
- "name 'a' is not defined",
+ "local variable 'a' is referenced before assignment",
"def foo():",
" a = 10",
" b = [a for a in range(3)]",
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 403bfa3..24179208 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,7 +93,6 @@
@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");
@@ -103,28 +102,12 @@
}
@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]");
- checkError("name 'a' is not defined", "def f():", " for a in a: pass");
- }
-
- @Test
public void testLocalValidationEnvironmentsAreSeparated() throws Exception {
parse("def func1():", " a = 1", "def func2():", " a = 'abc'\n");
}
@Test
- 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");
}
