Skip to content

Commit

Permalink
Delete dead code for attribute validation
Browse files Browse the repository at this point in the history 
This CL follows 32def70 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
  • Loading branch information
@brandjon @copybara-github
brandjon authored and copybara-github committed Apr 17, 2024
1 parent dc5ec2b commit 6967b64
Show file tree
Hide file tree
Showing 5 changed files with 1 addition and 128 deletions.
5 changes: 0 additions & 5 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 @@ -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;
}
Expand Down
50 changes: 0 additions & 50 deletions src/main/java/com/google/devtools/build/lib/packages/Attribute.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object> {

Expand Down Expand Up @@ -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;
Expand All @@ -275,7 +259,6 @@ private ImmutableAttributeFactory(
RuleClassNamePredicate allowedRuleClassesForLabels,
RuleClassNamePredicate allowedRuleClassesForLabelsWarning,
FileTypeSet allowedFileTypesForLabels,
ValidityPredicate validityPredicate,
AttributeValueSource valueSource,
boolean valueSet,
PredicateWithMessage<Object> allowedValues,
Expand All @@ -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;
Expand All @@ -303,7 +285,6 @@ private ImmutableAttributeFactory(
allowedRuleClassesForLabels,
allowedRuleClassesForLabelsWarning,
allowedFileTypesForLabels,
validityPredicate,
value,
valueSource,
valueSet,
Expand Down Expand Up @@ -359,7 +340,6 @@ public Attribute build(String name) {
allowedRuleClassesForLabels,
allowedRuleClassesForLabelsWarning,
allowedFileTypesForLabels,
validityPredicate,
allowedValues,
requiredProviders,
aspects);
Expand All @@ -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
Expand Down Expand Up @@ -417,7 +396,6 @@ public static class Builder<TYPE> {
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;
Expand Down Expand Up @@ -1015,14 +993,6 @@ public Builder<TYPE> aspect(final Aspect aspect) {
return this;
}

/** Sets the predicate-like edge validity checker. */
@CanIgnoreReturnValue
public Builder<TYPE> 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<TYPE> allowedValues(PredicateWithMessage<Object> allowedValues) {
Expand Down Expand Up @@ -1080,7 +1050,6 @@ public ImmutableAttributeFactory buildPartial() {
allowedRuleClassesForLabels,
allowedRuleClassesForLabelsWarning,
allowedFileTypesForLabels,
validityPredicate,
valueSource,
valueSet,
allowedValues,
Expand Down Expand Up @@ -1818,12 +1787,6 @@ public static LabelListLateBoundDefault<Void> 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<Object> allowedValues;

private final RequiredProviders requiredProviders;
Expand Down Expand Up @@ -1854,7 +1817,6 @@ private Attribute(
RuleClassNamePredicate allowedRuleClassesForLabels,
RuleClassNamePredicate allowedRuleClassesForLabelsWarning,
FileTypeSet allowedFileTypesForLabels,
ValidityPredicate validityPredicate,
PredicateWithMessage<Object> allowedValues,
RequiredProviders requiredProviders,
AspectsList aspects) {
Expand All @@ -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;
Expand All @@ -1891,7 +1852,6 @@ private Attribute(
allowedRuleClassesForLabels,
allowedRuleClassesForLabelsWarning,
allowedFileTypesForLabels,
validityPredicate,
allowedValues,
requiredProviders,
aspects);
Expand Down Expand Up @@ -2114,10 +2074,6 @@ public FileTypeSet getAllowedFileTypesPredicate() {
return allowedFileTypesForLabels;
}

public ValidityPredicate getValidityPredicate() {
return validityPredicate;
}

public PredicateWithMessage<Object> getAllowedValues() {
return allowedValues;
}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);
Expand All @@ -2336,7 +2287,6 @@ public <TypeT> Attribute.Builder<TypeT> cloneBuilder(Type<TypeT> 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;
Expand Down
4 changes: 1 addition & 3 deletions src/main/java/com/google/devtools/build/lib/rules/android/AarImportBaseRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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())
/* <!-- #BLAZE_RULE(aar_import).ATTRIBUTE(srcjar) -->
A JAR file that contains source code for the compiled JAR files in the AAR.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
Expand Down
9 changes: 0 additions & 9 deletions src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down
61 changes: 0 additions & 61 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 @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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");
Expand Down

0 comments on commit 6967b64

Please sign in to comment.