starlark: always record resolver information in syntax tree

FileOptions.recordScope(false) is no longer needed.

PiperOrigin-RevId: 343392062

src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java

@@ -1399,8 +1399,6 @@ private CompiledBuildFile compileBuildFile(
     // Options for processing BUILD files.
     FileOptions options =
         FileOptions.builder()
-            // TODO(adonovan): remove recordScope opt-out.
-            .recordScope(false) // don't mutate BUILD syntax
             .requireLoadStatementsFirst(false)
             .allowToplevelRebinding(true)
             .restrictStringEscapes(

src/main/java/net/starlark/java/eval/Eval.java

@@ -322,25 +322,8 @@ private static void assign(StarlarkThread.Frame fr, Expression lhs, Object value
   private static void assignIdentifier(StarlarkThread.Frame fr, Identifier id, Object value)
       throws EvalException {
     Resolver.Binding bind = id.getBinding();
-    // Legacy hack for incomplete identifier resolution.
-    // In a <toplevel> function, assignments to unresolved identifiers
-    // update the module, except for load statements and comprehensions,
-    // which should both be file-local.
-    // Load statements don't yet use assignIdentifier,
-    // so we need consider only comprehensions.
-    // In effect, we do the missing resolution using fr.compcount.
-    Resolver.Scope scope;
-    if (bind == null) {
-      scope =
-          fn(fr).isToplevel() && fr.compcount == 0
-              ? Resolver.Scope.GLOBAL //
-              : Resolver.Scope.LOCAL;
-    } else {
-      scope = bind.getScope();
-    }
-
     String name = id.getName();
-    switch (scope) {
+    switch (bind.getScope()) {
       case LOCAL:
         fr.locals.put(name, value);
         break;
@@ -353,7 +336,7 @@ private static void assignIdentifier(StarlarkThread.Frame fr, Identifier id, Obj
         module.exportedGlobals.add(name);
         break;
       default:
-        throw new IllegalStateException(scope.toString());
+        throw new IllegalStateException(bind.getScope().toString());
     }
   }
 
@@ -766,7 +749,6 @@ private static Object evalComprehension(StarlarkThread.Frame fr, Comprehension c
         }
       }
     }
-    fr.compcount++;
 
     // The Lambda class serves as a recursive lambda closure.
     class Lambda {
@@ -823,7 +805,6 @@ void execClauses(int index) throws EvalException, InterruptedException {
       }
     }
     new Lambda().execClauses(0);
-    fr.compcount--;
 
     // Restore outer scope variables.
     // This loop implicitly undefines comprehension variables.

src/main/java/net/starlark/java/eval/StarlarkThread.java

@@ -126,8 +126,6 @@ static final class Frame implements Debug.Frame {
     @Nullable
     final Debug.Debugger dbg = Debug.debugger.get(); // the debugger, if active for this frame
 
-    int compcount = 0; // number of enclosing comprehensions
-
     Object result = Starlark.NONE; // the operand of a Starlark return statement
 
     // Current PC location. Initially fn.getLocation(); for Starlark functions,

src/main/java/net/starlark/java/syntax/FileOptions.java

@@ -76,21 +76,14 @@ public abstract class FileOptions {
    */
   public abstract boolean requireLoadStatementsFirst();
 
-  /**
-   * Record the results of name resolution in the syntax tree by setting {@code Identifer.scope}.
-   * (Disabled for Bazel BUILD files, as its prelude's syntax trees are shared.)
-   */
-  public abstract boolean recordScope();
-
   public static Builder builder() {
     // These are the DEFAULT values.
     return new AutoValue_FileOptions.Builder()
         .restrictStringEscapes(true)
         .allowLoadPrivateSymbols(false)
         .allowToplevelRebinding(false)
         // .loadBindsGlobally(false)
-        .requireLoadStatementsFirst(true)
-        .recordScope(true);
+        .requireLoadStatementsFirst(true);
   }
 
   public abstract Builder toBuilder();
@@ -109,8 +102,6 @@ public abstract static class Builder {
 
     public abstract Builder requireLoadStatementsFirst(boolean value);
 
-    public abstract Builder recordScope(boolean value);
-
     public abstract FileOptions build();
   }
 }

src/main/java/net/starlark/java/syntax/Identifier.java

@@ -107,7 +107,7 @@ public static boolean isValid(String name) {
   //   When it works in a single pass, it is more efficient to process bindings in order,
   //   deferring (rare) forward references until the end of the block.
   // - Eval calls boundIdentifiers for comprehensions. This can be eliminated when
-  //   recordScope is always enabled and variables are assigned frame slot indices.
+  //   variables are assigned frame slot indices.
   // - Eval calls boundIdentifiers for the 'export' hack. This can be eliminated
   //   when we switch to compilation by emitting EXPORT instructions for the necessary
   //   bindings. (Ideally we would eliminate Bazel's export hack entirely.)

src/main/java/net/starlark/java/syntax/Resolver.java

@@ -191,10 +191,10 @@ public ImmutableList<String> getParameterNames() {
 
     /**
      * isToplevel indicates that this is the <toplevel> function containing top-level statements of
-     * a file. It causes assignments to unresolved identifiers to update the module, not the lexical
-     * frame.
+     * a file.
      */
-    // TODO(adonovan): remove this hack when identifier resolution is accurate.
+    // TODO(adonovan): remove this when we remove Bazel's "export" hack,
+    // or switch to a compiled representation of function bodies.
     public boolean isToplevel() {
       return isToplevel;
     }
@@ -371,9 +371,7 @@ public void visit(Identifier id) {
     for (Block b = block; b != null; b = b.parent) {
       Binding bind = b.bindings.get(id.getName());
       if (bind != null) {
-        if (options.recordScope()) {
-          id.setBinding(bind);
-        }
+        id.setBinding(bind);
         return;
       }
     }
@@ -697,19 +695,15 @@ private boolean bind(Identifier id) {
         }
       }
 
-      if (options.recordScope()) {
-        id.setBinding(bind);
-      }
+      id.setBinding(bind);
       return true;
     }
 
     // new binding
     // TODO(adonovan): accumulate locals in the enclosing function/file block.
     bind = new Binding(block.scope, id, block.bindings.size());
     block.bindings.put(id.getName(), bind);
-    if (options.recordScope()) {
-      id.setBinding(bind);
-    }
+    id.setBinding(bind);
     return false;
   }
 

src/test/java/net/starlark/java/eval/EvaluationTest.java

@@ -396,29 +396,10 @@ ev.new Scenario()
 
   @Test
   public void testListComprehensionDefinitionOrder() throws Exception {
-    // This exercises the .bzl file behavior. This is a dynamic error.
-    // (The error message for BUILD files is slightly different (no "local")
-    // because it doesn't record the scope in the syntax tree.)
     ev.new Scenario()
         .testIfErrorContains(
-            "local variable 'y' is referenced before assignment", //
+            "local variable 'y' is referenced before assignment",
             "[x for x in (1, 2) if y for y in (3, 4)]");
-
-    // This is the corresponding test for BUILD files.
-    EvalException ex =
-        assertThrows(
-            EvalException.class, () -> execBUILD("[x for x in (1, 2) if y for y in (3, 4)]"));
-    assertThat(ex).hasMessageThat().isEqualTo("variable 'y' is referenced before assignment");
-  }
-
-  private static void execBUILD(String... lines)
-      throws SyntaxError.Exception, EvalException, InterruptedException {
-    ParserInput input = ParserInput.fromLines(lines);
-    FileOptions options = FileOptions.builder().recordScope(false).build();
-    try (Mutability mu = Mutability.create("test")) {
-      StarlarkThread thread = new StarlarkThread(mu, StarlarkSemantics.DEFAULT);
-      Starlark.execFile(input, options, Module.create(), thread);
-    }
   }
 
   @Test