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

src/main/java/com/google/devtools/build/lib/syntax/Environment.java

@@ -1112,7 +1112,14 @@ public Object moduleLookup(String varname) {
  /** 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. */

src/main/java/com/google/devtools/build/lib/syntax/Identifier.java

@@ -107,9 +107,12 @@ Object doEval(Environment env) throws EvalException {
  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 @@ public Kind kind() {
  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 @@ EvalException createInvalidIdentifierException(Set<String> symbols) {
  + " 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);

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 @@ public String getQualifier() {
 
  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 @@ private static class ValidationException extends RuntimeException {
  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) {
@@ -165,41 +153,23 @@ private void collectDefinitions(LValue left) {
  }
  }
 
- @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 @@ public void visit(DotExpression node) {
  @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 @@ public void visit(FunctionDefStatement node) {
  declare(param.getName(), param.getLocation());
  }
  }
- if (semantics.incompatibleStaticNameResolution()) {
- collectDefinitions(node.getStatements());
- }
+ collectDefinitions(node.getStatements());
  visitAll(node.getStatements());
  closeBlock();
  }
@@ -298,25 +264,13 @@ public void visit(AugmentedAssignmentStatement node) {
 
  /** 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 @@ private void validateAst(List<Statement> statements) {
 
  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);

src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java

@@ -217,7 +217,7 @@ public void testVariableIsReferencedBeforeAssignment() throws Exception {
  } catch (EvalExceptionWithStackTrace e) {
  assertThat(e)
  .hasMessageThat()
- .contains("Variable 'global_var' is referenced before assignment.");
+ .contains("local variable 'global_var' is referenced before assignment.");
  }
  }
 

src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java

@@ -357,8 +357,15 @@ public void testListComprehensionsWithFiltering() throws Exception {
 
  @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

src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java

@@ -105,7 +105,8 @@ public void testFunctionDefLocalGlobalScope() throws Exception {
 
  @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 @@ public void testFunctionDefLocalVariableReferencedBeforeAssignment() throws Exce
 
  @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",

src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java

@@ -1789,15 +1789,17 @@ public void testFunctionCallOrdering() throws Exception {
 
  @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 @@ public void testShadowisNotInitialized() throws Exception {
  }
 
  @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 @@ public void testStructMethodNotDefined() throws Exception {
  @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)]",

src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java

@@ -93,7 +93,6 @@ public void testFunctionParameterDoesNotEffectGlobalValidationEnv() throws Excep
 
  @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");
@@ -102,29 +101,13 @@ public void testDefinitionByItself() throws Exception {
  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]");
- 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");
  }