Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use pattern variables for instanceof for type casting in equals #84315

Closed

Conversation

arteam
Copy link
Contributor

@arteam arteam commented Feb 23, 2022

Leverage the JEP 394 and express type casting and the equality check as a single expression.

The biggest issue seems to be that using instanceof instead of getClass actually can change the semantic of equals for non-final classes: two different children of the same parent can be equal if they don't override parent's equals.

@arteam arteam force-pushed the use-pattern-variables-for-equals branch from 36fbd57 to aa45195 Compare February 23, 2022 19:46
@arteam arteam changed the title Use pattern variables for casting to type in equals Use pattern variables for instanceof for type casting in equals Feb 24, 2022
@arteam arteam force-pushed the use-pattern-variables-for-equals branch from aa45195 to 1f5b9b3 Compare February 24, 2022 09:26
@arteam arteam added the :Core/Infra/Core Core issues without another label label Feb 24, 2022
@arteam arteam marked this pull request as ready for review February 24, 2022 17:04
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Feb 24, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@pugnascotia
Copy link
Contributor

The biggest issue seems to be that using instanceof instead of getClass actually can change the semantic of equals for non-final classes: two different children of the same parent can be equal if they don't override parent's equals.

How concerning is this? Should we be being this to Core/Infra for discussion, before we change 800+ classes? I do think this PR is too big to review. If we want to proceed, we should break this down into smaller chunks.

@arteam
Copy link
Contributor Author

arteam commented Feb 24, 2022

How concerning is this? Should we be being this to Core/Infra for discussion, before we change 800+ classes? I do think this PR is too big to review. If we want to proceed, we should break this down into smaller chunks.

I'm okay to breaking this PR in multiple chunks. I believe it's not very concerning since all the "abstract" classes have children which override the equals method. But I can specifically extract such classes from the change. I can find 101 matches for super.equals(obj) == false in the code base, so there should be around 20-30 abstract classes provided that a base class has a handful amount of children.

@arteam arteam force-pushed the use-pattern-variables-for-equals branch 6 times, most recently from 9ca893a to d27452f Compare February 25, 2022 12:51
@arteam arteam force-pushed the use-pattern-variables-for-equals branch from d27452f to f8bc92a Compare February 25, 2022 12:55
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to do this, for two reasons:

  1. equals is then no longer symmetric. Suppose you have class B extends class A and corresponding objects b and a. If you do a.equals(b), it could be true, whereas b.equals(a) would not be.
  2. The equals generated by IDEA differs from this. We thus have to hand-craft equals, which is error-prone.

@arteam
Copy link
Contributor Author

arteam commented Mar 3, 2022

Thank you Henning!

I actually was going to close the PR exactly for these both reasons. I found that there are too many side effects for going from getClass to instanceof (especially for parent classes) and also because Intellij doesn't provide a template for generating equals with instanceof for non-final classes. Anyways, this PR was a very useful exercise for me for learning some advanced refactoring capabilities of IntelliJ.

@arteam arteam closed this Mar 3, 2022
@arteam arteam deleted the use-pattern-variables-for-equals branch March 3, 2022 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >non-issue >refactoring Team:Core/Infra Meta label for core/infra team v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants