Automated rollback of commit 395fbbd.

*** Reason for rollback ***

Breaking blaze guitar tests

*** Original change description ***

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: 212017164

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

@@ -1112,14 +1112,7 @@ 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.
-    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;
+    return globalFrame.get(varname);
   }
 
   /** Returns the value of a variable defined with setupDynamic. */

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

@@ -107,12 +107,9 @@ 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.
-      EvalException e = getSpecialException();
-      throw e != null
-          ? e
-          : new EvalException(
-              getLocation(),
-              scope.getQualifier() + " variable '" + name + "' is referenced before assignment.");
+      throw new EvalException(
+          getLocation(),
+          scope.getQualifier() + " variable '" + name + "' is referenced before assignment.");
     }
 
     return result;
@@ -128,8 +125,11 @@ public Kind kind() {
     return Kind.IDENTIFIER;
   }
 
-  /** Exception to provide a better error message for using PACKAGE_NAME or REPOSITORY_NAME. */
-  private EvalException getSpecialException() {
+  EvalException createInvalidIdentifierException(Set<String> symbols) {
+    if (name.equals("$error$")) {
+      return new EvalException(getLocation(), "contains syntax error(s)", true);
+    }
+
     if (name.equals("PACKAGE_NAME")) {
       return new EvalException(
           getLocation(),
@@ -148,18 +148,6 @@ private EvalException getSpecialException() {
               + " 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,12 +26,17 @@
 /**
  * A class for doing static checks on files, before evaluating them.
  *
- * <p>We implement the semantics discussed in
+ * <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 {
 
@@ -56,6 +61,7 @@ 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;
 
@@ -96,12 +102,18 @@ 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) {
@@ -153,23 +165,41 @@ 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()));
     }
-    node.setScope(b.scope);
+    if (semantics.incompatibleStaticNameResolution()) {
+      // The scoping information is reliable only with the new behavior.
+      node.setScope(b.scope);
+    }
   }
 
   private void validateLValue(Location loc, Expression expr) {
-    if (expr instanceof IndexExpression) {
+    if (expr instanceof Identifier) {
+      if (!semantics.incompatibleStaticNameResolution()) {
+        declare(((Identifier) expr).getName(), loc);
+      }
+    } else if (expr instanceof IndexExpression) {
       visit(expr);
     } else if (expr instanceof ListLiteral) {
       for (Expression e : ((ListLiteral) expr).getElements()) {
         validateLValue(loc, e);
       }
-    } else if (!(expr instanceof Identifier)) {
+    } else {
       throw new ValidationException(loc, "cannot assign to '" + expr + "'");
     }
   }
@@ -214,9 +244,11 @@ public void visit(DotExpression node) {
   @Override
   public void visit(AbstractComprehension node) {
     openBlock(Scope.Local);
-    for (AbstractComprehension.Clause clause : node.getClauses()) {
-      if (clause.getLValue() != null) {
-        collectDefinitions(clause.getLValue());
+    if (semantics.incompatibleStaticNameResolution()) {
+      for (AbstractComprehension.Clause clause : node.getClauses()) {
+        if (clause.getLValue() != null) {
+          collectDefinitions(clause.getLValue());
+        }
       }
     }
     super.visit(node);
@@ -236,7 +268,9 @@ public void visit(FunctionDefStatement node) {
         declare(param.getName(), param.getLocation());
       }
     }
-    collectDefinitions(node.getStatements());
+    if (semantics.incompatibleStaticNameResolution()) {
+      collectDefinitions(node.getStatements());
+    }
     visitAll(node.getStatements());
     closeBlock();
   }
@@ -264,13 +298,25 @@ public void visit(AugmentedAssignmentStatement node) {
 
   /** Declare a variable and add it to the environment. */
   private void declare(String varname, Location location) {
-    if (block.scope == Scope.Module && block.variables.contains(varname)) {
-      // Symbols defined in the module scope cannot be reassigned.
+    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) {
       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);
   }
 
@@ -332,9 +378,20 @@ private void validateAst(List<Statement> statements) {
 
     openBlock(Scope.Module);
 
-    // Add each variable defined by statements, not including definitions that appear in
-    // sub-scopes of the given statements (function bodies and comprehensions).
-    collectDefinitions(statements);
+    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());
+        }
+      }
+    }
 
     // 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("local variable 'global_var' is referenced before assignment.");
+          .contains("Variable 'global_var' is referenced before assignment.");
     }
   }
 

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

@@ -357,15 +357,8 @@ public void testListComprehensionsWithFiltering() throws Exception {
 
   @Test
   public void testListComprehensionDefinitionOrder() throws Exception {
-    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)]");
+    newTest().testIfErrorContains("name 'y' is not defined",
+        "[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,8 +105,7 @@ public void testFunctionDefLocalGlobalScope() throws Exception {
 
   @Test
   public void testFunctionDefLocalVariableReferencedBeforeAssignment() throws Exception {
-    checkEvalErrorContains(
-        "local variable 'a' is referenced before assignment.",
+    checkEvalErrorContains("Variable 'a' is referenced before assignment.",
         "a = 1",
         "def func():",
         "  b = a",
@@ -117,8 +116,7 @@ public void testFunctionDefLocalVariableReferencedBeforeAssignment() throws Exce
 
   @Test
   public void testFunctionDefLocalVariableReferencedInCallBeforeAssignment() throws Exception {
-    checkEvalErrorContains(
-        "local variable 'a' is referenced before assignment.",
+    checkEvalErrorContains("Variable 'a' is referenced before assignment.",
         "def dummy(x):",
         "  pass",
         "a = 1",

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

@@ -1789,17 +1789,15 @@ public void testFunctionCallOrdering() throws Exception {
 
   @Test
   public void testFunctionCallBadOrdering() throws Exception {
-    new SkylarkTest()
-        .testIfErrorContains(
-            "global variable 'foo' is referenced before assignment",
-            "def func(): return foo() * 2",
-            "x = func()",
-            "def foo(): return 2");
+    new SkylarkTest().testIfErrorContains("name 'foo' is not defined",
+         "def func(): return foo() * 2",
+         "x = func()",
+         "def foo(): return 2");
   }
 
   @Test
   public void testLocalVariableDefinedBelow() throws Exception {
-    new SkylarkTest()
+    new SkylarkTest("--incompatible_static_name_resolution=true")
         .setUp(
             "def beforeEven(li):", // returns the value before the first even number
             "    for i in li:",
@@ -1824,11 +1822,10 @@ public void testShadowisNotInitialized() throws Exception {
   }
 
   @Test
-  public void testShadowBuiltin() throws Exception {
-    // TODO(laurentlb): Forbid this.
+  public void testLegacyGlobalIsNotInitialized() throws Exception {
     new SkylarkTest("--incompatible_static_name_resolution=false")
-        .setUp("x = len('abc')", "len = 2", "y = x + len")
-        .testLookup("y", 5);
+        .setUp("a = len")
+        .testIfErrorContains("Variable len is read only", "len = 2");
   }
 
   @Test
@@ -2197,7 +2194,7 @@ public void testStructMethodNotDefined() throws Exception {
   @Test
   public void testListComprehensionsDoNotLeakVariables() throws Exception {
     checkEvalErrorContains(
-        "local variable 'a' is referenced before assignment",
+        "name 'a' is not defined",
         "def foo():",
         "  a = 10",
         "  b = [a for a in range(3)]",

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

@@ -93,6 +93,7 @@ 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");
@@ -101,13 +102,29 @@ 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");
   }