Automated rollback of commit 4b6da43.

*** Reason for rollback ***

roll forward, after fix and new test

*** Original change description ***

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(.....

***

#5827

RELNOTES: None.
PiperOrigin-RevId: 212436910

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

@@ -81,8 +81,8 @@ void setScope(ValidationEnvironment.Scope scope) {
   @Override
   Object doEval(Environment env) throws EvalException {
     Object result;
-    if (scope == null) {
-      // Legacy behavior, in case the AST was not analyzed.
+    if (scope == null || !env.getSemantics().incompatibleStaticNameResolution()) {
+      // Legacy behavior, to be removed.
       result = env.lookup(name);
       if (result == null) {
         throw createInvalidIdentifierException(env.getVariableNames());
@@ -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/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(
+            "name 'foo' is not defined",
+            "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:",
@@ -1815,17 +1817,32 @@ public void testShadowisNotInitialized() throws Exception {
         .testIfErrorContains(
             /* error message */ "local variable 'gl' is referenced before assignment",
             "gl = 5",
-            "def foo():", // returns the value before the first even number
+            "def foo():",
             "    if False: gl = 2",
             "    return gl",
             "res = foo()");
   }
 
   @Test
-  public void testLegacyGlobalIsNotInitialized() throws Exception {
+  public void testLegacyGlobalVariableNotShadowed() throws Exception {
+    new SkylarkTest("--incompatible_static_name_resolution=false")
+        .setUp(
+            "gl = 5",
+            "def foo():",
+            "    if False: gl = 2",
+            // The legacy behavior is that the global variable is returned.
+            // With --incompatible_static_name_resolution set to true, this becomes an error.
+            "    return gl",
+            "res = foo()")
+        .testLookup("res", 5);
+  }
+
+  @Test
+  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

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