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
