Spotbugs: Adhering to EQ_DOESNT_OVERRIDE_EQUALS requires violation of equals method contract

[jjlharrison]

This issue was reported on the FindBugs SourceForge issue tracker but was closed without any discussion (see #1379). I'm reposting the issue here as this has recently surfaced issues within my organisation.

The bug EQ_DOESNT_OVERRIDE_EQUALS is defined as:

This class extends a class that defines an equals method and adds fields, but doesn't define an equals method itself. Thus, equality on instances of this class will ignore the identity of the subclass and the added fields. Be sure this is what is intended, and that you don't need to override the equals method. Even if you don't need to override the equals method, consider overriding it anyway to document the fact that the equals method for the subclass just return the result of invoking super.equals(o).
In other words, if class A defines equals, and you declare B extends A, if B adds any fields, it should override equals.

As explained in Effective Java's item 8, doing so is harmful, since you are breaking the invariant of symmetry.
Quoting the official documentation:

The equals method implements an equivalence relation on non-null object references:
It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns true.

To be more precise, let's see this example (taken from Effective Java, 2nd edition itself):

public class Point {
  private final int x;
  private final int y;
  public Point(int x, int y) {
    this.x = x;
    this.y = y;
  }
  @Override public boolean equals(Object o) {
    if (!(o instanceof Point))
      return false;
    Point p = (Point)o;
    return p.x == x && p.y == y;
  }
  ...
  // Remainder omitted
}

This is completely legal, valid, and good looking code.
Now, let's assume we extend it to add a color:

public class ColorPoint extends Point {
  private final Color color;
  public ColorPoint(int x, int y, Color color) {
    super(x, y);
    this.color = color;
  }
  ...
  // Remainder omitted
}

This class would get reported for violating EQ_DOESNT_OVERRIDE_EQUALS
Yet doing:

  // Broken - violates symmetry!
  @Override public boolean equals(Object o) {
    if (!(o instanceof ColorPoint))
      return false;
    return super.equals(o) && ((ColorPoint) o).color == color;
  }

breaks symmetry, since:

new Point(2, 3).equals(new ColorPoint(2, 3, Color.BLACK)); // true!
new ColorPoint(2, 3, Color.BLACK).equals(new Point(2, 3)); // false!

[rudikershaw]

Seconded. I can't see any reasonable justification for this rule. If anything, this is encouraging the adding of bugs rather than their removal.

x.equals(y) == y.equals(x) should always be true for any two non-null x and y where x extends y or vice versa.

[iloveeclipse]

One of your basic assumptions is wrong. You assume that all subclasses should be "comparable", which is may or may not be true. If you believe that all your children should be comparable, you can leave it. If not, you have a bug.

The example with "points" sounds right, but make same example with animals. Parent "Animal" class with the field "color", child "Human" and "Fish" with additional fields. Would you consider Human.equals(Fish) == true if they have same color? I guess not. This is what the bug warning is about.

So this is not a bug in FindBugs, closing as invalid.

[rudikershaw]

@iloveeclipse, the animal example would be fixed by including an this.getClass().equals(object.getClass()) in the super-type equals and has nothing to do with the fields of the class.

In the example, if the Human adds field comparisons in an overridden equals method, animal.equals(human) could be true but human.equals(animal) would be false which is asking for trouble.

The ideal would be for Animal to implement a class comparison check so that both would be false always. That would not require an equals override and still gives the benefits you describe without violating the equals contract.

The real bug only occurs if you decide that sub-types should not be comparable to their parents, and then you implement this in the sub-type using an equals override. If a decision is made that sub-types should not be comparable, this should be implemented in the super-type equals method.

The EQ_DOESNT_OVERRIDE_EQUALS rule does not solve the problem you describe, it just advises that you break the equals contract as well. If this is what the bug is really about, then I would suggest that the description is wrong.

[krallus]

I think the concern raised by the issue is valid. Imagine Animal is a concrete class where two instances of Animal are equal if they have the same DNA. Human extends Animal to have a first name and last name. If you instantiate a Human and Animal to have the same DNA, then they should be equal regardless of whether it is animal.equal(human) or human.equal(animal). However, although I think the concern is valid, I also think it is misplaced.

I would say that EQ_DOESNT_OVERRIDE_EQUALS raises a completely valid concern because it's saying "Hey! You've got some new properties that you're not comparing for equality here! Is that what you really want? Maybe you should be explicit.". Rather, the problem that should be detected in the example given by the OP is inside the implementation of the overridden Equals method:

if (!(o instanceof ColorPoint))
      return false;

Specifically, I respectfully suggest that a new detector in SpotBugs be created such that if Equals is overridden in a class that extends a superclass that itself overrides Equals, the usual intanceof conditional should call Equals in the superclass. So, the ColorPoint override should look like:

@Override
public boolean equals(Object o) {
  if (!(o instanceof ColorPoint)) {
    return super.equals(o);
  }
  return super.equals(o) && ((ColorPoint) o).color == color;
}

This would give the superclass a chance to recognize o as an instance of itself and compare for equality ignoring the extended properties.

This isn't entirely perfect. For example if you had two different ColorPoints with the same coordinates but different colours, then colorPoint1.equals(colorPoint2) would be false but ((Point) colorPoint1).equals(colorPoint2) would be true. I expect most people would find that acceptable though, and maybe even expected.

[Maaartinus]

@iloveeclipse

So this is not a bug in FindBugs, closing as invalid.

At the very least, the error text should be fixed as it's misleading. It's all much more complicated and overriding equals is more probably wrong than not overriding it.

You may want to override equals sometimes, but there's no way to get it right (for a non-redundant implementation) without the superclass knowing about it. Simply fixing "equality on instances of this class will ignore the identity of the subclass and the added fields" in the straightforward way is much worse than doing nothing as it breaks symmetry. If you ever want that added fields don't get ignored and that a superclass instance can equal to a subclass instance, then you need some canEqual.

FWIW, my Entity class defines equals based solely on the id and the actual class (modulo Hibernate proxies) and the method is intentionally final. I'm aware about the gotchas, but there's no perfect solution (because of mutability) and this one is IMHO the most sane.

[rudikershaw]

I think there could a useful clarification, and EQ_DOESNT_OVERRIDE_EQUALS could be flagged in a more limited set of circumstances.

Situation 1

Imagine a parent class Animal with the following in it's equals method;

if (!this.getClass().equals(object.getClass())) return false;

In this instance a sub-type of Animal cannot be considered equal to an Animal subclass instance. If this is the architects intention, that is all well and good. In this situation the EQ_DOESNT_OVERRIDE_EQUALS makes perfect sense, because adding additional criteria/fields to the sub-classes equals method will not break equals symmetry (which is part of the equals contract). Because human.equals(animal) can never be true and vice versa symmetry remains intact between classes in their object hierarchy.

Situation 2

Imagine a parent class Animal with an equals implementation that just checks instanceof and checks against some primary key or ID. In this instance a sub-type of Animal can be considered equal to an Animal subclass instance. If this is the architects intention, that is still all well and good. In this situation EQ_DOESNT_OVERRIDE_EQUALS makes no sense. Because in this situation, implementing the advice in the EQ_DOESNT_OVERRIDE_EQUALS always leads to breaking the equals contract.

I would never expect breaking the equals contract to be considered acceptable.

[iloveeclipse]

OK, reopening: feel free to provide a better warning message.

[jjlharrison]

Would it be possible to handle the 2 situations described by @rudikershaw separately? i.e, if the super class checks for class equality, show a warning as it is now, but if the super class does an instanceof or a Class.isAssignableFrom check it triggers a new warning that mentions equals symmetry?

The main thing I would like to avoid is people seeing the current warning and blindly following the advice and breaking symmetry.

[ThrawnCA]

EQ_DOESNT_OVERRIDE_EQUALS is problematic in another situation: reflective implementations like Apache Commons EqualsBuilder.reflectionEquals. An equals based on this method can, in many cases, be safely inherited.