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 complains about reflexivity, then complains about unnecessary suppression if reflexivity is suppressed #188

Closed
142Old2 opened this issue Mar 1, 2018 · 1 comment

Comments

@142Old2
Copy link

142Old2 commented Mar 1, 2018

What steps will reproduce the problem?

A class with a BigDecimal field fails reflexivity (known issue due to BigDecimal using compareTo() for its natural ordering and equality), but when the reflexivity warning is suppressed verifier complains about unnecessary suppression.

What is the code that triggers this problem?

`public class Ex {
private String field1;
private String field2;
private BigDecimal number;

[... etc ...]
}`

Relevant equals method code:
this.getNumber() != null && that.getNumber() != null && this.getNumber().compareTo(that.getNumber()) == 0;

And hashCode:
return Objects.hash(getField1(), getField2(), getNumber() != null ? ((Double)getNumber().doubleValue()).hashCode() : getNumber());

What error message or stack trace does EqualsVerifier give?

Without suppression, EqualsVerifier gives the reflexivity error. After suppression, it warns about unnecessary suppression.

What did you expect?

I expect that there should not be a Catch-22 where I must suppress the warning to pass a test but cannot suppress the warning without generating another warning.

Which version of EqualsVerifier are you using?

2.4.1

Please provide any additional information below.

@jqno
Copy link
Owner

jqno commented Mar 2, 2018

Hi,

EqualsVerifier's warning is actually legitimate.

It complains about reflexivity when the BigDecimal value is null. It compares two Ex instances where number are null, and which returns false in the expression you gave.

When you suppress the warning, it fails when both BigDecimals have the same value (EqualsVerifier uses BigDecimal.ZERO), which is also legitimate.

The problem lies in the comparison you give: this.getNumber() != null && that.getNumber() != null && this.getNumber().compareTo(that.getNumber()) == 0;, which should have been something like this.getNumber() == null ? that.getNumber() == null ? this.getNumber().compareTo(that.getNumber());, which will also fail because x.compareTo(null) throws an exception. So the final form would be something a bit more awkward, like (this.getNumber() == null && that.getNumber() == null) || (this.getNumber() != null && that.getNumber() != null && this.getNumber().compareTo(that.getNumber()));

You're right that BigDecimal's equals method can fail reflexivity (for instance when comparing new BigDecimal("1") and new BigDecimal("1.00")), but using compareTo takes away that reflexivity issue. Either way, it's irrelevant for EqualsVerifier, because it only uses BigDecimal.ZERO and BigDecimal.ONE. When it compares two instances of your Ex class to see if they're equal, it makes sure that the number field is set to BigDecimal.ZERO in both cases. Since BigDecimal.ZERO.equals(BigDecimal.ZERO) (and even BigDecimal.ZERO == BigDecimal.ZERO), the reflexivity issue doesn't occur here.

@jqno jqno closed this as completed Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants