Avoid creating a Composing*Transition if one of the transitions is NoTransition.

RELNOTES: None.
PiperOrigin-RevId: 194536202

5 files changed, 67 insertions, 31 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 777bfc9579..ade168c05d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -38,7 +38,6 @@ import com.google.devtools.build.lib.actions.CommandLines.CommandLineLimits;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
-import com.google.devtools.build.lib.analysis.config.transitions.ComposingPatchTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.buildeventstream.BuildEventId;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
@@ -1979,7 +1978,8 @@ public class BuildConfiguration {
       } else if (currentTransition == null) {
         currentTransition = fragmentTransition;
       } else {
-        currentTransition = new ComposingPatchTransition(currentTransition, fragmentTransition);
+        currentTransition =
+            TransitionResolver.composePatchTransitions(currentTransition, fragmentTransition);
       }
     }
     return currentTransition;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java
index ca23d24e9f..ec8cfc35f6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingRuleTransitionFactory.java
@@ -15,9 +15,6 @@
 
 package com.google.devtools.build.lib.analysis.config;
 
-import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.analysis.config.transitions.ComposingPatchTransition;
-import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleTransitionFactory;
@@ -41,17 +38,7 @@ public class ComposingRuleTransitionFactory implements RuleTransitionFactory {
 
   @Override
   public PatchTransition buildTransitionFor(Rule rule) {
-    PatchTransition transition1 = Preconditions.checkNotNull(rtf1.buildTransitionFor(rule));
-    PatchTransition transition2 = Preconditions.checkNotNull(rtf2.buildTransitionFor(rule));
-
-    if (transition1 == NoTransition.INSTANCE) {
-      return transition2;
-    }
-
-    if (transition2 == NoTransition.INSTANCE) {
-      return transition1;
-    }
-
-    return new ComposingPatchTransition(transition1, transition2);
+    return TransitionResolver.composePatchTransitions(
+        rtf1.buildTransitionFor(rule), rtf2.buildTransitionFor(rule));
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
index 946f9599fb..11bbc27402 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.analysis.config;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
 import com.google.devtools.build.lib.analysis.config.transitions.ComposingSplitTransition;
@@ -157,9 +156,10 @@ public final class TransitionResolver {
   /**
    * Composes two transitions together efficiently.
    */
-  @VisibleForTesting
   public static ConfigurationTransition composeTransitions(ConfigurationTransition transition1,
       ConfigurationTransition transition2) {
+    Preconditions.checkNotNull(transition1);
+    Preconditions.checkNotNull(transition2);
     if (isFinal(transition1) || transition2 == NoTransition.INSTANCE) {
       return transition1;
     } else if (isFinal(transition2) || transition1 == NoTransition.INSTANCE) {
@@ -173,6 +173,28 @@ public final class TransitionResolver {
     }
   }
 
+  /** Composes two patch transitions together efficiently. */
+  public static PatchTransition composePatchTransitions(
+      PatchTransition transition1, PatchTransition transition2) {
+    // composeTransitions knows all the tricks for composing transitions, so call into that rather
+    // than reimplementing it here. It will return one of the following:
+    // * one of the two input transitions (which is a PatchTransition by this method's signature,
+    //   and needs only be cast)
+    // -or-
+    // * a new ComposingSplitTransition of (transition1, transition2) - which consists only of
+    //   PatchTransitions and can thus be converted to a ComposingPatchTransition with asPatch.
+    ConfigurationTransition composed = composeTransitions(transition1, transition2);
+    if (composed instanceof ComposingSplitTransition) {
+      return ((ComposingSplitTransition) composed).asPatch();
+    } else if (composed instanceof PatchTransition) {
+      return (PatchTransition) composed;
+    } else {
+      throw new AssertionError(
+          "composeTransitions returned something other than a ComposingSplitTransition or "
+              + "one of the input PatchTransitions");
+    }
+  }
+
   /**
    * Returns true if once the given transition is applied to a dep no followup transitions should
    * be composed after it.
@@ -191,9 +213,7 @@ public final class TransitionResolver {
         "cannot apply splits after null transitions (null transitions are expected to be final)");
     Preconditions.checkState(currentTransition != HostTransition.INSTANCE,
         "cannot apply splits after host transitions (host transitions are expected to be final)");
-    return currentTransition == NoTransition.INSTANCE
-        ? split
-        : new ComposingSplitTransition(currentTransition, split);
+    return composeTransitions(currentTransition, split);
   }
 
   /**
@@ -221,14 +241,8 @@ public final class TransitionResolver {
       return currentTransition;
     }
     if (transitionFactory != null) {
-      PatchTransition ruleClassTransition =
-          Preconditions.checkNotNull(
-              transitionFactory.buildTransitionFor(toTarget.getAssociatedRule()));
-      if (currentTransition == NoTransition.INSTANCE) {
-        return ruleClassTransition;
-      } else if (ruleClassTransition != NoTransition.INSTANCE) {
-        return new ComposingSplitTransition(currentTransition, ruleClassTransition);
-      }
+      return composeTransitions(
+          currentTransition, transitionFactory.buildTransitionFor(toTarget.getAssociatedRule()));
     }
     return currentTransition;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingPatchTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingPatchTransition.java
index 2f99542151..6703b5c3a7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingPatchTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingPatchTransition.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.analysis.config.transitions;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
@@ -27,13 +28,23 @@ import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 public class ComposingPatchTransition implements PatchTransition {
   private final ComposingSplitTransition delegate;
 
+  /**
+   * Creates a {@link ComposingPatchTransition} that applies the sequence: {@code fromOptions ->
+   * transition1 -> transition2 -> toOptions }.
+   *
+   * <p>Note that it's possible to create silly transitions with this constructor (e.g., if one or
+   * both of the transitions is NoTransition). Use composePatchTransitions instead, which checks for
+   * these states and avoids instantiation appropriately.
+   *
+   * @see TransitionResolver#composePatchTransitions
+   */
   public ComposingPatchTransition(PatchTransition transition1, PatchTransition transition2) {
     this(new ComposingSplitTransition(transition1, transition2));
   }
 
   @AutoCodec.Instantiator
-  @AutoCodec.VisibleForSerialization
   ComposingPatchTransition(ComposingSplitTransition delegate) {
+    Preconditions.checkArgument(delegate.isPatchOnly());
     this.delegate = delegate;
   }
 
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 340a5b7b12..73fa1cf4e5 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
@@ -48,6 +48,12 @@ public class ComposingSplitTransition implements SplitTransition {
   /**
    * Creates a {@link ComposingSplitTransition} that applies the sequence: {@code fromOptions ->
    * transition1 -> transition2 -> toOptions }.
+   *
+   * <p>Note that it's possible to create silly transitions with this constructor (e.g., if one or
+   * both of the transitions is NoTransition). Use composeTransitions instead, which checks for
+   * these states and avoids instantiation appropriately.
+   *
+   * @see TransitionResolver#composeTransitions
    */
   @AutoCodec.Instantiator
   public ComposingSplitTransition(
@@ -76,6 +82,24 @@ public class ComposingSplitTransition implements SplitTransition {
   }
 
   /**
+   * Returns whether this transition contains only patches (and is thus suitable as a delegate
+   * for {@link ComposingPatchTransition}).
+   */
+  public boolean isPatchOnly() {
+    return transition1 instanceof PatchTransition && transition2 instanceof PatchTransition;
+  }
+
+  /**
+   * Allows this transition to be used in patch-only contexts if it contains only
+   * {@link PatchTransition}s.
+   *
+   * <p>Can only be called if {@link #isPatchOnly()} returns true.
+   */
+  public ComposingPatchTransition asPatch() {
+    return new ComposingPatchTransition(this);
+  }
+
+  /**
    * Applies the given transition over the given {@link BuildOptions}, returns the result.
    */
   // TODO(gregce): move this somewhere more general. This isn't intrinsic to composed splits.