From 04288289f90c6a81415cd89c6ad74eee2e17da5b Mon Sep 17 00:00:00 2001 From: John Cater Date: Wed, 28 Jul 2021 10:36:20 -0700 Subject: [PATCH] Add TransitionType enum and add checks that transitions aren't used inappropriately. Fixes #13751. Closes #13755. PiperOrigin-RevId: 387382475 --- .../config/ExecutionTransitionFactory.java | 5 ++++ .../ComposingTransitionFactory.java | 3 +++ .../config/transitions/TransitionFactory.java | 24 +++++++++++++++++ .../StarlarkAttributeTransitionProvider.java | 5 ++++ .../starlark/StarlarkRuleClassFunctions.java | 17 +++++++++--- .../StarlarkRuleTransitionProvider.java | 5 ++++ .../test/TestTrimmingTransitionFactory.java | 5 ++++ ...reFlagTaggedTrimmingTransitionFactory.java | 5 ++++ .../ConfigFeatureFlagTransitionFactory.java | 5 ++++ .../MultiArchSplitTransitionProvider.java | 5 ++++ .../rules/python/PyStarlarkTransitions.java | 5 ++++ .../build/lib/packages/AttributeTest.java | 5 ++++ .../starlark_configurations_test.sh | 27 +++++++++++++++++++ 13 files changed, 112 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java index a4f33bece00702..175ddf73d56da9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java @@ -53,6 +53,11 @@ public static ExecutionTransitionFactory create() { return new ExecutionTransitionFactory(DEFAULT_EXEC_GROUP_NAME); } + @Override + public TransitionType transitionType() { + return TransitionType.ATTRIBUTE; + } + /** * Returns a new {@link ExecutionTransitionFactory} for the given {@link * com.google.devtools.build.lib.packages.ExecGroup}. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactory.java index 7ad4d523d95709..99147ef50ba148 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactory.java @@ -45,6 +45,9 @@ public static TransitionFactory of( Preconditions.checkNotNull(transitionFactory1); Preconditions.checkNotNull(transitionFactory2); + Preconditions.checkArgument( + transitionFactory1.transitionType().isCompatibleWith(transitionFactory2.transitionType()), + "transition factory types must be compatible"); Preconditions.checkArgument( !transitionFactory1.isSplit() || !transitionFactory2.isSplit(), "can't compose two split transition factories"); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionFactory.java index ef76b3c0a5875e..3b852b3c6a185a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionFactory.java @@ -31,12 +31,36 @@ */ public interface TransitionFactory { + /** Enum that describes what type of transition a TransitionFactory creates. */ + enum TransitionType { + /** A transition that can be used for rules or attributes. */ + ANY, + /** A transition that can be used for rules only. */ + RULE, + /** A transition that can be used for attributes only. */ + ATTRIBUTE; + + public boolean isCompatibleWith(TransitionType other) { + if (this == ANY) { + return true; + } + if (other == ANY) { + return true; + } + return this == other; + } + } + /** A marker interface for classes that provide data to TransitionFactory instances. */ interface Data {} /** Returns a new {@link ConfigurationTransition}, based on the given data. */ ConfigurationTransition create(T data); + default TransitionType transitionType() { + return TransitionType.ANY; + } + // TODO(https://github.com/bazelbuild/bazel/issues/7814): Once everything uses TransitionFactory, // remove these methods. /** Returns {@code true} if the result of this {@link TransitionFactory} is a host transition. */ diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java index e6c3e88d18f5c6..63f1d2e62458b9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java @@ -74,6 +74,11 @@ public SplitTransition create(AttributeTransitionData data) { starlarkDefinedConfigTransition, (ConfiguredAttributeMapper) attributeMap); } + @Override + public TransitionType transitionType() { + return TransitionType.ATTRIBUTE; + } + @Override public boolean isSplit() { return true; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index 3f00539d4c4392..89ee416a16fcde 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -43,6 +43,7 @@ import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition; 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.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; @@ -414,10 +415,18 @@ public StarlarkCallable rule( } else if (cfg instanceof PatchTransition) { builder.cfg((PatchTransition) cfg); } else if (cfg instanceof TransitionFactory) { - @SuppressWarnings("unchecked") - TransitionFactory transitionFactory = - (TransitionFactory) cfg; - builder.cfg(transitionFactory); + TransitionFactory transitionFactory = + (TransitionFactory) cfg; + if (transitionFactory.transitionType().isCompatibleWith(TransitionType.RULE)) { + @SuppressWarnings("unchecked") // Actually checked due to above isCompatibleWith call. + TransitionFactory ruleTransitionFactory = + (TransitionFactory) 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 { // This is not technically true: it could also be a native transition, but this is the // most likely error case. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java index b39a2cdd78febe..847592a7c02f4c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java @@ -130,6 +130,11 @@ public PatchTransition create(RuleTransitionData ruleData) { unused -> new FunctionPatchTransition(starlarkDefinedConfigTransition, ruleData.rule())); } + @Override + public TransitionType transitionType() { + return TransitionType.RULE; + } + @Override public boolean isSplit() { // The transitions returned by this factory are guaranteed not to be splits. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java index 9f0b8e8528d0f2..3517a07fdc4e7f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java @@ -130,4 +130,9 @@ public PatchTransition create(RuleTransitionData ruleData) { } return TestTrimmingTransition.TESTONLY_FALSE; } + + @Override + public TransitionType transitionType() { + return TransitionType.RULE; + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTaggedTrimmingTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTaggedTrimmingTransitionFactory.java index c1ccb36fb40862..72c9a629cdd8f7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTaggedTrimmingTransitionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTaggedTrimmingTransitionFactory.java @@ -122,4 +122,9 @@ public PatchTransition create(RuleTransitionData ruleData) { return new ConfigFeatureFlagTaggedTrimmingTransition(requiredLabels); } + + @Override + public TransitionType transitionType() { + return TransitionType.RULE; + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTransitionFactory.java index d06253405d98b4..5c82a2a586877b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTransitionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTransitionFactory.java @@ -112,6 +112,11 @@ public PatchTransition create(RuleTransitionData ruleData) { } } + @Override + public TransitionType transitionType() { + return TransitionType.RULE; + } + /** * Returns the attribute examined by this transition factory. */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchSplitTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchSplitTransitionProvider.java index 388f4953b75cab..7cbbbc4a161bcc 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchSplitTransitionProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchSplitTransitionProvider.java @@ -171,6 +171,11 @@ public SplitTransition create(AttributeTransitionData data) { return new AppleBinaryTransition(platformType, minimumOsVersion); } + @Override + public TransitionType transitionType() { + return TransitionType.ATTRIBUTE; + } + @Override public boolean isSplit() { return true; diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitions.java index eb7ac6dca57738..e8996b90f62bb0 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyStarlarkTransitions.java @@ -68,6 +68,11 @@ public ConfigurationTransition create(AttributeTransitionData data) { } } + @Override + public TransitionType transitionType() { + return TransitionType.ATTRIBUTE; + } + @Override public void repr(Printer printer) { printer.append(""); diff --git a/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java b/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java index b23bf991152ee5..6bce873cb1c4b6 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java @@ -302,6 +302,11 @@ public SplitTransition create(AttributeTransitionData data) { return new TestSplitTransition(); } + @Override + public TransitionType transitionType() { + return TransitionType.ATTRIBUTE; + } + @Override public boolean isSplit() { return true; diff --git a/src/test/shell/integration/starlark_configurations_test.sh b/src/test/shell/integration/starlark_configurations_test.sh index ddf298ffdfface..14683d95cd815c 100755 --- a/src/test/shell/integration/starlark_configurations_test.sh +++ b/src/test/shell/integration/starlark_configurations_test.sh @@ -701,4 +701,31 @@ function test_rc_flag_alias_unsupported_with_space_assignment_syntax() { write_default_bazelrc } +# Regression test for https://github.com/bazelbuild/bazel/issues/13751. +function test_rule_exec_transition_warning() { + local -r pkg=$FUNCNAME + mkdir -p $pkg + + cat > "${pkg}/rules.bzl" < "${pkg}/BUILD" <& "$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" +} + run_suite "${PRODUCT_NAME} starlark configurations tests"