Gracefully fail when exceptions are thrown during starlark transition…

… application/validation.

Most of this change is just exception propagation immediately after configuration resolution since we usually wait and realize we've made a mistake later on.

bazelbuild#5574

RELNOTES: None.
PiperOrigin-RevId: 229555753

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

@@ -23,6 +23,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
 import com.google.devtools.build.lib.analysis.config.ConfigurationResolver;
 import com.google.devtools.build.lib.analysis.config.TransitionResolver;
+import com.google.devtools.build.lib.analysis.skylark.StarlarkTransition.TransitionException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.BuildType;
@@ -174,14 +175,17 @@ public static <T extends TransitiveInfoProvider> void checkProvider(Class<T> cla
    * each target.
    *
    * <p>Preserves the original input ordering.
+   *
+   * @throws TransitionException if there was a problem applying top level transitions.
    */
   // Keep this in sync with PrepareAnalysisPhaseFunction.
   public static List<TargetAndConfiguration> getTargetsWithConfigs(
       BuildConfigurationCollection configurations,
       Collection<Target> targets,
       ExtendedEventHandler eventHandler,
       ConfiguredRuleClassProvider ruleClassProvider,
-      SkyframeExecutor skyframeExecutor) {
+      SkyframeExecutor skyframeExecutor)
+      throws TransitionException {
     // We use a hash set here to remove duplicate nodes; this can happen for input files and package
     // groups.
     LinkedHashSet<TargetAndConfiguration> nodes = new LinkedHashSet<>(targets.size());

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

@@ -37,6 +37,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.constraints.TopLevelConstraintSemantics;
+import com.google.devtools.build.lib.analysis.skylark.StarlarkTransition.TransitionException;
 import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory;
 import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory.CoverageReportActionsWrapper;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
@@ -248,6 +249,8 @@ public AnalysisResult update(
         topLevelTargetsWithConfigs =
             AnalysisUtils.getTargetsWithConfigs(
                 configurations, targets, eventHandler, ruleClassProvider, skyframeExecutor);
+      } catch (TransitionException e) {
+        throw new InvalidConfigurationException(e);
       }
     }
 

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

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
 import com.google.devtools.build.lib.analysis.skylark.StarlarkTransition;
+import com.google.devtools.build.lib.analysis.skylark.StarlarkTransition.TransitionException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.events.Event;
@@ -219,7 +220,11 @@ public static OrderedSetMultimap<Attribute, Dependency> resolveConfigurations(
         transitionsMap.put(transitionKey, toOptions);
       }
 
-      postProcessStarlarkTransitions(env, transition);
+      try {
+        StarlarkTransition.postProcessStarlarkTransitions(env.getListener(), transition);
+      } catch (TransitionException e) {
+        throw new ConfiguredTargetFunction.DependencyEvaluationException(e);
+      }
 
       // If the transition doesn't change the configuration, trivially re-use the original
       // configuration.
@@ -543,16 +548,17 @@ private static OrderedSetMultimap<Attribute, Dependency> sortResolvedDeps(
   /**
    * This method allows resolution of configurations outside of a skyfunction call.
    *
-   * Unlike {@link #resolveConfigurations}, this doesn't expect the current context to be evaluating
-   * dependencies of a parent target. So this method is also suitable for top-level targets.
+   * <p>Unlike {@link #resolveConfigurations}, this doesn't expect the current context to be
+   * evaluating dependencies of a parent target. So this method is also suitable for top-level
+   * targets.
    *
-   * Resolution consists of two steps:
+   * <p>Resolution consists of two steps:
    *
    * <ol>
    *   <li>Apply the per-target transitions specified in {@code asDeps}. This can be used, e.g., to
    *       apply {@link RuleTransitionFactory}s over global top-level configurations.
-   *   <li>(Optionally) trim configurations to only the fragments the targets actually need. This
-   *       is triggered by {@link BuildConfiguration.Options#trimConfigurations}.
+   *   <li>(Optionally) trim configurations to only the fragments the targets actually need. This is
+   *       triggered by {@link BuildConfiguration.Options#trimConfigurations}.
    * </ol>
    *
    * <p>Preserves the original input order (but merges duplicate nodes that might occur due to
@@ -564,12 +570,13 @@ private static OrderedSetMultimap<Attribute, Dependency> sortResolvedDeps(
    * to evaluate and no more (e.g. there's no need for Android settings in a C++ configured target).
    *
    * @param defaultContext the original targets and starting configurations before applying rule
-   *   transitions and trimming. When actual configurations can't be evaluated, these values are
-   *   returned as defaults. See TODO below.
+   *     transitions and trimming. When actual configurations can't be evaluated, these values are
+   *     returned as defaults. See TODO below.
    * @param targetsToEvaluate the inputs repackaged as dependencies, including rule-specific
-   *   transitions
+   *     transitions
    * @param eventHandler the error event handler
    * @param skyframeExecutor the executor used for resolving Skyframe keys
+   * @throws TransitionException if there was an issue applying a Starlark-defined transition
    */
   // TODO(bazel-team): error out early for targets that fail - failed configuration evaluations
   //   should never make it through analysis (and especially not seed ConfiguredTargetValues)
@@ -580,7 +587,8 @@ public static LinkedHashSet<TargetAndConfiguration> getConfigurationsFromExecuto
       Iterable<TargetAndConfiguration> defaultContext,
       Multimap<BuildConfiguration, Dependency> targetsToEvaluate,
       ExtendedEventHandler eventHandler,
-      SkyframeExecutor skyframeExecutor) {
+      SkyframeExecutor skyframeExecutor)
+      throws TransitionException {
 
     Map<Label, Target> labelsToTargets = new LinkedHashMap<>();
     for (TargetAndConfiguration targetAndConfig : defaultContext) {
@@ -619,3 +627,4 @@ public static LinkedHashSet<TargetAndConfiguration> getConfigurationsFromExecuto
     return result;
   }
 }
+

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

@@ -60,15 +60,15 @@ public class FunctionTransitionUtil {
    * @param buildOptions the pre-transition build options
    * @param starlarkTransition the transition to apply
    * @param attrObject the attributes of the rule to which this transition is attached
-   * @return the post-transition build options
+   * @return the post-transition build options.
    */
   static List<BuildOptions> applyAndValidate(
       BuildOptions buildOptions,
       StarlarkDefinedConfigTransition starlarkTransition,
-      StructImpl attrObject) {
+      StructImpl attrObject)
+      throws EvalException, InterruptedException {
     // TODO(waltl): consider building this once and use it across different split
     // transitions.
-    try {
       Map<String, OptionInfo> optionInfoMap = buildOptionInfo(buildOptions);
       SkylarkDict<String, Object> settings =
           buildSettings(buildOptions, optionInfoMap, starlarkTransition);
@@ -86,11 +86,6 @@ static List<BuildOptions> applyAndValidate(
         splitBuildOptions.add(transitionedOptions);
       }
       return splitBuildOptions.build();
-
-    } catch (InterruptedException | EvalException e) {
-      // TODO(juliexxia): Throw an exception better than RuntimeException.
-      throw new RuntimeException(e);
-    }
   }
 
   private static void validateFunctionOutputs(

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

@@ -19,9 +19,11 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.SplitTransitionProvider;
 import com.google.devtools.build.lib.packages.AttributeMap;
@@ -30,6 +32,7 @@
 import com.google.devtools.build.lib.packages.StructProvider;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
 import com.google.devtools.build.lib.syntax.Environment;
+import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkType;
 import java.util.LinkedHashMap;
@@ -89,9 +92,25 @@ class FunctionSplitTransition extends StarlarkTransition implements SplitTransit
       attrObject = StructProvider.STRUCT.create(attributes, ERROR_MESSAGE_FOR_NO_ATTR);
     }
 
+    /**
+     * @return the post-transition build options or a clone of the original build options if an
+     *     error was encountered during transition application/validation.
+     */
     @Override
     public final List<BuildOptions> split(BuildOptions buildOptions) {
-      return applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject);
+      List<BuildOptions> toReturn;
+      try {
+        toReturn = applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject);
+      } catch (InterruptedException | EvalException e) {
+        starlarkDefinedConfigTransition
+            .getEventHandler()
+            .handle(
+                Event.error(
+                    starlarkDefinedConfigTransition.getLocationForErrorReporting(),
+                    e.getMessage()));
+        return ImmutableList.of(buildOptions.clone());
+      }
+      return toReturn;
     }
   }
 }

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

@@ -21,6 +21,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.RawAttributeMapper;
@@ -29,6 +30,7 @@
 import com.google.devtools.build.lib.packages.StructImpl;
 import com.google.devtools.build.lib.packages.StructProvider;
 import com.google.devtools.build.lib.syntax.Environment;
+import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkType;
 import java.util.LinkedHashMap;
@@ -92,16 +94,34 @@ class FunctionPatchTransition extends StarlarkTransition implements PatchTransit
                   + "currently cannot read attributes behind selects.");
     }
 
+    /**
+     * @return the post-transition build options or a clone of the original build options if an
+     *     error was encountered during transition application/validation.
+     */
     // TODO(b/121134880): validate that the targets these transitions are applied on don't read any
     // attributes that are then configured by the outputs of these transitions.
     @Override
     public BuildOptions patch(BuildOptions buildOptions) {
-      List<BuildOptions> result =
-          applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject);
+      List<BuildOptions> result;
+      try {
+        result = applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject);
+      } catch (EvalException | InterruptedException e) {
+        starlarkDefinedConfigTransition
+            .getEventHandler()
+            .handle(
+                Event.error(
+                    starlarkDefinedConfigTransition.getLocationForErrorReporting(),
+                    e.getMessage()));
+        return buildOptions.clone();
+      }
       if (result.size() != 1) {
-        // TODO(b/121134880): handle this with a better (checked) exception)
-        throw new RuntimeException(
-            "Rule transition only allowed to return a single transitioned configuration.");
+        starlarkDefinedConfigTransition
+            .getEventHandler()
+            .handle(
+                Event.error(
+                    starlarkDefinedConfigTransition.getLocationForErrorReporting(),
+                    "Rule transition only allowed to return a single transitioned configuration."));
+        return buildOptions.clone();
       }
       return Iterables.getOnlyElement(result);
     }

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

@@ -14,16 +14,18 @@
 package com.google.devtools.build.lib.analysis.skylark;
 
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
+import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import java.util.List;
 import java.util.Objects;
 
 /** A marker class for configuration transitions that are defined in Starlark. */
 public abstract class StarlarkTransition implements ConfigurationTransition {
 
   private final StarlarkDefinedConfigTransition starlarkDefinedConfigTransition;
 
-  StarlarkTransition(StarlarkDefinedConfigTransition starlarkDefinedConfigTransition) {
+  public StarlarkTransition(StarlarkDefinedConfigTransition starlarkDefinedConfigTransition) {
     this.starlarkDefinedConfigTransition = starlarkDefinedConfigTransition;
   }
 
@@ -32,6 +34,43 @@ public void replayOn(ExtendedEventHandler eventHandler) {
     starlarkDefinedConfigTransition.getEventHandler().clear();
   }
 
+  public boolean hasErrors() {
+    return starlarkDefinedConfigTransition.getEventHandler().hasErrors();
+  }
+
+  /** Exception class for exceptions thrown during application of a starlark-defined transition */
+  public static class TransitionException extends Exception {
+    public TransitionException(String message) {
+      super(message);
+    }
+  }
+
+  /**
+   * Method to be called after Starlark-transitions are applied. Logs any events (e.g. {@code
+   * print()}s, errors} to output and throws an error if we had any errors. Right now, Starlark
+   * transitions are the only kind that knows how to throw errors so we know this will only report
+   * and throw if a Starlark transition caused a problem.
+   *
+   * @param eventHandler eventHandler to replay events on
+   * @param root transition that was applied. Could be a composing transition so we pull and
+   *     post-process all StarlarkTransitions out of whatever transition is passed here
+   * @throws TransitionException if an error occurred during Starlark transition application.
+   */
+  public static void postProcessStarlarkTransitions(
+      ExtendedEventHandler eventHandler, ConfigurationTransition root) throws TransitionException {
+    List<ConfigurationTransition> transitions = ComposingTransition.decomposeTransition(root);
+    for (ConfigurationTransition transition : transitions) {
+      if (transition instanceof StarlarkTransition) {
+        StarlarkTransition starlarkTransition = (StarlarkTransition) transition;
+        boolean hasErrors = starlarkTransition.hasErrors();
+        starlarkTransition.replayOn(eventHandler);
+        if (hasErrors) {
+          throw new TransitionException("Errors encountered while applying Starlark transition");
+        }
+      }
+    }
+  }
+
   @Override
   public boolean equals(Object object) {
     if (object == this) {

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

@@ -42,6 +42,7 @@
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.configuredtargets.MergedConfiguredTarget.DuplicateException;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
+import com.google.devtools.build.lib.analysis.skylark.StarlarkTransition.TransitionException;
 import com.google.devtools.build.lib.buildeventstream.BuildEventId;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId.ConfigurationId;
 import com.google.devtools.build.lib.causes.AnalysisFailedCause;
@@ -84,6 +85,7 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.Semaphore;
 import java.util.stream.Collectors;
@@ -119,6 +121,10 @@ public DependencyEvaluationException(InconsistentAspectOrderException cause) {
       super(cause);
     }
 
+    public DependencyEvaluationException(TransitionException cause) {
+      super(cause);
+    }
+
     @Override
     public synchronized Exception getCause() {
       return (Exception) super.getCause();
@@ -263,7 +269,8 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc
       // more root causes during computeDependencies.
       // Note that this doesn't apply to AspectFunction, because aspects can't have configurable
       // attributes.
-      if (!transitiveRootCauses.isEmpty() && configConditions != NO_CONFIG_CONDITIONS) {
+      if (!transitiveRootCauses.isEmpty()
+          && !Objects.equals(configConditions, NO_CONFIG_CONDITIONS)) {
         throw new ConfiguredTargetFunctionException(
             new ConfiguredValueCreationException(
                 "Cannot compute config conditions", configuration, transitiveRootCauses.build()));