Che: Widespread use of bogus equals() Implementation

[tsmaeder]

In many places, we override equals() in a way that is simply wrong. Here's a snippet from ResourceNode.java:

public boolean equals(Object o) {
    if (this == o) return true;
    if (!(o instanceof ResourceNode)) return false; >>>>>> Problem here!
    ResourceNode<?> that = (ResourceNode<?>) o;
    return Objects.equal(resource, that.resource);
  }

This code is not a correct because it does not correctly implement the contract for equals(), in particular it is not symmetric. For example, ContainerNode extends ResourceNode, so you can have cases where resNode.equals(folderNode), but not folderNode.equals(resNode).

This leads to all kinds of unexpected results, for example folder nodes not being correctly replaced with package nodes, in the project navigator because they are considered equal.

Depending on how collections are implemented, bogus equals implementations can lead to cases where we can have duplicate entries in sets or entries in maps not being found after being added with the same key.

[metlos]

Are you sure that is the problem? The instanceof check will pass for any instance of any class inheriting from ResourceNode.

If the check wasn't there, you could not do basically any kind of comparison, because you could not be sure of the type of the object being compared to.

[dkuleshov]

I don't see the problem as well. Can you please provide a unit test? This also might be helpfull.

[tsmaeder]

Are you sure that is the problem? The instanceof check will pass for any instance of any class inheriting from ResourceNode.

The check needs to be if (getClass() == other.getClass()), not instanceof. Otherwise, you will have
subclass.equals(superclass)

but at the same time

!superclass.equals(subclass)

This violates the symmetry requirement of the equals API doc

[tsmaeder]

... This also might be helpfull.

Thanks for the reference, it proves my point.

[dkuleshov]

I assume you mean this case?

class B {}
class A extends B {}

A a = new A();
B b = new B();
...
(a.equals(b)!=(b.equals(a));

[tsmaeder]

yes

[metlos]

Ok, so let's try to dissect this a bit, because I think we're starting to talk about at least 2 different things here.

Let's expand on @dkuleshov 's example a little to conform more to the situation of ResourceNode and ContainerNode:

class ResourceNode {
  boolean equals(Object o) {
    if (this == o) return true; <1>
    if (!(o instanceof ResourceNode)) return false; <2>
    ... <3>
  }
}

class ContainerNode extends ResourceNode {
}

Now, let's have:

ResourceNode r = new ResourceNode();
ContainerNode c = new ContainerNode();

Now, let's "debug" r.equals(c):

• (this == o) == false, so the method continues
• o instanceof ResourceNode == true, so the method continues
• returns whatever <3> prescribes

Ok, now let's do the same with c.equals(r):

• (this == o) == false, so the method continues
• o instanceof ResourceNode == true, so the method continues
• returns whatever <3> prescribes

Notice that in both cases, we got to the same point in execution, so the instanceof check cannot be the source of asymmetric behavior.

In Java, it is generally admissible to inherit equals() and hashCode() from superclasses, because it is assumed that subclasses can exhibit same behavior under the equality and that it is admissible for instances of 2 different types to be declared equal.

Now if we observe that r.equals(c) != c.equals(r), we can only conclude that either the bug is in <3> or that ContainerNode re-implemented equals(). That is not the case for ResourceNode and ContainerNode as far as I looked though.

In the current case, 2 different subtypes of ResourceNode can theoretically be equal, while with your proposed change, they can never be. I'd say though that the logic in ResourceNode is still correct, because after it did the rudimentary checks for type compatibility it actually delegates the equals (and hashCode) to the resource field.

So in another words, ResourceNode is just a wrapper of the actual resource, and it is the resource that is ultimately responsible for any and all comparisons.

If we went with your proposal, we'd violate that logic and would basically tightly couple a resource type to a node type (which might or might not be advisable, that's beyond my knowledge of the codebase).

[l0rd]

@tsmaeder it would be interesting if you could provide a failing unit test. It would be particularly useful to identify other classes affected by the same problem.

[dkuleshov]

@metlos you are absolutely correct, the idea that you described was my first thought as well. However after some meditation I came to a conclusion that the reporter has something different in his mind.

The problem and confusion probably is that the example that is used is not really the most appropriate one. As I see, ContainerNode and all other classes that extends ResourceNode (most likely intentionally) do not override equals method, so the check is always o instanceof ResourceNode thus c.equals(r) == r.equals(c), equals is symmetric - expected behavior, no problem here!

However my guess is that the idea of the issue is probably to point out that in cases where equals method is overridden for each class in inheritance hierarchy we may face the erroneous use of instanceof operator. The next unit test should probably clarify the idea:

import org.junit.jupiter.api.Test;

import java.util.Objects;

import static org.junit.jupiter.api.Assertions.assertEquals;

class EqualsTest {
    @Test
    void equalsShouldSymmetricForInheritance() {
        X x = new X(1);
        Y y = new Y(1);

        assertEquals(x.equals(y), y.equals(x)); // will fail 
    }
}

class X {
    int a;

    X(int a) {
        this.a = a;
    }

    public boolean equals(Object o) {
        if (this == o) {
            return true;
        }
        if (!(o instanceof X)) {
            return false;
        }

        X that = (X) o;
        return Objects.equals(a, that.a);
    }
}

class Y extends X {
    Y(int a) {
        super(a);
    }

    public boolean equals(Object o) {
        if (this == o) {
            return true;
        }
        if (!(o instanceof Y)){
            return false;
        }

        Y that = (Y) o;
        return Objects.equals(a, that.a);
    }
}

[tsmaeder]

Actually, symmetry is given with the implementation in question, so I am wrong on that one :man_facepalming:, but the case that remains is that folder.equals(package), which IMO only makes sense in very particular cases. In the case of folder vs. package node, it's clearly not what the artist intended.

[tsmaeder]

@metlos you're right. It's still bogus, though.

[gorkem]

I am closing this one as I am not sure if there is an action to take here. @tsmaeder reopen if you feel we still need to act on it