Cleanup incoming configuration transitions on Starlark rules

Remove PatchTransition and TransitionFactory. Those don't seem to be possible inputs in Starlark, because they don't implement StarlarkValue, with exception of ExecTransitionFactory. Using exec as an incoming transition seems like a really weird use case, and it's probably better not to support it, than making Bazel code more complex.

There seem to be no uses of exec incoming transition on github: https://github.com/search?q=%22cfg+%3D+config%22+language%3AStarlark&type=repositories&ref=advsearch

Remove RuleClass.cfg(PatchTransition). It was only used in tests. Inline the uses.
Add Nullable to RuleClass.transitionFactory.

This is preparation to support cfg on extended rules.

RELNOTES[INC]: Incoming transitions on rules can't be set to "exec" transition.

PiperOrigin-RevId: 582969396
Change-Id: I1e6391d17671fdfa60e1945561396107172c6a4a

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -44,10 +44,8 @@
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
 import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
 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.analysis.config.transitions.StarlarkExposedRuleTransitionFactory;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
-import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory.TransitionType;
 import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
 import com.google.devtools.build.lib.analysis.starlark.StarlarkAttrModule.Descriptor;
 import com.google.devtools.build.lib.analysis.test.TestConfiguration;
@@ -84,7 +82,6 @@
 import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
 import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException;
 import com.google.devtools.build.lib.packages.RuleFunction;
-import com.google.devtools.build.lib.packages.RuleTransitionData;
 import com.google.devtools.build.lib.packages.StarlarkAspect;
 import com.google.devtools.build.lib.packages.StarlarkCallbackHelper;
 import com.google.devtools.build.lib.packages.StarlarkDefinedAspect;
@@ -650,31 +647,17 @@ public static StarlarkRuleFunction createRule(
     }
     if (!cfg.equals(Starlark.NONE)) {
       if (cfg instanceof StarlarkDefinedConfigTransition) {
+        // defined in Starlark via, cfg = transition
         StarlarkDefinedConfigTransition starlarkDefinedConfigTransition =
             (StarlarkDefinedConfigTransition) cfg;
         builder.cfg(new StarlarkRuleTransitionProvider(starlarkDefinedConfigTransition));
         hasStarlarkDefinedTransition = true;
-      } else if (cfg instanceof PatchTransition) {
-        builder.cfg((PatchTransition) cfg);
       } else if (cfg instanceof StarlarkExposedRuleTransitionFactory) {
+        // only used for native Android transitions (platforms and feature flags)
         StarlarkExposedRuleTransitionFactory transition =
             (StarlarkExposedRuleTransitionFactory) cfg;
         builder.cfg(transition);
         transition.addToStarlarkRule(ruleDefinitionEnvironment, builder);
-      } else if (cfg instanceof TransitionFactory) {
-        // This may be redundant with StarlarkExposedRuleTransitionFactory infra
-        TransitionFactory<? extends TransitionFactory.Data> transitionFactory =
-            (TransitionFactory<? extends TransitionFactory.Data>) cfg;
-        if (transitionFactory.transitionType().isCompatibleWith(TransitionType.RULE)) {
-          @SuppressWarnings("unchecked") // Actually checked due to above isCompatibleWith call.
-          TransitionFactory<RuleTransitionData> ruleTransitionFactory =
-              (TransitionFactory<RuleTransitionData>) transitionFactory;
-          builder.cfg(ruleTransitionFactory);
-        } else {
-          throw Starlark.errorf(
-              "`cfg` must be set to a transition appropriate for a rule, not an attribute-specific"
-                  + " transition.");
-        }
       } else {
         throw Starlark.errorf(
             "`cfg` must be set to a transition object initialized by the transition() function.");

src/main/java/com/google/devtools/build/lib/packages/RuleClass.java

@@ -35,8 +35,6 @@
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.analysis.config.Fragment;
 import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
-import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
-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.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
@@ -59,7 +57,6 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.errorprone.annotations.FormatMethod;
-import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.BitSet;
@@ -772,7 +769,7 @@ public String toString() {
         ImmutableList.builder();
     private boolean ignoreLicenses = false;
     private ImplicitOutputsFunction implicitOutputsFunction = ImplicitOutputsFunction.NONE;
-    private TransitionFactory<RuleTransitionData> transitionFactory;
+    @Nullable private TransitionFactory<RuleTransitionData> transitionFactory;
     private ConfiguredTargetFactory<?, ?, ?> configuredTargetFactory = null;
     private PredicateWithMessage<Rule> validityPredicate = PredicatesWithMessage.alwaysTrue();
     private final AdvertisedProviderSet.Builder advertisedProviders =
@@ -1171,26 +1168,8 @@ public Builder setImplicitOutputsFunction(ImplicitOutputsFunction implicitOutput
       return this;
     }
 
-    /**
-     * Applies the given transition to all incoming edges for this rule class.
-     *
-     * <p>This cannot be a {@link SplitTransition} because that requires coordination with the
-     * rule's parent: use {@link Attribute.Builder#cfg(TransitionFactory)} on the parent to declare
-     * splits.
-     *
-     * <p>If you need the transition to depend on the rule it's being applied to, use {@link
-     * #cfg(TransitionFactory)}.
-     */
-    public Builder cfg(PatchTransition transition) {
-      // Make sure this is cast to Serializable to avoid autocodec serialization errors.
-      return cfg((TransitionFactory<RuleTransitionData> & Serializable) unused -> transition);
-    }
-
     /**
      * Applies the given transition factory to all incoming edges for this rule class.
-     *
-     * <p>Unlike {@link #cfg(PatchTransition)}, the factory can examine the rule when deciding what
-     * transition to use.
      */
     @CanIgnoreReturnValue
     public Builder cfg(TransitionFactory<RuleTransitionData> transitionFactory) {
@@ -1724,7 +1703,7 @@ public Attribute.Builder<?> copy(String name) {
    * A factory which will produce a configuration transition that should be applied on any edge of
    * the configured target graph that leads into a target of this rule class.
    */
-  private final TransitionFactory<RuleTransitionData> transitionFactory;
+  @Nullable private final TransitionFactory<RuleTransitionData> transitionFactory;
 
   /** The factory that creates configured targets from this rule. */
   private final ConfiguredTargetFactory<?, ?, ?> configuredTargetFactory;
@@ -1836,7 +1815,7 @@ public Attribute.Builder<?> copy(String name) {
       ImmutableList<AllowlistChecker> allowlistCheckers,
       boolean ignoreLicenses,
       ImplicitOutputsFunction implicitOutputsFunction,
-      TransitionFactory<RuleTransitionData> transitionFactory,
+      @Nullable TransitionFactory<RuleTransitionData> transitionFactory,
       ConfiguredTargetFactory<?, ?, ?> configuredTargetFactory,
       PredicateWithMessage<Rule> validityPredicate,
       AdvertisedProviderSet advertisedProviders,
@@ -1945,6 +1924,7 @@ public ImplicitOutputsFunction getDefaultImplicitOutputsFunction() {
     return implicitOutputsFunction;
   }
 
+  @Nullable
   public TransitionFactory<RuleTransitionData> getTransitionFactory() {
     return transitionFactory;
   }

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

@@ -32,7 +32,8 @@ public class NoConfigTransitionTest extends BuildViewTestCase {
   private static final MockRule NO_CONFIG_RULE =
       () ->
           MockRule.define(
-              "no_config_rule", (builder, env) -> builder.cfg(NoConfigTransition.INSTANCE));
+              "no_config_rule",
+              (builder, env) -> builder.cfg(unused -> NoConfigTransition.INSTANCE));
 
   @Override
   protected ConfiguredRuleClassProvider createRuleClassProvider() {

src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQuerySemanticsTest.java

@@ -241,7 +241,8 @@ public void testTopLevelTransition() throws Exception {
         () ->
             MockRule.define(
                 "rule_class_transition",
-                (builder, env) -> builder.cfg(new FooPatchTransition("SET BY PATCH")).build());
+                (builder, env) ->
+                    builder.cfg(unused -> new FooPatchTransition("SET BY PATCH")).build());
 
     helper.useRuleClassProvider(setRuleClassProviders(ruleClassTransition).build());
     helper.setUniverseScope("//test:rule_class");

src/test/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterTest.java

@@ -205,7 +205,7 @@ private void setUpRules() throws Exception {
                 "my_rule",
                 (builder, env) ->
                     builder
-                        .cfg(ruleClassTransition)
+                        .cfg(unused -> ruleClassTransition)
                         .add(
                             attr("patched", LABEL_LIST)
                                 .allowedFileTypes(FileTypeSet.ANY_FILE)

src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java

@@ -555,7 +555,8 @@ public void testMultipleTopLevelConfigurations() throws Exception {
         () ->
             MockRule.define(
                 "transitioned_rule",
-                (builder, env) -> builder.cfg(new FooPatchTransition("SET BY PATCH")).build());
+                (builder, env) ->
+                    builder.cfg(unused -> new FooPatchTransition("SET BY PATCH")).build());
 
     MockRule untransitionedRule = () -> MockRule.define("untransitioned_rule");
 
@@ -587,7 +588,7 @@ public void testMultipleTopLevelConfigurations_multipleConfigsPrefersTopLevel()
                 "rule_with_transition_and_dep",
                 (builder, env) ->
                     builder
-                        .cfg(new FooPatchTransition("SET BY PATCH"))
+                        .cfg(unused -> new FooPatchTransition("SET BY PATCH"))
                         .addAttribute(
                             attr("dep", LABEL).allowedFileTypes(FileTypeSet.ANY_FILE).build())
                         .build());

src/test/shell/integration/starlark_configurations_test.sh

@@ -740,7 +740,7 @@ EOF
 
   bazel build //$pkg:demo >& "$TEST_log" && fail "Expected failure"
   expect_not_log "crashed due to an internal error"
-  expect_log "`cfg` must be set to a transition appropriate for a rule"
+  expect_log "`cfg` must be set to a transition object initialized by the transition() function"
 }
 
 run_suite "${PRODUCT_NAME} starlark configurations tests"