Move whitelist checks so they don't crash on user error. As a bonus, these are now only done on attributes of starlark-defined rules instead of all rules attributes. PiperOrigin-RevId: 226952583
diff --git a/src/main/java/com/google/devtools/build/docgen/RuleDocumentationAttribute.java b/src/main/java/com/google/devtools/build/docgen/RuleDocumentationAttribute.java index 922c331..2d5ea8f 100644 --- a/src/main/java/com/google/devtools/build/docgen/RuleDocumentationAttribute.java +++ b/src/main/java/com/google/devtools/build/docgen/RuleDocumentationAttribute.java
@@ -173,7 +173,7 @@ return ""; } String prefix = "; default is "; - Object value = attribute.getDefaultValueForTesting(); + Object value = attribute.getDefaultValueUnchecked(); if (value instanceof Boolean) { return prefix + ((Boolean) value ? "True" : "False"); } else if (value instanceof Integer) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java index bc9385a..c9931dc8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -47,6 +47,7 @@ import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.AttributeValueSource; import com.google.devtools.build.lib.packages.BuildSetting; +import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.FunctionSplitTransitionWhitelist; import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SkylarkImplicitOutputsFunctionWithCallback; import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SkylarkImplicitOutputsFunctionWithMap; @@ -652,17 +653,63 @@ throw new EvalException(definitionLocation, "Invalid rule class name '" + ruleClassName + "', test rule class names must end with '_test' and other rule classes must not"); } + boolean hasStarlarkDefinedTransition = false; + boolean hasFunctionTransitionWhitelist = false; for (Pair<String, SkylarkAttr.Descriptor> attribute : attributes) { String name = attribute.getFirst(); SkylarkAttr.Descriptor descriptor = attribute.getSecond(); - addAttribute(definitionLocation, builder, descriptor.build(name)); + Attribute attr = descriptor.build(name); + hasStarlarkDefinedTransition |= attr.hasStarlarkDefinedTransition(); + if (attr.hasAnalysisTestTransition() && !builder.isAnalysisTest()) { + throw new EvalException( + location, + "Only rule definitions with analysis_test=True may have attributes with " + + "analysis_test_transition transitions"); + } // Check for existence of the function transition whitelist attribute. + // TODO(b/121385274): remove when we stop whitelisting starlark transitions if (name.equals(FunctionSplitTransitionWhitelist.WHITELIST_ATTRIBUTE_NAME)) { + if (!BuildType.isLabelType(attr.getType())) { + throw new EvalException( + location, "_whitelist_function_transition attribute must be a label type"); + } + if (attr.getDefaultValueUnchecked() == null) { + throw new EvalException( + location, "_whitelist_function_transition attribute must have a default value"); + } + if (!attr.getDefaultValueUnchecked() + .equals(FunctionSplitTransitionWhitelist.WHITELIST_LABEL)) { + throw new EvalException( + location, + "_whitelist_function_transition attribute does not have the expected value " + + FunctionSplitTransitionWhitelist.WHITELIST_LABEL); + } + hasFunctionTransitionWhitelist = true; builder.setHasFunctionTransitionWhitelist(); } + addAttribute(definitionLocation, builder, attr); } + // TODO(b/121385274): remove when we stop whitelisting starlark transitions + if (hasStarlarkDefinedTransition) { + if (!hasFunctionTransitionWhitelist) { + throw new EvalException( + location, + String.format( + "Use of function-based split transition without whitelist: %s %s", + builder.getRuleDefinitionEnvironmentLabel(), builder.getType())); + } + } else { + if (hasFunctionTransitionWhitelist) { + throw new EvalException( + location, + String.format( + "Unused function-based split transition whitelist: %s %s", + builder.getRuleDefinitionEnvironmentLabel(), builder.getType())); + } + } + try { this.ruleClass = builder.build(ruleClassName, skylarkLabel + "%" + ruleClassName); } catch (IllegalArgumentException | IllegalStateException ex) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index 7f20fa6..0399edc 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -2002,19 +2002,6 @@ "a late bound default value using the host configuration must use the host transition"); } - if (name.equals(FunctionSplitTransitionWhitelist.WHITELIST_ATTRIBUTE_NAME)) { - Preconditions.checkArgument( - BuildType.isLabelType(type), - "_whitelist_function_transition attribute must be a label"); - Preconditions.checkArgument( - defaultValue != null, - "_whitelist_function_transition attribute must have a default value"); - Preconditions.checkArgument( - ((Label) defaultValue).equals(FunctionSplitTransitionWhitelist.WHITELIST_LABEL), - "_whitelist_function_transition attribute does not have the expected value " - + FunctionSplitTransitionWhitelist.WHITELIST_LABEL); - } - this.name = name; this.type = type; this.propertyFlags = propertyFlags; @@ -2339,7 +2326,7 @@ * or a late-bound default. */ @VisibleForTesting - public Object getDefaultValueForTesting() { + public Object getDefaultValueUnchecked() { return defaultValue; }
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 index 28184d6..71d2e97 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -781,9 +781,6 @@ .nonconfigurable("Used in toolchain resolution") .value(ImmutableList.of())); } - if (skylark) { - assertRuleClassProperStarlarkDefinedTransitionUsage(); - } if (buildSetting != null) { Type<?> type = buildSetting.getType(); Attribute.Builder<?> attrBuilder = @@ -853,35 +850,6 @@ type); } - private void assertRuleClassProperStarlarkDefinedTransitionUsage() { - boolean hasStarlarkDefinedTransition = false; - boolean hasAnalysisTestTransitionAttribute = false; - for (Attribute attribute : attributes.values()) { - hasStarlarkDefinedTransition |= attribute.hasStarlarkDefinedTransition(); - hasAnalysisTestTransitionAttribute |= attribute.hasAnalysisTestTransition(); - } - - if (hasAnalysisTestTransitionAttribute) { - Preconditions.checkState( - isAnalysisTest, - "Only rule definitions with analysis_test=True may have attributes with " - + "analysis_test_transition transitions"); - } - if (hasStarlarkDefinedTransition) { - Preconditions.checkState( - hasFunctionTransitionWhitelist, - "Use of function based split transition without whitelist: %s %s", - ruleDefinitionEnvironmentLabel, - type); - } else { - Preconditions.checkState( - !hasFunctionTransitionWhitelist, - "Unused function based split transition whitelist: %s %s", - ruleDefinitionEnvironmentLabel, - type); - } - } - /** * Declares that the implementation of the associated rule class requires the given * fragments to be present in this rule's host and target configurations. @@ -1187,6 +1155,10 @@ return this; } + public Label getRuleDefinitionEnvironmentLabel() { + return this.ruleDefinitionEnvironmentLabel; + } + /** * Removes an attribute with the same name from this rule class. * @@ -1215,6 +1187,10 @@ return this; } + public boolean isAnalysisTest() { + return this.isAnalysisTest; + } + /** * This rule class has the _whitelist_function_transition attribute. Intended only for Skylark * rules. @@ -1224,6 +1200,10 @@ return this; } + public RuleClassType getType() { + return this.type; + } + /** * Sets the kind of output files this rule creates. * DO NOT USE! This only exists to support the non-open-sourced {@code fileset} rule.
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/FunctionSplitTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/FunctionSplitTransitionProviderTest.java index 02d32f3..8e5d867 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/FunctionSplitTransitionProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/FunctionSplitTransitionProviderTest.java
@@ -23,7 +23,6 @@ import com.google.devtools.build.lib.packages.util.BazelMockAndroidSupport; import java.util.List; import java.util.Map; -import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -34,12 +33,6 @@ @RunWith(JUnit4.class) public class FunctionSplitTransitionProviderTest extends BuildViewTestCase { - @Before - public void disablePackageLoadingChecks() throws Exception { - // TODO(b/121335551): Re-enable the checks. - initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ false); - } - private void writeWhitelistFile() throws Exception { scratch.file( "tools/whitelists/function_transition_whitelist/BUILD",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java index 1bd508a..8aa2866 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java
@@ -57,7 +57,7 @@ private static Label getDefaultMallocLabel(Rule rule) { return Verify.verifyNotNull( - (Label) rule.getRuleClassObject().getAttributeByName("malloc").getDefaultValueForTesting()); + (Label) rule.getRuleClassObject().getAttributeByName("malloc").getDefaultValueUnchecked()); } /**
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java index eb3544b..85ba7fc 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -524,8 +524,8 @@ */ private void checkValidComputedDefault(Object expectedValue, Attribute computedDefault, ImmutableMap<String, Object> attrValueMap) throws Exception { - assertThat(computedDefault.getDefaultValueForTesting() instanceof Attribute.ComputedDefault) - .isTrue(); + assertThat(computedDefault.getDefaultValueUnchecked()) + .isInstanceOf(Attribute.ComputedDefault.class); Rule rule = createRule(getRuleClassWithComputedDefault(computedDefault), "myRule", attrValueMap, testRuleLocation); AttributeMap attributes = RawAttributeMapper.of(rule);
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java index 88c6e28..f58e332 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static com.google.devtools.build.lib.analysis.OutputGroupInfo.INTERNAL_SUFFIX; +import static com.google.devtools.build.lib.packages.FunctionSplitTransitionWhitelist.WHITELIST_ATTRIBUTE_NAME; import static org.junit.Assert.fail; import com.google.common.base.Joiner; @@ -2434,6 +2435,180 @@ dependingRulesChain.toString()); } + @Test + public void testBadWhitelistTransition_onNonLabelAttr() throws Exception { + scratch.file( + "test/rules.bzl", + "def _impl(ctx):", + " return []", + "", + "my_rule = rule(_impl, attrs = {'" + + WHITELIST_ATTRIBUTE_NAME + + "':attr.string(default = 'blah')})"); + scratch.file("test/BUILD", "load('//test:rules.bzl', 'my_rule')", "my_rule(name = 'my_rule')"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test:my_rule"); + assertContainsEvent("_whitelist_function_transition attribute must be a label type"); + } + + @Test + public void testBadWhitelistTransition_noDefaultValue() throws Exception { + scratch.file( + "test/rules.bzl", + "def _impl(ctx):", + " return []", + "", + "my_rule = rule(_impl, attrs = {'" + WHITELIST_ATTRIBUTE_NAME + "':attr.label()})"); + scratch.file("test/BUILD", "load('//test:rules.bzl', 'my_rule')", "my_rule(name = 'my_rule')"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test:my_rule"); + assertContainsEvent("_whitelist_function_transition attribute must have a default value"); + } + + @Test + public void testBadWhitelistTransition_wrongDefaultValue() throws Exception { + scratch.file( + "test/rules.bzl", + "def _impl(ctx):", + " return []", + "", + "my_rule = rule(_impl, attrs = {'" + + WHITELIST_ATTRIBUTE_NAME + + "':attr.label(default = Label('//test:my_other_rule'))})"); + scratch.file( + "test/BUILD", + "load('//test:rules.bzl', 'my_rule')", + "my_rule(name = 'my_rule')", + "my_rule(name = 'my_other_rule')"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test:my_rule"); + assertContainsEvent( + "_whitelist_function_transition attribute does not have the expected value"); + } + + @Test + public void testBadAnalysisTestRule_notAnalysisTest() throws Exception { + scratch.file( + "test/extension.bzl", + "", + "def outer_rule_impl(ctx):", + " return [AnalysisTestResultInfo(success = True, message = 'message contents')]", + "def dep_rule_impl(ctx):", + " return []", + "", + "my_transition = analysis_test_transition(", + " settings = {", + " '//command_line_option:experimental_strict_java_deps' : 'WARN' }", + ")", + "dep_rule = rule(", + " implementation = dep_rule_impl,", + " attrs = {'dep': attr.label()}", + ")", + "outer_rule = rule(", + " implementation = outer_rule_impl,", + "# analysis_test = True,", + " fragments = ['java'],", + " attrs = {", + " 'dep': attr.label(cfg = my_transition),", + " })"); + + scratch.file( + "test/BUILD", + "load('//test:extension.bzl', 'dep_rule', 'outer_rule_test')", + "", + "outer_rule(name = 'r', dep = ':inner')", + "dep_rule(name = 'inner')"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test:outer_rule"); + assertContainsEvent( + "Only rule definitions with analysis_test=True may have attributes with " + + "analysis_test_transition transitions"); + } + + @Test + public void testBadWhitelistTransition_noWhitelist() throws Exception { + scratch.file( + "tools/whitelists/function_transition_whitelist/BUILD", + "package_group(", + " name = 'function_transition_whitelist',", + " packages = [", + " '//test/...',", + " ],", + ")"); + scratch.file( + "test/rules.bzl", + "def transition_func(settings):", + " return {'t0': {'//command_line_option:cpu': 'k8'}}", + "my_transition = transition(implementation = transition_func, inputs = [],", + " outputs = ['//command_line_option:cpu'])", + "def _my_rule_impl(ctx): ", + " return []", + "my_rule = rule(", + " implementation = _my_rule_impl,", + " attrs = {", + " 'dep': attr.label(cfg = my_transition),", + "# '_whitelist_function_transition': attr.label(", + "# default = '//tools/whitelists/function_transition_whitelist',", + "# ),", + " })", + "def _simple_rule_impl(ctx):", + " return []", + "simple_rule = rule(_simple_rule_impl)"); + + scratch.file( + "test/BUILD", + "load('//test:rules.bzl', 'my_rule', 'simple_rule')", + "my_rule(name = 'my_rule', dep = ':dep')", + "simple_rule(name = 'dep')"); + setSkylarkSemanticsOptions("--experimental_starlark_config_transitions"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test:my_rule"); + assertContainsEvent("Use of function-based split transition without whitelist"); + } + + @Test + public void testBadWhitelistTransition_whitelistNoCfg() throws Exception { + scratch.file( + "tools/whitelists/function_transition_whitelist/BUILD", + "package_group(", + " name = 'function_transition_whitelist',", + " packages = [", + " '//test/...',", + " ],", + ")"); + scratch.file( + "test/rules.bzl", + "def _my_rule_impl(ctx): ", + " return []", + "my_rule = rule(", + " implementation = _my_rule_impl,", + " attrs = {", + "# 'dep': attr.label(cfg = my_transition),", + " '_whitelist_function_transition': attr.label(", + " default = '//tools/whitelists/function_transition_whitelist',", + " ),", + " })", + "def _simple_rule_impl(ctx):", + " return []", + "simple_rule = rule(_simple_rule_impl)"); + + scratch.file( + "test/BUILD", + "load('//test:rules.bzl', 'my_rule', 'simple_rule')", + "my_rule(name = 'my_rule', dep = ':dep')", + "simple_rule(name = 'dep')"); + setSkylarkSemanticsOptions("--experimental_starlark_config_transitions"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test:my_rule"); + assertContainsEvent("Unused function-based split transition whitelist"); + } + /** * Skylark integration test that forces inlining. */
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java index 7579775..7d15cee 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -488,13 +488,13 @@ @Test public void testAttrDefaultValue() throws Exception { Attribute attr = buildAttribute("a1", "attr.string(default = 'some value')"); - assertThat(attr.getDefaultValueForTesting()).isEqualTo("some value"); + assertThat(attr.getDefaultValueUnchecked()).isEqualTo("some value"); } @Test public void testLabelAttrDefaultValueAsString() throws Exception { Attribute sligleAttr = buildAttribute("a1", "attr.label(default = '//foo:bar')"); - assertThat(sligleAttr.getDefaultValueForTesting()) + assertThat(sligleAttr.getDefaultValueUnchecked()) .isEqualTo( Label.parseAbsolute( "//foo:bar", @@ -503,7 +503,7 @@ Attribute listAttr = buildAttribute("a2", "attr.label_list(default = ['//foo:bar', '//bar:foo'])"); - assertThat(listAttr.getDefaultValueForTesting()) + assertThat(listAttr.getDefaultValueUnchecked()) .isEqualTo( ImmutableList.of( Label.parseAbsolute( @@ -517,7 +517,7 @@ Attribute dictAttr = buildAttribute("a3", "attr.label_keyed_string_dict(default = {'//foo:bar': 'my value'})"); - assertThat(dictAttr.getDefaultValueForTesting()) + assertThat(dictAttr.getDefaultValueUnchecked()) .isEqualTo( ImmutableMap.of( Label.parseAbsolute( @@ -839,8 +839,8 @@ + "attr.label(default = Label('//foo:foo'), allow_files=True)})"); RuleClass c = ((SkylarkRuleFunction) lookup("r1")).getRuleClass(); Attribute a = c.getAttributeByName("a1"); - assertThat(a.getDefaultValueForTesting()).isInstanceOf(Label.class); - assertThat(a.getDefaultValueForTesting().toString()).isEqualTo("//foo:foo"); + assertThat(a.getDefaultValueUnchecked()).isInstanceOf(Label.class); + assertThat(a.getDefaultValueUnchecked().toString()).isEqualTo("//foo:foo"); } @Test @@ -850,7 +850,7 @@ "r1 = rule(impl, attrs = {'a1': attr.int(default = 40+2)})"); RuleClass c = ((SkylarkRuleFunction) lookup("r1")).getRuleClass(); Attribute a = c.getAttributeByName("a1"); - assertThat(a.getDefaultValueForTesting()).isEqualTo(42); + assertThat(a.getDefaultValueUnchecked()).isEqualTo(42); } @Test