Make StarlarkDefinedConfigTransition immutable.

Generally speaking, it's not safe for config transitions to keep state because
a transition instance may be shared across multiple rule instances. Rules should
not be able to affect each other through shared transitions.

This particular case removes an event handler that stores errors (like trying
to load invalid build settings) which the configuration machinery replays later
to communicate to the user. The risk trajectory is that a transition that fails
on a badly formed rule might repeat the same error for a well-formed rule
becuase of the shared state.

The most likely user-visible output of this problem is non-deterministic build
errors on rules using Starlark build settings and transitions. i.e. repeat the
same build twice and get different results.

This is a huge change (apologies), but conceptually straightforward. We simply
add an EventHandler parameter to ConfigurationTransition.apply to provide an
alternative place to record errors. This lifts StarlarkDefinedConfigTransition
of the burden to define its own.

Most of the hugeness in this change is that the ConfigurationTransition
interface is used in many places, so we have to pass / define this parameter
everywhere. A less invavise alternative could be to use Java interface defaults,
but I think the current approach is a better API (and the pain is limited just
to this change: there's no real extra API burden as a result).

Testing note: I'm impressed that this change only broke one test:
StarlarkRuleTransitionProviderTest#testAliasedBuildSetting, and for
straightforward reasons. See new comments in ConfigurationResolver for details.

PiperOrigin-RevId: 304043250

src/main/java/com/google/devtools/build/lib/BUILD

@@ -400,6 +400,7 @@ java_library(
         "analysis/config/FragmentOptions.java",
     ],
     deps = [
+        ":events",
         ":util",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/concurrent",

src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java

@@ -908,7 +908,8 @@ public List<ConfiguredTargetAndData> getPrerequisiteConfiguredTargetAndTargets(
                         .executionPlatform(getToolchainContext().executionPlatform().label())
                         .build());
     BuildOptions fromOptions = getConfiguration().getOptions();
-    Map<String, BuildOptions> splitOptions = transition.split(fromOptions);
+    Map<String, BuildOptions> splitOptions =
+        transition.split(fromOptions, getAnalysisEnvironment().getEventHandler());
     List<ConfiguredTargetAndData> deps = getConfiguredTargetAndTargetDeps(attributeName);
 
     if (SplitTransition.equals(fromOptions, splitOptions.values())) {

src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java

@@ -39,6 +39,7 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 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.Attribute;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.Target;
@@ -548,10 +549,21 @@ public static Map<String, BuildOptions> applyTransition(
     }
 
     // TODO(bazel-team): Add safety-check that this never mutates fromOptions.
-    Map<String, BuildOptions> result = transition.apply(fromOptions);
+    StoredEventHandler handlerWithErrorStatus = new StoredEventHandler();
+    Map<String, BuildOptions> result = transition.apply(fromOptions, handlerWithErrorStatus);
 
     if (doesStarlarkTransition) {
-      StarlarkTransition.replayEvents(eventHandler, transition);
+      // We use a temporary StoredEventHandler instead of the caller's event handler because
+      // StarlarkTransition.validate assumes no errors occurred. We need a StoredEventHandler to be
+      // able to check that, and fail out early if there are errors.
+      //
+      // TODO(bazel-team): harden StarlarkTransition.validate so we can eliminate this step.
+      // StarlarkRuleTransitionProviderTest#testAliasedBuildSetting_outputReturnMismatch shows the
+      // effect.
+      handlerWithErrorStatus.replayOn(eventHandler);
+      if (handlerWithErrorStatus.hasErrors()) {
+        throw new TransitionException("Errors encountered while applying Starlark transition");
+      }
       result = StarlarkTransition.validate(transition, buildSettingPackages, result);
     }
     return result;

src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java

@@ -20,6 +20,7 @@
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.AttributeTransitionData;
 import javax.annotation.Nullable;
 
@@ -88,7 +89,7 @@ public boolean isHostTransition() {
     private static final BuildOptionsCache<Label> cache = new BuildOptionsCache<>();
 
     @Override
-    public BuildOptions patch(BuildOptions options) {
+    public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
       if (executionPlatform == null) {
         // No execution platform is known, so don't change anything.
         return options;

src/main/java/com/google/devtools/build/lib/analysis/config/HostTransition.java

@@ -16,6 +16,7 @@
 import com.google.auto.value.AutoValue;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 
 /** Dynamic transition to the host configuration. */
@@ -31,7 +32,7 @@ public boolean isHostTransition() {
   }
 
   @Override
-  public BuildOptions patch(BuildOptions options) {
+  public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
     if (options.get(CoreOptions.class).isHost) {
       // If the input already comes from the host configuration, just return the existing values.
       //

src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java

@@ -18,8 +18,8 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.BazelStarlarkContext;
 import com.google.devtools.build.lib.packages.StructImpl;
 import com.google.devtools.build.lib.skylarkbuildapi.config.ConfigurationTransitionApi;
@@ -46,14 +46,12 @@ public abstract class StarlarkDefinedConfigTransition implements ConfigurationTr
   private final List<String> inputs;
   private final List<String> outputs;
   private final Location location;
-  private final StoredEventHandler eventHandler;
 
   private StarlarkDefinedConfigTransition(
       List<String> inputs, List<String> outputs, Location location) {
     this.inputs = inputs;
     this.outputs = outputs;
     this.location = location;
-    this.eventHandler = new StoredEventHandler();
   }
 
   /**
@@ -86,10 +84,6 @@ public Location getLocationForErrorReporting() {
     return location;
   }
 
-  public StoredEventHandler getEventHandler() {
-    return eventHandler;
-  }
-
   /**
    * Given a map of a subset of the "previous" build settings, returns the changed build settings as
    * a result of applying this transition.
@@ -104,7 +98,7 @@ public StoredEventHandler getEventHandler() {
    * @throws InterruptedException if evaluating the transition is interrupted
    */
   public abstract ImmutableMap<String, Map<String, Object>> evaluate(
-      Map<String, Object> previousSettings, StructImpl attributeMap)
+      Map<String, Object> previousSettings, StructImpl attributeMap, EventHandler eventHandler)
       throws EvalException, InterruptedException;
 
   public static StarlarkDefinedConfigTransition newRegularTransition(
@@ -137,7 +131,9 @@ public Boolean isForAnalysisTesting() {
 
     @Override
     public ImmutableMap<String, Map<String, Object>> evaluate(
-        Map<String, Object> previousSettings, StructImpl attributeMapper) {
+        Map<String, Object> previousSettings,
+        StructImpl attributeMapper,
+        EventHandler eventHandler) {
       return ImmutableMap.of(PATCH_TRANSITION_KEY, changedSettings);
     }
 
@@ -209,11 +205,12 @@ public Boolean isForAnalysisTesting() {
     @Override
     @SuppressWarnings("rawtypes")
     public ImmutableMap<String, Map<String, Object>> evaluate(
-        Map<String, Object> previousSettings, StructImpl attributeMapper)
+        Map<String, Object> previousSettings, StructImpl attributeMapper, EventHandler eventHandler)
         throws EvalException, InterruptedException {
       Object result;
       try {
-        result = evalFunction(impl, ImmutableList.of(previousSettings, attributeMapper));
+        result =
+            evalFunction(impl, ImmutableList.of(previousSettings, attributeMapper), eventHandler);
       } catch (EvalException e) {
         throw new EvalException(impl.getLocation(), e.getMessage());
       }
@@ -282,14 +279,15 @@ public void repr(Printer printer) {
     }
 
     /** Evaluate the input function with the given argument, and return the return value. */
-    private Object evalFunction(BaseFunction function, ImmutableList<Object> args)
+    private Object evalFunction(
+        BaseFunction function, ImmutableList<Object> args, EventHandler eventHandler)
         throws InterruptedException, EvalException {
       try (Mutability mutability = Mutability.create("eval_transition_function")) {
         StarlarkThread thread =
             StarlarkThread.builder(mutability)
                 .setSemantics(semantics)
                 .build();
-        thread.setPrintHandler(StarlarkThread.makeDebugPrintHandler(getEventHandler()));
+        thread.setPrintHandler(StarlarkThread.makeDebugPrintHandler(eventHandler));
         starlarkContext.storeInThread(thread);
         return Starlark.call(thread, function, args, /*kwargs=*/ ImmutableMap.of());
       }

src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java

@@ -17,6 +17,7 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import java.util.Map;
 import java.util.Objects;
@@ -48,11 +49,12 @@ public class ComposingTransition implements ConfigurationTransition {
   }
 
   @Override
-  public Map<String, BuildOptions> apply(BuildOptions buildOptions) {
+  public Map<String, BuildOptions> apply(BuildOptions buildOptions, EventHandler eventHandler) {
     ImmutableMap.Builder<String, BuildOptions> toOptions = ImmutableMap.builder();
-    for (Map.Entry<String, BuildOptions> entry1 : transition1.apply(buildOptions).entrySet()) {
+    for (Map.Entry<String, BuildOptions> entry1 :
+        transition1.apply(buildOptions, eventHandler).entrySet()) {
       for (Map.Entry<String, BuildOptions> entry2 :
-          transition2.apply(entry1.getValue()).entrySet()) {
+          transition2.apply(entry1.getValue(), eventHandler).entrySet()) {
         toOptions.put(composeKeys(entry1.getKey(), entry2.getKey()), entry2.getValue());
       }
     }

src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.analysis.config.transitions;
 
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.events.EventHandler;
 import java.util.Map;
 
 /**
@@ -34,7 +35,7 @@ public interface ConfigurationTransition {
    *
    * <p>Returning an empty or null map triggers a {@link RuntimeException}.
    */
-  Map<String, BuildOptions> apply(BuildOptions buildOptions);
+  Map<String, BuildOptions> apply(BuildOptions buildOptions, EventHandler eventHandler);
 
   /**
    * We want to keep the number of transition interfaces no larger than what's necessary to maintain

src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NoTransition.java

@@ -15,6 +15,7 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 
 /** No-op configuration transition. */
@@ -25,7 +26,7 @@ public final class NoTransition implements PatchTransition {
   private NoTransition() {}
 
   @Override
-  public BuildOptions patch(BuildOptions options) {
+  public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
     return options;
   }
 

src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java

@@ -15,6 +15,7 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 
 /** A {@link PatchTransition} to a null configuration. */
@@ -26,7 +27,7 @@ private NullTransition() {
   }
 
   @Override
-  public BuildOptions patch(BuildOptions options) {
+  public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
     throw new UnsupportedOperationException(
         "This is only referenced in a few places, so it's easier and more efficient to optimize "
             + "Blaze's transition logic in the presence of null transitions vs. actually call this "

src/main/java/com/google/devtools/build/lib/analysis/config/transitions/PatchTransition.java

@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.analysis.config.transitions;
 
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.events.EventHandler;
 import java.util.Collections;
 import java.util.Map;
 
@@ -61,13 +62,14 @@ public interface PatchTransition extends ConfigurationTransition {
    *
    * @param options the options representing the input configuration to this transition. <b>DO NOT
    *     MODIFY THIS VARIABLE WITHOUT CLONING IT FIRST!</b>
+   * @param eventHandler
    * @return the options representing the desired post-transition configuration
    */
-  BuildOptions patch(BuildOptions options);
+  BuildOptions patch(BuildOptions options, EventHandler eventHandler);
 
   @Override
-  default Map<String, BuildOptions> apply(BuildOptions buildOptions) {
-    return Collections.singletonMap(PATCH_TRANSITION_KEY, patch(buildOptions));
+  default Map<String, BuildOptions> apply(BuildOptions buildOptions, EventHandler eventHandler) {
+    return Collections.singletonMap(PATCH_TRANSITION_KEY, patch(buildOptions, eventHandler));
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java

@@ -18,6 +18,7 @@
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
+import com.google.devtools.build.lib.events.EventHandler;
 import java.util.Collection;
 import java.util.Map;
 
@@ -26,11 +27,11 @@
  * output {@link BuildOptions}. This provides the ability to transition to multiple configurations
  * simultaneously.
  *
- * <p>Also see {@link PatchTransition}, which maps a single input {@BuildOptions} to a single
- * output. If your transition never needs to produce multiple outputs, you should use a
- * {@link PatchTransition}.
+ * <p>Also see {@link PatchTransition}, which maps a single input {@link BuildOptions} to a single
+ * output. If your transition never needs to produce multiple outputs, you should use a {@link
+ * PatchTransition}.
  *
- * Corresponding rule implementations may require special support to handle this in an organized
+ * <p>Corresponding rule implementations may require special support to handle this in an organized
  * way (e.g. for determining which CPU corresponds to which dep for a multi-arch split dependency).
  */
 @ThreadSafety.Immutable
@@ -43,10 +44,10 @@ public interface SplitTransition extends ConfigurationTransition {
    *
    * <p>Returning an empty or null list triggers a {@link RuntimeException}.
    */
-  Map<String, BuildOptions> split(BuildOptions buildOptions);
+  Map<String, BuildOptions> split(BuildOptions buildOptions, EventHandler eventHandler);
 
   /**
-   * Returns true iff {@code option} and {@splitOptions} are equal.
+   * Returns true iff {@code option} and {@code splitOptions} are equal.
    *
    * <p>This can be used to determine if a split is a noop.
    */
@@ -55,8 +56,8 @@ static boolean equals(BuildOptions options, Collection<BuildOptions> splitOption
   }
 
   @Override
-  default Map<String, BuildOptions> apply(BuildOptions buildOptions) {
-    Map<String, BuildOptions> splitOptions = split(buildOptions);
+  default Map<String, BuildOptions> apply(BuildOptions buildOptions, EventHandler eventHandler) {
+    Map<String, BuildOptions> splitOptions = split(buildOptions, eventHandler);
     Verify.verifyNotNull(splitOptions, "Split transition output may not be null");
     Verify.verify(!splitOptions.isEmpty(), "Split transition output may not be empty");
     return splitOptions;

src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java

@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.StructImpl;
 import com.google.devtools.build.lib.syntax.Dict;
@@ -66,7 +67,8 @@ public class FunctionTransitionUtil {
   static Map<String, BuildOptions> applyAndValidate(
       BuildOptions buildOptions,
       StarlarkDefinedConfigTransition starlarkTransition,
-      StructImpl attrObject)
+      StructImpl attrObject,
+      EventHandler eventHandler)
       throws EvalException, InterruptedException {
     // TODO(waltl): consider building this once and use it across different split
     // transitions.
@@ -76,7 +78,7 @@ static Map<String, BuildOptions> applyAndValidate(
     ImmutableMap.Builder<String, BuildOptions> splitBuildOptions = ImmutableMap.builder();
 
     ImmutableMap<String, Map<String, Object>> transitions =
-        starlarkTransition.evaluate(settings, attrObject);
+        starlarkTransition.evaluate(settings, attrObject, eventHandler);
     validateFunctionOutputsMatchesDeclaredOutputs(transitions.values(), starlarkTransition);
 
     for (Map.Entry<String, Map<String, Object>> entry : transitions.entrySet()) {

src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkAttributeTransitionProvider.java

@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.AttributeTransitionData;
@@ -102,25 +103,22 @@ class FunctionSplitTransition extends StarlarkTransition implements SplitTransit
      *     error was encountered during transition application/validation.
      */
     @Override
-    public final Map<String, BuildOptions> split(BuildOptions buildOptions) {
+    public final Map<String, BuildOptions> split(
+        BuildOptions buildOptions, EventHandler eventHandler) {
       try {
-        return applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject);
+        return applyAndValidate(
+            buildOptions, starlarkDefinedConfigTransition, attrObject, eventHandler);
       } catch (InterruptedException e) {
         Thread.currentThread().interrupt();
-        starlarkDefinedConfigTransition
-            .getEventHandler()
-            .handle(
-                Event.error(
-                    starlarkDefinedConfigTransition.getLocationForErrorReporting(),
-                    "Starlark transition interrupted during attribute transition implementation"));
+        eventHandler.handle(
+            Event.error(
+                starlarkDefinedConfigTransition.getLocationForErrorReporting(),
+                "Starlark transition interrupted during attribute transition implementation"));
         return ImmutableMap.of("error", buildOptions.clone());
       } catch (EvalException e) {
-        starlarkDefinedConfigTransition
-            .getEventHandler()
-            .handle(
-                Event.error(
-                    starlarkDefinedConfigTransition.getLocationForErrorReporting(),
-                    e.getMessage()));
+        eventHandler.handle(
+            Event.error(
+                starlarkDefinedConfigTransition.getLocationForErrorReporting(), e.getMessage()));
         return ImmutableMap.of("error", buildOptions.clone());
       }
     }