Skip to content

Commit

Permalink
Mark built-in name attribute as mandatory
Browse files Browse the repository at this point in the history
Update a few tests to stick in a dummy name argument if one is not present. This only applies to low-level tests of RuleClass, since other tests go through machinery that already validates the presence of `name`.

Work toward #19922.

PiperOrigin-RevId: 620267659
Change-Id: Ie77db99ac4c4f4234ca298654656716ff4d8546d
  • Loading branch information
brandjon authored and copybara-github committed Mar 29, 2024
1 parent f83c20f commit 90f3a50
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 37 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ public class RuleClass implements RuleClassData {
static final Attribute NAME_ATTRIBUTE =
attr("name", STRING_NO_INTERN)
.nonconfigurable("All rules have a non-customizable \"name\" attribute")
.mandatory()
.build();

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,38 +53,42 @@ public boolean apply(final String input) {
}
};

/**
* Predicate for checking if a rule has any mandatory attributes.
*/
public static final Predicate<RuleClass> MANDATORY_ATTRIBUTES = new Predicate<RuleClass>() {
@Override
public boolean apply(final RuleClass input) {
List<Attribute> li = new ArrayList<>(input.getAttributes());
return Iterables.any(li, MANDATORY);
}
};
/** Predicate for checking if a rule has any mandatory attributes, aside from name. */
public static final Predicate<RuleClass> MANDATORY_ATTRIBUTES =
new Predicate<RuleClass>() {
@Override
public boolean apply(final RuleClass input) {
List<Attribute> li = new ArrayList<>(input.getAttributes());
return Iterables.any(li, RuleSetUtils::mandatoryExcludingName);
}
};

/**
* Predicate for checking that the rule can have a deps attribute, and does not have any
* other mandatory attributes besides deps.
* Predicate for checking that the rule can have a deps attribute, and does not have any other
* mandatory attributes besides deps and name.
*/
public static final Predicate<RuleClass> DEPS_ONLY_ALLOWED = new Predicate<RuleClass>() {
@Override
public boolean apply(final RuleClass input) {
List<Attribute> li = new ArrayList<>(input.getAttributes());
// TODO(bazel-team): after the API migration we shouldn't check srcs separately
boolean emptySrcsAllowed = input.hasAttr("srcs", BuildType.LABEL_LIST)
? !input.getAttributeByName("srcs").isNonEmpty() : true;
if (!(emptySrcsAllowed && Iterables.any(li, DEPS))) {
return false;
}
public static final Predicate<RuleClass> DEPS_ONLY_ALLOWED =
new Predicate<RuleClass>() {
@Override
public boolean apply(final RuleClass input) {
List<Attribute> li = new ArrayList<>(input.getAttributes());
// TODO(bazel-team): after the API migration we shouldn't check srcs separately
boolean emptySrcsAllowed =
input.hasAttr("srcs", BuildType.LABEL_LIST)
? !input.getAttributeByName("srcs").isNonEmpty()
: true;
if (!(emptySrcsAllowed && Iterables.any(li, DEPS))) {
return false;
}

Iterator<Attribute> it = li.iterator();
boolean mandatoryAttributesBesidesDeps =
Iterables.any(Lists.newArrayList(Iterators.filter(it, MANDATORY)), Predicates.not(DEPS));
return !mandatoryAttributesBesidesDeps;
}
};
Iterator<Attribute> it = li.iterator();
boolean mandatoryAttributesBesidesDeps =
Iterables.any(
Lists.newArrayList(Iterators.filter(it, RuleSetUtils::mandatoryExcludingName)),
Predicates.not(DEPS));
return !mandatoryAttributesBesidesDeps;
}
};

/**
* Predicate for checking if a RuleClass has certain attributes
Expand All @@ -108,15 +112,10 @@ public static Predicate<RuleClass> hasAnyAttributes(
return new HasAttributes(attributes);
}

/**
* Predicate for checking if an attribute is mandatory.
*/
private static final Predicate<Attribute> MANDATORY = new Predicate<Attribute>() {
@Override
public boolean apply(final Attribute input) {
return input.isMandatory();
}
};
/** Predicate for checking if an attribute (other than name) is mandatory. */
private static boolean mandatoryExcludingName(Attribute input) {
return input.isMandatory() && !input.getName().equals("name");
}

/**
* Predicate for checking if an attribute is the "deps" attribute.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,20 @@ private static RuleClass createRuleClassB(RuleClass ruleClassA) {
attributes.toArray(new Attribute[0]));
}

// Helper method to paper over how some test cases don't supply the mandatory name attribute for
// historic reasons.
private static ImmutableMap<String, Object> ensureNameAttrValuePresent(
Map<String, Object> attrValues) {
if (attrValues.containsKey("name")) {
return ImmutableMap.copyOf(attrValues);
} else {
return ImmutableMap.<String, Object>builder()
.putAll(attrValues)
.put("name", "my-name")
.buildOrThrow();
}
}

@Test
public void testRuleClassBasics() throws Exception {
RuleClass ruleClassA = createRuleClassA();
Expand Down Expand Up @@ -932,6 +946,7 @@ private Rule createRule(RuleClass ruleClass, String name, Map<String, Object> at
} catch (LabelSyntaxException e) {
throw new IllegalArgumentException("Rule has illegal label", e);
}
attributeValues = ensureNameAttrValuePresent(attributeValues);
Rule rule =
ruleClass.createRule(
pkgBuilder,
Expand Down

0 comments on commit 90f3a50

Please sign in to comment.