not taking all null paths

[xenoterracide]

package com.xenoterracide.entity;

import org.springframework.data.domain.Persistable;

import java.io.Serializable;
import java.util.Objects;

public interface Identified<ID extends Serializable> extends Persistable<ID> {
    Object[] EMPTY_ARRAY = new Object[ 0 ];

    default boolean isEqualTo( final Object o ) {
        if ( this == o ) {
            return true;
        }
        if ( null != o && o instanceof Identified ) {
            Identified that = (Identified) o;

            if ( !this.isNew() ) {
                if ( this.getId().equals( that.getId() ) ) {
                    return true;
                }
            }
            else if ( that.isNew() ) { 
                return this.newCanEqual( that ); // never gets called
            }
        }

        return false;
    }

    default boolean isNew() {
        return this.getId() == null;
    }

    default boolean newCanEqual( final Identified o ) {
        return true;
    }

    default int hash() {
        return Objects.hash( this.getId(), hashFields() );
    }

    default Object[] hashFields() {
        return EMPTY_ARRAY;
    }
}

note: this time I am calling verify, because I have caused failures.

   @Test
    public void testAEquals() {
        EqualsVerifier.forClass( TestA.class ).suppress( Warning.NONFINAL_FIELDS ).verify();
        EqualsVerifier.forClass( TestB.class ).suppress( Warning.NONFINAL_FIELDS ).verify();

    }

    private static class TestA implements Identified<Long> {
        private Long id;

        @Override
        public Long getId() {
            return this.id;
        }

        @Override
        public final int hashCode() {
            return this.hash();
        }

        @Override
        public final boolean equals( final Object obj ) {
            return this.isEqualTo( obj );
        }
    }

    private static class TestB extends TestA {
        private String name;

        @Override
        public boolean newCanEqual( final Identified o ) {
            throw new RuntimeException( "reached" ); // won't throw
        }
    }

and not sure why they both need non-final fields in this example but in a non example where the "TestA" is in a different jar from "TestB". Observed trying to use newCanEqual to verify subclass equality on name.

[jqno]

The reason why the exception is not reached, is that EqualsVerifier does not check the entire state space (see also the FAQ). Depending on whether id is null, your equals method behaves very differently. Of course EqualsVerifier does some basic checks for null, but it doesn't check everything with regards to null, just like it doesn't check everything with regards to 1, 1337 or 123456789. There are a lot of possible long values.

That said, null is a special enough value that it might warrant a few extra checks. I'll look into that. I think at least the EqualsVerifier check for TestB should have failed.

Also, if you're trying to do the "canEqual" thing as described in the article bij Odersky, Venners and Spoon, you should notice that it should be "that.newCanEqual(this)", and not "this.newCanEqual(that)", though it doesn't lead to different results in the EqualsVerifier check.

I didn't quite understand your final paragraph about non-final and different jars.

[xenoterracide]

so I have a git repository for my AbstractEntityBase which is implementing Identified. Different Repositories use it. In another repository I don't have to suppress Warning.NONFINAL_FIELDS (the test on test be can just be EqualsVerifier.forClass( TestB.class ).verify();. I trimmed this example down because of code quantity, but I have no idea why it would think suddenly that id was final`, just because it was in another jar, or something... I suspect if we moved TestA and TestB into different jars/git/maven and installed TestA, and ran EqualsVerifier against TestB, we'd see the same behavior, not that it makes sense. It might have nothing to do with different jars, but that's just a theory.

That said, null is a special enough value that it might warrant a few extra checks. I'll look into that. I think at least the EqualsVerifier check for TestB should have failed.

yeah, I agree, I wasn't really expecting more.

[jqno]

Hi,

I would be very surprised if that were true. Bytecode doesn't just change if it's in another jar, and EqualsVerifier certainly doesn't check what jar a class came from. Maybe there's a stale jar in a cache somewhere though?

In principle, Warning.NONFINAL_FIELDS is always needed if fields aren't final. The only exception is, when the field isn't actually referenced in the equals method. That applies to TestB's name field. So perhaps TestB's superclass doesn't get loaded correctly?

[xenoterracide]

maybe? but that'd be just as weird, and I don't think that hibernate and such would work in that case. although you can see in Identified I don't reference it directly, I'm only referencing getId() . maybe it has something to do with accessing things through methods, and defaults, though then I'd think it show up here. I might spend time later trying to figure out a verifiable reduced example. Thought I'd mention it though since it's weird.

[jqno]

OK, I'm looking forward to that.

I've spent some time trying to understand what's happening in your equals method above, and I've managed to find a way to make EqualsVerifier fail on TestB. I'll include that in the next release. I'll look at finding a way to do the same for TestA, and if I do I'll include that too. I'll let you know when it's released.

I have to say though, this might be one of the more complicated equals methods I've seen so far, and I've seen a few. The cyclomatic complexity of that thing must be pretty high, it took me quite a while to figure out how it works.

BTW -- if you remove the else before if (that.isNew()), it does call newCanEqual.

[jqno]

I wasn't able to find a way to make the TestA test fail, because technically, it isn't wrong (as long as newCanEqual returns true). The only way to make it incorrect, is from a subclass such as TestB, that takes an extension point of the equals method and implements it incorrectly. (Which is why I recommend against suppressing STRICT_INHERITANCE.)

Hoewever, I just released EqualsVerifier 1.7.4, which contains the change I mentioned before that makes the TestB test fail.

[xenoterracide]

TestA wasn't really supposed to fail (in this example too). I think I've managed to reproduce the other "issue"

package com.xenoterracide.entity;

import nl.jqno.equalsverifier.EqualsVerifier;
import org.junit.Test;

public class IdentifiedTest {

    @Test
    public void testEquals() throws Exception {
        EqualsVerifier.forClass( Test1Identified.class ).verify();
        EqualsVerifier.forClass( Test2Identified.class ).verify();
    }

    private static class Test1Identified implements Identified<Long> {

        private final Long id = 1L;

        @Override
        public Long getId() {
            return this.id;
        }

        @Override
        public final int hashCode() {
            return this.hash();
        }

        @Override
        public final boolean equals( final Object o ) {
            return this.isEqualTo( o );
        }
    }

    private static class Test2Identified extends Test1Identified {
        private String name = "bar";

        @Override
        public boolean newCanEqual( final Identified o ) {
            if ( o instanceof Test2Identified ) {
                Test2Identified that = (Test2Identified) o;
                return !( this.name != null && that.name != null ) || this.name.equals( that.name );
            }
            return false;
        }
    }
}

I updated Identified too

package com.xenoterracide.entity;

import org.springframework.data.domain.Persistable;

import java.io.Serializable;
import java.util.Objects;

public interface Identified<ID extends Serializable> extends Persistable<ID> {

    default boolean isEqualTo( final Object o ) {
        if ( this == o ) {
            return true;
        }
        if ( null != o && o instanceof Identified ) {
            Identified that = (Identified) o;

            if ( !this.isNew() ) {
                if ( this.getId().equals( that.getId() ) ) {
                    return true;
                }
            }
            else if ( that.isNew() ) { // this.id must already be null
                return that.newCanEqual( this );
            }
        }

        return false;
    }

    default boolean isNew() {
        return this.getId() == null;
    }

    default boolean newCanEqual( final Identified o ) {
        return true;
    }

    default int hash() {
        return Objects.hash( this.getId() );
    }
}

[jqno]

Hi,

Well, I still would've liked it if it could have flagged TestA, because while technically correct, it's not a good equals method.

I can't reproduce any issue with the code you just posted. I've tried it both with 1.7.3 and 1.7.4, and the test passes just fine. It would help a lot if you could create a zip with a complete maven project (or another build tool) that reproduces the issue.

[xenoterracide]

well in theory the problem is is that it passes, I would think it should fail with "non final fields" and not including fields in hashcode. I did notice after sharing this that I'm still not presenting the full problem which is it's not catching that id is not marked final in my code (it is here). I can tar up the 4 repos of my code? though, not sure where you'd like me to put said archive? or just give you access on bitbucket...

Though the hashcode thing is fun, as soon as I include name in hashcode it'll fail with superclass not matching, which is probably correct, but not what that article you suggest demonstrates...

so why is it bad this time?

[jqno]

Hi,

Thanks for the update. I misunderstood, and didn't realise that you expected it to fail when it didn't, instead of the other way around. I've spent some time looking at it, and I finally understand what's going on.

You ask why EV succeeds when the fields are non-final, without suppressing Warning.NONFINAL_FIELDS. First of all, it does fail if id is non-final, but you probably already knew this; I'm just saying it for the sake of completeness.

Why it doesn't fail on name is more strange. It turns out that this is because EqualsVerifier doesn't detect that name is actually used in equals. You will notice, if you call allFieldsShouldBeUsed() on Test2Identified, that EqualsVerifier complains that name isn't used. EqualsVerifier passes with a non-final name because it thinks name is irrelevant to equals: the NONFINAL_FIELDS warning only applies to fields that are involved in the equality relation. This is also why this class only passes EqualsVerifier when it doesn't override hashCode in Test2Identified: if it was there, EqualsVerifier would notice that name was used in hashCode but not in equals, as you've already seen.

This obviously begs the question: why does EqualsVerifier think that name is irrelevant? There are two parts to this. First, a change to the value of name only influences the outcome of equals in very specific circumstances: id has to be null on both objects, and name has to be different but non-null in both objects. This is a situation that isn't tested by EqualsVerifier because of the reasons I state in the FAQ: it simply cannot test the entire state space, and the situation where some fields are null and other fields aren't null but they are different, is simply not tested. Null is a common edge case though, so you might ask: shouldn't it test this case after all? Maybe. However, this brings us to the second part of the answer, which is actually a bug in your code. The expression !(this.name != null && that.name != null) returns true when at least one of the names is null. In other words, if this.name == null and that.name == "Caleb", the test passes, which is probably not what you wanted. You should use Objects.equals() instead, and if you do, you'll notice that the EqualsVerifier test does fail as expected.

Once you fix this, you'll start running into issues relating to inheritance and subclasses, which is where my main issue with this implementation of equals lies. You're attempting several things here at once: first, Identified is what I call a versioned entity: it compares objects based on their id, and only looks at the fields when id has a specific value (null in this case). Second, it overrides its equality in subclasses that add state using the canEqual pattern. Finally, I suspect it attempts to have a generic (or at least as-generic-as-possible) equals method in the Identified interface, so that ideally, equals doesn't have to be overridden in the implementing classes. (Otherwise, why bother trying to handle it in the interface instead of in Test1Identified? Probably because you have implementations of Identified parallel to Test1Identified, and you don't want to repeat equals logic there.)

Each of these are hard things that are difficult to get right on their own, let alone when you mix them together. These behaviours interact in subtle ways, which in this case result in a seriously flawed implementation of equals that, nevertheless, was accepted by EqualsVerifier. EqualsVerifier is very strict so in a way, I was actually quite impressed by that ;).

So, why do I think that this implementation is so flawed? First of all, I'm not a big fan of the 'versioned entity' thing. It's trying to merge two very different equivalence relations into one, which may better be kept separate, for instance using a Guava Equivalence. But that's just my personal opinion: feel free to disagree.

The main issue, however, is that the way newCanEqual is implemented, is incorrect, and it seems to be going down a path that will never work. canEqual (or newCanEqual, as you call it) shouldn't be inspecting fields; it should only do an instanceof check on its parameter. This is because it's part of a carefully choreographed little dance it does with its corresponding equals method, where they do instanceof checks on each other to determine whether the types of their respective parameters are compatible with each other. Only if these types match, does equals proceed to actually inspect the fields of the object. In this implementation, however, half of the dance is missing, because equals is made final in the interface, and is never overridden to update the instanceof check. Every class that adds state to the equality relation must do its own check, otherwise you will lose symmetry, transitivity, Liskov, or some combination of these three. All this is described in the article by Odersky, Venners & Spoon that I mentioned earlier. They explain it more clearly than I could.

Also, there's the unintended consequence of the Identified interface, that it (necessarily) renames the equals and hashCode methods. EqualsVerifier does some additional checks when these methods are present, which it obviously cannot do if classes implement differently named isEqualTo and hash methods.

So, my advice would be to re-think what you want out of your equals method. Do you really need the interface on top? Does it really make it easier to extend your system correctly? Because in my opinion, it has actually made things more complicated than they need to be. Also: do you really need to add state in subclasses, or is an equality relation based on id sufficient? Because that would be trivially easy to implement, and you can still have a state-based equality using Guava Equivalence (although you probably still would have to do a canEqual-like dance in that case).

Also, while I'm at it, I can't help but notice the redundant null check in isEqualTo (since instanceof also checks for null); and also to ask: why not simply return the result of this.getId().equals(that.getId()) directly, instead of either returning true if it's true or falling through to the end of the isEqualTo method and returning false if it's false? Having less conditional logic would make this code a lot easier to follow for other people, such as myself. (I actually had to make a truth table to figure out this equals method.)

Having said all that, I'll try to come up with a few checks to add to EqualsVerifier to reduce the likelyhood that classes like this one fall through the cracks. I already have a couple of ideas for that, which I will probably add in the next version.

I hope this helps.

Cheers,
Jan

[xenoterracide]

Also, while I'm at it, I can't help but notice the redundant null check in isEqualTo (since instanceof also checks for null); and also to ask: why not simply return the result of this.getId().equals(that.getId()) directly, instead of either returning true if it's true or falling through to the end of the isEqualTo method and returning false if it's false? Having less conditional logic would make this code a lot easier to follow for other people, such as myself. (I actually had to make a truth table to figure out this equals method.)

obviously because I'm dumb... and I've done something I tell people not to do, "don't over complicate it". The first is definitely some artifact of a generated equals that might be a little over-complicated that I merged into the method signature. The second I suspect is an artifact of some other check I had, where if id is equivalent return true, else do something else (not wanting to return on false). I'll be fixing these for sure and then combing through all the things.

I think I should still send you this other archive, it may "pass" for some of the reasons you've outlined, but it encounters the non final field issue (where it passes) on id, which is not final in that example. I haven't been able to come up with a trivial example of this.

[jqno]

So, all this, and the problem you care most about still hasn't come up? ;)
Sure, send me a link to the project, and tell me which class(es) to look at, and I'll take a look :). It might be a few days before I get to that though, I don't have as much time this weekend.

[xenoterracide]

So, all this, and the problem you care most about still hasn't come up? ;)

well.. the problem I care about most is rather relative, it may be the same problem name suffers from, but it's hard to tell. Again, seems to pass where id is a non final field. Mostly I'm just trying to give feedback via my stupidity to improve the coverage that Equalsverifier can provide.

Sure, send me a link to the project, and tell me which class(es) to look at, and I'll take a look :). It might be a few days before I get to that though, I don't have as much time this weekend.
no worries, I'm not in a rush, I appreciate the responsiveness it's refreshing compared to a lot of projects.

Here's the tarball, yes I own the code I just haven't licensed it yet.

Sorry about the complexity I know it's not an ideal test case... I just haven't been able to find the ideal test case.

The test that should fail is in mmp-domain StationTest.java. You should be able to ignore platform and hibernate-hacks entirely, included for completeness. entity-api contains Identified, entity-jpa contains the AbstractEntityBase, and of course mmp-domain contains station. mvn test should just work as my private repository server is publicly available for CI.

You'll note that although id is a non final field in AbstractEntityBase, running Equalsverifier against Station which extends AbstractEntityBase, does not need me to suppress non final fields. Also name equals is a lot less dumb here since it shouldn't be null, though theoretically contains an NPE problem. I might change it to use Objects.equal. This might be because, as you noted, isEqualTo is not named the same as equals. I'm unsure, but asking you to look at it in case it's a bug.

As a final note, that "name" implementation, was only written up for that test, the Station one is the real one; though probably just as bad ;).

[jqno]

Your Station class is a JPA @entity. EqualsVerifier disables the mutability check for JPA Entities because Entities don't work if they're immutable. That's why it passes without the NONNULL_FIELDS warning.

All my other issues still stand, though: the canEqual pattern is applied incorrectly, and if you're serious about "don't overcomplicate it", you should seriously consider moving the equals logic from Identified to AbstractEntityBase. ;)

In the mean time, I'm going to keep trying to find a way to make EqualsVerifier flag this, because I consider it a bug that it doesn't.

[xenoterracide]

huh, I'd never considered that it was the annotations. Why doesn't it do that for the Abstract then? Shouldn't it also have detected @MappedSuperClass?

[jqno]

You're right. I'll fix that in the next release, as well.

[xenoterracide]

Prefix thought on my accidental bug report without reading your source (I don't know how easy this would be). in order to make EqualsVerifier accident proof... you'd have to change its API. EqualsVerifier.suppress( Warnings...).verify( My.class ); might work.

I am fixing the canEqual logic (and some of the other logic), I'm not so sure about moving it out of Identified though. Well, I guess I am considering moving it a different interface, and/or making canEqual not a default thus requiring it to be implemented.

I'm a bit surprised at the Scala article as it doesn't really talk about the "Traits" issue of using Composition over Inheritance, it is preferred for re-usability. Java of course has only recently gained this option, in fact it's one of the reasons I even considered migrating to Java. I wouldn't flag the method incorrect just because of usage of Composition. Honestly I only have an Abstract because JPA doesn't recognize Composition, otherwise I would annotate the interface to get the reusable bits. If everything needed to make the comparison is available in the Trait, why shouldn't it be used? Obviously Java could't make this change for legacy reasons.

Though of course, for me, it's worth asking (rhetorical) what the correct behavior of a Set of Identified is, if I use a Long instead of a UUID. That would currently result in the wrong behavior. To be fair I'm not sure that 2 AbstractEntityBase should be equals either... that's a side effect of desire for DRYness, as mentioned above that class wouldn't exist if I could help it, as it isn't actually a thing in the domain.

[xenoterracide]

partially to help me understand the canEquals and the differences between what you're saying about checking fields and what I read in that article, I implemented a Point example of equals whilst delegating to an interface. I included the articles final implementations of Point and ColoredPoint. You'll note that the articles does check fields in the sub class, but it also completely reimplements equals in the subclass, that doesn't seem necessary if you put the addional checks into a delegated method like canEquals (and has the added benefit of checking pointer/reference equivalence early instead of late in the call to equals).

no bugs in this example, I think.

package com.xenoterracide.entity;

import nl.jqno.equalsverifier.EqualsVerifier;
import org.junit.Test;

import java.awt.Color;
import java.util.Objects;

public class EqualsVerifierTest {

    @Test
    public void testCoordinatedPoints() {
        EqualsVerifier.forClass( PointImpl.class ).withRedefinedSubclass( ColoredPointImpl.class ).verify();
    }

    @Test
    public void testOriginalPoints() {
        EqualsVerifier.forClass( Point.class ).withRedefinedSubclass( ColoredPoint.class ).verify();
    }

    public interface Coordinates {
        int getX();
        int getY();

        default boolean isEqualTo( final Object o ) {
            if ( this == o) {
                return true;
            }
            if (o instanceof Coordinates) {
                Coordinates that = (Coordinates) o;
                return that.canEqual(this)
                        && this.getX() == that.getX()
                        && this.getY() == that.getY();
            }
            return false;
        }

        default boolean canEqual( final Coordinates o ) {
            return o.getClass().isAssignableFrom( this.getClass() );
        }
    }

    public class PointImpl implements Coordinates {

        private final int x;
        private final int y;

        public PointImpl( final int x, final int y ) {
            this.x = x;
            this.y = y;
        }

        public int getX() {
            return x;
        }

        public int getY() {
            return y;
        }

        @Override
        public boolean equals( final Object obj ) {
            return this.isEqualTo( obj );
        }

        @Override
        public int hashCode() {
            return Objects.hash( this.getX(), this.getY() );
        }

    }

    public class ColoredPointImpl extends PointImpl { // No longer violates symmetry requirement

        private final Color color;

        public ColoredPointImpl( final int x, final int y, final Color color ) {
            super(x, y);
            this.color = color;
        }

        @Override
        public boolean canEqual( final Coordinates o ) {
            if (o instanceof ColoredPointImpl ) {
                ColoredPointImpl that = (ColoredPointImpl) o;
                return this.color.equals( that.color );
            }
            return false;
        }

        @Override
        public int hashCode() {
            return super.hashCode() + this.color.hashCode();
        }
    }

    public class Point {

        private final int x;
        private final int y;

        public Point(int x, int y) {
            this.x = x;
            this.y = y;
        }

        public int getX() {
            return x;
        }

        public int getY() {
            return y;
        }

        @Override public boolean equals(Object other) {
            boolean result = false;
            if (other instanceof Point) {
                Point that = (Point) other;
                result = (that.canEqual(this) && this.getX() == that.getX() && this.getY() == that.getY());
            }
            return result;
        }

        @Override public int hashCode() {
            return (41 * (41 + getX()) + getY());
        }

        public boolean canEqual(Object other) {
            return (other instanceof Point);
        }
    }

    public class ColoredPoint extends Point { // No longer violates symmetry requirement

        private final Color color;

        public ColoredPoint(int x, int y, Color color) {
            super(x, y);
            this.color = color;
        }

        @Override public boolean equals(Object other) {
            boolean result = false;
            if (other instanceof ColoredPoint) {
                ColoredPoint that = (ColoredPoint) other;
                result = (that.canEqual(this) && this.color.equals(that.color) && super.equals(that));
            }
            return result;
        }

        @Override public int hashCode() {
            return (41 * super.hashCode() + color.hashCode());
        }

        @Override public boolean canEqual(Object other) {
            return (other instanceof ColoredPoint);
        }
    }

}

Thanks for the improvements, criticisms and great tool

[jqno]

Hi Caleb,

Your "prefix thought" is interesting indeed. I hadn't thought of solving it that way; I'll think about it!

Your points example is interesting, but I'll have to investigate further to be sure that it's actually correct. You've forgotten to test the ColoredPointImpl with EqualsVerifier, but that too seems to pass (after adding a few null checks and some prefab values). However, we've seen EqualsVerifier be wrong before! So I'm still not convinced. I'll get back to you on that.

I hope you'll agree that, if it's indeed needed to override both equals and canEqual like they do in the article, having the implementation also in an interface is redundant, and removing it would simplify things. Of course, all that I've said so far, was assuming that overriding both equals and canEqual is necessary.
(BTW, the way you're using the trait/interface here is still inheritance, not composition.)

Just as an aside, the reason why the Station class from your tarball is still incorrect, even though it passes the EqualsVerifier test, is that it's not symmetric. That can easily be shown with this test:

@Test
public void symmetry() {
    AbstractEntityBase a = new Station(new TestSeeder(null, ""));
    AbstractEntityBase b = new AbstractEntityBase() {};

    assertTrue(a.equals(b) == b.equals(a));
}

[jqno]

OK, I found a counter-example:

@Test
public void colorSymmetryImpl() {
    Coordinates c = new ColoredPointImpl(3, 3, Color.BLUE);
    Coordinates b = new BogusSubPointImpl(3, 3);

    assertTrue(c.equals(b) == b.equals(c));
}

public class BogusSubPointImpl extends PointImpl {
    public BogusSubPointImpl(int x, int y) {
        super(x, y);
    }

    @Override
    public boolean canEqual(Coordinates o) {
        return true;
    }
}

The test fails, because the implementation not symmetric. Is this a contrived example? Maybe, but I could have made the code look more like a "real" equals method and have it fail in the same way. What you're trying to do, until you get it 100% right, isn't much different! These problems aren't always obvious. However, the original implementation guards against this:

@Test
public void colorSymmetryOriginal() {
    Point c = new ColoredPoint(3, 3, Color.BLUE);
    Point b = new BogusSubPointOriginal(3, 3);

    assertTrue(c.equals(b) == b.equals(c));
}

public class BogusSubPointOriginal extends Point {
    public BogusSubPointOriginal(int x, int y) {
        super(x, y);
    }

    @Override
    public boolean canEqual(Object o) {
        return true;
    }
}

This test passes, because ColoredPoint does the proper instanceof checks in the proper order.

And that's the real lesson I've learned from creating EqualsVerifier: incorrectness often comes from outside. Your equals method might be 100% correct inside its little bubble, but three months later, a co-worker comes along who wants to extend Coordinates or PointImpl, and he makes a small, non-obvious mistake. Or worse: he tries to out-smart the system. His equals method also works correctly on its own, but he forgets to test it against the other implementations that are already there. Two months after that, somebody decides to put these objects into a HashSet, and...suddenly elements get lost in the Map. That'll be a debugging session that I don't want to attend.

As I said in my earlier post, Odersky et al.'s canEqual pattern is a carefully choreographed dance of interlocking instanceof checks. Change one thing, and people might start tripping over each other's toes. (It's kind of like this: https://twitter.com/SciencePorn/status/633020570343444480 -- imagine what a non-self-driving car might do here.)

As an encore, I will now break Coordinates's symmetry:

@Test
public void sidewaysSymmetry() {
    Coordinates p = new PointImpl(3, 3);
    Coordinates b = new BogusCoordinatesImpl();

    assertTrue(p.equals(b) == b.equals(p));
}


public class BogusCoordinatesImpl implements Coordinates {
    @Override
    public int getX() { return 0; }

    @Override
    public int getY() { return 0; }

    @Override
    public boolean equals(Object obj) {
        return true;
    }
}

See? It's easy! :)

Also, keep in mind that equals methods are often used in tight inner loops. Your canEqual method uses reflection, so it might actually affect performance.

[xenoterracide]

I surrender! and let you go figure out how to make equals verifier defeat my stupidity ;)

[jqno]

Well, I wouldn't call it stupidity. I'd call it misapplied ingenuity ;).

I'll let you know when I release a new version with fixes for some of the things we've been discussing.

[xenoterracide]

Thanks. I did try one thing that might fix that, but I've really temporarily given up. that.canEquals( this ) && this.canEquals( that ) though I'm not sure that anything can prevent malicious or stupidity.

[jqno]

Well, given the academic backgrounds of both Odersky and Spoon (2 of the co-authors of the article), I'd be surprised if they hadn't constructed a mathematical proof to show that their solution is correct, and resistant to malice and stupidity. There's another guy who invented a similar (though slightly less elegant) mechanism back in 2002, who also mentions mathematical proofs.

So, I'm not saying it's impossible, but ... know what you're getting yourself into ;).

[jqno]

A heads-up: I just released version 1.7.5, which fixes a number of the things we've discussed. See also the changelog. I'm closing the issue now, but feel free to re-open it, or add a new one, if you find anything else!

[xenoterracide]

awesome, thanks!