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.
