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 706d986193872d..5eac0d686a26e9 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 @@ public List getFiles() { private RuleContext( Builder builder, + AttributeMap attributes, ListMultimap targetMap, ListMultimap filesetEntryMap, Set configConditions, @@ -172,8 +173,7 @@ private RuleContext( 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; @@ -1306,11 +1306,14 @@ RuleContext build() { Preconditions.checkNotNull(prerequisiteMap); Preconditions.checkNotNull(configConditions); Preconditions.checkNotNull(visibility); + AttributeMap attributes = ConfiguredAttributeMapper.of(rule, configConditions); + validateAttributes(attributes); ListMultimap targetMap = createTargetMap(); ListMultimap filesetEntryMap = createFilesetEntryMap(rule, configConditions); return new RuleContext( this, + attributes, targetMap, filesetEntryMap, configConditions, @@ -1319,6 +1322,10 @@ RuleContext build() { aspectAttributes != null ? aspectAttributes : ImmutableMap.of()); } + private void validateAttributes(AttributeMap attributes) { + rule.getRuleClassObject().checkAttributesNonEmpty(rule, reporter, attributes); + } + Builder setVisibility(NestedSet 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 9accac3899522c..ed2ad3064ceb19 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 @@ private BitSet populateDefinedRuleAttributeValues( 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 @@ private void populateDefaultRuleAttributeValues( 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 @@ private static void populateConfigDependenciesAttribute(Rule rule) { /*explicit=*/false); } - private void checkAttrValNonEmpty( - Rule rule, EventHandler eventHandler, Object attributeValue, Attribute attr) { - if (!attr.isNonEmpty()) { - return; - } - - 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(); - } + 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; + 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 541356f75feb1e..abb7a79772e019 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 getErrorMsgMisplacedRules(String attrName, String ruleType, Str } 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 93ca76a9b376f2..36d539a6ece580 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 @@ public void testOrderIndependentAttribute() throws Exception { 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 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 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.alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, - ImmutableSet.>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.>of(), - MissingFragmentPolicy.FAIL_ANALYSIS, true, attributes); - return mandNonEmptyRuleClass; - } - - @Test - public void testNonEmptyWrongDefVal() throws Exception { - List