diff options
5 files changed, 92 insertions, 31 deletions
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 57e680824e..f77a271926 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,7 +158,6 @@ public final class RuleContext extends TargetContext private RuleContext( Builder builder, - AttributeMap attributes, ListMultimap<String, ConfiguredTarget> targetMap, ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap, Set<ConfigMatchingProvider> configConditions, @@ -173,7 +172,8 @@ public final class RuleContext extends TargetContext this.targetMap = targetMap; this.filesetEntryMap = filesetEntryMap; this.configConditions = configConditions; - this.attributes = attributes; + this.attributes = + ConfiguredAttributeMapper.of(builder.rule, configConditions); this.features = getEnabledFeatures(); this.ruleClassNameForLogging = ruleClassNameForLogging; this.aspectAttributes = aspectAttributes; @@ -1330,14 +1330,11 @@ public final class RuleContext extends TargetContext 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, @@ -1346,10 +1343,6 @@ public final class RuleContext extends TargetContext 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 ed2ad3064c..9accac3899 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,6 +1401,7 @@ public final class RuleClass { boolean explicit = attributeValues.isAttributeExplicitlySpecified(attributeName); setRuleAttributeValue(rule, eventHandler, attr, nativeAttributeValue, explicit); definedAttrIndices.set(attrIndex); + checkAttrValNonEmpty(rule, eventHandler, attributeValue, attr); } return definedAttrIndices; } @@ -1456,6 +1457,7 @@ public final class RuleClass { attrsWithComputedDefaults.add(attr); } else { Object defaultValue = getAttributeNoncomputedDefaultValue(attr, pkgBuilder); + checkAttrValNonEmpty(rule, eventHandler, defaultValue, attr); rule.setAttributeValue(attr, defaultValue, /*explicit=*/ false); checkAllowedValues(rule, attr, eventHandler); } @@ -1498,27 +1500,26 @@ public final class RuleClass { /*explicit=*/false); } - 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(); - } + private void checkAttrValNonEmpty( + Rule rule, EventHandler eventHandler, Object attributeValue, Attribute attr) { + if (!attr.isNonEmpty()) { + return; + } - if (isEmpty) { - ruleErrorConsumer.attributeError(attr.getName(), "attribute must be non empty"); - } + 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); } } 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 a690340f7c..c7fa352d40 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 @@ public abstract class BuildViewTestCase extends FoundationTestCase { } protected String getErrorMsgNonEmptyList(String attrName, String ruleType, String ruleName) { - return "in " + attrName + " attribute of " + ruleType + " rule " + ruleName + ": attribute " - + "must be non empty"; + return "non empty attribute '" + attrName + "' in '" + ruleType + + "' rule '" + ruleName + "' has to have at least one value"; } 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 36d539a6ec..93ca76a9b3 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,6 +613,68 @@ public class RuleClassTest extends PackageLoadingTestCase { 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 7af31eccd8..1a0c5d9877 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,4 +276,9 @@ public abstract class PackageLoadingTestCase extends FoundationTestCase { 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"; + } } |