Use `instanceof` pattern-matching where possible

[JarvisCraft]

Description

This PR enables the usage of instanceof pattern matching wherever possible (without changing semantics) reducing error-proneness and potentially enhancing readability.

Important notes

1. It is also woth mentioning that whenever used with inverted conditions (such as !(foo instanceof Bar bar) this must use prefix invertion notation (!expr) instead of comparisoj to false (expr == false).

2. There are multiple occurrences of the following pattern:

assert foo instanceof Bar;
Bar bar = (Bar) foo;

Formally, this cannot be rewritten as

assert foo instanceof Bar;
Bar bar = (Bar) foo;

because the assertion may be skipped if not enabled.

I have kept such cases untouched although I suggest to update them to something like

if (!foo instanceof Bar bar) {
    throw new AssertionError();
}

which seems to have no performance penalty considering that typecheck is done anyway when trying to cast foo to Bar so toggleable assertion seems to not be helping with anything

(please write in the comments, if this should be done).

[uschindler]

I am fine with this, unless it makes backports harder. So basically I use this for new code in new 10.x features only, but I'd like to prevent this from happing in code that is possibly backported.

[uschindler]

The precommit checks already answer most of your questions: Only use that feature for now if it is easy to apply. :-)

[JarvisCraft]

The precommit checks already answer most of your questions: Only use that feature for now if it is easy to apply. :-)

Fair point! As for now, I will run them locally (my bad here) and update the code accordingly

[JarvisCraft]

I've fixed the issues and applied ./gradlew tidy.

As for the backporting burden, I, unfortunately, lack the expertise in providing Lucene backports, so I guess that I'd better leave it up to you to decide which of these are okay to be applied and which ones are in "hot code" (although I possitively expect this specific lines to be rarely visited).

Considering that this changes also require Java version higher than 11, this seems to be a Lucene 10 change, so I guess there is plenty of room for discussion here whether specific files should be changed.

[uschindler]

I already assigned the Lucene 10 milestone, not lucene 9.x

[uschindler]

I tend to leave out the equals() methods. Many people in Lucene prefer to have a "step-by-step equals", as it is easier to read. In addition, if we go that route: Please add brackets to clarify if && has precendence over || or vice versa: Actually I don't know it!

The general problem is that we add "modern" equals and hashCode methods that do not use instanceof but compare classes directly. The examples you changed are old-style instanceof versions, which are buggy in class hiererachies (unless it is a final class).

So I tend to change the equals methods with instanceof to follow the more modern approach consistently. This would be a separate PR. The problem of all those methods is that they were created by some IDE without human intervention (and all IDEs use different patterns, although modern ones use class comparisons instead of instanceof).

[JarvisCraft]

So I tend to change the equals methods with instanceof to follow the more modern approach consistently.

In this case, I may roll back changes to equals methods and create a separate PR replacing their equals implementations where it is backwards-compatible (i.e. on final classes).

[mikemccand]

In general it's great for Lucene devs to use the new language features we gain by setting a minimum Java version. This is (part of?) why we have such minimums!

This nice instanceof pattern matching was introduced in Java 14, and since main (to be 10.0) requires Java 17 or 18, we can use nice new feature.

But we cannot backport to 9.x since it supports Java 11+, but I think that added burden (on backport) is OK. We accumulate such burden as the source code diverges anyways...

Thanks for persisting here @JarvisCraft -- +1 to the plan to revert the equals method changes (and maybe do a follow-on PR to "standardize" those), and merge the rest of this PR.

[JarvisCraft]

@mikemccand, thanks for the comments!

I've undone the changes to equals() methods and applied the fix to the remaining fixable occurrences of the pattern.

[iverase]

can we keep explicit == false checks instead of less readable !?

[JarvisCraft]

can we keep explicit == false checks instead of less readable !?

No, since javac only recognizes (!(EXPR instanceof TYPE NAME))

[uschindler]

P.S.: When pushing more changes, please do not squash and force-push! This makes reviewing not working easy. We squash later when merging, it is not needed to do this while developing a PR.

When You add separate commits I can verify what you have changed, when its squashed I have to do all the work again.

[github-actions[bot]]

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

[JarvisCraft]

Apologies for the late reply, I've lost the track of the message in the thread. I will soon coma back to it and see what should be changed.

[github-actions[bot]]

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!