Skip to content

Commit

Permalink
Use value equality instead of reference equality for Starlark attr ob…
Browse files Browse the repository at this point in the history 
…jects

We already use value equality for native Attribute objects in Java:
attr("foo", STRING).value("x").build().equals(attr("foo", STRING).value("x").build())

We ought to do the same in Starlark. Motivated by the attempt to move python rule
documentation from Java stubs to @_builtins .bzl files - and running afoul of the
attribute list merger logic in union_attr in @_builtins//:common/python/common.bzl

Note that this would change the behavior of Starlark code which uses attr objects as
dict keys - but the breakage is very unlikely, and I would arguge the new behavior
is correct.

RELNOTES: attr objects in Starlark now use value equality rather than reference
equality.
PiperOrigin-RevId: 561704016
Change-Id: I8712696a9e5fa3bee809098e950f88966959ab48
  • Loading branch information
@tetromino @copybara-github
tetromino authored and copybara-github committed Aug 31, 2023
1 parent 74e3be6 commit 31fd464
Show file tree
Hide file tree
Showing 5 changed files with 176 additions and 0 deletions.
20 changes: 20 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import com.google.devtools.build.lib.util.FileTypeSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import javax.annotation.Nullable;
import net.starlark.java.eval.Dict;
Expand Down Expand Up @@ -790,6 +791,25 @@ public Attribute build(String name) {
public void repr(Printer printer) {
printer.append("<attr." + name + ">");
}

// Value equality semantics - same as for native Attribute.
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof Descriptor)) {
return false;
}
Descriptor that = (Descriptor) o;
return Objects.equals(name, that.name)
&& Objects.equals(attributeFactory, that.attributeFactory);
}

@Override
public int hashCode() {
return Objects.hash(name, attributeFactory);
}
}

// Returns an immutable map from a list of alternating name/value pairs,
Expand Down
50 changes: 50 additions & 0 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 @@ -265,6 +265,7 @@ public static class ImmutableAttributeFactory {
private final PredicateWithMessage<Object> allowedValues;
private final RequiredProviders requiredProviders;
private final ImmutableList<AspectDetails<?>> aspects;
private final int hashCode;

private ImmutableAttributeFactory(
Type<?> type,
Expand Down Expand Up @@ -295,6 +296,22 @@ private ImmutableAttributeFactory(
this.allowedValues = allowedValues;
this.requiredProviders = requiredProviders;
this.aspects = aspects;
this.hashCode =
Objects.hash(
type,
doc,
transitionFactory,
allowedRuleClassesForLabels,
allowedRuleClassesForLabelsWarning,
allowedFileTypesForLabels,
validityPredicate,
value,
valueSource,
valueSet,
propertyFlags,
allowedValues,
requiredProviders,
aspects);
}

public AttributeValueSource getValueSource() {
Expand Down Expand Up @@ -344,6 +361,39 @@ public Attribute build(String name) {
requiredProviders,
aspects);
}

// Value equality semantics - same as for Attribute.
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof ImmutableAttributeFactory)) {
return false;
}
ImmutableAttributeFactory that = (ImmutableAttributeFactory) o;
return hashCode == that.hashCode
&& Objects.equals(type, that.type)
&& Objects.equals(doc, that.doc)
&& Objects.equals(transitionFactory, that.transitionFactory)
&& Objects.equals(allowedRuleClassesForLabels, that.allowedRuleClassesForLabels)
&& 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
&& Objects.equals(propertyFlags, that.propertyFlags)
&& Objects.equals(allowedValues, that.allowedValues)
&& Objects.equals(requiredProviders, that.requiredProviders)
&& Objects.equals(aspects, that.aspects);
}

@Override
public int hashCode() {
return hashCode;
}
}

/**
Expand Down
77 changes: 77 additions & 0 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 @@ -17,13 +17,16 @@
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;
import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL;
import static com.google.devtools.build.lib.packages.Type.INTEGER;
import static com.google.devtools.build.lib.packages.Type.STRING;
import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
import static org.junit.Assert.assertThrows;

import com.google.common.base.Predicates;
import com.google.common.collect.ImmutableMap;
import com.google.common.testing.EqualsTester;
import com.google.devtools.build.lib.analysis.DefaultInfo;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
Expand All @@ -34,6 +37,7 @@
import com.google.devtools.build.lib.analysis.util.TestAspects;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassNamePredicate;
import com.google.devtools.build.lib.testutil.FakeAttributeMapper;
import com.google.devtools.build.lib.util.FileType;
Expand Down Expand Up @@ -325,4 +329,77 @@ public void allowedRuleClassesAndAllowedRuleClassesWithWarningsCannotOverlap() {
.build());
assertThat(e).hasMessageThat().contains("may not contain the same rule classes");
}

@Test
public void factoryEquality() throws Exception {
new EqualsTester()
.addEqualityGroup(attr("foo", LABEL).buildPartial(), attr("foo", LABEL).buildPartial())
.addEqualityGroup(
attr("foo", LABEL).value(Label.parseCanonicalUnchecked("//a:b")).buildPartial(),
attr("foo", LABEL).value(Label.parseCanonicalUnchecked("//a:b")).buildPartial())
.addEqualityGroup(
attr("foo", NODEP_LABEL).value(Label.parseCanonicalUnchecked("//a:b")).buildPartial(),
attr("foo", NODEP_LABEL).value(Label.parseCanonicalUnchecked("//a:b")).buildPartial())
.addEqualityGroup(
attr("foo", LABEL).value(Label.parseCanonicalUnchecked("//c:d")).buildPartial(),
attr("foo", LABEL).value(Label.parseCanonicalUnchecked("//c:d")).buildPartial())
.addEqualityGroup(
attr("foo", LABEL)
.value(Label.parseCanonicalUnchecked("//a:b"))
.setDoc("My doc")
.buildPartial(),
attr("foo", LABEL)
.value(Label.parseCanonicalUnchecked("//a:b"))
.setDoc("My doc")
.buildPartial())
.addEqualityGroup(
// PredicateWithMessage does not define any particular equality semantics
attr("foo", LABEL)
.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"))
.allowedRuleClasses("java_binary")
.buildPartial(),
attr("foo", LABEL)
.value(Label.parseCanonicalUnchecked("//a:b"))
.allowedRuleClasses("java_binary")
.buildPartial())
.addEqualityGroup(
attr("foo", LABEL)
.value(Label.parseCanonicalUnchecked("//a:b"))
.allowedFileTypes(FileTypeSet.ANY_FILE)
.buildPartial(),
attr("foo", LABEL)
.value(Label.parseCanonicalUnchecked("//a:b"))
.allowedFileTypes(FileTypeSet.ANY_FILE)
.buildPartial())
.addEqualityGroup(
attr("foo", LABEL)
.value(Label.parseCanonicalUnchecked("//a:b"))
.mandatoryProviders(DefaultInfo.PROVIDER.id())
.buildPartial(),
attr("foo", LABEL)
.value(Label.parseCanonicalUnchecked("//a:b"))
.mandatoryProviders(DefaultInfo.PROVIDER.id())
.buildPartial())
.addEqualityGroup(
// Aspects list builder does not define any particular equality semantics
attr("foo", LABEL)
.value(Label.parseCanonicalUnchecked("//a:b"))
.aspect(TestAspects.SIMPLE_ASPECT)
.buildPartial())
.testEquals();
}
}
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/starlark/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ java_test(
"//src/test/java/com/google/devtools/build/lib/testutil",
"//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils",
"//third_party:guava",
"//third_party:guava-testlib",
"//third_party:jsr305",
"//third_party:junit4",
"//third_party:mockito",
Expand Down
28 changes: 28 additions & 0 deletions src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.testing.EqualsTester;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
Expand Down Expand Up @@ -287,6 +288,33 @@ public void testAttrNameCannotStartWithDigit() throws Exception {
ev.assertContainsError("attribute name `2_foo` is not a valid identifier");
}

@Test
public void testAttrEquality() throws Exception {
new EqualsTester()
.addEqualityGroup(
buildAttribute("foo", "attr.string_list(default = [])"),
buildAttribute("foo", "attr.string_list(default = [])"))
.addEqualityGroup(
buildAttribute("bar", "attr.string_list(default = [])"),
buildAttribute("bar", "attr.string_list(default = [])"))
.addEqualityGroup(
buildAttribute("bar", "attr.label_list(default = [])"),
buildAttribute("bar", "attr.label_list(default = [])"))
.addEqualityGroup(
buildAttribute("foo", "attr.string_list(default = ['hello'])"),
buildAttribute("foo", "attr.string_list(default = ['hello'])"))
.addEqualityGroup(
buildAttribute("foo", "attr.string_list(doc = 'Blah blah blah', default = [])"),
buildAttribute("foo", "attr.string_list(doc = 'Blah blah blah', default = [])"))
.addEqualityGroup(
buildAttribute("foo", "attr.string_list(mandatory = True, default = [])"),
buildAttribute("foo", "attr.string_list(mandatory = True, default = [])"))
.addEqualityGroup(
buildAttribute("foo", "attr.string_list(allow_empty = False, default = [])"),
buildAttribute("foo", "attr.string_list(allow_empty = False, default = [])"))
.testEquals();
}

@Test
public void testRuleClassTooManyAttributes() throws Exception {
ev.setFailFast(false);
Expand Down

0 comments on commit 31fd464

Please sign in to comment.