Skip to content

Commit

Permalink
Make non-empty attribute checks happen during analysis of the target …
Browse files Browse the repository at this point in the history 
…in question, rather than during loading of the target's package. This way a target's package won't be in error if e.g. an unrelated target has empty 'srcs'.

--
MOS_MIGRATED_REVID=119079777
  • Loading branch information
@haxorz @lberki
haxorz authored and lberki committed Apr 7, 2016
1 parent eb89ccc commit 3514756
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 92 deletions.
11 changes: 9 additions & 2 deletions src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ public List<TransitiveInfoCollection> getFiles() {

private RuleContext(
Builder builder,
AttributeMap attributes,
ListMultimap<String, ConfiguredTarget> targetMap,
ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap,
Set<ConfigMatchingProvider> configConditions,
Expand All @@ -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;
Expand Down Expand Up @@ -1306,11 +1306,14 @@ RuleContext build() {
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,
Expand All @@ -1319,6 +1322,10 @@ RuleContext build() {
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;
Expand Down
41 changes: 20 additions & 21 deletions src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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");
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
62 changes: 0 additions & 62 deletions src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<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();
Expand Down
5 changes: 0 additions & 5 deletions src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,9 +276,4 @@ protected void invalidatePackages() throws InterruptedException {
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";
}
}

0 comments on commit 3514756

Please sign in to comment.