projectlombok / lombok

Very spicy additions to the Java programming language.
https://projectlombok.org/
Other
12.87k stars 2.38k forks source link

@EqualsAndHashCode should allow skipping the hashCode method #1747

Open ccronemberger opened 6 years ago

ccronemberger commented 6 years ago

I am working on a project where we are required to have a coverage of at least 80%. One problem we have is that we are using @EqualsAndHashCode because we need to have the equals method, but we don't use hashCode. Because of this multiple times we don't reach the limit for the coverage. In the Javadoc for the java.lang.Object#equals method we have this description:

"Note that it is generally necessary to override the {@code hashCode} method whenever this method is overridden"

This means hashCode is not really always necessary. On the other hand if we are using hashCode then we should have equals.

Maaartinus commented 6 years ago

This means hashCode is not really always necessary.

Not really... when you override equals, but not hashCode,

That's no good idea. The proper solution for your coverage problem is to configure the coverage tool to ignore Lombok-generated methods. [Configure Lombok to add an annotation][g] and use it.

[g]: https://projectlombok.org/api/lombok/ConfigurationKeys.html#ADD_LOMBOK_GENERATED_ANNOTATIONS

randakar commented 6 years ago

Actually, ignoring the method is a cop out too, I think. That way you can never be sure you have covered everything that needs covering.

Best way: Use http://jqno.nl/equalsverifier/ on it. And ToStringVerifier on the tostring method. I've written a (really) small library for data / value annotated objects that basically calls those things in sequence, dealing with odd cases like jacoco fields and the like. Unit test for each individual class is really really small - one line of actual code for standard value objects - and you get good coverage for it in return.

On Fri, Jun 29, 2018 at 5:38 PM, Maaartinus notifications@github.com wrote:

This means hashCode is not really always necessary.

Not really... when you override equals, but not hashCode,

  • you break the contract: Equal objects must have the same hash code.
  • one year later, someone uses your object as a key in a HashMap and nothing works

That's no good idea. The proper solution for your coverage problem is to configure the coverage tool to ignore Lombok-generated methods. Configure Lombok to add an annotation https://projectlombok.org/api/lombok/ConfigurationKeys.html#ADD_LOMBOK_GENERATED_ANNOTATIONS and use it.

— 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/1747#issuecomment-401391774, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRfJFOo-XjYFzhXOyozwmsp-zVYPBks5uBknYgaJpZM4U9KY8 .

-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven

ccronemberger commented 6 years ago

Adding "lombok.addLombokGeneratedAnnotation = false" to "lombok.config" file, fixed the issue, but also pretty much disabled the check of which attributes were covered in the tests, so this workaround is not very good. I think that in this case ideally the annotation @EqualsAndHashCode should have a onMethod attribute where I could specify that only those two methods would be annotated.

randakar commented 6 years ago

Don't try to fit a square peg in a round hole. Either don't cover generated methods, or do. But be consistent about it.

And seriously - testing those methods is not hard. If you do and Lombok ever breaks them you will immediately have test failures to warn you about it. It does have benefits. Especially since equalsverifier teaches you a lot about what this thing called 'equals' actually means..

Op vr 29 jun. 2018 19:56 schreef Constantino Cronemberger < notifications@github.com>:

Reopened #1747 https://github.com/rzwitserloot/lombok/issues/1747.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1747#event-1709200435, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRQEltfBA9NQ1ZGHqownjARCPV3wsks5uBmpQgaJpZM4U9KY8 .

ccronemberger commented 6 years ago

Don't try to fit a square peg in a round hole you too! In this case you should either support "onMethod" on all annotations instead of having it only on some of them.

randakar commented 6 years ago

For the record, I am not a member of the lombok team at all ;-)

On Mon, Jul 2, 2018 at 6:06 PM, Constantino Cronemberger < notifications@github.com> wrote:

Don't try to fit a square peg in a round hole you too! In this case you should either support "onMethod" on all annotations instead of having it only on some of them.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1747#issuecomment-401854413, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRQcVpeY8zRtXP-uoBLPk_YAg-z5Uks5uCkT1gaJpZM4U9KY8 .

-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven

janrieke commented 6 years ago

I agree with @randakar and @Maaartinus that adding this would be a bad idea. Allowing lombok users breaking the equals-hashcode contract (let it be on purpose or by accident) will contradict almost everything lombok aims for. If you need a special-purpose solution, then implement it. And if you don't want to write lengthy equals implementations, use EqualsBuilder from Apache Commons or similar helpers.

sniffertine commented 4 years ago

From the Javadoc: https://docs.oracle.com/javase/10/docs/api/java/lang/Object.html#hashCode()

It is not required that if two objects are unequal according to the equals(java.lang.Object) method, then calling the hashCode method on each of the two objects must produce distinct integer results. However, the programmer should be aware that producing distinct integer results for unequal objects may improve the performance of hash tables.

For JPA entities I would like to let equals be generated by lombok but hashCode just be "return 31". This way, I can guarantee that hash codes never change when entities are persisted.

Details here (Chapter: Using a Generated Primary Key): https://thoughts-on-java.org/ultimate-guide-to-implementing-equals-and-hashcode-with-hibernate

--> maybe add an option to @EqualsAndHashCode to use a constant for hashCode?

Maaartinus commented 4 years ago

hashCode just be "return 31"

'cause 42 is too common, right? ;) But seriously, this would break a contract, too, namely that of all Hash* classes, as they're supposed to be O(1) and you'd make them O(n). IOW, you'd get a serious slowdown pretty everywhere.

Some people claim that equality of entities should be content based, i.e., include everything but the id. Other claim that nothing but the id (and the class) should be used. I followed the latter and added assert id != 0 to both equals and hashCode (which were trivial to write manually). This way, my IDs never change when it matters (as I test with assertion on).

I obviously can't use transient entities as hash keys, but that's an acceptable price.

sniffertine commented 4 years ago

If only 42 was a prime... I agree that you decrease the performance of Hash* classes. That's why I would make this option opt-in. Currently I am not sure if it is save to use Lombok-generated equals and hashCode with JPA/Hibernate. And I think you also cannot put transient entities into a *HashSet, which seem quite a restriction. When you write your equals and hashCode yourself, there is obviously no issue to discuss here.

Maaartinus commented 4 years ago

If only 42 was a prime...

It'd be enough when it wasn't even. As all used hash tables (I've ever seen) in Java use power of two sizes, all you need is co-primality to two, i.e., an odd number. Primality itself is irrelevant, 33 about as good as 31 and a bigger number would be somewhat better.

Currently I am not sure if it is save to use Lombok-generated equals and hashCode with JPA/Hibernate.

Nothing is save with mutable keys. Anyway, you have to choose whether you want to use the ID or the "business keys" in your equals (IMHO using both would be crazy).

And I think you also cannot put transient entities into a *HashSet, which seem quite a restriction.

When there are multiple transient entities, then I use a mapping like from itemNo to item, so there's no problem. There's pretty always a simple workaround. The persistent entities are more common.

Note that with a constant hashCode, you could use a HashSet only as efficiently as List, and that'd be a problem with hundreds or thousands of entities.

rspilker commented 4 years ago

Did you test having a hand-written hashcode, annotated with @Tolerate, and @EqualsAndHashcode on the type?

Maaartinus commented 4 years ago

@rspilker Does it work? Should it? Given https://github.com/rzwitserloot/lombok/pull/52#issuecomment-72106077, I'd say no.

TreffnonX commented 4 years ago

While I agree with all of you on both the usefulness and the necessity of generating both equals and hashCode at the same time, and ensuring uniformity across all test cases, I have to agree with @ccronemberger too. The lombok frameworks main purpose is to add syntax in a configurable way. This should be consistent in itself too, and features should be as general as possible (even if we don't see the usefulness, or would decide against using it). To be adaptable to different project requirements is the reason lombok has as many users as it does.

randakar commented 4 years ago

The purpose of Lombok is to kill boilerplate. Being adaptable is a side effect ;)

On Wed, Dec 11, 2019, 06:47 TreffnonX notifications@github.com wrote:

While I agree with all of you on both the usefulness and the necessity of generating both equals and hashCode at the same time, and ensuring uniformity across all test cases, I have to agree with @ccronemberger https://github.com/ccronemberger too. The lombok frameworks main purpose is to add syntax in a configurable way. This should be consistent in itself too, and features should be as general as possible (even if we don't see the usefulness, or would decide against using it). To be adaptable to different project requirements is the reason lombok has as many users as it does.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1747?email_source=notifications&email_token=AABIERJHUOJQHY47GIDQHJ3QYB5IVA5CNFSM4FHUUY6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGR6UBY#issuecomment-564390407, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABIERPPU6EC5ISW5EBYIZLQYB5IVANCNFSM4FHUUY6A .

vicon-byte commented 4 years ago

Lombok hash implementation runs quickly into hashmap collisions when working with key-value in memory caches like Ignite, Redis, Memcached. The solution is to use apache HashCodeBuilder. So you still want lombok to cover the equals, but let developer customize hash implementation with HashCodeBuilder. So maybe you can find a middle ground like Extend @EqualsAndHashCode capabilities to allow using HashCodeBuilder (or other builders).

Rawi01 commented 4 years ago

@vicon-byte why exactly should the generated hashCode method produce collisions more often than one generated by a third party library?