Add new ComposingTransitionFactory class.

Part of #7814.

Closes #7877.

PiperOrigin-RevId: 241032661

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

@@ -12,17 +12,18 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-
 package com.google.devtools.build.lib.analysis.config;
 
 import com.google.common.collect.Iterables;
+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.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleTransitionFactory;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 
 /** A {@link RuleTransitionFactory} that composes other {@link RuleTransitionFactory}s. */
+// TODO(https://github.com/bazelbuild/bazel/issues/7814): Replace with ComposingTransitionFactory.
 @AutoCodec
 public class ComposingRuleTransitionFactory implements RuleTransitionFactory {
 
@@ -40,8 +41,8 @@ public ComposingRuleTransitionFactory(RuleTransitionFactory rtf1, RuleTransition
 
   @Override
   public PatchTransition buildTransitionFor(Rule rule) {
-    ConfigurationTransition composed = TransitionResolver.composeTransitions(
-        rtf1.buildTransitionFor(rule), rtf2.buildTransitionFor(rule));
+    ConfigurationTransition composed =
+        ComposingTransition.of(rtf1.buildTransitionFor(rule), rtf2.buildTransitionFor(rule));
     if (composed instanceof PatchTransition) {
       // This is one of the two input transitions. Especially if it's a NoTransition or
       // HostTransition, we should give it back so it can be specially identified as described

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

@@ -81,9 +81,5 @@ public boolean isHost() {
       return true;
     }
 
-    @Override
-    public boolean isFinal() {
-      return true;
-    }
   }
 }

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.base.Preconditions;
 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.analysis.config.transitions.NoTransition;
@@ -88,34 +87,6 @@ public static ConfigurationTransition evaluateTransition(
     return applyTransitionFromFactory(currentTransition, toTarget, trimmingTransitionFactory);
   }
 
-  /**
-   * Composes two transitions together efficiently.
-   */
-  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) {
-      // When the second transition is a HOST transition, there's no need to compose. But this also
-      // improves performance: host transitions are common, and ConfiguredTargetFunction has special
-      // optimized logic to handle them. If they were buried in the last segment of a
-      // ComposingTransition, those optimizations wouldn't trigger.
-      return transition2;
-    } else {
-      return new ComposingTransition(transition1, transition2);
-    }
-  }
-
-  /**
-   * Returns true if once the given transition is applied to a dep no followup transitions should
-   * be composed after it.
-   */
-  private static boolean isFinal(ConfigurationTransition transition) {
-    return (transition == NullTransition.INSTANCE || transition.isHostTransition());
-  }
-
   /**
    * @param currentTransition a pre-existing transition to be composed with
    * @param toTarget target whose associated rule's incoming transition should be applied
@@ -137,11 +108,8 @@ private static ConfigurationTransition applyTransitionFromFactory(
       ConfigurationTransition currentTransition,
       Target toTarget,
       @Nullable RuleTransitionFactory transitionFactory) {
-    if (isFinal(currentTransition)) {
-      return currentTransition;
-    }
     if (transitionFactory != null) {
-      return composeTransitions(
+      return ComposingTransition.of(
           currentTransition, transitionFactory.buildTransitionFor(toTarget.getAssociatedRule()));
     }
     return currentTransition;

src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.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.ImmutableList;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
@@ -40,15 +41,9 @@ public class ComposingTransition implements ConfigurationTransition {
   /**
    * Creates a {@link ComposingTransition} 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 {@link NoTransition}). Use
-   * {@link com.google.devtools.build.lib.analysis.config.TransitionResolver#composeTransitions}
-   * for these cases - it checks for for these states and avoids instantiation appropriately.
    */
   @AutoCodec.Instantiator
-  public ComposingTransition(
-      ConfigurationTransition transition1, ConfigurationTransition transition2) {
+  ComposingTransition(ConfigurationTransition transition1, ConfigurationTransition transition2) {
     this.transition1 = transition1;
     this.transition2 = transition2;
   }
@@ -84,6 +79,46 @@ public boolean equals(Object other) {
         && ((ComposingTransition) other).transition2.equals(this.transition2);
   }
 
+  /**
+   * Creates a {@link ComposingTransition} that applies the sequence: {@code fromOptions ->
+   * transition1 -> transition2 -> toOptions }.
+   *
+   * <p>Note that this method checks for transitions that cannot be composed, such as if one of the
+   * transitions is {@link NoTransition} or the host transition, and returns an efficiently composed
+   * transition.
+   */
+  public static ConfigurationTransition of(
+      ConfigurationTransition transition1, ConfigurationTransition transition2) {
+    Preconditions.checkNotNull(transition1);
+    Preconditions.checkNotNull(transition2);
+
+    if (isFinal(transition1)) {
+      // Since no other transition can be composed with transition1, use it directly.
+      return transition1;
+    } else if (transition1 == NoTransition.INSTANCE) {
+      // Since transition1 causes no changes, use transition2 directly.
+      return transition2;
+    }
+
+    if (transition2 == NoTransition.INSTANCE) {
+      // Since transition2 causes no changes, use transition 1 directly.
+      return transition1;
+    } else if (isFinal(transition2)) {
+      // When the second transition is null or a HOST transition, there's no need to compose. But
+      // this also
+      // improves performance: host transitions are common, and ConfiguredTargetFunction has special
+      // optimized logic to handle them. If they were buried in the last segment of a
+      // ComposingTransition, those optimizations wouldn't trigger.
+      return transition2;
+    }
+
+    return new ComposingTransition(transition1, transition2);
+  }
+
+  private static boolean isFinal(ConfigurationTransition transition) {
+    return transition == NullTransition.INSTANCE || transition.isHostTransition();
+  }
+
   /**
    * Recursively decompose a composing transition into all the {@link ConfigurationTransition}
    * instances that it holds.

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

@@ -0,0 +1,106 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.analysis.config.transitions;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory.TransitionFactoryData;
+
+/**
+ * A transition factory that composes two other transition factories in an ordered sequence.
+ *
+ * <p>Example:
+ *
+ * <pre>
+ *   transitionFactory1: { someSetting = $oldVal + " foo" }
+ *   transitionFactory2: { someSetting = $oldVal + " bar" }
+ *   ComposingTransitionFactory(transitionFactory1, transitionFactory2):
+ *     { someSetting = $oldVal + " foo bar" }
+ * </pre>
+ */
+@AutoValue
+public abstract class ComposingTransitionFactory<T extends TransitionFactoryData>
+    implements TransitionFactory<T> {
+
+  /**
+   * Creates a {@link ComposingTransitionFactory} that applies the given factories in sequence:
+   * {@code fromOptions -> transition1 -> transition2 -> toOptions }.
+   *
+   * <p>Note that this method checks for transition factories that cannot be composed, such as if
+   * one of the transitions is {@link NoTransition} or the host transition, and returns an
+   * efficiently composed transition.
+   */
+  public static <T extends TransitionFactoryData> TransitionFactory<T> of(
+      TransitionFactory<T> transitionFactory1, TransitionFactory<T> transitionFactory2) {
+
+    Preconditions.checkNotNull(transitionFactory1);
+    Preconditions.checkNotNull(transitionFactory2);
+
+    if (isFinal(transitionFactory1)) {
+      // Since no other transition can be composed with transitionFactory1, use it directly.
+      return transitionFactory1;
+    } else if (NoTransition.isInstance(transitionFactory1)) {
+      // Since transitionFactory1 causes no changes, use transitionFactory2 directly.
+      return transitionFactory2;
+    }
+
+    if (NoTransition.isInstance(transitionFactory2)) {
+      // Since transitionFactory2 causes no changes, use transitionFactory1 directly.
+      return transitionFactory1;
+    } else if (isFinal(transitionFactory2)) {
+      // When the second transition is null or a HOST transition, there's no need to compose. But
+      // this also
+      // improves performance: host transitions are common, and ConfiguredTargetFunction has special
+      // optimized logic to handle them. If they were buried in the last segment of a
+      // ComposingTransition, those optimizations wouldn't trigger.
+      return transitionFactory2;
+    }
+
+    return create(transitionFactory1, transitionFactory2);
+  }
+
+  private static <T extends TransitionFactoryData> boolean isFinal(
+      TransitionFactory<T> transitionFactory) {
+    return NullTransition.isInstance(transitionFactory) || transitionFactory.isHost();
+  }
+
+  private static <T extends TransitionFactoryData> TransitionFactory<T> create(
+      TransitionFactory<T> transitionFactory1, TransitionFactory<T> transitionFactory2) {
+    return new AutoValue_ComposingTransitionFactory(transitionFactory1, transitionFactory2);
+  }
+
+  abstract TransitionFactory<T> transitionFactory1();
+
+  abstract TransitionFactory<T> transitionFactory2();
+
+  @Override
+  public ConfigurationTransition create(T data) {
+    ConfigurationTransition transition1 = transitionFactory1().create(data);
+    ConfigurationTransition transition2 = transitionFactory2().create(data);
+    return new ComposingTransition(transition1, transition2);
+  }
+
+  @Override
+  public boolean isHost() {
+    return transitionFactory1().isHost() || transitionFactory2().isHost();
+  }
+
+  @Override
+  public boolean isSplit() {
+    return transitionFactory1().isSplit() || transitionFactory2().isSplit();
+  }
+
+  // TODO(https://github.com/bazelbuild/bazel/issues/7814): Move ComposingTransition here and make
+  // it private.
+}

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

@@ -56,10 +56,5 @@ abstract static class Factory<T extends TransitionFactoryData> implements Transi
     public ConfigurationTransition create(TransitionFactoryData unused) {
       return INSTANCE;
     }
-
-    @Override
-    public boolean isFinal() {
-      return true;
-    }
   }
 }

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

@@ -50,9 +50,4 @@ default boolean isHost() {
   default boolean isSplit() {
     return false;
   }
-
-  /** Returns {@code true} if the result of this {@link TransitionFactory} is a final transition. */
-  default boolean isFinal() {
-    return false;
-  }
 }

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

@@ -762,7 +762,7 @@ java_test(
 java_test(
     name = "analysis_config_test",
     srcs = glob([
-        "analysis/config/*.java",
+        "analysis/config/**/*.java",
     ]),
     tags = ["analysis"],
     test_class = "com.google.devtools.build.lib.AllTests",

src/test/java/com/google/devtools/build/lib/analysis/config/TransitionFactoriesTest.java

@@ -37,7 +37,6 @@ public void hostTransition() {
     assertThat(HostTransition.isInstance(factory)).isTrue();
     assertThat(factory.isHost()).isTrue();
     assertThat(factory.isSplit()).isFalse();
-    assertThat(factory.isFinal()).isTrue();
   }
 
   @Test
@@ -48,7 +47,6 @@ public void noTransition() {
     assertThat(NoTransition.isInstance(factory)).isTrue();
     assertThat(factory.isHost()).isFalse();
     assertThat(factory.isSplit()).isFalse();
-    assertThat(factory.isFinal()).isFalse();
   }
 
   @Test
@@ -59,7 +57,6 @@ public void nullTransition() {
     assertThat(NullTransition.isInstance(factory)).isTrue();
     assertThat(factory.isHost()).isFalse();
     assertThat(factory.isSplit()).isFalse();
-    assertThat(factory.isFinal()).isTrue();
   }
 
   @Test
@@ -70,6 +67,5 @@ public void splitTransition() {
     assertThat(factory).isNotNull();
     assertThat(factory.isHost()).isFalse();
     assertThat(factory.isSplit()).isTrue();
-    assertThat(factory.isFinal()).isFalse();
   }
 }