Equals Verfier assigns valus ignoring generic types

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Using Equals Verifier test a class bearing List<non-String> property
2. Observe incomprehensible exception


What is the code (equals method, hashCode method, relevant fields) that
triggers the problem?

import java.util.List;

import nl.jqno.equalsverifier.EqualsVerifier;
import nl.jqno.equalsverifier.Warning;

public class EqVTest {

    public static class ClassA {

    }

    public static class ClassB {
        private List<ClassA> elements;

        public List<ClassA> getElements() {
            return elements;
        }

        public void setElements(List<ClassA> elements) {
            this.elements = elements;
        }

        @Override
        public int hashCode() {
            final int prime = 31;
            int result = 1;
            result = prime * result + ((elements == null) ? 0 : elements.hashCode());
            return result;
        }

        @Override
        public boolean equals(Object obj) {
            if (this == obj)
                return true;
            if (obj == null)
                return false;
            if (getClass() != obj.getClass())
                return false;
            ClassB other = (ClassB) obj;
            if (elements == null) {
                if (other.elements != null)
                    return false;
            } else if (!elements.equals(other.elements))
                return false;
            return true;
        }

        @Override
        public String toString() {
            String tos = "ClassB(";
            for (ClassA e : elements) {
                tos += "" + e + ",";
            }
            tos += ")";
            return tos;
        }
    }

    public static void main(String[] args) {
        EqualsVerifier.forClass(ClassB.class) //
                .withRedefinedSuperclass() //
                .suppress(Warning.STRICT_INHERITANCE) //
                .suppress(Warning.NONFINAL_FIELDS) //
                .suppress(Warning.TRANSIENT_FIELDS) //
                .suppress(Warning.IDENTICAL_COPY_FOR_VERSIONED_ENTITY) //
                .verify();
    }
}


What error message does EqualsVerifier give?

Exception in thread "main" java.lang.AssertionError: Subclass: object is not 
equal to an instance of a trivial subclass with equal fields:
  [ClassB elements=[red]]-throws ClassCastException(java.lang.String cannot be cast to EqVTest$ClassA)
Consider making the class final.
For more information, go to: http://www.jqno.nl/equalsverifier/errormessages
    at nl.jqno.equalsverifier.EqualsVerifier.handleError(EqualsVerifier.java:347)
    at nl.jqno.equalsverifier.EqualsVerifier.verify(EqualsVerifier.java:330)
    at EqVTest.main(EqVTest.java:66)



What stacktrace does EqualsVerifier print, when called with the debug()
method?


What did you expect?
Passing test. Or a comprehensible exception at least. Not ClassCastException.

What version of EqualsVerifier are you using?
1.3.1

Original issue reported on code.google.com by [email protected] on 3 Sep 2013 at 7:11

[GoogleCodeExporter]

At the first and second time I though that my equals() throws 
ClassCastException. When I started to debug it became apparent that this is 
toString() which throws. And I insist that my toString() *is* correct (note: in 
real life I never use String concat in a loop, rest assured:)

Original comment by [email protected] on 3 Sep 2013 at 7:14

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Hello Piotr,

Thanks for reporting this.

First of all, toString indeed throws a classcastexception because 
EqualsVerifier tries to put objects of the wrong type into the List. This 
happens because of type erasure; at runtime, the JVM doesn't know which type 
belongs in the list. Therefore, EqualsVerifier doesn't, either. There's no 
(feasible) way around this, unfortunately.

However, this is not why EV fails.

The reason is right there in the error message you post: "Subclass: object is 
not equal to an instance of a trivial subclass with equal fields."
EV creates a subclass on the fly and compares this to an instance of your own 
class. Normally, they should be equal to each other, but they aren't. This 
happens because you use a getClass() comparison in your equals. You can fix 
this in two ways: add usingGetClass() to your EqualsVerifier invocation, or 
make your class final.

(I recommend using instanceof in equals, but that's a different discussion.)

I do agree with you that the error message you got is confusing. EV tries to 
use the object's own toString method, but when it throws an exception, EV 
generates its own string representation. Usually, this is very helpful in 
diagnosing problems, but I agree with you that in this particular case, it's 
confusing.

I'll try to come up with way to make the error message more comprehensible, 
without compromising the cases where an error message like this one is actually 
useful.

Original comment by [email protected] on 6 Sep 2013 at 7:15

• Changed state: Accepted
• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Hi Jan,

Yes, I know why EV reports my class as invalid. There are possible fixes
 * using 'instanceof'
 * using 'instanceof' + canEqual()
 * using 'ClassUtils.getUserClass(getClass()) != ClassUtils.getUserClass(obj.getClass())' (see [1])

> This happens because of type erasure; at runtime, the JVM doesn't know which 
type belongs in the list

This is not exactly true.
While field.getType() returns List.class
field.getGenericType() returns List<String>

I know that List.class is much easier to handle (just create new list, e.g. 
ArrayList and put something into it), a "List<String>" information remains 
accessible at run-time and EV *could* use it.

As long as EV does not use generic type information when assigning dummy List 
(or any other generic, including but not limited to all collections) variables, 
it should *never* put anything inside those Lists (or Set or whatever).


[1] 
http://static.springsource.org/spring/docs/3.0.x/javadoc-api/org/springframework
/util/ClassUtils.html#getUserClass%28java.lang.Class%29

Original comment by [email protected] on 6 Sep 2013 at 11:27

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Hi Piotr,

You're right, it is possible to get to the generic type. I could look into 
fixing this, because I agree with you, just putting strings into collections 
regardless of their generic type isn't very nice. However, it does work, and it 
is very effective for what EV needs to do. It's "the simplest thing that could 
possibly work."

The only place where this issue "leaks out", is in the error reporting (which 
is where you found it), or when methods specific to the generic class are 
called (which you probably shouldn't do anyway).

The error reporting was recently improved (in version 1.2); in earlier 
versions, this wouldn't have come up anyway. So I could fix it there, too; 
which may be a lot easier to do. In fact, I may need to do that anyway, whether 
or not I do the "real" fix as well. That's what I meant with making the error 
message more comprehensible.


Regards,
Jan

Original comment by [email protected] on 10 Sep 2013 at 9:36

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Hi Jan,

I am not sure I can agree with you. If my class has a 'List<ClassA> elements' 
property (where ClassA is concrete) then I am certain *it is* a good idea to 
use ClassA in both toString() *and equals()*. Imagine that my class does not 
use default equality for elements but it is using Comparator<ClassA> or Guava's 
Equivalence<ClassA> instead!

I agree, however, that I cannot rely on elements being of ClassA if ClassA was 
just a generic type parameter of the whole class (i.e. ClassB<ClassA>) -- but 
in my example, and in many many many real-life examples it is not so.

Best regards,
Piotr

Original comment by [email protected] on 11 Sep 2013 at 6:16

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Hi Piotr,

You have a good point with the Comparator; also I wasn't familiar with Guava's 
Equivalence.
On the other hand: are you really going to call these things on the individual 
members of a collection inside your equals method? That would mean completely 
side-stepping the collection's own equals method (and its equality contract)! I 
mean, calling compareTo on a member field is one thing, but calling it 
transitively on the members field of a member field, sounds a bit fishy to me 
(and is in clear violation of the Law of Demeter).

Which is not to say that I don't want to rectify this. (I think your argument 
about the Comparator has won me over.) But the reality of the situation is that 
it's never come up before in practice. You only found out about it yourself 
because it leaked through into the error message. Combined with the difficulty 
of actually implementing this fix, I won't put it very high on my list of 
priorities.

Regards,
Jan

Original comment by [email protected] on 13 Sep 2013 at 2:55

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

I'm running into the same problem here, but I don't think it's any of my 
toString methods (at least I'm never hitting a break point in any of them.)

I, too, have a generic collection type as a field of the class I'm testing for 
equality. At runtime, EqualsVerifier invoked equals() with instances of Object, 
which I then downcast to the desired type, and which obviously fails. This 
looks like a pretty serious design flaw to me. Why would you put something in 
the collection that's clearly impossible to add in the real program? It looks 
as if it's testing behavior that could never actually occur in reality.

Original comment by [email protected] on 9 Jun 2014 at 8:49

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

First of all, I've already stated that I agree it isn't pretty, and that I want 
to fix it. However, I did give it a low priority.

EqualsVerfier was never meant to be able to test *all* possible equals methods. 
It says so right in the FAQ. The reason for this is that this is simply 
impossible. EqualsVerifier was designed to work for the 99% of cases that 
follow certain standard patterns. The current implementation works perfectly 
fine in these cases.

To be honest, I think downcasting a member collection's elements belongs to the 
1%. Not only are you bypassing the collection's own equals method, you're even 
bypassing its elements's equals method. This, to me, indicates a clear 
violation of the Law of Demeter. Now, I'm not criticising your code without 
having seen it; there might be a perfectly valid reason to do things like that, 
but I do strongly believe it puts you in one of the 4 categories I discuss in 
the FAQ.

After all, under normal circumstances, why would a collection need to know its 
elements's type in order to determine if it's equal to another collection?

However, I would very much like to see your code. I don't know everything, 
maybe your use case will convince me to bump the priority.

Original comment by [email protected] on 10 Jun 2014 at 3:01

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

The problem is that in a generic collection of type T, all elements are 
accessed as Ts. However, EqualsVerifier puts Objects in that collection, so the 
following expression will fail:

List<String> list = ...

// in equals:
String el = list.get(0);

that's because at runtime, el will be Object, not String. Sorry if my previous 
post came across harsh, I wrote it in a hurry and also managed to bake in 
multiple typos :-) 

However, I honestly don't see how that falls in the 1% bucket, it seems like a 
fairly obvious thing to do with a generic collection.

Original comment by [email protected] on 10 Jun 2014 at 3:13

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Forgot to mention:

You ask rightfully why I even need to peek into the list (very fair question.) 
The reason being: the collection I'm using is an Android SparseArray, and it 
does not define equals. That means in order to compare two objects that both 
wrap SparseArrays, I have to find out myself whether their contents match or 
not.

That said, this might actually fall into the 1% bucket...

Original comment by [email protected] on 10 Jun 2014 at 3:19

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Hi!

Thank you for the update.

Actually, I was not aware of the SparseArray, let alone that it doesn't 
implement equals. I find it strange that Google wouldn't implement equals, but 
given that they haven't, I see you don't have a lot of choice. Actually, with 
this example you have bumped it out of the 1% bucket and into the ± 5% one for 
me :). I'll see what I can do. But it still might take a little while.

In the mean time, I can see that you would want to use the actual types, and 
not do something like Object el = list.get(0); in your production code. As an 
alternative, you could of course use withPrefabValues:

EqualsVerifier.forClass(...)
    .withPrefabValues(SparseArray.class, someSparseArray1, someSparseArray2)
    .verify();

Original comment by [email protected] on 10 Jun 2014 at 5:49

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

thanks -- I'll give that a try!

Original comment by [email protected] on 19 Jun 2014 at 7:39

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Issue 99 has been merged into this issue.

Original comment by [email protected] on 28 Nov 2014 at 9:50

• Added labels: ****
• Removed labels: ****

[jqno]

This has been included in version 2.0!