Move PackageContext fields into Package.Builder

This is to prepare for eliminating the PackageContext class itself, in favor of Package.Builder.

PackageContext's getLabel() and setMakeVariable() accessors are replaced by the corresponding methods of the underlying Builder.

The globber and eventHandler fields are migrated to the Builder as well. Since we don't have a PackageContext in all circumstances (e.g., when a Package.Builder is instantiated for bzlmod logic or unit tests), these fields are @nullable, and the caller must beware. The eventHandler field in particular is just a direct migration from PackageContext; a follow-up CL will further refactor it so that eventHandler is not set by the caller but rather owned outright by the builder, and can never be null.

Minor notes:
- Since globber is now stored on the builder, it no longer needs to be threaded as a separate arg to executeBuildFile[Impl]().
- Callers who would've read the eventHandler off the PackageContext now retrieve it from the Builder, so we now need to assign the eventHandler to the Builder via the new setLocalEventHandler() in PackageFactory and WorkspaceFactory. Again, this is temporary.

Continued from unknown commit.

Work toward #19922.

PiperOrigin-RevId: 594123106
Change-Id: I2bd898c57755a4d7bd687aff526305168478ce94

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -1147,7 +1147,9 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
             thread
                 .getSemantics()
                 .getBool(BuildLanguageOptions.INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES),
-            pkgContext.getEventHandler(),
+            // TODO(#19922): Delete this arg once it's definitely redundant (when the builder owns
+            // its eventHandler).
+            pkgContext.getBuilder().getLocalEventHandler(),
             thread.getCallStack());
       } catch (InvalidRuleException | NameConflictException e) {
         throw new EvalException(e);

src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java

@@ -93,7 +93,7 @@ public NoneType environmentGroup(
     try {
       Location loc = thread.getCallerLocation();
       context.pkgBuilder.addEnvironmentGroup(
-          name, environments, defaults, context.eventHandler, loc);
+          name, environments, defaults, context.getBuilder().getLocalEventHandler(), loc);
       return Starlark.NONE;
     } catch (LabelSyntaxException e) {
       throw Starlark.errorf("environment group has invalid name: %s: %s", name, e.getMessage());
@@ -125,8 +125,12 @@ public NoneType licenses(
       License license = BuildType.LICENSE.convert(licensesList, "'licenses' operand");
       context.pkgBuilder.mergePackageArgsFrom(PackageArgs.builder().setLicense(license));
     } catch (ConversionException e) {
-      context.eventHandler.handle(
-          Package.error(thread.getCallerLocation(), e.getMessage(), Code.LICENSE_PARSE_FAILURE));
+      context
+          .getBuilder()
+          .getLocalEventHandler()
+          .handle(
+              Package.error(
+                  thread.getCallerLocation(), e.getMessage(), Code.LICENSE_PARSE_FAILURE));
       context.pkgBuilder.setContainsErrors();
     }
     return Starlark.NONE;
@@ -149,9 +153,12 @@ public NoneType distribs(Object object, StarlarkThread thread) throws EvalExcept
           BuildType.DISTRIBUTIONS.convert(object, "'distribs' operand");
       context.pkgBuilder.mergePackageArgsFrom(PackageArgs.builder().setDistribs(distribs));
     } catch (ConversionException e) {
-      context.eventHandler.handle(
-          Package.error(
-              thread.getCallerLocation(), e.getMessage(), Code.DISTRIBUTIONS_PARSE_FAILURE));
+      context
+          .getBuilder()
+          .getLocalEventHandler()
+          .handle(
+              Package.error(
+                  thread.getCallerLocation(), e.getMessage(), Code.DISTRIBUTIONS_PARSE_FAILURE));
       context.pkgBuilder.setContainsErrors();
     }
     return Starlark.NONE;

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -42,7 +42,9 @@
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.EventKind;
+import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
+import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.PackageLoading;
@@ -858,6 +860,9 @@ default boolean precomputeTransitiveLoads() {
     private final TreeMap<String, String> makeEnv = new TreeMap<>();
     private final List<Event> events = Lists.newArrayList();
     private final List<Postable> posts = Lists.newArrayList();
+    // Assigned by setLocalEventHandler, but not necessarily in all contexts.
+    // TODO(#19922): Make non-nullable, init in constructor.
+    @Nullable private StoredEventHandler localEventHandler = null;
     @Nullable private String ioExceptionMessage = null;
     @Nullable private IOException ioException = null;
     @Nullable private DetailedExitCode ioExceptionDetailedExitCode = null;
@@ -876,6 +881,10 @@ default boolean precomputeTransitiveLoads() {
     // serialize events emitted during its construction/evaluation.
     @Nullable private FailureDetail failureDetailOverride = null;
 
+    // Used by glob(). Null for contexts where glob() is disallowed, including WORKSPACE files and
+    // some tests.
+    @Nullable private Globber globber = null;
+
     // All targets added to the package. We use SnapshottableBiMap to help track insertion order of
     // Rule targets, for use by native.existing_rules().
     private BiMap<String, Target> targets =
@@ -1092,7 +1101,7 @@ public Builder setFilename(RootedPath filename) {
       return this;
     }
 
-    Label getBuildFileLabel() {
+    public Label getBuildFileLabel() {
       return pkg.metadata.buildFileLabel;
     }
 
@@ -1135,8 +1144,31 @@ public List<Event> getEvents() {
       return events;
     }
 
+    /** Associates a {@link StoredEventHandler} with this builder. */
+    // TODO(#19922): This is a temporary method resulting from migrating PackageContext.eventHandler
+    // to Package.Builder. The next step is to create the StoredEventHandler in Package.Builder's
+    // constructor, and use that to eliminate the separate `events` and `posts` fields.
     @CanIgnoreReturnValue
-    Builder setMakeVariable(String name, String value) {
+    Builder setLocalEventHandler(StoredEventHandler eventHandler) {
+      this.localEventHandler = eventHandler;
+      return this;
+    }
+
+    /**
+     * Returns the {@link ExtendedEventHandler} associated with this builder. Using this handler
+     * <i>should</i> be equivalent to calling {@link #addEvent} / {@link #addPosts}.
+     *
+     * <p>This field may be null in tests.
+     */
+    // TODO(#19922): The "should" in the above javadoc will be guaranteed by eliminating the
+    // separate `events` and `posts` fields. We'll also make this non-nullable.
+    @Nullable
+    public ExtendedEventHandler getLocalEventHandler() {
+      return localEventHandler;
+    }
+
+    @CanIgnoreReturnValue
+    public Builder setMakeVariable(String name, String value) {
       makeEnv.put(name, value);
       return this;
     }
@@ -1299,6 +1331,25 @@ private void checkLoadsNotSet() {
           pkg.metadata.transitiveLoads);
     }
 
+    /** Sets the {@link Globber}. */
+    // TODO(#19922): Move this to Builder() constructor.
+    @CanIgnoreReturnValue
+    public Builder setGlobber(Globber globber) {
+      Preconditions.checkState(this.globber == null);
+      this.globber = globber;
+      return this;
+    }
+
+    /**
+     * Returns the {@link Globber} used to implement {@code glob()} functionality during BUILD
+     * evaluation. Null for contexts where globbing is not possible, including WORKSPACE files and
+     * some tests.
+     */
+    @Nullable
+    public Globber getGlobber() {
+      return globber;
+    }
+
     /**
      * Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package}
      * associated with this {@link Builder}.

src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java

@@ -22,7 +22,6 @@
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.actions.ThreadStateReceiver;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.concurrent.NamedForkJoinPool;
@@ -210,6 +209,7 @@ public RuleClassProvider getRuleClassProvider() {
   }
 
   /** Get the PackageContext by looking up in the environment. */
+  // TODO(#19922): Eliminate PackageContext and retrieve the Package.Builder directly instead.
   public static PackageContext getContext(StarlarkThread thread) throws EvalException {
     PackageContext value = thread.getThreadLocal(PackageContext.class);
     if (value == null) {
@@ -291,42 +291,20 @@ public NonSkyframeGlobber createNonSkyframeGlobber(
    * unreachable once the StarlarkThread is discarded at the end of evaluation. Please be aware of
    * your memory footprint when making changes here!
    */
-  // TODO(adonovan): is there any reason not to merge this with Package.Builder?
+  // TODO(#19922): Merge this into Package.Builder, store the builder directly using
+  // StarlarkThread.setThreadLocal.
   public static class PackageContext {
     final Package.Builder pkgBuilder;
-    final Globber globber;
-    final ExtendedEventHandler eventHandler;
 
     @VisibleForTesting
-    public PackageContext(
-        Package.Builder pkgBuilder, Globber globber, ExtendedEventHandler eventHandler) {
+    public PackageContext(Package.Builder pkgBuilder) {
       this.pkgBuilder = pkgBuilder;
-      this.eventHandler = eventHandler;
-      this.globber = globber;
-    }
-
-    /** Returns the Label of this Package's BUILD file. */
-    public Label getLabel() {
-      return pkgBuilder.getBuildFileLabel();
-    }
-
-    /** Sets a Make variable. */
-    public void setMakeVariable(String name, String value) {
-      pkgBuilder.setMakeVariable(name, value);
     }
 
     /** Returns the builder of this Package. */
     public Package.Builder getBuilder() {
       return pkgBuilder;
     }
-
-    /**
-     * Returns the event handler that should be used to report events happening while building this
-     * package.
-     */
-    public ExtendedEventHandler getEventHandler() {
-      return eventHandler;
-    }
   }
 
   /**
@@ -397,9 +375,10 @@ public void executeBuildFile(
       ImmutableList<String> subpackages,
       ImmutableMap<String, Object> predeclared,
       ImmutableMap<String, Module> loadedModules,
-      StarlarkSemantics starlarkSemantics,
-      Globber globber)
+      StarlarkSemantics starlarkSemantics)
       throws InterruptedException {
+    Globber globber = pkgBuilder.getGlobber();
+
     // Prefetch glob patterns asynchronously.
     if (maxDirectoriesToEagerlyVisitInGlobbing == -2) {
       try {
@@ -422,7 +401,7 @@ public void executeBuildFile(
         cpuSemaphore.acquire();
       }
       executeBuildFileImpl(
-          pkgBuilder, buildFileProgram, predeclared, loadedModules, starlarkSemantics, globber);
+          pkgBuilder, buildFileProgram, predeclared, loadedModules, starlarkSemantics);
     } catch (InterruptedException e) {
       globber.onInterrupt();
       throw e;
@@ -439,19 +418,19 @@ private void executeBuildFileImpl(
       Program buildFileProgram,
       ImmutableMap<String, Object> predeclared,
       ImmutableMap<String, Module> loadedModules,
-      StarlarkSemantics semantics,
-      Globber globber)
+      StarlarkSemantics semantics)
       throws InterruptedException {
     pkgBuilder.setLoads(loadedModules.values());
 
     StoredEventHandler eventHandler = new StoredEventHandler();
-    PackageContext pkgContext = new PackageContext(pkgBuilder, globber, eventHandler);
+    PackageContext pkgContext = new PackageContext(pkgBuilder);
+    pkgBuilder.setLocalEventHandler(eventHandler);
 
     try (Mutability mu = Mutability.create("package", pkgBuilder.getFilename())) {
       Module module = Module.withPredeclared(semantics, predeclared);
       StarlarkThread thread = new StarlarkThread(mu, semantics);
       thread.setLoader(loadedModules::get);
-      thread.setPrintHandler(Event.makeDebugPrintHandler(pkgContext.eventHandler));
+      thread.setPrintHandler(Event.makeDebugPrintHandler(eventHandler));
 
       new BazelStarlarkContext(
               BazelStarlarkContext.Phase.LOADING,
@@ -473,7 +452,7 @@ private void executeBuildFileImpl(
       try {
         Starlark.execFileProgram(buildFileProgram, module, thread);
       } catch (EvalException ex) {
-        pkgContext.eventHandler.handle(
+        eventHandler.handle(
             Package.error(null, ex.getMessageWithStack(), Code.STARLARK_EVAL_ERROR));
         pkgBuilder.setContainsErrors();
       } catch (InterruptedException ex) {

src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java

@@ -300,7 +300,8 @@ public NoneType call(StarlarkThread thread, Tuple args, Dict<String, Object> kwa
             thread
                 .getSemantics()
                 .getBool(BuildLanguageOptions.INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES),
-            context.eventHandler,
+            // TODO(#19922): createAndAddRule() should get this from the builder arg directly.
+            context.getBuilder().getLocalEventHandler(),
             thread.getCallStack());
       } catch (RuleFactory.InvalidRuleException | Package.NameConflictException e) {
         throw new EvalException(e);

src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java

@@ -546,13 +546,14 @@ public NoneType packageGroup(
           name,
           packages,
           includes,
-          /*allowPublicPrivate=*/ thread
+          /* allowPublicPrivate= */ thread
               .getSemantics()
               .getBool(BuildLanguageOptions.INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX),
-          /*repoRootMeansCurrentRepo=*/ thread
+          /* repoRootMeansCurrentRepo= */ thread
               .getSemantics()
               .getBool(BuildLanguageOptions.INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX),
-          context.eventHandler,
+          // TODO(#19922): addPackageGroup should access the builder's own eventHandler directly.
+          context.getBuilder().getLocalEventHandler(),
           loc);
       return Starlark.NONE;
     } catch (LabelSyntaxException e) {
@@ -864,8 +865,11 @@ private List<String> runGlobOperation(
         // being acquired more times than it is released.
         cpuSemaphore.release();
       }
-      Globber.Token globToken = context.globber.runAsync(includes, excludes, operation, allowEmpty);
-      return context.globber.fetchUnsorted(globToken);
+      // getGlobber() is not null because we're called from glob() and subpackages(), both of which
+      // are guarded with checkLoadingPhase().
+      Globber.Token globToken =
+          context.getBuilder().getGlobber().runAsync(includes, excludes, operation, allowEmpty);
+      return context.getBuilder().getGlobber().fetchUnsorted(globToken);
     } catch (IOException e) {
       logger.atWarning().withCause(e).log(
           "Exception processing includes=%s, excludes=%s)", includes, excludes);
@@ -887,7 +891,7 @@ private List<String> runGlobOperation(
               e instanceof FileSymlinkException
                   ? Code.EVAL_GLOBS_SYMLINK_ERROR
                   : Code.GLOB_IO_EXCEPTION);
-      context.eventHandler.handle(error);
+      context.getBuilder().getLocalEventHandler().handle(error);
       context.pkgBuilder.setIOException(e, errorMessage, error.getProperty(DetailedExitCode.class));
       return ImmutableList.of();
     } catch (BadGlobException e) {

src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java

@@ -121,8 +121,8 @@ public void execute(
       thread.setLoader(loadedModules::get);
       thread.setPrintHandler(Event.makeDebugPrintHandler(localReporter));
       thread.setThreadLocal(
-          PackageFactory.PackageContext.class,
-          new PackageFactory.PackageContext(builder, null, localReporter));
+          PackageFactory.PackageContext.class, new PackageFactory.PackageContext(builder));
+      builder.setLocalEventHandler(localReporter);
 
       // The workspace environment doesn't need the tools repository or the fragment map
       // because executing workspace rules happens before analysis and it doesn't need a

src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java

@@ -113,7 +113,7 @@ public void analysisTest(
     // TODO(b/291752414): Fix.
     Label dummyBzlFile = Label.createUnvalidated(PackageIdentifier.EMPTY_PACKAGE_ID, "dummy_label");
     Fingerprint fingerprint = new Fingerprint();
-    fingerprint.addString(pkgContext.getLabel().getPackageName());
+    fingerprint.addString(pkgContext.getBuilder().getBuildFileLabel().getPackageName());
     fingerprint.addString(name);
     byte[] transitiveDigestToUse = fingerprint.digestAndReset();
 
@@ -164,7 +164,9 @@ public void analysisTest(
     // evaluation in BzlLoadFunction#execAndExport.
     StoredEventHandler handler = new StoredEventHandler();
     starlarkRuleFunction.export(
-        handler, pkgContext.getLabel(), name + "_test"); // export in BUILD thread
+        handler,
+        pkgContext.getBuilder().getBuildFileLabel(),
+        name + "_test"); // export in BUILD thread
     if (handler.hasErrors()) {
       StringBuilder errors =
           handler.getEvents().stream()