Make the SplitTransition, PatchTransition interfaces more compatible.

Part of https://docs.google.com/document/d/1_UJKmAQ9EE8i3Pl0il3YLTYr-Q9EKYYyLatt2zohfyM/edit#

PiperOrigin-RevId: 197800831

8 files changed, 83 insertions, 20 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index a72b346cbc..4a35e373e7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -407,7 +407,7 @@ public abstract class DependencyResolver {
       List<BuildOptions> splitOptions = null;
       if (attribute.hasSplitConfigurationTransition()) {
         splitOptions =
-            attribute.getSplitTransition(attributeMap).checkedSplit(ruleConfig.getOptions());
+            attribute.getSplitTransition(attributeMap).split(ruleConfig.getOptions());
         hasSplitTransition = !SplitTransition.equals(ruleConfig.getOptions(), splitOptions);
       }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index fb17814f1d..49bd743e68 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -791,7 +791,7 @@ public final class RuleContext extends TargetContext
         attributeDefinition.getSplitTransition(
             ConfiguredAttributeMapper.of(rule, configConditions));
     BuildOptions fromOptions = getConfiguration().getOptions();
-    List<BuildOptions> splitOptions = transition.checkedSplit(fromOptions);
+    List<BuildOptions> splitOptions = transition.split(fromOptions);
     List<ConfiguredTargetAndData> deps = getConfiguredTargetAndTargetDeps(attributeName);
 
     if (SplitTransition.equals(fromOptions, splitOptions)) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
index 531c6f120e..f26ca9f920 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -429,7 +429,7 @@ public final class ConfigurationResolver {
       // TODO(bazel-team): safety-check that this never mutates fromOptions.
       result = ImmutableList.of(((PatchTransition) transition).patch(fromOptions));
     } else if (transition instanceof SplitTransition) {
-      return ((SplitTransition) transition).checkedSplit(fromOptions);
+      return ((SplitTransition) transition).split(fromOptions);
     } else {
       throw new IllegalStateException(String.format(
           "unsupported config transition type: %s", transition.getClass().getName()));
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingSplitTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingSplitTransition.java
index 71e377038e..190b867329 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingSplitTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingSplitTransition.java
@@ -120,7 +120,7 @@ public class ComposingSplitTransition implements SplitTransition {
     if (transition instanceof PatchTransition) {
       return ImmutableList.<BuildOptions>of(((PatchTransition) transition).patch(fromOptions));
     } else if (transition instanceof SplitTransition) {
-      return ((SplitTransition) transition).checkedSplit(fromOptions);
+      return ((SplitTransition) transition).split(fromOptions);
     } else {
       throw new IllegalStateException(
           String.format("Unsupported composite transition type: %s",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
index 3c6f5b94c8..8995bf63f5 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
@@ -14,11 +14,35 @@
 
 package com.google.devtools.build.lib.analysis.config.transitions;
 
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import java.util.List;
+
 /**
  * A configuration transition.
  */
 public interface ConfigurationTransition {
   /**
+   * Returns the list of {@code BuildOptions} after applying this transition.
+   *
+   * <p>Returning an empty or null list triggers a {@link RuntimeException}.
+   */
+  List<BuildOptions> apply(BuildOptions buildOptions);
+
+  /**
+   * We want to keep the number of transition interfaces no larger than what's necessary to
+   * maintain a clear configuration API.
+   *
+   * <p>This method provides a speed bump against creating new interfaces too casually. While we
+   * could provide stronger enforcement by making {@link ConfigurationTransition} an abstract class
+   * with a limited access constructor, keeping it as an interface supports definining transitions
+   * with lambdas.
+   *
+   * <p>If you're considering adding a new override, contact bazel-dev@googlegroups.com to discuss.
+   */
+  @SuppressWarnings("unused")
+  String reasonForOverride();
+
+  /**
    * Does this transition switch to a "host" configuration?
    */
   default boolean isHostTransition() {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/PatchTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/PatchTransition.java
index 1fdfa95fd3..b04b167643 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/PatchTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/PatchTransition.java
@@ -13,10 +13,16 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis.config.transitions;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import java.util.List;
 
 /**
- * Interface for a configuration transition.
+ * A configuration transition that maps a single input {@link BuildOptions} to a single output
+ * {@link BuildOptions}.
+ *
+ * <p>Also see {@link SplitTransition}, which maps a single input {@link BuildOptions} to possibly
+ * multiple {@link BuildOptions}.
  *
  * <p>The concept is simple: given the input configuration's build options, the
  * transition does whatever it wants to them and returns the modified result.
@@ -48,14 +54,24 @@ import com.google.devtools.build.lib.analysis.config.BuildOptions;
  * {@link com.google.devtools.build.lib.analysis.config.BuildOptions} doesn't currently
  * enforce immutability. So care must be taken not to modify the wrong instance.
  */
+@FunctionalInterface
 public interface PatchTransition extends ConfigurationTransition {
-
   /**
    * Applies the transition.
    *
-   * @param options the options representing the input configuration to this transition. DO NOT
-   *     MODIFY THIS VARIABLE WITHOUT CLONING IT FIRST.
+   * @param options the options representing the input configuration to this transition. <b>DO NOT
+   *     MODIFY THIS VARIABLE WITHOUT CLONING IT FIRST!</b>
    * @return the options representing the desired post-transition configuration
    */
   BuildOptions patch(BuildOptions options);
+
+  @Override
+  default List<BuildOptions> apply(BuildOptions buildOptions) {
+    return ImmutableList.of(patch(buildOptions));
+  }
+
+  @Override
+  default String reasonForOverride() {
+    return "This is a fundamental transition modeling the simple, common case 1-1 options mapping";
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java
index eb71869379..525269cf2e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java
@@ -21,9 +21,16 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import java.util.List;
 
 /**
- * A configuration split transition; this should be used to transition to multiple configurations
- * simultaneously. Note that the corresponding rule implementations must have special support to
- * handle this.
+ * A configuration transition that maps a single input {@link BuildOptions} to possibly multiple
+ * 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}.
+ *
+ * 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
 @FunctionalInterface
@@ -37,21 +44,24 @@ public interface SplitTransition extends ConfigurationTransition {
   List<BuildOptions> split(BuildOptions buildOptions);
 
   /**
-   * Calls {@link #split} and throws a {@link RuntimeException} if the split output is empty.
+   * Returns true iff {@code option} and {@splitOptions} are equal.
+   *
+   * <p>This can be used to determine if a split is a noop.
    */
-  default List<BuildOptions> checkedSplit(BuildOptions buildOptions) {
+  static boolean equals(BuildOptions options, List<BuildOptions> splitOptions) {
+    return splitOptions.size() == 1 && Iterables.getOnlyElement(splitOptions).equals(options);
+  }
+
+  @Override
+  default List<BuildOptions> apply(BuildOptions buildOptions) {
     List<BuildOptions> splitOptions = split(buildOptions);
     Verify.verifyNotNull(splitOptions, "Split transition output may not be null");
     Verify.verify(!splitOptions.isEmpty(), "Split transition output may not be empty");
     return splitOptions;
   }
 
-  /**
-   * Returns true iff {@code option} and {@splitOptions} are equal.
-   *
-   * <p>This can be used to determine if a split is a noop.
-   */
-  static boolean equals(BuildOptions options, List<BuildOptions> splitOptions) {
-    return splitOptions.size() == 1 && Iterables.getOnlyElement(splitOptions).equals(options);
+  @Override
+  default String reasonForOverride() {
+    return "This is a fundamental transition modeling the need for multiply configured deps";
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicyTest.java b/src/test/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicyTest.java
index 18857c7306..09ca8fb930 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/ConfigurationFragmentPolicyTest.java
@@ -15,11 +15,14 @@ package com.google.devtools.build.lib.packages;
 
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
 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.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import java.util.List;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -74,6 +77,16 @@ public final class ConfigurationFragmentPolicyTest {
   private static final ConfigurationTransition TEST_HOST_TRANSITION =
       new ConfigurationTransition() {
         @Override
+        public List<BuildOptions> apply(BuildOptions buildOptions) {
+          return ImmutableList.of(buildOptions);
+        }
+
+        @Override
+        public String reasonForOverride() {
+          return null;
+        }
+
+        @Override
         public boolean isHostTransition() {
           return true;
         }