Require TransitionFactory subclasses to implement transitionType.

This guarantees that it is accurate, the default of `ANY` was too broad. Part of #22248. PiperOrigin-RevId: 632513554 Change-Id: I90b66b45b640ba84b658f23cb56a9016dd853310
bazelbuild · May 10, 2024 · 81ca4aa · 81ca4aa
1 parent 694aab0
commit 81ca4aa
Show file tree

Hide file tree

Showing 21 changed files with 124 additions and 21 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionFactories.java b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionFactories.java
@@ -58,6 +58,11 @@ abstract static class IdentityFactory<T extends TransitionFactory.Data>
     public ConfigurationTransition create(T data) {
       return transition();
     }
+
+    @Override
+    public TransitionType transitionType() {
+      return TransitionType.ANY;
+    }
   }
 
   /** A {@link TransitionFactory} implementation that wraps a split transition. */
@@ -66,6 +71,12 @@ abstract static class SplitTransitionFactory<T extends TransitionFactory.Data>
       implements TransitionFactory<T> {
     abstract SplitTransition splitTransition();
 
+    @Override
+    public TransitionType transitionType() {
+      // Splits can only be used on attributes.
+      return TransitionType.ATTRIBUTE;
+    }
+
     @Override
     public SplitTransition create(T data) {
       return splitTransition();

diff --git a/...com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactory.java b/...com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactory.java
@@ -87,6 +87,12 @@ public ConfigurationTransition create(T data) {
     return new ComposingTransition(transition1, transition2);
   }
 
+  @Override
+  public TransitionType transitionType() {
+    // Both types must match so this is correct.
+    return transitionFactory1().transitionType();
+  }
+
   abstract TransitionFactory<T> transitionFactory1();
 
   abstract TransitionFactory<T> transitionFactory2();

diff --git a/...in/java/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransition.java b/...in/java/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransition.java
@@ -69,6 +69,11 @@ public PatchTransition create(T unused) {
       return INSTANCE;
     }
 
+    @Override
+    public TransitionType transitionType() {
+      return TransitionType.ANY;
+    }
+
     @Override
     public boolean isTool() {
       return true;

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NoTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NoTransition.java
@@ -52,5 +52,10 @@ abstract static class Factory<T extends TransitionFactory.Data> implements Trans
     public ConfigurationTransition create(T unused) {
       return INSTANCE;
     }
+
+    @Override
+    public TransitionType transitionType() {
+      return TransitionType.ANY;
+    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java
@@ -57,5 +57,10 @@ abstract static class Factory<T extends TransitionFactory.Data> implements Trans
     public ConfigurationTransition create(T unused) {
       return INSTANCE;
     }
+
+    @Override
+    public TransitionType transitionType() {
+      return TransitionType.ANY;
+    }
   }
 }
diff --git a/...ain/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionFactory.java b/...ain/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionFactory.java
@@ -57,9 +57,11 @@ interface Data {}
   /** Returns a new {@link ConfigurationTransition}, based on the given data. */
   ConfigurationTransition create(T data);
 
-  default TransitionType transitionType() {
-    return TransitionType.ANY;
-  }
+  /**
+   * Returns a {@link TransitionType} to clarify what data (if any) the factory requires to create a
+   * transation.
+   */
+  TransitionType transitionType();
 
   // TODO(https://github.com/bazelbuild/bazel/issues/7814): Once everything uses TransitionFactory,
   // remove these methods.

diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -36,6 +36,7 @@
 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.TransitionFactory;
+import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory.TransitionType;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -1142,6 +1143,8 @@ public Builder cfg(TransitionFactory<RuleTransitionData> transitionFactory) {
           name);
       Preconditions.checkState(this.transitionFactory == null, "Property cfg has already been set");
       Preconditions.checkNotNull(transitionFactory);
+      Preconditions.checkArgument(
+          transitionFactory.transitionType().isCompatibleWith(TransitionType.RULE));
       Preconditions.checkArgument(!transitionFactory.isSplit());
       this.transitionFactory = transitionFactory;
       return this;

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidPlatformsTransition.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidPlatformsTransition.java
@@ -43,6 +43,11 @@ public static final class AndroidPlatformsTransitionFactory
     public PatchTransition create(RuleTransitionData unused) {
       return INSTANCE;
     }
+
+    @Override
+    public TransitionType transitionType() {
+      return TransitionType.RULE;
+    }
   }
 
   public static TransitionFactory<RuleTransitionData> create() {

diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/BUILD b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/BUILD
@@ -18,6 +18,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/starlark_exposed_rule_transition_factory",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_factory",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/packages",

diff --git a/...gle/devtools/build/skydoc/fakebuildapi/config/FakeConfigFeatureFlagTransitionFactory.java b/...gle/devtools/build/skydoc/fakebuildapi/config/FakeConfigFeatureFlagTransitionFactory.java
@@ -31,4 +31,9 @@ public void addToStarlarkRule(RuleDefinitionEnvironment ctx, RuleClass.Builder b
   public PatchTransition create(RuleTransitionData ruleData) {
     return null;
   }
+
+  @Override
+  public TransitionType transitionType() {
+    return TransitionType.RULE;
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
@@ -27,11 +27,14 @@
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.config.RequiresOptions;
+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.config.transitions.PatchTransition;
+import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
 import com.google.devtools.build.lib.analysis.util.AnalysisCachingTestBase;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.RuleTransitionData;
 import com.google.devtools.build.lib.rules.java.JavaInfo;
 import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider;
 import com.google.devtools.build.lib.testutil.TestConstants;
@@ -1073,11 +1076,19 @@ private void setupDiffResetTesting() throws Exception {
           return !optionsThatCanChange.contains(changedOption);
         });
     builder.overrideTrimmingTransitionFactoryForTesting(
-        (ruleData) -> {
-          if (ruleData.rule().getRuleClassObject().getName().equals("uses_irrelevant")) {
-            return NoTransition.INSTANCE;
+        new TransitionFactory<>() {
+          @Override
+          public ConfigurationTransition create(RuleTransitionData ruleData) {
+            if (ruleData.rule().getRuleClassObject().getName().equals("uses_irrelevant")) {
+              return NoTransition.INSTANCE;
+            }
+            return DiffResetOptions.CLEAR_IRRELEVANT;
+          }
+
+          @Override
+          public TransitionType transitionType() {
+            return TransitionType.RULE;
           }
-          return DiffResetOptions.CLEAR_IRRELEVANT;
         });
     useRuleClassProvider(builder.build());
     scratch.file(

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -441,6 +441,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_factory",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform",

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java b/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java
@@ -306,7 +306,7 @@ def _impl(ctx):
                   .mandatory()
                   .allowedFileTypes()
                   .cfg(
-                      new TransitionFactory<AttributeTransitionData>() {
+                      new TransitionFactory<>() {
                         @Override
                         public SplitTransition create(AttributeTransitionData data) {
                           return new SplitTransition() {
@@ -332,6 +332,11 @@ public Map<String, BuildOptions> split(
                           };
                         }
 
+                        @Override
+                        public TransitionType transitionType() {
+                          return TransitionType.ATTRIBUTE;
+                        }
+
                         @Override
                         public boolean isSplit() {
                           return true;

diff --git a/...ava/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransitionTest.java b/...ava/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransitionTest.java
@@ -32,8 +32,7 @@ public class NoConfigTransitionTest extends BuildViewTestCase {
   private static final MockRule NO_CONFIG_RULE =
       () ->
           MockRule.define(
-              "no_config_rule",
-              (builder, env) -> builder.cfg(unused -> NoConfigTransition.INSTANCE));
+              "no_config_rule", (builder, env) -> builder.cfg(NoConfigTransition.createFactory()));
 
   @Override
   protected ConfiguredRuleClassProvider createRuleClassProvider() {

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java
@@ -28,13 +28,14 @@
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
+import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
-import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory.Data;
 import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.AttributeTransitionData;
 import com.google.devtools.build.lib.packages.AttributeValueSource;
 import com.google.devtools.build.lib.packages.Provider;
 import com.google.devtools.build.lib.packages.StarlarkInfo;
@@ -701,7 +702,19 @@ public void testSubruleAttrs_cannotHaveStarlarkTransitions() throws Exception {
 
   @Test
   public void testSubruleAttrs_cannotHaveNativeTransitions() throws Exception {
-    ev.update("native_transition", (TransitionFactory<Data>) data -> null);
+    ev.update(
+        "native_transition",
+        new TransitionFactory<AttributeTransitionData>() {
+          @Override
+          public ConfigurationTransition create(AttributeTransitionData data) {
+            return null;
+          }
+
+          @Override
+          public TransitionType transitionType() {
+            return TransitionType.ATTRIBUTE;
+          }
+        });
     ev.checkEvalErrorContains(
         "bad cfg for attribute '_foo': subrules may only have target/exec attributes.",
         "_my_subrule = subrule(",

diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java
@@ -123,6 +123,11 @@ public PatchTransition create(RuleTransitionData ruleData) {
       return new PathTransition(
           NonconfigurableAttributeMapper.of(ruleData.rule()).get("path", STRING));
     }
+
+    @Override
+    public TransitionType transitionType() {
+      return TransitionType.RULE;
+    }
   }
 
   private static final class UselessOptionTransition implements PatchTransition {
@@ -152,6 +157,11 @@ public PatchTransition create(RuleTransitionData ruleData) {
       return new UselessOptionTransition(
           NonconfigurableAttributeMapper.of(ruleData.rule()).get("value", STRING));
     }
+
+    @Override
+    public TransitionType transitionType() {
+      return TransitionType.RULE;
+    }
   }
 
   private static final class PathTestRulesModule extends BlazeModule {

diff --git a/src/test/java/com/google/devtools/build/lib/query2/cquery/BUILD b/src/test/java/com/google/devtools/build/lib/query2/cquery/BUILD
@@ -162,6 +162,7 @@ java_test(
         ":configured_target_query_test",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transition_factories",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_factory",
         "//src/main/java/com/google/devtools/build/lib/events",

diff --git a/.../java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQuerySemanticsTest.java b/.../java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQuerySemanticsTest.java
@@ -287,7 +287,9 @@ public void testTopLevelTransition() throws Exception {
             MockRule.define(
                 "rule_class_transition",
                 (builder, env) ->
-                    builder.cfg(unused -> new FooPatchTransition("SET BY PATCH")).build());
+                    builder
+                        .cfg(TransitionFactories.of(new FooPatchTransition("SET BY PATCH")))
+                        .build());
 
     helper.useRuleClassProvider(setRuleClassProviders(ruleClassTransition).build());
     helper.setUniverseScope("//test:rule_class");

diff --git a/...test/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterTest.java b/...test/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterTest.java
@@ -21,6 +21,7 @@
 import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.config.TransitionFactories;
+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.config.transitions.TransitionFactory;
 import com.google.devtools.build.lib.analysis.util.MockRule;
@@ -219,12 +220,20 @@ public void nonAttributeDependencySkipped() throws Exception {
 
   private void setUpRules() throws Exception {
     TransitionFactory<RuleTransitionData> infixTrimmingTransitionFactory =
-        (ruleData) -> {
-          if (!ruleData.rule().getName().contains("trimmed")) {
-            return NoTransition.INSTANCE;
+        new TransitionFactory<>() {
+          @Override
+          public ConfigurationTransition create(RuleTransitionData ruleData) {
+            if (!ruleData.rule().getName().contains("trimmed")) {
+              return NoTransition.INSTANCE;
+            }
+            // rename the transition so it's distinguishable from the others in tests
+            return new FooPatchTransition("SET BY TRIM", "FooPatchTransition(trim)");
+          }
+
+          @Override
+          public TransitionType transitionType() {
+            return TransitionType.RULE;
           }
-          // rename the transition so it's distinguishable from the others in tests
-          return new FooPatchTransition("SET BY TRIM", "FooPatchTransition(trim)");
         };
     FooPatchTransition ruleClassTransition = new FooPatchTransition("SET BY RULE CLASS PATCH");
     FooPatchTransition attributePatchTransition = new FooPatchTransition("SET BY PATCH");
@@ -237,7 +246,7 @@ private void setUpRules() throws Exception {
                 "my_rule",
                 (builder, env) ->
                     builder
-                        .cfg(unused -> ruleClassTransition)
+                        .cfg(TransitionFactories.of(ruleClassTransition))
                         .add(
                             attr("patched", LABEL_LIST)
                                 .allowedFileTypes(FileTypeSet.ANY_FILE)

diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD b/src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD
@@ -25,6 +25,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/transition_factories",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
         "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:common",

diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java
@@ -27,6 +27,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
+import com.google.devtools.build.lib.analysis.config.TransitionFactories;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.util.DummyTestFragment.DummyTestOptions;
 import com.google.devtools.build.lib.analysis.util.MockRule;
@@ -618,7 +619,9 @@ public void testMultipleTopLevelConfigurations() throws Exception {
             MockRule.define(
                 "transitioned_rule",
                 (builder, env) ->
-                    builder.cfg(unused -> new FooPatchTransition("SET BY PATCH")).build());
+                    builder
+                        .cfg(TransitionFactories.of(new FooPatchTransition("SET BY PATCH")))
+                        .build());
 
     MockRule untransitionedRule = () -> MockRule.define("untransitioned_rule");
 
@@ -653,7 +656,7 @@ public void testMultipleTopLevelConfigurations_multipleConfigsPrefersTopLevel()
                 "rule_with_transition_and_dep",
                 (builder, env) ->
                     builder
-                        .cfg(unused -> new FooPatchTransition("SET BY PATCH"))
+                        .cfg(TransitionFactories.of(new FooPatchTransition("SET BY PATCH")))
                         .addAttribute(
                             attr("dep", LABEL).allowedFileTypes(FileTypeSet.ANY_FILE).build())
                         .build());