Lombok: [BUG] Instance of BaseType does not equal to instance of SuperType, even though all fields from BaseType are equal

[rolaca11]

Describe the bug
Given 2 classes: A and B, where B is super type of A. A has a field f.

Given 1 instance of A (_a_) and B (_b_), where a.f = _SOME_TEXT_ and b.f = _SOME_TEXT_, a.equals(b) returns false with the generated equals method.

To Reproduce
Here is a test class, where the method test() fails at the last assertion:

import static org.assertj.core.api.Assertions.assertThat;

import lombok.EqualsAndHashCode;
import org.junit.jupiter.api.Test;

public class LombokEqualsTest {

    public static final String F = "F";

    @Test
    void test() {
        A a1 = new A();
        A a2 = new A();

        a1.f = F;
        a2.f = F;

        assertThat(a1).isEqualTo(a2);

        A b = new B();
        b.f = F;

        assertThat(b).isNotEqualTo(a1); // maybe this should return true as well, not sure
        assertThat(a1).isEqualTo(b);
    }

    @EqualsAndHashCode
    static class A {
        private String f;
    }

    @EqualsAndHashCode(callSuper = true)
    static class B extends A {
        private String g;
    }
}

Expected behavior
I'd expect that if all I know about two objects is that their base type is A and both objects have equal field values for all fields defined in A then they should be equal

Version info (please complete the following information):

• Lombok version: 1.18.10
• Platform JDK11

Additional context
The problem seems to be with the fact that lombok overrides the canEqual() method in the super type and that A.equals() calls that canEqual() method, which returns false, so A.equals() doesn't even get to comparing the fields.

I might have made an error of principle here, I'm open to discussing if I'm right with my expectations.

P.S.: An example equals() method generated by lombok:

public boolean equals(Object o) {
    if (o == this) {
        return true;
    } else if (!(o instanceof LombokEqualsTest.A)) {
        return false;
    } else {
        LombokEqualsTest.A other = (LombokEqualsTest.A)o;
        if (!other.canEqual(this)) {
            return false;
        } else {
            Object this$f = this.f;
            Object other$f = other.f;
            if (this$f == null) {
                if (other$f != null) {
                    return false;
                }
            } else if (!this$f.equals(other$f)) {
                return false;
            }
            return true;
        }
    }
}

I'd say that the problem area is the line if (!other.canEqual(this)) {. I'd say a better replacement would be if (!this.canEqual(this)) {, even tho that's redundant in this case because of } else if (!(o instanceof LombokEqualsTest.A)) {.

[janrieke]

Your expectation is in fact wrong. equals() must be symmetric, i.e.

assertThat(b).isNotEqualTo(a1);
assertThat(a1).isEqualTo(b);

must never succeed.

Furthermore, the typical expectation is that an instance of a subclass will always be different to an instance of its superclass, even if it has no additional fields (that is because the behavior of the subclass may be different). If you need a different behavior for equals(), you have to implement it manually.

[rolaca11]

Furthermore, the general expectation is that an instance of a subclass will always be different to an instance of its superclass, even if it has no additional field (that is because the behaviour of the subclass may be different).

I don't necessarily agree with this part.

Given a structure, where I have some kind of FieldDescriptor, that can let's say describe a field of a given parent class.
Extending this, a FieldWrapper, that not only describes a given field, but it can also retrieve it, since it not only is aware of the type of the parent class, but also a concrete instance of it.

In this case, an instance of FieldDescriptor and FieldWrapper could be equal, since they can describe the very same field, even though the latter has additional knowledge about the given field.

I'd think about the functionality of the equals() method as comparing the states of instances as opposed to their behavior.

[rolaca11]

Let's think about a comfortable middle ground.

I thought that if I override the canEqual() method, then I can get around the principle that an instance of a subclass will always be different to an instance of its superclass.

The nicest way I can think of could be to define the method on the superclass. However, Lombok will always just override this method in subclasses. If I define it as final, then I get a compiler error.

My suggestion would be to handle the case, where canEqual() is final, so that Lombok simply omits the method implementation altogether in subtypes.

[randakar]

What happens if you have Equalsverifier ( https://jqno.nl/equalsverifier/)
verify the resulting classes?

On Sat, Jan 11, 2020, 15:08 rolaca11 [e-mail] wrote:

Let's think about a comfortable middle ground.

I thought that if I override the canEqual() method, then I can get around
the principle that an instance of a subclass will always be different to
an instance of its superclass.

The nicest way I can think of could be to define the method on the
superclass. However, Lombok will always just override this method in
subclasses. If I define it as final, then I get a compiler error.

My suggestion would be to handle the case, where canEqual() is final, so
that Lombok simply omits the method implementation altogether in subtypes.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/2333?email_source=notifications&email_token=AABIERIDEP4WNWB525XLRU3Q5HHEFA5CNFSM4KFRSBC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWCWMA#issuecomment-573319984,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AABIERLVJBQPVVK263KAPNLQ5HHEFANCNFSM4KFRSBCQ
.

[rolaca11]

What happens if you have Equalsverifier ( https://jqno.nl/equalsverifier/) verify the resulting classes?

That symmetry is broken. Fixing canEqual() is not enough :\

[randakar]

Thing is, writing proper equals methods is hard.

You may be correct that if all fields in two objects are equal then the
objects are arguably equal (at least, when one inherits from the other).
From what I read on the subject, it does look that way.

But whatever implementation change you make for this is going to have to
abide by the rules. Equalsverifier at least should still give those methods
a thumbs up.

Secondly, there is backwards compatibility to consider. Can't have code in
existing projects suddenly breaking because the implementation changed
underneath them. Which in turn means lombok can't silently change this.

Don't know about this one. Allowing the user to shoot themselves in the
foot equals contract wise may be the best option, but it doesn't feel
pretty, either

On Sat, Jan 11, 2020, 15:52 rolaca11 [e-mail] wrote:

What happens if you have Equalsverifier ( https://jqno.nl/equalsverifier/)
verify the resulting classes?

That symetry is broken. That is not enough :\

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/2333?email_source=notifications&email_token=AABIERNOEX3TGOLWTMBHA3TQ5HMMBA5CNFSM4KFRSBC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWDRWY#issuecomment-573323483,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AABIERK6PEZGM656BGH6353Q5HMMBANCNFSM4KFRSBCQ
.

[Maaartinus]

Extending this, a FieldWrapper, that not only describes a given field, but it can also retrieve it, since it not only is aware of the type of the parent class, but also a concrete instance of it.

But this also implies that two field wrappers corresponding to the same field but different concrete instances must be equal.

That's strange but OK if you want it this way. Anyway, what Lombok does is correct as IMHO it does what's required most of the time.

I thought that if I override the canEqual() method, then I can get around the principle that an instance of a subclass will always be different to an instance of its superclass.

Right that's what canEqual is for.

The nicest way I can think of could be to define the method on the superclass. However, Lombok will always just override this method in subclasses.

There's a trivial solution: Do not put @EqualsAndHashCode at the subclass as the inherited methods do exactly what you want.

If I define it as final, then I get a compiler error.

Sure, Lombok knows nothing about other compilation units, that'd require resolution.

My suggestion would be to handle the case, where canEqual() is final, so that Lombok simply omits the method implementation altogether in subtypes.

That's impossible, but you don't need it.

Summarized:

• there's no bug; you're requesting a not-so-common behavior (not default but supported)
• use @EqualsAndHashCode on the superclass
• define canEqual() returning this instanceof A in the superclass
• leave subclasses alone