Skip to content

Commit 31fd464

Browse files
tetrominocopybara-github
tetromino
authored and
copybara-github
committed
Use value equality instead of reference equality for Starlark attr objects
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
1 parent 74e3be6 commit 31fd464

File tree

5 files changed

+176
-0
lines changed

5 files changed

+176
-0
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import com.google.devtools.build.lib.util.FileTypeSet;
4949
import java.util.List;
5050
import java.util.Map;
51+
import java.util.Objects;
5152
import java.util.Optional;
5253
import javax.annotation.Nullable;
5354
import net.starlark.java.eval.Dict;
@@ -790,6 +791,25 @@ public Attribute build(String name) {
790791
public void repr(Printer printer) {
791792
printer.append("<attr." + name + ">");
792793
}
794+
795+
// Value equality semantics - same as for native Attribute.
796+
@Override
797+
public boolean equals(Object o) {
798+
if (this == o) {
799+
return true;
800+
}
801+
if (!(o instanceof Descriptor)) {
802+
return false;
803+
}
804+
Descriptor that = (Descriptor) o;
805+
return Objects.equals(name, that.name)
806+
&& Objects.equals(attributeFactory, that.attributeFactory);
807+
}
808+
809+
@Override
810+
public int hashCode() {
811+
return Objects.hash(name, attributeFactory);
812+
}
793813
}
794814

795815
// Returns an immutable map from a list of alternating name/value pairs,

‎src/main/java/com/google/devtools/build/lib/packages/Attribute.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,7 @@ public static class ImmutableAttributeFactory {
265265
private final PredicateWithMessage<Object> allowedValues;
266266
private final RequiredProviders requiredProviders;
267267
private final ImmutableList<AspectDetails<?>> aspects;
268+
private final int hashCode;
268269

269270
private ImmutableAttributeFactory(
270271
Type<?> type,
@@ -295,6 +296,22 @@ private ImmutableAttributeFactory(
295296
this.allowedValues = allowedValues;
296297
this.requiredProviders = requiredProviders;
297298
this.aspects = aspects;
299+
this.hashCode =
300+
Objects.hash(
301+
type,
302+
doc,
303+
transitionFactory,
304+
allowedRuleClassesForLabels,
305+
allowedRuleClassesForLabelsWarning,
306+
allowedFileTypesForLabels,
307+
validityPredicate,
308+
value,
309+
valueSource,
310+
valueSet,
311+
propertyFlags,
312+
allowedValues,
313+
requiredProviders,
314+
aspects);
298315
}
299316

300317
public AttributeValueSource getValueSource() {
@@ -344,6 +361,39 @@ public Attribute build(String name) {
344361
requiredProviders,
345362
aspects);
346363
}
364+
365+
// Value equality semantics - same as for Attribute.
366+
@Override
367+
public boolean equals(Object o) {
368+
if (this == o) {
369+
return true;
370+
}
371+
if (!(o instanceof ImmutableAttributeFactory)) {
372+
return false;
373+
}
374+
ImmutableAttributeFactory that = (ImmutableAttributeFactory) o;
375+
return hashCode == that.hashCode
376+
&& Objects.equals(type, that.type)
377+
&& Objects.equals(doc, that.doc)
378+
&& Objects.equals(transitionFactory, that.transitionFactory)
379+
&& Objects.equals(allowedRuleClassesForLabels, that.allowedRuleClassesForLabels)
380+
&& Objects.equals(
381+
allowedRuleClassesForLabelsWarning, that.allowedRuleClassesForLabelsWarning)
382+
&& Objects.equals(allowedFileTypesForLabels, that.allowedFileTypesForLabels)
383+
&& Objects.equals(validityPredicate, that.validityPredicate)
384+
&& Objects.equals(value, that.value)
385+
&& Objects.equals(valueSource, that.valueSource)
386+
&& valueSet == that.valueSet
387+
&& Objects.equals(propertyFlags, that.propertyFlags)
388+
&& Objects.equals(allowedValues, that.allowedValues)
389+
&& Objects.equals(requiredProviders, that.requiredProviders)
390+
&& Objects.equals(aspects, that.aspects);
391+
}
392+
393+
@Override
394+
public int hashCode() {
395+
return hashCode;
396+
}
347397
}
348398

349399
/**

‎src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,16 @@
1717
import static com.google.devtools.build.lib.packages.Attribute.attr;
1818
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
1919
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
20+
import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL;
2021
import static com.google.devtools.build.lib.packages.Type.INTEGER;
2122
import static com.google.devtools.build.lib.packages.Type.STRING;
2223
import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
2324
import static org.junit.Assert.assertThrows;
2425

2526
import com.google.common.base.Predicates;
2627
import com.google.common.collect.ImmutableMap;
28+
import com.google.common.testing.EqualsTester;
29+
import com.google.devtools.build.lib.analysis.DefaultInfo;
2730
import com.google.devtools.build.lib.analysis.config.BuildOptions;
2831
import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
2932
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
@@ -34,6 +37,7 @@
3437
import com.google.devtools.build.lib.analysis.util.TestAspects;
3538
import com.google.devtools.build.lib.cmdline.Label;
3639
import com.google.devtools.build.lib.events.EventHandler;
40+
import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
3741
import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassNamePredicate;
3842
import com.google.devtools.build.lib.testutil.FakeAttributeMapper;
3943
import com.google.devtools.build.lib.util.FileType;
@@ -325,4 +329,77 @@ public void allowedRuleClassesAndAllowedRuleClassesWithWarningsCannotOverlap() {
325329
.build());
326330
assertThat(e).hasMessageThat().contains("may not contain the same rule classes");
327331
}
332+
333+
@Test
334+
public void factoryEquality() throws Exception {
335+
new EqualsTester()
336+
.addEqualityGroup(attr("foo", LABEL).buildPartial(), attr("foo", LABEL).buildPartial())
337+
.addEqualityGroup(
338+
attr("foo", LABEL).value(Label.parseCanonicalUnchecked("//a:b")).buildPartial(),
339+
attr("foo", LABEL).value(Label.parseCanonicalUnchecked("//a:b")).buildPartial())
340+
.addEqualityGroup(
341+
attr("foo", NODEP_LABEL).value(Label.parseCanonicalUnchecked("//a:b")).buildPartial(),
342+
attr("foo", NODEP_LABEL).value(Label.parseCanonicalUnchecked("//a:b")).buildPartial())
343+
.addEqualityGroup(
344+
attr("foo", LABEL).value(Label.parseCanonicalUnchecked("//c:d")).buildPartial(),
345+
attr("foo", LABEL).value(Label.parseCanonicalUnchecked("//c:d")).buildPartial())
346+
.addEqualityGroup(
347+
attr("foo", LABEL)
348+
.value(Label.parseCanonicalUnchecked("//a:b"))
349+
.setDoc("My doc")
350+
.buildPartial(),
351+
attr("foo", LABEL)
352+
.value(Label.parseCanonicalUnchecked("//a:b"))
353+
.setDoc("My doc")
354+
.buildPartial())
355+
.addEqualityGroup(
356+
// PredicateWithMessage does not define any particular equality semantics
357+
attr("foo", LABEL)
358+
.value(Label.parseCanonicalUnchecked("//a:b"))
359+
.allowedValues(new AllowedValueSet(Label.parseCanonical("//a:b")))
360+
.buildPartial())
361+
.addEqualityGroup(
362+
attr("foo", LABEL)
363+
.value(Label.parseCanonicalUnchecked("//a:b"))
364+
.validityPredicate(Attribute.ANY_EDGE)
365+
.buildPartial(),
366+
attr("foo", LABEL)
367+
.value(Label.parseCanonicalUnchecked("//a:b"))
368+
.validityPredicate(Attribute.ANY_EDGE)
369+
.buildPartial())
370+
.addEqualityGroup(
371+
attr("foo", LABEL)
372+
.value(Label.parseCanonicalUnchecked("//a:b"))
373+
.allowedRuleClasses("java_binary")
374+
.buildPartial(),
375+
attr("foo", LABEL)
376+
.value(Label.parseCanonicalUnchecked("//a:b"))
377+
.allowedRuleClasses("java_binary")
378+
.buildPartial())
379+
.addEqualityGroup(
380+
attr("foo", LABEL)
381+
.value(Label.parseCanonicalUnchecked("//a:b"))
382+
.allowedFileTypes(FileTypeSet.ANY_FILE)
383+
.buildPartial(),
384+
attr("foo", LABEL)
385+
.value(Label.parseCanonicalUnchecked("//a:b"))
386+
.allowedFileTypes(FileTypeSet.ANY_FILE)
387+
.buildPartial())
388+
.addEqualityGroup(
389+
attr("foo", LABEL)
390+
.value(Label.parseCanonicalUnchecked("//a:b"))
391+
.mandatoryProviders(DefaultInfo.PROVIDER.id())
392+
.buildPartial(),
393+
attr("foo", LABEL)
394+
.value(Label.parseCanonicalUnchecked("//a:b"))
395+
.mandatoryProviders(DefaultInfo.PROVIDER.id())
396+
.buildPartial())
397+
.addEqualityGroup(
398+
// Aspects list builder does not define any particular equality semantics
399+
attr("foo", LABEL)
400+
.value(Label.parseCanonicalUnchecked("//a:b"))
401+
.aspect(TestAspects.SIMPLE_ASPECT)
402+
.buildPartial())
403+
.testEquals();
404+
}
328405
}

‎src/test/java/com/google/devtools/build/lib/starlark/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ java_test(
8585
"//src/test/java/com/google/devtools/build/lib/testutil",
8686
"//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils",
8787
"//third_party:guava",
88+
"//third_party:guava-testlib",
8889
"//third_party:jsr305",
8990
"//third_party:junit4",
9091
"//third_party:mockito",

‎src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import com.google.common.collect.ImmutableList;
2626
import com.google.common.collect.ImmutableMap;
2727
import com.google.common.collect.Iterables;
28+
import com.google.common.testing.EqualsTester;
2829
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
2930
import com.google.devtools.build.lib.analysis.RuleContext;
3031
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
@@ -287,6 +288,33 @@ public void testAttrNameCannotStartWithDigit() throws Exception {
287288
ev.assertContainsError("attribute name `2_foo` is not a valid identifier");
288289
}
289290

291+
@Test
292+
public void testAttrEquality() throws Exception {
293+
new EqualsTester()
294+
.addEqualityGroup(
295+
buildAttribute("foo", "attr.string_list(default = [])"),
296+
buildAttribute("foo", "attr.string_list(default = [])"))
297+
.addEqualityGroup(
298+
buildAttribute("bar", "attr.string_list(default = [])"),
299+
buildAttribute("bar", "attr.string_list(default = [])"))
300+
.addEqualityGroup(
301+
buildAttribute("bar", "attr.label_list(default = [])"),
302+
buildAttribute("bar", "attr.label_list(default = [])"))
303+
.addEqualityGroup(
304+
buildAttribute("foo", "attr.string_list(default = ['hello'])"),
305+
buildAttribute("foo", "attr.string_list(default = ['hello'])"))
306+
.addEqualityGroup(
307+
buildAttribute("foo", "attr.string_list(doc = 'Blah blah blah', default = [])"),
308+
buildAttribute("foo", "attr.string_list(doc = 'Blah blah blah', default = [])"))
309+
.addEqualityGroup(
310+
buildAttribute("foo", "attr.string_list(mandatory = True, default = [])"),
311+
buildAttribute("foo", "attr.string_list(mandatory = True, default = [])"))
312+
.addEqualityGroup(
313+
buildAttribute("foo", "attr.string_list(allow_empty = False, default = [])"),
314+
buildAttribute("foo", "attr.string_list(allow_empty = False, default = [])"))
315+
.testEquals();
316+
}
317+
290318
@Test
291319
public void testRuleClassTooManyAttributes() throws Exception {
292320
ev.setFailFast(false);

0 commit comments

Comments
 (0)