Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EqualsVerifier gives false positive common error (using == instead of .equals()) #104

Closed
GoogleCodeExporter opened this issue Mar 29, 2015 · 10 comments

Comments

@GoogleCodeExporter
Copy link

What steps will reproduce the problem?
1.  I used == instead of .equals when comparing two fields.
2.
3.


What is the code (equals method, hashCode method, relevant fields) that
triggers the problem?
I've attached a full example.

What error message does EqualsVerifier give?
None.

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


What did you expect?
Unit test failure, as shown in the "by hand" test.


What version of EqualsVerifier are you using?
1.6


Please provide any additional information below.


Original issue reported on code.google.com by [email protected] on 8 Feb 2015 at 3:57

Attachments:

@GoogleCodeExporter
Copy link
Author

Nice catch!

Strictly speaking, your equals method is not incorrect. I mean, it doesn't 
violate the equals contract in anyway. But I agree it's not what you want in 
99,9% of cases, either, so I'll try to find a way to fix this without making it 
impossible to use == if you need to.


Jan

Original comment by [email protected] on 8 Feb 2015 at 7:48

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

@GoogleCodeExporter
Copy link
Author

I can see your point, that it doesn't violate equals.  To me == should only be 
for primitives, or enums.  Any other use is extremely unlikely to be the case 
wanted (but it is a relatively common mistake, I think). If you feel that you 
need to allow "==" for an Object, make it settable strategy, and NOT the 
default.

Original comment by [email protected] on 8 Feb 2015 at 11:34

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

@GoogleCodeExporter
Copy link
Author

One approach would be to allow == only for types that do not override #equals 
anywhere in their type hierarchy. Even that's not fool proof, though, cause 
it'd result in false positives when comparing interned objects.

Original comment by [email protected] on 9 Feb 2015 at 5:48

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

@GoogleCodeExporter
Copy link
Author

I agree that == for objects should not be the default, because, as you say, 
it's very unlikely. I was thinking more along the lines of adding a Warning 
that can be suppressed, but your idea is certainly interesting, too. I'll have 
to think about this some more.

Original comment by [email protected] on 9 Feb 2015 at 7:54

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

@GoogleCodeExporter
Copy link
Author

Fixed in version 1.7, which is just released.

Original comment by [email protected] on 4 Mar 2015 at 6:05

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

@GoogleCodeExporter
Copy link
Author

Unfortunately a false positive is elicited when jdk Object.equals() method is 
employed.  The following equals method will get this warning:

    @Override
    public boolean equals(Object obj) {
        if (this == obj) {
            return true;
        }
        if (obj == null || getClass() != obj.getClass()) {
            return false;
        }
        final MyClass other = (MyClass) obj;
        return Objects.equals(this.myField, other.myField);
    }

java.lang.AssertionError: Reflexivity: == used instead of .equals() on field: 
myField

Objects.equals is implemented as 'return (a == b) || (a != null && 
a.equals(b));' which I think is OK since it is a reference check followed by a 
deep equals check if that fails.  Understandable why the check fails, but 
unfortunate nonetheless.  Perhaps the check could be tightened to == used in 
absence of .equals()?

Original comment by [email protected] on 6 Mar 2015 at 1:34

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

@GoogleCodeExporter
Copy link
Author

Hi,

Thanks for reporting this. However, I'm unable to reproduce the issue. When I 
add this equals method to a class with a String myField member, my test goes 
green.

Can you please give me a full class to test, instead of just an equals method? 
Also, the call to EqualsVerifier itself would be very helpful.

Original comment by [email protected] on 6 Mar 2015 at 8:54

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

@GoogleCodeExporter
Copy link
Author

Here is a complete working example.  Note that I'm building/running this with 
jdk8 (u31 I think).  You should be able to load this directly into IntelliJ or 
Eclipse.  There are only three dependencies and they are all included.  Hope 
this helps.  Thanks.

Original comment by [email protected] on 6 Mar 2015 at 7:32

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

Attachments:

@GoogleCodeExporter
Copy link
Author

OK, thanks, I was able to reproduce it now. It had nothing to do with 
Objects.equals; the cause was that your field has type Attribute, and Attribute 
is an interface which doesn't define equals. A regular class with no equals 
method would have failed in the same way. I don't know how my tests didn't 
catch this when I implemented this feature; thanks for catching it!
I've committed a fix, and I'll release a new version as soon as I've fixed 
Issue 105 as well.

Original comment by [email protected] on 9 Mar 2015 at 8:58

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

@GoogleCodeExporter
Copy link
Author

I've just released a fix in version 1.7.1.

Original comment by [email protected] on 11 Mar 2015 at 3:13

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant