dev@glassfish.java.net

Re: non-symmetric equals method

From: Stephen Connolly <stephen.alan.connolly_at_gmail.com>
Date: Thu, 22 Apr 2010 06:49:48 +0100

On 21 April 2010 18:02, Bill Shannon <bill.shannon_at_oracle.com> wrote:

> I'm looking into bugs found by the latest version of FindBugs.
> An error that has occurred in a few places is a non-symmetric equals
> method. FindBugs describes the error like this:
>
> This class defines an equals method that overrides an equals
> method in a superclass. Both equals methods methods use
> instanceof in the determination of whether two objects are equal.
> This is fraught with peril, since it is important that the equals
> method is symmetrical (in other words, a.equals(b) ==
> b.equals(a)). If B is a subtype of A, and A's equals method
> checks that the argument is an instanceof A, and B's equals
> method checks that the argument is an instanceof B, it is quite
> likely that the equivalence relation defined by these methods is
> not symmetric.
>
>
> There's quite a few descriptions of the problem on the web, along with some
> proposed solutions:
>
> http://www.drdobbs.com/java/184405053
> http://tal.forum2.org/equals
> http://www.artima.com/lejava/articles/equality.html
>
> http://stackoverflow.com/questions/27581/overriding-equals-and-hashcode-in-java
>
>
> Have others run into this same problem? How have you solved it?
>
>
> I hear that the second edition of Josh's Effective Java book has a
> solution. I don't have a copy, can someone who does look this up
> for me?
>
>
> The approach that looks the best to me so far is this described by
> Gregor Zeitlinger in
> http://www.artima.com/forums/flat.jsp?forum=226&thread=259279
>
>
> There's a version that doesn't require canEqual and is even more flexible:
>
> Point.equals(Object o) {
> if (o instanceof Point) {
> if (!o.getClass.isAssignableFrom(getClass())) {
> //let the subclass decide if we are equal
> return o.equals(this);
> }
> Point p = (Point)o;
> return x == p.x && y == p.y;
> }
> return false;
> }
>
>
> now ColoredPoint could be defined in either way:
>
> ColoredPoint.equals(Object o) {
> if (o instanceof ColoredPoint) {
> if (!o.getClass.isAssignableFrom(getClass())) {
> //let the subclass decide if we are equal
> return o.equals(this);
> }
> ColoredPoint p = (ColoredPoint)o;
> return color == p.color && super.equals(o);
>
possible infinite recursion afaik

> }
> return false;
> }
>
> or
>
> ColoredPoint.equals(Object o) {
> if (o instanceof ColoredPoint) {
> if (!o.getClass.isAssignableFrom(getClass())) {
> //let the subclass decide if we are equal
> return o.equals(this);
> }
> ColoredPoint p = (ColoredPoint)o;
> return color == p.color && super.equals(o);
>
possible infinite recursion afaik

> } else if (o instanceof Point) {
> return color == Color.RED && super.equals(o);
>
possible infinite recursion afaik

> }
> return false;
> }
>
>
>
> What do others think?
>
>
I suspect a couple of paths could lead to an infinite recursion...

// ScentedPoint extends Point but not ColoredPoint

ScentedPoint.equals(Object o) {
 if (o instanceof ScentedPoint) {
   if (!o.getClass.isAssignableFrom(getClass())) {
      //let the subclass decide if we are equal
      return o.equals(this);
    }
     ScentedPoint p = (ScentedPoint)o;
    return color == p.color && super.equals(o);
 } else if (o instanceof Point) {
   return scent == Scent.NEUTRAL && super.equals(o);
 }
 return false;
}


ScentedPoint a;
ColoredPoint b;

a.equals(b); // could this keep looping?


> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>
>