Fix confusing semantics for Attribute.Builder.allowedRuleClasses.
1) Update the javadocs.
2) Clarify that allowedRuleClases and allowedRuleClassesWithWarning
must be disjoint sets.
3) Enforce 2).
4) Fix error messaging when only "with warnings" is set.
PiperOrigin-RevId: 163379567
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 80b2c4e..5e38598 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -2054,7 +2054,7 @@
/**
* Because some rules still have to use allowedRuleClasses to do rule dependency validation.
- * A dependency is valid if it is from a rule in allowedRuledClasses, OR if all of the providers
+ * A dependency is valid if it is from a rule in allowedRuleClasses, OR if all of the providers
* in mandatoryNativeProviders AND mandatoryProvidersList are provided by the target.
*/
private void validateRuleDependency(ConfiguredTarget prerequisite, Attribute attribute) {
@@ -2077,17 +2077,17 @@
false);
}
- if (attribute.getAllowedRuleClassesWarningPredicate() != Predicates.<RuleClass>alwaysTrue()) {
- Predicate<RuleClass> warningPredicate = attribute.getAllowedRuleClassesWarningPredicate();
- if (warningPredicate.apply(ruleClass)) {
- reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(), prerequisite,
- "expected " + attribute.getAllowedRuleClassesPredicate(), true);
- // prerequisite has a rule class allowed with a warning => accept, emitting a warning.
- return;
- }
+ if (attribute.getAllowedRuleClassesWarningPredicate().apply(ruleClass)) {
+ Predicate<RuleClass> allowedRuleClasses = attribute.getAllowedRuleClassesPredicate();
+ reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(), prerequisite,
+ allowedRuleClasses == Predicates.<RuleClass>alwaysTrue()
+ ? null : "expected " + allowedRuleClasses,
+ true);
+ // prerequisite has a rule class allowed with a warning => accept, emitting a warning.
+ return;
}
- // If we got there, we have no allowed rule class.
+ // If we get here, we have no allowed rule class.
// If mandatory providers are specified, try them.
Set<String> unfulfilledRequirements = new LinkedHashSet<>();
if (!attribute.getMandatoryNativeProvidersList().isEmpty()
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 0214a17..afcb9c5 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
@@ -32,6 +32,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassNamePredicate;
import com.google.devtools.build.lib.syntax.ClassObject;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
@@ -806,27 +807,39 @@
}
/**
- * If this is a label or label-list attribute, then this sets the allowed
- * rule types for the labels occurring in the attribute. If the attribute
- * contains Labels of any other rule type, then an error is produced during
- * the analysis phase. Defaults to allow any types.
+ * If this is a label or label-list attribute, then this sets the allowed rule types for the
+ * labels occurring in the attribute.
*
- * <p>This only works on a per-target basis, not on a per-file basis; with
- * other words, it works for 'deps' attributes, but not 'srcs' attributes.
+ * <p>If the attribute contains Labels of any other rule type, then if they're in
+ * {@link #allowedRuleClassesForLabelsWarning}, the build continues with a warning. Else if
+ * they fulfill {@link #getMandatoryNativeProvidersList()}, the build continues without error.
+ * Else the build fails during analysis.
+ *
+ * <p>If neither this nor {@link #allowedRuleClassesForLabelsWarning} is set, only rules that
+ * fulfill {@link #getMandatoryNativeProvidersList()} build without error.
+ *
+ * <p>This only works on a per-target basis, not on a per-file basis; with other words, it
+ * works for 'deps' attributes, but not 'srcs' attributes.
*/
public Builder<TYPE> allowedRuleClasses(Iterable<String> allowedRuleClasses) {
return allowedRuleClasses(
- new RuleClass.Builder.RuleClassNamePredicate(allowedRuleClasses));
+ new RuleClassNamePredicate(allowedRuleClasses));
}
/**
- * If this is a label or label-list attribute, then this sets the allowed
- * rule types for the labels occurring in the attribute. If the attribute
- * contains Labels of any other rule type, then an error is produced during
- * the analysis phase. Defaults to allow any types.
+ * If this is a label or label-list attribute, then this sets the allowed rule types for the
+ * labels occurring in the attribute.
*
- * <p>This only works on a per-target basis, not on a per-file basis; with
- * other words, it works for 'deps' attributes, but not 'srcs' attributes.
+ * <p>If the attribute contains Labels of any other rule type, then if they're in
+ * {@link #allowedRuleClassesForLabelsWarning}, the build continues with a warning. Else if
+ * they fulfill {@link #getMandatoryNativeProvidersList()}, the build continues without error.
+ * Else the build fails during analysis.
+ *
+ * <p>If neither this nor {@link #allowedRuleClassesForLabelsWarning} is set, only rules that
+ * fulfill {@link #getMandatoryNativeProvidersList()} build without error.
+ *
+ * <p>This only works on a per-target basis, not on a per-file basis; with other words, it
+ * works for 'deps' attributes, but not 'srcs' attributes.
*/
public Builder<TYPE> allowedRuleClasses(Predicate<RuleClass> allowedRuleClasses) {
Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY,
@@ -837,13 +850,19 @@
}
/**
- * If this is a label or label-list attribute, then this sets the allowed
- * rule types for the labels occurring in the attribute. If the attribute
- * contains Labels of any other rule type, then an error is produced during
- * the analysis phase. Defaults to allow any types.
+ * If this is a label or label-list attribute, then this sets the allowed rule types for the
+ * labels occurring in the attribute.
*
- * <p>This only works on a per-target basis, not on a per-file basis; with
- * other words, it works for 'deps' attributes, but not 'srcs' attributes.
+ * <p>If the attribute contains Labels of any other rule type, then if they're in
+ * {@link #allowedRuleClassesForLabelsWarning}, the build continues with a warning. Else if
+ * they fulfill {@link #getMandatoryNativeProvidersList()}, the build continues without error.
+ * Else the build fails during analysis.
+ *
+ * <p>If neither this nor {@link #allowedRuleClassesForLabelsWarning} is set, only rules that
+ * fulfill {@link #getMandatoryNativeProvidersList()} build without error.
+ *
+ * <p>This only works on a per-target basis, not on a per-file basis; with other words, it
+ * works for 'deps' attributes, but not 'srcs' attributes.
*/
public Builder<TYPE> allowedRuleClasses(String... allowedRuleClasses) {
return allowedRuleClasses(ImmutableSet.copyOf(allowedRuleClasses));
@@ -889,29 +908,39 @@
}
/**
- * If this is a label or label-list attribute, then this sets the allowed
- * rule types with warning for the labels occurring in the attribute. If the attribute
- * contains Labels of any other rule type (other than this or those set in
- * allowedRuleClasses()), then a warning is produced during
- * the analysis phase. Defaults to deny any types.
+ * If this is a label or label-list attribute, then this sets the allowed rule types with
+ * warning for the labels occurring in the attribute. This must be a disjoint set from
+ * {@link #allowedRuleClasses}.
*
- * <p>This only works on a per-target basis, not on a per-file basis; with
- * other words, it works for 'deps' attributes, but not 'srcs' attributes.
+ * <p>If the attribute contains Labels of any other rule type (other than this or those set in
+ * allowedRuleClasses()) and they fulfill {@link #getMandatoryNativeProvidersList()}}, the build
+ * continues without error. Else the build fails during analysis.
+ *
+ * <p>If neither this nor {@link #allowedRuleClassesForLabels} is set, only rules that
+ * fulfill {@link #getMandatoryNativeProvidersList()} build without error.
+ *
+ * <p>This only works on a per-target basis, not on a per-file basis; with other words, it
+ * works for 'deps' attributes, but not 'srcs' attributes.
*/
public Builder<TYPE> allowedRuleClassesWithWarning(Collection<String> allowedRuleClasses) {
return allowedRuleClassesWithWarning(
- new RuleClass.Builder.RuleClassNamePredicate(allowedRuleClasses));
+ new RuleClassNamePredicate(allowedRuleClasses));
}
/**
- * If this is a label or label-list attribute, then this sets the allowed
- * rule types for the labels occurring in the attribute. If the attribute
- * contains Labels of any other rule type (other than this or those set in
- * allowedRuleClasses()), then a warning is produced during
- * the analysis phase. Defaults to deny any types.
+ * If this is a label or label-list attribute, then this sets the allowed rule types with
+ * warning for the labels occurring in the attribute. This must be a disjoint set from
+ * {@link #allowedRuleClasses}.
*
- * <p>This only works on a per-target basis, not on a per-file basis; with
- * other words, it works for 'deps' attributes, but not 'srcs' attributes.
+ * <p>If the attribute contains Labels of any other rule type (other than this or those set in
+ * allowedRuleClasses()) and they fulfill {@link #getMandatoryNativeProvidersList()}}, the build
+ * continues without error. Else the build fails during analysis.
+ *
+ * <p>If neither this nor {@link #allowedRuleClassesForLabels} is set, only rules that
+ * fulfill {@link #getMandatoryNativeProvidersList()} build without error.
+ *
+ * <p>This only works on a per-target basis, not on a per-file basis; with other words, it
+ * works for 'deps' attributes, but not 'srcs' attributes.
*/
public Builder<TYPE> allowedRuleClassesWithWarning(Predicate<RuleClass> allowedRuleClasses) {
Preconditions.checkState(type.getLabelClass() == LabelClass.DEPENDENCY,
@@ -922,14 +951,19 @@
}
/**
- * If this is a label or label-list attribute, then this sets the allowed
- * rule types for the labels occurring in the attribute. If the attribute
- * contains Labels of any other rule type (other than this or those set in
- * allowedRuleClasses()), then a warning is produced during
- * the analysis phase. Defaults to deny any types.
+ * If this is a label or label-list attribute, then this sets the allowed rule types with
+ * warning for the labels occurring in the attribute. This must be a disjoint set from
+ * {@link #allowedRuleClasses}.
*
- * <p>This only works on a per-target basis, not on a per-file basis; with
- * other words, it works for 'deps' attributes, but not 'srcs' attributes.
+ * <p>If the attribute contains Labels of any other rule type (other than this or those set in
+ * allowedRuleClasses()) and they fulfill {@link #getMandatoryNativeProvidersList()}}, the build
+ * continues without error. Else the build fails during analysis.
+ *
+ * <p>If neither this nor {@link #allowedRuleClassesForLabels} is set, only rules that
+ * fulfill {@link #getMandatoryNativeProvidersList()} build without error.
+ *
+ * <p>This only works on a per-target basis, not on a per-file basis; with other words, it
+ * works for 'deps' attributes, but not 'srcs' attributes.
*/
public Builder<TYPE> allowedRuleClassesWithWarning(String... allowedRuleClasses) {
return allowedRuleClassesWithWarning(ImmutableSet.copyOf(allowedRuleClasses));
@@ -1123,6 +1157,16 @@
allowedFileTypesForLabels = FileTypeSet.ANY_FILE;
}
}
+
+ if (allowedRuleClassesForLabels instanceof RuleClassNamePredicate
+ && allowedRuleClassesForLabelsWarning instanceof RuleClassNamePredicate) {
+ Preconditions.checkState(
+ !((RuleClassNamePredicate) allowedRuleClassesForLabels)
+ .intersects((RuleClassNamePredicate) allowedRuleClassesForLabelsWarning),
+ "allowedRuleClasses and allowedRuleClassesWithWarning may not contain "
+ + "the same rule classes");
+ }
+
return new Attribute(
name,
type,
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 ae9ec03..ae0c358 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
@@ -57,6 +57,7 @@
import java.util.ArrayList;
import java.util.BitSet;
import java.util.Collection;
+import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
@@ -422,10 +423,6 @@
this.ruleClasses = ImmutableSet.copyOf(ruleClasses);
}
- public RuleClassNamePredicate() {
- this(ImmutableSet.<String>of());
- }
-
@Override
public boolean apply(RuleClass ruleClass) {
return ruleClasses.contains(ruleClass.getName());
@@ -442,6 +439,13 @@
&& ruleClasses.equals(((RuleClassNamePredicate) o).ruleClasses);
}
+ /**
+ * Returns true if this and the other predicate have common rule class entries.
+ */
+ public boolean intersects(RuleClassNamePredicate other) {
+ return !Collections.disjoint(ruleClasses, other.ruleClasses);
+ }
+
@Override
public String toString() {
return ruleClasses.isEmpty() ? "nothing" : StringUtil.joinEnglishList(ruleClasses);
