Roll-foward of commit 351475627b9e94e5afdf472cbf465f49c433a25e which was rolled back in commit 1250fdac4c7769cfa200af8b4f9b061024356fea. There was nothing wrong with that change.
--
MOS_MIGRATED_REVID=119756383
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 f77a271..57e6808 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
@@ -158,6 +158,7 @@
private RuleContext(
Builder builder,
+ AttributeMap attributes,
ListMultimap<String, ConfiguredTarget> targetMap,
ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap,
Set<ConfigMatchingProvider> configConditions,
@@ -172,8 +173,7 @@
this.targetMap = targetMap;
this.filesetEntryMap = filesetEntryMap;
this.configConditions = configConditions;
- this.attributes =
- ConfiguredAttributeMapper.of(builder.rule, configConditions);
+ this.attributes = attributes;
this.features = getEnabledFeatures();
this.ruleClassNameForLogging = ruleClassNameForLogging;
this.aspectAttributes = aspectAttributes;
@@ -1330,11 +1330,14 @@
Preconditions.checkNotNull(prerequisiteMap);
Preconditions.checkNotNull(configConditions);
Preconditions.checkNotNull(visibility);
+ AttributeMap attributes = ConfiguredAttributeMapper.of(rule, configConditions);
+ validateAttributes(attributes);
ListMultimap<String, ConfiguredTarget> targetMap = createTargetMap();
ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap =
createFilesetEntryMap(rule, configConditions);
return new RuleContext(
this,
+ attributes,
targetMap,
filesetEntryMap,
configConditions,
@@ -1343,6 +1346,10 @@
aspectAttributes != null ? aspectAttributes : ImmutableMap.<String, Attribute>of());
}
+ private void validateAttributes(AttributeMap attributes) {
+ rule.getRuleClassObject().checkAttributesNonEmpty(rule, reporter, attributes);
+ }
+
Builder setVisibility(NestedSet<PackageSpecification> visibility) {
this.visibility = visibility;
return this;
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 9accac3..ed2ad30 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
@@ -1401,7 +1401,6 @@
boolean explicit = attributeValues.isAttributeExplicitlySpecified(attributeName);
setRuleAttributeValue(rule, eventHandler, attr, nativeAttributeValue, explicit);
definedAttrIndices.set(attrIndex);
- checkAttrValNonEmpty(rule, eventHandler, attributeValue, attr);
}
return definedAttrIndices;
}
@@ -1457,7 +1456,6 @@
attrsWithComputedDefaults.add(attr);
} else {
Object defaultValue = getAttributeNoncomputedDefaultValue(attr, pkgBuilder);
- checkAttrValNonEmpty(rule, eventHandler, defaultValue, attr);
rule.setAttributeValue(attr, defaultValue, /*explicit=*/ false);
checkAllowedValues(rule, attr, eventHandler);
}
@@ -1500,26 +1498,27 @@
/*explicit=*/false);
}
- private void checkAttrValNonEmpty(
- Rule rule, EventHandler eventHandler, Object attributeValue, Attribute attr) {
- if (!attr.isNonEmpty()) {
- return;
- }
+ public void checkAttributesNonEmpty(
+ Rule rule, RuleErrorConsumer ruleErrorConsumer, AttributeMap attributes) {
+ for (String attributeName : attributes.getAttributeNames()) {
+ Attribute attr = attributes.getAttributeDefinition(attributeName);
+ if (!attr.isNonEmpty()) {
+ continue;
+ }
+ Object attributeValue = attributes.get(attributeName, attr.getType());
- boolean isEmpty = false;
+ boolean isEmpty = false;
+ if (attributeValue instanceof SkylarkList) {
+ isEmpty = ((SkylarkList) attributeValue).isEmpty();
+ } else if (attributeValue instanceof List<?>) {
+ isEmpty = ((List<?>) attributeValue).isEmpty();
+ } else if (attributeValue instanceof Map<?, ?>) {
+ isEmpty = ((Map<?, ?>) attributeValue).isEmpty();
+ }
- if (attributeValue instanceof SkylarkList) {
- isEmpty = ((SkylarkList) attributeValue).isEmpty();
- } else if (attributeValue instanceof List<?>) {
- isEmpty = ((List<?>) attributeValue).isEmpty();
- } else if (attributeValue instanceof Map<?, ?>) {
- isEmpty = ((Map<?, ?>) attributeValue).isEmpty();
- }
-
- if (isEmpty) {
- rule.reportError(rule.getLabel() + ": non empty attribute '" + attr.getName()
- + "' in '" + name + "' rule '" + rule.getLabel() + "' has to have at least one value",
- eventHandler);
+ if (isEmpty) {
+ ruleErrorConsumer.attributeError(attr.getName(), "attribute must be non empty");
+ }
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index c7fa352..a690340 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -1449,8 +1449,8 @@
}
protected String getErrorMsgNonEmptyList(String attrName, String ruleType, String ruleName) {
- return "non empty attribute '" + attrName + "' in '" + ruleType
- + "' rule '" + ruleName + "' has to have at least one value";
+ return "in " + attrName + " attribute of " + ruleType + " rule " + ruleName + ": attribute "
+ + "must be non empty";
}
protected String getErrorMsgMandatoryMissing(String attrName, String ruleType) {
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 93ca76a..36d539a 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
@@ -613,68 +613,6 @@
attributes.get("my-sorted-stringlist-attr", Type.STRING_LIST));
}
- @Test
- public void testNonEmptyGood() throws Exception {
- RuleClass mneRuleClass = setupNonEmpty(
- attr("list1", LABEL_LIST).mandatory().legacyAllowAnyFileType().build(),
- attr("list2", LABEL_LIST).nonEmpty().legacyAllowAnyFileType().build(),
- attr("list3", STRING_LIST).nonEmpty().build());
-
- Map<String, Object> attributeValues = new LinkedHashMap<>();
- attributeValues.put("list1", Lists.newArrayList());
- attributeValues.put("list2", Lists.newArrayList(":nodup1", ":nodup2"));
- attributeValues.put("list3", Lists.newArrayList("val1", "val2"));
-
- createRule(mneRuleClass, "ruleTestMNE", attributeValues, testRuleLocation);
- }
-
- @Test
- public void testNonEmptyFail() throws Exception {
- RuleClass mandNonEmptyRuleClass = setupNonEmpty(
- attr("list", LABEL_LIST).nonEmpty().legacyAllowAnyFileType().build());
-
- Map<String, Object> attributeValues = new LinkedHashMap<>();
- attributeValues.put("list", Lists.newArrayList());
-
- reporter.removeHandler(failFastHandler);
- createRule(mandNonEmptyRuleClass, "ruleTestMNE", attributeValues, testRuleLocation);
-
- assertSame(1, eventCollector.count());
- assertContainsEvent(getErrorMsgNonEmptyList(
- "list", "ruleMNE", "//testpackage:ruleTestMNE"));
- }
-
- private RuleClass setupNonEmpty(Attribute... attributes) {
- RuleClass mandNonEmptyRuleClass = new RuleClass(
- "ruleMNE", false, false, false, false, false, false,
- ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
- PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
- ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(),
- MissingFragmentPolicy.FAIL_ANALYSIS, true, attributes);
- return mandNonEmptyRuleClass;
- }
-
- @Test
- public void testNonEmptyWrongDefVal() throws Exception {
- List<Label> emptyList = ImmutableList.of();
- RuleClass mandNonEmptyRuleClass = new RuleClass(
- "ruleMNE", false, false, false, false, false, false,
- ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY,
- PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE,
- ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(),
- MissingFragmentPolicy.FAIL_ANALYSIS, true, attr("list", LABEL_LIST)
- .nonEmpty().legacyAllowAnyFileType().value(emptyList).build());
-
- Map<String, Object> attributeValues = new LinkedHashMap<>();
- reporter.removeHandler(failFastHandler);
- createRule(mandNonEmptyRuleClass, "ruleTestMNE", attributeValues, testRuleLocation);
-
- assertSame(1, eventCollector.count());
-
- assertContainsEvent(getErrorMsgNonEmptyList(
- "list", "ruleMNE", "//testpackage:ruleTestMNE"));
- }
-
private Rule createRule(RuleClass ruleClass, String name, Map<String, Object> attributeValues,
Location location) throws LabelSyntaxException, InterruptedException {
Package.Builder pkgBuilder = createDummyPackageBuilder();
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java
index 1a0c5d9..7af31ec 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java
@@ -276,9 +276,4 @@
skyframeExecutor.invalidateFilesUnderPathForTesting(
reporter, ModifiedFileSet.EVERYTHING_MODIFIED, rootDirectory);
}
-
- protected String getErrorMsgNonEmptyList(String attrName, String ruleType, String ruleName) {
- return "non empty attribute '" + attrName + "' in '" + ruleType
- + "' rule '" + ruleName + "' has to have at least one value";
- }
}
