BEGIN_OSS

starlark: make load statements bind file-locally

With this change, load statements create file-local bindings,
not global ones; a function that references a load binding
is a closure. There is no longer any need to maintain a
separate hash table in Module.getExportedGlobals that records
the "exported" globals.

The FileOptions.loadBindsGlobally option causes load statements
to create global bindings. It is used for the prelude
(which principally contains load statements), for the REPL,
where load statements in one chunk should be visible in the next,
and for BUILD files, which frequently, and for reasons unclear,
say "load(..., "x"); x=1".

This is a breaking Java API change for Copybara.
END_PUBLIC

PiperOrigin-RevId: 346753608

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

@@ -160,8 +160,11 @@ static BzlCompileValue computeInline(
     ParserInput input = ParserInput.fromLatin1(bytes, inputName);
     FileOptions options =
         FileOptions.builder()
-            // TODO(adonovan): add this, so that loads can normally be truly local.
-            // .loadBindsGlobally(key.isBuildPrelude())
+            // By default, Starlark load statements create file-local bindings.
+            // However, the BUILD prelude typically contains nothing but load
+            // statements whose bindings are intended to be visible in all BUILD
+            // files. The loadBindsGlobally flag allows us to retrieve them.
+            .loadBindsGlobally(key.isBuildPrelude())
             .restrictStringEscapes(
                 semantics.getBool(BuildLanguageOptions.INCOMPATIBLE_RESTRICT_STRING_ESCAPES))
             .build();

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

@@ -1384,6 +1384,11 @@ private CompiledBuildFile compileBuildFile(
     FileOptions options =
         FileOptions.builder()
             .requireLoadStatementsFirst(false)
+            // For historical reasons, BUILD files are allowed to load a symbol
+            // and then reassign it later. (It is unclear why this is neccessary).
+            // TODO(adonovan): remove this flag and make loads bind file-locally,
+            // as in .bzl files. One can always use a renaming load statement.
+            .loadBindsGlobally(true)
             .allowToplevelRebinding(true)
             .restrictStringEscapes(
                 semantics.getBool(BuildLanguageOptions.INCOMPATIBLE_RESTRICT_STRING_ESCAPES))

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

@@ -127,6 +127,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)
             // the set of valid load labels, so load statements cannot all
             // be migrated to the top of the file.
             .requireLoadStatementsFirst(false)
+            // Bindings created by load statements in one
+            // chunk must be accessible to later chunks.
+            .loadBindsGlobally(true)
             // Top-level rebinding is permitted because historically
             // WORKSPACE files followed BUILD norms, but this should
             // probably be flipped.

src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/TestUtils.java

@@ -103,9 +103,6 @@ public static <T> T roundTrip(T value) throws IOException, SerializationExceptio
   public static void assertModulesEqual(Module module1, Module module2) {
     assertThat(module1.getClientData()).isEqualTo(module2.getClientData());
     assertThat(module1.getGlobals()).containsExactlyEntriesIn(module2.getGlobals()).inOrder();
-    assertThat(module1.getExportedGlobals())
-        .containsExactlyEntriesIn(module2.getExportedGlobals())
-        .inOrder();
     assertThat(module1.getPredeclaredBindings())
         .containsExactlyEntriesIn(module2.getPredeclaredBindings())
         .inOrder();

src/main/java/com/google/devtools/build/skydoc/SkydocMain.java

@@ -275,7 +275,7 @@ public Module eval(
                 Collectors.toMap(AspectInfoWrapper::getIdentifierFunction, Functions.identity()));
 
     // Sort the globals bindings by name.
-    TreeMap<String, Object> sortedBindings = new TreeMap<>(module.getExportedGlobals());
+    TreeMap<String, Object> sortedBindings = new TreeMap<>(module.getGlobals());
 
     for (Entry<String, Object> envEntry : sortedBindings.entrySet()) {
       if (ruleFunctions.containsKey(envEntry.getValue())) {

src/main/java/net/starlark/java/cmd/Main.java

@@ -50,9 +50,9 @@ class Main {
   private static final StarlarkThread thread;
   private static final Module module = Module.create();
 
-  // TODO(adonovan): set load-binds-globally option when we support load,
-  // so that loads bound in one REPL chunk are visible in the next.
-  private static final FileOptions OPTIONS = FileOptions.DEFAULT;
+  // Variables bound by load in one REPL chunk are visible in the next.
+  private static final FileOptions OPTIONS =
+      FileOptions.DEFAULT.toBuilder().allowToplevelRebinding(true).loadBindsGlobally(true).build();
 
   static {
     Mutability mu = Mutability.create("interpreter");

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

@@ -223,32 +223,23 @@ private static void execLoad(StarlarkThread.Frame fr, LoadStatement node) throws
     Module module = loader.load(moduleName);
     if (module == null) {
       fr.setErrorLocation(node.getStartLocation());
-      throw Starlark.errorf(
-          "file '%s' was not correctly loaded. Make sure the 'load' statement appears in the"
-              + " global scope in your file",
-          moduleName);
+      throw Starlark.errorf("module '%s' not found", moduleName);
     }
-    Map<String, Object> globals = module.getExportedGlobals();
 
     for (LoadStatement.Binding binding : node.getBindings()) {
       // Extract symbol.
       Identifier orig = binding.getOriginalName();
-      Object value = globals.get(orig.getName());
+      Object value = module.getGlobal(orig.getName());
       if (value == null) {
         fr.setErrorLocation(orig.getStartLocation());
         throw Starlark.errorf(
             "file '%s' does not contain symbol '%s'%s",
-            moduleName, orig.getName(), SpellChecker.didYouMean(orig.getName(), globals.keySet()));
+            moduleName,
+            orig.getName(),
+            SpellChecker.didYouMean(orig.getName(), module.getGlobals().keySet()));
       }
 
-      // Define module-local variable.
-      // TODO(adonovan): eventually the default behavior should be that
-      // loads bind file-locally. Either way, the resolver should designate
-      // the proper scope of binding.getLocalName() and this should become
-      // simply assign(binding.getLocalName(), value).
-      // Currently, we update the module but not module.exportedGlobals.
-      // Change it to a local binding now that closures are supported.
-      fn(fr).setGlobal(binding.getLocalName().getBinding().getIndex(), value);
+      assignIdentifier(fr, binding.getLocalName(), value);
     }
   }
 
@@ -349,11 +340,7 @@ private static void assignIdentifier(StarlarkThread.Frame fr, Identifier id, Obj
         ((StarlarkFunction.Cell) fr.locals[bind.getIndex()]).x = value;
         break;
       case GLOBAL:
-        // Updates a module binding and sets its 'exported' flag.
-        // (Only load bindings are not exported.
-        // But exportedGlobals does at run time what should be done in the resolver.)
         fn(fr).setGlobal(bind.getIndex(), value);
-        fn(fr).getModule().exportedGlobals.add(id.getName());
         break;
       default:
         throw new IllegalStateException(bind.getScope().toString());

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

@@ -56,11 +56,6 @@ public final class Module implements Resolver.Module {
   private final LinkedHashMap<String, Integer> globalIndex = new LinkedHashMap<>();
   private Object[] globals = new Object[8];
 
-  // Names of globals that are exported and can be loaded from other modules.
-  // TODO(adonovan): eliminate this field when the resolver treats loads as local bindings.
-  // Then all globals are exported.
-  final HashSet<String> exportedGlobals = new HashSet<>();
-
   // An optional piece of metadata associated with the module/file.
   // May be set after construction (too obscure to burden the constructors).
   // Its toString appears to Starlark in str(function): "<function f from ...>".
@@ -174,23 +169,6 @@ public ImmutableMap<String, Object> getGlobals() {
     return m.build();
   }
 
-  /**
-   * Returns a map of bindings that are exported (i.e. symbols declared using `=` and `def`, but not
-   * `load`).
-   */
-  // TODO(adonovan): whether bindings are exported should be decided by the resolver;
-  //  non-exported bindings should never be added to the module.  Delete this,
-  //  once loads bind locally (then all globals will be exported).
-  public ImmutableMap<String, Object> getExportedGlobals() {
-    ImmutableMap.Builder<String, Object> result = new ImmutableMap.Builder<>();
-    for (Map.Entry<String, Object> entry : getGlobals().entrySet()) {
-      if (exportedGlobals.contains(entry.getKey())) {
-        result.put(entry);
-      }
-    }
-    return result.build();
-  }
-
   /** Implements the resolver's module interface. */
   @Override
   public Resolver.Scope resolve(String name) throws Undefined {

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

@@ -41,14 +41,14 @@ public abstract class FileOptions {
   /** The default options for Starlark static processing. New clients should use these defaults. */
   public static final FileOptions DEFAULT = builder().build();
 
-  // Options are presented in phase order: scanner, parser, validator, compiler.
+  // Options are presented in phase order: scanner, parser, resolver, compiler.
 
   // --- scanner options ---
 
   /** Disallow ineffective escape sequences such as {@code \a} when scanning string literals. */
   public abstract boolean restrictStringEscapes();
 
-  // --- validator options ---
+  // --- resolver options ---
 
   /**
    * During resolution, permit load statements to access private names such as {@code _x}. <br>
@@ -57,18 +57,21 @@ public abstract class FileOptions {
   public abstract boolean allowLoadPrivateSymbols();
 
   /**
-   * During resolution, permit multiple bindings of top-level variables. <br>
+   * During resolution, permit multiple assignments to a given top-level binding, whether file-local
+   * or global. However, as usual, you may not create both a file-local and a global binding of the
+   * same name (e.g. {@code load(..., x="x"); x=1}), so if you use this option, you probably want
+   * {@link #loadBindsGlobally} too, to avoid confusing errors. <br>
    * (Required for continued support of Bazel BUILD files and Copybara files.)
    */
   public abstract boolean allowToplevelRebinding();
 
-  // TODO(adonovan): implement this option to support the REPL and prelude.
-  //
-  // /**
-  //  * During resolution, make load statements bind global variables of the module, not file-local
-  //  * variables. (Intended for use in REPLs, and the prelude.)
-  //  */
-  // public abstract boolean loadBindsGlobally();
+  /**
+   * During resolution, make load statements bind global variables of the module, not file-local
+   * variables.<br>
+   * (Intended for use in REPLs, and the Bazel prelude; and in Bazel BUILD files, which make
+   * frequent use of {@code load(..., "x"); x=1} for reasons unclear.)
+   */
+  public abstract boolean loadBindsGlobally();
 
   /**
    * During resolution, require load statements to appear before other kinds of statements. <br>
@@ -82,7 +85,7 @@ public static Builder builder() {
         .restrictStringEscapes(true)
         .allowLoadPrivateSymbols(false)
         .allowToplevelRebinding(false)
-        // .loadBindsGlobally(false)
+        .loadBindsGlobally(false)
         .requireLoadStatementsFirst(true);
   }
 
@@ -98,7 +101,7 @@ public abstract static class Builder {
 
     public abstract Builder allowToplevelRebinding(boolean value);
 
-    // public abstract Builder loadBindsGlobally(boolean value);
+    public abstract Builder loadBindsGlobally(boolean value);
 
     public abstract Builder requireLoadStatementsFirst(boolean value);
 

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

@@ -15,7 +15,7 @@
 
 import com.google.common.collect.ImmutableList;
 
-/** Syntax node for an import statement. */
+/** Syntax node for a load statement. */
 public final class LoadStatement extends Statement {
 
   /**