From 6967b646ff8ad045439fc2399cc86a52c019aea4 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 17 Apr 2024 10:48:39 -0700 Subject: [PATCH] Delete dead code for attribute validation This CL follows https://github.com/bazelbuild/bazel/commit/32def7092155a0fcdb138cfd80c51608a7326bcb by removing Attribute.ValidityPredicate, along with its sole non-test (trivial) implementation, ANY_EDGE. It looks like Attribute.ValidityPredicate was added ten years ago to replace an earlier mechanism on RuleClass. At that time it was only used to enforce android-related constraints on srcs/deps attributes in the native java rules. Opportunistic cleanup discovered during work on #19922. PiperOrigin-RevId: 625736563 Change-Id: I1355785546549613f3982b760ca0638b0a76267a --- .../build/lib/analysis/RuleContext.java | 5 -- .../build/lib/packages/Attribute.java | 50 --------------- .../lib/rules/android/AarImportBaseRule.java | 4 +- .../build/lib/packages/AttributeTest.java | 9 --- .../build/lib/packages/RuleClassTest.java | 61 ------------------- 5 files changed, 1 insertion(+), 128 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 6919338f17e3d2..56e2f14005eef6 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 @@ -1829,11 +1829,6 @@ private void validateDirectPrerequisiteType( ConfiguredTargetAndData prerequisite, Attribute attribute) { String ruleClass = prerequisite.getRuleClass(); if (!ruleClass.isEmpty()) { - String reason = - attribute.getValidityPredicate().checkValid(target.getAssociatedRule(), ruleClass); - if (reason != null) { - reportBadPrerequisite(attribute, prerequisite, reason, false); - } validateRuleDependency(prerequisite, attribute); return; } diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index 4ae6a1031acc40..dd2497fe4ae10b 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java @@ -185,21 +185,6 @@ private enum PropertyFlag { SKIP_VALIDATIONS, } - // TODO(bazel-team): modify this interface to extend Predicate and have an extra error - // message function like AllowedValues does - /** A predicate-like class that determines whether an edge between two rules is valid or not. */ - public interface ValidityPredicate { - /** - * This method should return null if the edge is valid, or a suitable error message if it is - * not. Note that warnings are not supported. - */ - @Nullable - String checkValid(Rule from, String toRuleClass); - } - - @SerializationConstant - public static final ValidityPredicate ANY_EDGE = (from, toRuleClass) -> null; - /** A predicate class to check if the value of the attribute comes from a predefined set. */ public static class AllowedValueSet implements PredicateWithMessage { @@ -256,7 +241,6 @@ public static class ImmutableAttributeFactory { private final RuleClassNamePredicate allowedRuleClassesForLabels; private final RuleClassNamePredicate allowedRuleClassesForLabelsWarning; private final FileTypeSet allowedFileTypesForLabels; - private final ValidityPredicate validityPredicate; private final Object value; private final AttributeValueSource valueSource; private final boolean valueSet; @@ -275,7 +259,6 @@ private ImmutableAttributeFactory( RuleClassNamePredicate allowedRuleClassesForLabels, RuleClassNamePredicate allowedRuleClassesForLabelsWarning, FileTypeSet allowedFileTypesForLabels, - ValidityPredicate validityPredicate, AttributeValueSource valueSource, boolean valueSet, PredicateWithMessage allowedValues, @@ -287,7 +270,6 @@ private ImmutableAttributeFactory( this.allowedRuleClassesForLabels = allowedRuleClassesForLabels; this.allowedRuleClassesForLabelsWarning = allowedRuleClassesForLabelsWarning; this.allowedFileTypesForLabels = allowedFileTypesForLabels; - this.validityPredicate = validityPredicate; this.value = value; this.valueSource = valueSource; this.valueSet = valueSet; @@ -303,7 +285,6 @@ private ImmutableAttributeFactory( allowedRuleClassesForLabels, allowedRuleClassesForLabelsWarning, allowedFileTypesForLabels, - validityPredicate, value, valueSource, valueSet, @@ -359,7 +340,6 @@ public Attribute build(String name) { allowedRuleClassesForLabels, allowedRuleClassesForLabelsWarning, allowedFileTypesForLabels, - validityPredicate, allowedValues, requiredProviders, aspects); @@ -383,7 +363,6 @@ public boolean equals(Object o) { && Objects.equals( allowedRuleClassesForLabelsWarning, that.allowedRuleClassesForLabelsWarning) && Objects.equals(allowedFileTypesForLabels, that.allowedFileTypesForLabels) - && Objects.equals(validityPredicate, that.validityPredicate) && Objects.equals(value, that.value) && Objects.equals(valueSource, that.valueSource) && valueSet == that.valueSet @@ -417,7 +396,6 @@ public static class Builder { private RuleClassNamePredicate allowedRuleClassesForLabels = ANY_RULE; private RuleClassNamePredicate allowedRuleClassesForLabelsWarning = NO_RULE; private FileTypeSet allowedFileTypesForLabels; - private ValidityPredicate validityPredicate = ANY_EDGE; private Object value; private String doc; private AttributeValueSource valueSource = AttributeValueSource.DIRECT; @@ -1015,14 +993,6 @@ public Builder aspect(final Aspect aspect) { return this; } - /** Sets the predicate-like edge validity checker. */ - @CanIgnoreReturnValue - public Builder validityPredicate(ValidityPredicate validityPredicate) { - propertyFlags.add(PropertyFlag.STRICT_LABEL_CHECKING); - this.validityPredicate = validityPredicate; - return this; - } - /** The value of the attribute must be one of allowedValues. */ @CanIgnoreReturnValue public Builder allowedValues(PredicateWithMessage allowedValues) { @@ -1080,7 +1050,6 @@ public ImmutableAttributeFactory buildPartial() { allowedRuleClassesForLabels, allowedRuleClassesForLabelsWarning, allowedFileTypesForLabels, - validityPredicate, valueSource, valueSet, allowedValues, @@ -1818,12 +1787,6 @@ public static LabelListLateBoundDefault fromRuleAndAttributesOnly( */ private final FileTypeSet allowedFileTypesForLabels; - /** - * This predicate-like object checks if the edge between two rules using this attribute is valid - * in the dependency graph. Returns null if valid, otherwise an error message. - */ - private final ValidityPredicate validityPredicate; - private final PredicateWithMessage allowedValues; private final RequiredProviders requiredProviders; @@ -1854,7 +1817,6 @@ private Attribute( RuleClassNamePredicate allowedRuleClassesForLabels, RuleClassNamePredicate allowedRuleClassesForLabelsWarning, FileTypeSet allowedFileTypesForLabels, - ValidityPredicate validityPredicate, PredicateWithMessage allowedValues, RequiredProviders requiredProviders, AspectsList aspects) { @@ -1876,7 +1838,6 @@ private Attribute( this.allowedRuleClassesForLabels = allowedRuleClassesForLabels; this.allowedRuleClassesForLabelsWarning = allowedRuleClassesForLabelsWarning; this.allowedFileTypesForLabels = allowedFileTypesForLabels; - this.validityPredicate = validityPredicate; this.allowedValues = allowedValues; this.requiredProviders = requiredProviders; this.aspects = aspects; @@ -1891,7 +1852,6 @@ private Attribute( allowedRuleClassesForLabels, allowedRuleClassesForLabelsWarning, allowedFileTypesForLabels, - validityPredicate, allowedValues, requiredProviders, aspects); @@ -2114,10 +2074,6 @@ public FileTypeSet getAllowedFileTypesPredicate() { return allowedFileTypesForLabels; } - public ValidityPredicate getValidityPredicate() { - return validityPredicate; - } - public PredicateWithMessage getAllowedValues() { return allowedValues; } @@ -2273,10 +2229,6 @@ public void failIfNotAValidOverride() throws EvalException { allowedFileTypesForLabels != FileTypeSet.NO_FILE, "attribute `%s`: can't override allowed files", name); - failIf( - validityPredicate != Attribute.ANY_EDGE, - "attribute `%s`: can't override allowed files", - name); failIf( !requiredProviders.acceptsAny(), "attribute `%s`: can't override required providers", name); failIf( @@ -2316,7 +2268,6 @@ public boolean equals(Object o) { && Objects.equals( allowedRuleClassesForLabelsWarning, attribute.allowedRuleClassesForLabelsWarning) && Objects.equals(allowedFileTypesForLabels, attribute.allowedFileTypesForLabels) - && Objects.equals(validityPredicate, attribute.validityPredicate) && Objects.equals(allowedValues, attribute.allowedValues) && Objects.equals(requiredProviders, attribute.requiredProviders) && Objects.equals(aspects, attribute.aspects); @@ -2336,7 +2287,6 @@ public Attribute.Builder cloneBuilder(Type tp) { builder.allowedRuleClassesForLabels = allowedRuleClassesForLabels; builder.allowedRuleClassesForLabelsWarning = allowedRuleClassesForLabelsWarning; builder.requiredProvidersBuilder = requiredProviders.copyAsBuilder(); - builder.validityPredicate = validityPredicate; builder.transitionFactory = transitionFactory; builder.propertyFlags = newEnumSet(propertyFlags, PropertyFlag.class); builder.value = defaultValue; diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AarImportBaseRule.java b/src/main/java/com/google/devtools/build/lib/rules/android/AarImportBaseRule.java index 7e70199866234e..a802e865bec14f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AarImportBaseRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AarImportBaseRule.java @@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.rules.android; -import static com.google.devtools.build.lib.packages.Attribute.ANY_EDGE; import static com.google.devtools.build.lib.packages.Attribute.attr; import static com.google.devtools.build.lib.packages.BuildType.LABEL; import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST; @@ -54,8 +53,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .add( attr("exports", LABEL_LIST) .allowedRuleClasses("aar_import", "java_import") - .allowedFileTypes() - .validityPredicate(ANY_EDGE)) + .allowedFileTypes()) /* A JAR file that contains source code for the compiled JAR files in the AAR. */ diff --git a/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java b/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java index 575868b3c9bbe1..c6d70a52ff4de9 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java @@ -358,15 +358,6 @@ public void factoryEquality() throws Exception { .value(Label.parseCanonicalUnchecked("//a:b")) .allowedValues(new AllowedValueSet(Label.parseCanonical("//a:b"))) .buildPartial()) - .addEqualityGroup( - attr("foo", LABEL) - .value(Label.parseCanonicalUnchecked("//a:b")) - .validityPredicate(Attribute.ANY_EDGE) - .buildPartial(), - attr("foo", LABEL) - .value(Label.parseCanonicalUnchecked("//a:b")) - .validityPredicate(Attribute.ANY_EDGE) - .buildPartial()) .addEqualityGroup( attr("foo", LABEL) .value(Label.parseCanonicalUnchecked("//a:b")) 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 b17329ed3c193f..25be24b9ee2919 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 @@ -31,7 +31,6 @@ import static com.google.devtools.build.lib.packages.Type.STRING; import static com.google.devtools.build.lib.packages.Types.STRING_LIST; import static org.junit.Assert.assertThrows; -import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -49,7 +48,6 @@ import com.google.devtools.build.lib.events.EventCollector; import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate.CannotPrecomputeDefaultsException; -import com.google.devtools.build.lib.packages.Attribute.ValidityPredicate; import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SafeImplicitOutputsFunction; import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType; import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory; @@ -1104,65 +1102,6 @@ private static RuleClass createChildRuleClass(RuleClass parentRuleClass) { .build(); } - @Test - public void testValidityChecker() throws Exception { - Rule dep1 = - createRule( - new RuleClass.Builder("dep1class", RuleClassType.NORMAL, false) - .factory(DUMMY_CONFIGURED_TARGET_FACTORY) - .add(attr("tags", STRING_LIST)) - .build(), - "dep1", - ImmutableMap.of()); - Rule dep2 = - createRule( - new RuleClass.Builder("dep2class", RuleClassType.NORMAL, false) - .factory(DUMMY_CONFIGURED_TARGET_FACTORY) - .add(attr("tags", STRING_LIST)) - .build(), - "dep2", - ImmutableMap.of()); - - ValidityPredicate checker = - new ValidityPredicate() { - @Override - public String checkValid(Rule from, String toRuleClass) { - assertThat(from.getName()).isEqualTo("top"); - switch (toRuleClass) { - case "dep1class": - return "pear"; - case "dep2class": - return null; - default: - fail("invalid dependency"); - return null; - } - } - }; - - RuleClass topClass = new RuleClass.Builder("top", RuleClassType.NORMAL, false) - .factory(DUMMY_CONFIGURED_TARGET_FACTORY) - .add(attr("tags", STRING_LIST)) - .add(attr("deps", LABEL_LIST).legacyAllowAnyFileType() - .validityPredicate(checker)) - .build(); - - Rule topRule = createRule(topClass, "top", ImmutableMap.of()); - - assertThat( - topClass - .getAttributeByName("deps") - .getValidityPredicate() - .checkValid(topRule, dep1.getRuleClass())) - .isEqualTo("pear"); - assertThat( - topClass - .getAttributeByName("deps") - .getValidityPredicate() - .checkValid(topRule, dep2.getRuleClass())) - .isNull(); - } - @Test public void testBadRuleClassNames() { expectError(RuleClassType.NORMAL, "8abc");