AbstractPackageSanityTests equals test fails for AutoValue classes with Set members

[GoogleCodeExporter]

For an @AutoValue that has a field that is a parameterised set is tested by 
AbstractPackageSanityTests, for some types, the tests seem to generate 
incorrect equality groups.  For example, this class:

@AutoValue
public abstract class EqualsBreaker {
  EqualsBreaker() {}

  public static EqualsBreaker create(Set<HostAndPort> hosts) {
    return new AutoValue_EqualsBreaker(ImmutableSet.copyOf(hosts));
  }

  public abstract Set<HostAndPort> hosts();
}

Leads to a failure like this: 

Caused by: junit.framework.AssertionFailedError: 
com.spotify.hermes.cache.AutoValue_CachedResponse([2]) must not be 
Object#equals to com.spotify.hermes.cache.AutoValue_CachedResponse(Set@1)

The following changes make this work as expected:
- changing to Set<String>
- changing to List<HostAndPort>

I originally had another @AutoValue class as Set type parameter, but changed to 
HostAndPort to pick something that was 'not almost primitive' and that I was 
very sure wouldn't have any issues with its equals method.

Explicitly testing the class using EqualsTester and hand-coded equality groups 
works as expected, so I'm pretty sure the issue is in how 
AbstractPackageSanityTests generates equality groups for sets.

Original issue reported on code.google.com by petterma...@gmail.com on 11 Feb 2014 at 8:31

[GoogleCodeExporter]

Original comment by lowas...@google.com on 18 Feb 2014 at 7:02

• Added labels: Package-Testing, Type-Defect

[GoogleCodeExporter]

The root cause of this is a little complicated. I'll try my best to explain, at 
least for the record.

A few things happening when APST tries to test equals for this AutoValue class:

1. It tries to generate distinct values for HostAndPort. But couldn't because 
it's not smart enough to use the factory methods in HostAndPort (maybe it 
should?)

2. Failing to get distinct values, it just generates a very bad proxy of Set, 
that is only good for calling equals()/hashCode(). If you try to call other 
methods, it behaves weirdly.

3. When trying to generate a second equality group, APST mistakenly uses the 
raw type instead of the parameterized Set<HostAndPort> type, so it generated a 
LinkedHashSet<Object> that contains a String. This isn't as bad as it sounds 
because it kinda worked in practice. If all you care is equals(), 
String.equals(HostAndPort) correctly returns the "false" we need so that the 
second equality group is indeed inequal to the first. 

4. APST doesn't know to use the static create() method. It uses the constructor 
of the generated subclass, which bypasses the ImmutableSet.copyOf() call.

Put all that together,  when LinkedHashSet<>.equals(bad_proxy_of_set) is 
called, it's fooled by the weird Set proxy and thinks it's equal, even though 
it's not and meant not to be. I'm to be blamed for once changing the dummy 
value of int to 1 instead of 0, across the board, hoping to avoid div-by-0 
problems. But that changed caused these collection proxies to return 1 for 
size() yet still return false for iterator().hasNext().

This whole "when no distinct value can be found, generate a proxy for type T 
and make it distinct and 'work'" thing is a hack. There is no such thing as a 
proxy that works for all types. We could special-case it to make collections 
work better. Though it can still cause surprises when non-collection types need 
proxies.

A quick solution to the problem, is to skip AutoValue classes altogether. For 
example, with ignoreClasses(Predicate).

But when this problem bites for non-AutoValue class, the current work-around is 
to explicitly register distinct value through 
ClassSanityTester.setSampleInstances(). It also means that this class needs to 
have its own testEquals(). The APST auto-test doesn't work well in such cases.

Long term solution? We could more aggresively find a static factory method to 
call and auto-magically figure out the parameter values to pass to the factory 
methods. I'm still a bit scared of that path because things seem to become even 
more magical.

Or, we could just patch by adding proper proxies for common collections.

Original comment by be...@google.com on 20 Feb 2014 at 11:35

[GoogleCodeExporter]

Skipping AutoValue classes is problematic as it's possible to override equals() 
in the abstract class, I guess.

And I'd agree about the problems with magic and static factories. So perhaps 
better Collections proxies might be an OK way forward? 

Original comment by petterma...@gmail.com on 23 Feb 2014 at 6:18

[GoogleCodeExporter]

Since it's relatively common for constructors to copy the passed-in Set/List 
into ImmutableSet/ImmutableList, there is no way we can generate distinct 
proxies that remain distinct after copied into their immutable counter-part, 
when we can't generate distinct values for T.

Another possibility, is to take advantage of the fact that few constructors 
inspect the List/Set content. The Set/List are either directly stored in a 
field, or copied to ImmutableSet/ImmutableList. So if we passed in a 
Set<Object> with a String in it, no ClassCastException will happen until the 
set is really used by some methods.

Since we only care about equality, we can temporarily pass in a Set with a 
String element, verify that equals() works as expected, and throw away this 
badly constructed object.

This allows us to construct truely distinct collections even when we don't know 
how to construct T.

I tested this approach internally in Google codebase and found no type error.

Original comment by be...@google.com on 23 Feb 2014 at 4:53

[GoogleCodeExporter]

Oh, just curious: when equals()/hashCode() needs to be implemented, what do you 
want to gain from AutoValue? The free toString() implementation? Or the free 
generated constructor?

Original comment by be...@google.com on 23 Feb 2014 at 4:56

[GoogleCodeExporter]

It's not really that I want to implement them, it's just something that isn't 
forbidden by AutoValue, if I understand the docs right. There is a 
recommendation that if you do provide an implementation, you should make it 
final so that readers know that the auto-generated subclass doesn't override it.

The use case where I ran into this was that I wanted to be able to use 
AutoValue classes, where there is less value in APST, in the same package that 
I had 'normal' classes, where I think it's a good idea to not have to write all 
the boilerplate tests.

Original comment by petterma...@gmail.com on 23 Feb 2014 at 8:35

[GoogleCodeExporter]

Fixed, though we cut the version 17 release branch previous to that. If you'd 
like it to be cherry-picked, we can check how far along the release is. Let me 
know.

http://code.google.com/p/guava-libraries/source/detail?r=6c2d78402229cc267bddb26
f174078d1353337d0

Original comment by cpov...@google.com on 10 Apr 2014 at 1:48

• Changed state: Fixed
• Added labels: Milestone-Release18

[GoogleCodeExporter]

I can easily include this in 17, since I need to pull in a BloomFilter related 
change to the branch anyway.

Original comment by cgdecker@google.com on 10 Apr 2014 at 3:51

• Added labels: Milestone-Release17
• Removed labels: Milestone-Release18

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<issue id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:10

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:07

• Added labels: Restrict-AddIssueComment-Commit