From 90f3a501724d300597c9ba1a4a9c0b2c67931d2e Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 29 Mar 2024 09:53:07 -0700 Subject: [PATCH] Mark built-in name attribute as mandatory 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 --- .../build/lib/packages/RuleClass.java | 1 + .../util/RuleSetUtils.java | 73 +++++++++---------- .../build/lib/packages/RuleClassTest.java | 15 ++++ 3 files changed, 52 insertions(+), 37 deletions(-) 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 4983190b526861..bdac9b1c55bd02 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 @@ -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(); /** diff --git a/src/test/java/com/google/devtools/build/lib/generatedprojecttest/util/RuleSetUtils.java b/src/test/java/com/google/devtools/build/lib/generatedprojecttest/util/RuleSetUtils.java index 68f9a3ed927940..0fcd577757050d 100644 --- a/src/test/java/com/google/devtools/build/lib/generatedprojecttest/util/RuleSetUtils.java +++ b/src/test/java/com/google/devtools/build/lib/generatedprojecttest/util/RuleSetUtils.java @@ -53,38 +53,42 @@ public boolean apply(final String input) { } }; - /** - * Predicate for checking if a rule has any mandatory attributes. - */ - public static final Predicate MANDATORY_ATTRIBUTES = new Predicate() { - @Override - public boolean apply(final RuleClass input) { - List 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 MANDATORY_ATTRIBUTES = + new Predicate() { + @Override + public boolean apply(final RuleClass input) { + List 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 DEPS_ONLY_ALLOWED = new Predicate() { - @Override - public boolean apply(final RuleClass input) { - List 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 DEPS_ONLY_ALLOWED = + new Predicate() { + @Override + public boolean apply(final RuleClass input) { + List 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 it = li.iterator(); - boolean mandatoryAttributesBesidesDeps = - Iterables.any(Lists.newArrayList(Iterators.filter(it, MANDATORY)), Predicates.not(DEPS)); - return !mandatoryAttributesBesidesDeps; - } - }; + Iterator 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 @@ -108,15 +112,10 @@ public static Predicate hasAnyAttributes( return new HasAttributes(attributes); } - /** - * Predicate for checking if an attribute is mandatory. - */ - private static final Predicate MANDATORY = new Predicate() { - @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. 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 1534718dbf10c9..46eefb328eddfa 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 @@ -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 ensureNameAttrValuePresent( + Map attrValues) { + if (attrValues.containsKey("name")) { + return ImmutableMap.copyOf(attrValues); + } else { + return ImmutableMap.builder() + .putAll(attrValues) + .put("name", "my-name") + .buildOrThrow(); + } + } + @Test public void testRuleClassBasics() throws Exception { RuleClass ruleClassA = createRuleClassA(); @@ -932,6 +946,7 @@ private Rule createRule(RuleClass ruleClass, String name, Map at } catch (LabelSyntaxException e) { throw new IllegalArgumentException("Rule has illegal label", e); } + attributeValues = ensureNameAttrValuePresent(attributeValues); Rule rule = ruleClass.createRule( pkgBuilder,