Make Package.Builder own its event handler

Previously there were three mechanisms for reporting events during package building:

1) a list of events/posts on the Package.Builder, which get accumulated until the package is constructed, at which they're replayed on the skyframe environment's eventHandler

2) a separate StoredEventHandler managed by PackageFactory, used for the odd message here and there, but ultimately merged into the Builder's events/posts

3) the StarlarkThread's print() handler, which points to the handler in (2)

unknown commit moved the handler in (2) from PackageContext to a field of Package.Builder, but left the initialization and merging of this handler in PackageFactory#executeBuildFileImpl(). This CL closes the loop by making Package.Builder responsible for initializing this handler, and uses it to replace the separate events/posts list.

Package.java:
- events/posts fields on Builder go away, localEventHandler becomes non-nullable. addEvent[s]/addPosts/getEvents/getPosts accessors also go away, there were very few uses and they can all be replaced using getLocalEventHandler().

PackageFactory.java / WorkspaceFactory.java:
- no need to create a StoredEventHandler and associate it with the Builder, or merge it into events/posts when done

Event handling machinery:
- add replayPostsOn(), for symmetry with replayEventsOn()

This CL is *probably* a no-op, but there may be slight differences in error reporting at the margins, e.g. whether a certain type of error triggers Builder.setContainsErrors().

Work toward #19922.

PiperOrigin-RevId: 595397832
Change-Id: I09d29dcc58870581a59a8275eff7533fa739be42

src/main/java/com/google/devtools/build/lib/events/ExtendedEventHandler.java

@@ -34,6 +34,13 @@ default void reportTo(ExtendedEventHandler handler) {
     default Postable withTag(@Nullable String tag) {
       return this; // No tag-based filtering.
     }
+
+    /** Replays a sequence of posts on {@code handler}. */
+    public static void replayPostsOn(ExtendedEventHandler handler, Iterable<Postable> posts) {
+      for (Postable post : posts) {
+        handler.post(post);
+      }
+    }
   }
 
   /** Posts a {@link Postable} object about an important build event. */

src/main/java/com/google/devtools/build/lib/events/StoredEventHandler.java

@@ -15,21 +15,22 @@
 
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import java.util.ArrayList;
 import java.util.List;
 
 /** Stores error and warning events, and later replays them. Thread-safe. */
 public class StoredEventHandler implements ExtendedEventHandler {
 
   private final List<Event> events = new ArrayList<>();
-  private final List<ExtendedEventHandler.Postable> posts = new ArrayList<>();
+  private final List<Postable> posts = new ArrayList<>();
   private boolean hasErrors;
 
   public synchronized ImmutableList<Event> getEvents() {
     return ImmutableList.copyOf(events);
   }
 
-  public synchronized ImmutableList<ExtendedEventHandler.Postable> getPosts() {
+  public synchronized ImmutableList<Postable> getPosts() {
     return ImmutableList.copyOf(posts);
   }
 
@@ -46,16 +47,14 @@ public synchronized void handle(Event e) {
   }
 
   @Override
-  public synchronized void post(ExtendedEventHandler.Postable e) {
+  public synchronized void post(Postable e) {
     posts.add(e);
   }
 
   /** Replay all events stored in this object on the given eventHandler, in the same order. */
   public synchronized void replayOn(ExtendedEventHandler eventHandler) {
     Event.replayEventsOn(eventHandler, events);
-    for (ExtendedEventHandler.Postable obj : posts) {
-      eventHandler.post(obj);
-    }
+    Postable.replayPostsOn(eventHandler, posts);
   }
 
   /**

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

@@ -25,7 +25,6 @@
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Interner;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.cmdline.BazelModuleContext;
@@ -42,8 +41,6 @@
 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;
@@ -858,21 +855,19 @@ default boolean precomputeTransitiveLoads() {
     // (which may change due to serialization). This is useful so that the serialized representation
     // is deterministic.
     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;
+
+    private final StoredEventHandler localEventHandler = new StoredEventHandler();
+
     @Nullable private String ioExceptionMessage = null;
     @Nullable private IOException ioException = null;
     @Nullable private DetailedExitCode ioExceptionDetailedExitCode = null;
     // TODO(#19922): Consider having separate containsErrors fields on Metadata and Package. In that
     // case, this field is replaced by the one on Metadata.
     private boolean containsErrors = false;
-    // A package's FailureDetail field derives from its Builder's events. During package
-    // deserialization, those events are unavailable, because those events aren't serialized [*].
-    // Its FailureDetail value is serialized, however. During deserialization, that value is
-    // assigned here, so that it can be assigned to the deserialized package.
+    // A package's FailureDetail field derives from the events on its Builder's event handler.
+    // During package deserialization, those events are unavailable, because those events aren't
+    // serialized [*]. Its FailureDetail value is serialized, however. During deserialization, that
+    // value is assigned here, so that it can be assigned to the deserialized package.
     //
     // Likewise, during workspace part assembly, errors from parent parts should propagate to their
     // children.
@@ -1124,46 +1119,8 @@ RootedPath getFilename() {
       return pkg.metadata.filename;
     }
 
-    /**
-     * Returns {@link Postable}s accumulated while building the package.
-     *
-     * <p>Should retrieved and reported as close to after {@link #build()} or {@link #finishBuild()}
-     * as possible - any earlier and the data may be incomplete.
-     */
-    public List<Postable> getPosts() {
-      return posts;
-    }
-
-    /**
-     * Returns {@link Event}s accumulated while building the package.
-     *
-     * <p>Should retrieved and reported as close to after {@link #build()} or {@link #finishBuild()}
-     * as possible - any earlier and the data may be incomplete.
-     */
-    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 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() {
+    /** Returns the {@link StoredEventHandler} associated with this builder. */
+    public StoredEventHandler getLocalEventHandler() {
       return localEventHandler;
     }
 
@@ -1236,8 +1193,14 @@ Builder setIOException(IOException e, String message, DetailedExitCode detailedE
      * #addEvent} or {@link #addEvents} should already have been called with an {@link Event} of
      * type {@link EventKind#ERROR} that includes a {@link FailureDetail}.
      */
+    // TODO(bazel-team): For simplicity it would be nice to replace this with
+    // getLocalEventHandler().hasErrors(), since that would prevent the kind of inconsistency where
+    // we have reported an ERROR event but not called setContainsErrors(), or vice versa.
     @CanIgnoreReturnValue
     public Builder setContainsErrors() {
+      // TODO(bazel-team): Maybe do Preconditions.checkState(localEventHandler.hasErrors()).
+      // Maybe even assert that it has a FailureDetail, though that's a linear scan unless we
+      // customize the event handler.
       containsErrors = true;
       return this;
     }
@@ -1246,28 +1209,6 @@ public boolean containsErrors() {
       return containsErrors;
     }
 
-    @CanIgnoreReturnValue
-    Builder addPosts(Iterable<Postable> posts) {
-      for (Postable post : posts) {
-        this.posts.add(post);
-      }
-      return this;
-    }
-
-    @CanIgnoreReturnValue
-    Builder addEvents(Iterable<Event> events) {
-      for (Event event : events) {
-        addEvent(event);
-      }
-      return this;
-    }
-
-    @CanIgnoreReturnValue
-    public Builder addEvent(Event event) {
-      this.events.add(event);
-      return this;
-    }
-
     void setFailureDetailOverride(FailureDetail failureDetail) {
       failureDetailOverride = failureDetail;
     }
@@ -1279,7 +1220,7 @@ FailureDetail getFailureDetail() {
       }
 
       List<Event> undetailedEvents = null;
-      for (Event event : this.events) {
+      for (Event event : localEventHandler.getEvents()) {
         if (event.getKind() != EventKind.ERROR) {
           continue;
         }
@@ -1782,7 +1723,9 @@ public Package finishBuild() {
       for (EnvironmentGroup envGroup : ImmutableSet.copyOf(environmentGroups.values())) {
         List<Event> errors = envGroup.processMemberEnvironments(targets);
         if (!errors.isEmpty()) {
-          addEvents(errors);
+          Event.replayEventsOn(localEventHandler, errors);
+          // TODO(bazel-team): Can't we automatically infer containsError from the presence of
+          // ERRORs on our handler?
           setContainsErrors();
         }
       }

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

@@ -27,7 +27,6 @@
 import com.google.devtools.build.lib.concurrent.NamedForkJoinPool;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
-import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Globber.BadGlobException;
 import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings;
 import com.google.devtools.build.lib.packages.PackageValidator.InvalidPackageException;
@@ -401,14 +400,11 @@ private void executeBuildFileImpl(
       throws InterruptedException {
     pkgBuilder.setLoads(loadedModules.values());
 
-    StoredEventHandler eventHandler = new StoredEventHandler();
-    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(eventHandler));
+      thread.setPrintHandler(Event.makeDebugPrintHandler(pkgBuilder.getLocalEventHandler()));
 
       new BazelStarlarkContext(
               BazelStarlarkContext.Phase.LOADING,
@@ -428,8 +424,9 @@ private void executeBuildFileImpl(
       try {
         Starlark.execFileProgram(buildFileProgram, module, thread);
       } catch (EvalException ex) {
-        eventHandler.handle(
-            Package.error(null, ex.getMessageWithStack(), Code.STARLARK_EVAL_ERROR));
+        pkgBuilder
+            .getLocalEventHandler()
+            .handle(Package.error(null, ex.getMessageWithStack(), Code.STARLARK_EVAL_ERROR));
         pkgBuilder.setContainsErrors();
       } catch (InterruptedException ex) {
         if (pkgBuilder.containsErrors()) {
@@ -444,9 +441,6 @@ private void executeBuildFileImpl(
       }
       pkgBuilder.setComputationSteps(thread.getExecutedSteps());
     }
-
-    pkgBuilder.addPosts(eventHandler.getPosts());
-    pkgBuilder.addEvents(eventHandler.getEvents());
   }
 
   /**

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

@@ -2117,6 +2117,7 @@ <T> Rule createRule(
       Label ruleLabel,
       AttributeValues<T> attributeValues,
       boolean failOnUnknownAttributes,
+      // TODO(#19922): elim eventHandler param, it's redundant with pkgBuilder
       EventHandler eventHandler,
       List<StarlarkThread.CallStackEntry> callstack)
       throws LabelSyntaxException, InterruptedException, CannotPrecomputeDefaultsException {

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

@@ -56,6 +56,7 @@ public static Rule createRule(
       RuleClass ruleClass,
       BuildLangTypedAttributeValuesMap attributeValues,
       boolean failOnUnknownAttributes,
+      // TODO(#19922): elim eventHandler param, it's redundant with pkgBuilder
       EventHandler eventHandler,
       ImmutableList<StarlarkThread.CallStackEntry> callstack)
       throws InvalidRuleException, InterruptedException {

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

@@ -19,7 +19,6 @@
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.NullEventHandler;
-import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Package.NameConflictException;
 import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.devtools.build.lib.server.FailureDetails.PackageLoading;
@@ -110,7 +109,6 @@ public void execute(
     predeclared.putAll(bindings); // (may shadow bindings in default environment)
     Module module = Module.withPredeclared(starlarkSemantics, predeclared);
 
-    StoredEventHandler localReporter = new StoredEventHandler();
     try {
       // compile
       new DotBazelFileSyntaxChecker("WORKSPACE files", /* canLoadBzl= */ true).check(file);
@@ -119,9 +117,8 @@ public void execute(
       // create thread
       StarlarkThread thread = new StarlarkThread(mutability, starlarkSemantics);
       thread.setLoader(loadedModules::get);
-      thread.setPrintHandler(Event.makeDebugPrintHandler(localReporter));
+      thread.setPrintHandler(Event.makeDebugPrintHandler(builder.getLocalEventHandler()));
       thread.setThreadLocal(Package.Builder.class, 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
@@ -134,8 +131,11 @@ public void execute(
       try {
         Starlark.execFileProgram(prog, module, thread);
       } catch (EvalException ex) {
-        localReporter.handle(
-            Package.error(null, ex.getMessageWithStack(), PackageLoading.Code.STARLARK_EVAL_ERROR));
+        builder
+            .getLocalEventHandler()
+            .handle(
+                Package.error(
+                    null, ex.getMessageWithStack(), PackageLoading.Code.STARLARK_EVAL_ERROR));
       }
 
       // Accumulate the global bindings created by this chunk of the WORKSPACE file,
@@ -146,7 +146,7 @@ public void execute(
 
     } catch (SyntaxError.Exception ex) {
       // compilation failed
-      Event.replayEventsOn(localReporter, ex.errors());
+      Event.replayEventsOn(builder.getLocalEventHandler(), ex.errors());
       builder.setFailureDetailOverride(
           FailureDetails.FailureDetail.newBuilder()
               .setMessage(ex.getMessage())
@@ -157,9 +157,9 @@ public void execute(
     }
 
     // cleanup (success or failure)
-    builder.addPosts(localReporter.getPosts());
-    builder.addEvents(localReporter.getEvents());
-    if (localReporter.hasErrors()) {
+    // TODO(bazel-team): Package.Builder should manage its own containsErrors bit based on whether
+    // its handler has errors, without our telling it to.
+    if (builder.getLocalEventHandler().hasErrors()) {
       builder.setContainsErrors();
     }
   }

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

@@ -45,7 +45,7 @@ public static boolean originatesInWorkspaceSuffix(
 
   @CanIgnoreReturnValue
   public static Rule createAndAddRepositoryRule(
-      Package.Builder pkg,
+      Package.Builder pkgBuilder,
       RuleClass ruleClass,
       RuleClass bindRuleClass,
       Map<String, Object> kwargs,
@@ -54,17 +54,20 @@ public static Rule createAndAddRepositoryRule(
           Package.NameConflictException,
           LabelSyntaxException,
           InterruptedException {
-    StoredEventHandler eventHandler = new StoredEventHandler();
     BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
     Rule rule =
-        RuleFactory.createRule(pkg, ruleClass, attributeValues, true, eventHandler, callstack);
-    pkg.addEvents(eventHandler.getEvents());
-    pkg.addPosts(eventHandler.getPosts());
-    overwriteRule(pkg, rule);
+        RuleFactory.createRule(
+            pkgBuilder,
+            ruleClass,
+            attributeValues,
+            true,
+            pkgBuilder.getLocalEventHandler(),
+            callstack);
+    overwriteRule(pkgBuilder, rule);
     for (Map.Entry<String, Label> entry :
         ruleClass.getExternalBindingsFunction().apply(rule).entrySet()) {
       Label nameLabel = Label.parseCanonical("//external:" + entry.getKey());
-      addBindRule(pkg, bindRuleClass, nameLabel, entry.getValue(), callstack);
+      addBindRule(pkgBuilder, bindRuleClass, nameLabel, entry.getValue(), callstack);
     }
     // NOTE(wyv): What is this madness?? This is the only instance where a repository rule can
     // register toolchains upon being instantiated. We should look into converting
@@ -77,7 +80,7 @@ public static Rule createAndAddRepositoryRule(
         throw new LabelSyntaxException(e.getMessage());
       }
     }
-    pkg.addRegisteredToolchains(toolchains.build(), originatesInWorkspaceSuffix(callstack));
+    pkgBuilder.addRegisteredToolchains(toolchains.build(), originatesInWorkspaceSuffix(callstack));
     return rule;
   }