New InstanceOfPatternMatch recipe

[knutwannheden]

Implement a recipe which for Java 17+ transforms instanceof expressions which have a corresponding type cast within their flow scope (see https://openjdk.org/jeps/394 for details) to an instanceof with a pattern variable and then also substitutes the type cast expressions accordingly.

[knutwannheden]

This is still a draft. There are still a few TODOs and the code is probably not very OpenRewrite-idiomatic, as I probably missed out on some helper utilities or misunderstood how to use all available constructs. A critical review would be most appreciated!

[knutwannheden]

An interesting enhancement could be to inline the expression, if it isn't referenced anywhere else. E.g.

Object o = foo.bar();
if (o instanceof String && ((String) o).isEmpty()) {
    // ...
}

would become:

if (foo.bar() instanceof String o && o.isEmpty()) {
    // ...
}

if o isn't referenced anywhere outside the if-statement. I would however propose to file a separate enhancement request for this and maybe that should even be a completely independent (reusable) visitor.

Note that since Java isn't a lazily evaluated language, this "optimization" is only possible when the variable declaration immediately precedes the if-statement or there are no statements in between which could have side-effects (or the expression itself is known to be free of side-effects).

[knutwannheden]

@sambsnyd I consider this PR to be ready to be reviewed now. I still think that some things may be a bit non-idiomatic and would be glad for some tips on how to improve the code. (I am for instance wondering if it would be more idiomatic to schedule a separate after-recipe for each occurrence, rather than try to collect them all into a single input for the visitor or possibly it is even considered better practice to directly "inline" the visitor into the respective visitXyz() method and call the visit(Tree, P, Cursor) method on it.)

To be on the safe side, I currently exclude any expressions, which may have side effects (as determined by Expression#getSideEffects()). I noticed that a few expression types were missing the implementation for this method or had an incorrect implementation. I will look into this in a separate PR. (I would also like to consider support for IDEA's @Contract(pure=true) and possibly also the Checker framework's corresponding annotation, but also that will be a separate issue.)

[sambsnyd]

Nice work. The recipe is complex, but the operation being performed is complex, so I don't see an obvious way to simplify things.

[sambsnyd]

@sambsnyd I consider this PR to be ready to be reviewed now. I still think that some things may be a bit non-idiomatic and would be glad for some tips on how to improve the code. (I am for instance wondering if it would be more idiomatic to schedule a separate after-recipe for each occurrence, rather than try to collect them all into a single input for the visitor or possibly it is even considered better practice to directly "inline" the visitor into the respective visitXyz() method and call the visit(Tree, P, Cursor) method on it.)

Collecting them all into a single input for the visitor is definitely better for performance than separate after-recipe for each. Better to pay the cost of traversing the LST once rather than n times.
I have developed a preference for visitXyz() rather than doNext(). I often find it easier to debug and reason about the state of the LST when the change I'm looking to effect is made immediately. There is nothing wrong with using doNext(), and it is particularly convenient when the change you are trying to effect is going to happen elsewhere in the tree.

To be on the safe side, I currently exclude any expressions, which may have side effects (as determined by Expression#getSideEffects()). I noticed that a few expression types were missing the implementation for this method or had an incorrect implementation. I will look into this in a separate PR. (I would also like to consider support for IDEA's @Contract(pure=true) and possibly also the Checker framework's corresponding annotation, but also that will be a separate issue.)

Those sound like good improvements

[knutwannheden]

Nice work. The recipe is complex, but the operation being performed is complex, so I don't see an obvious way to simplify things.

I did figure out a way to slightly simplify things. Since I already used the cursor for the containing statement to "communicate" between the instanceof and the type cast expressions, I now added a postVisit() method which simply checks for the corresponding message in the cursor and if it finds a non-empty result, it means that there is a replacement to be performed and it can post-process the statement using the other visitor. This way the second visitor only needs to process small sub-trees rather than the whole source again.

I was looking for a mechanism to instruct a cursor to perform some post-processing action, once the visitor leaves that scope. As it turns out the postVisit() method can be used for this purpose when adding a message to the corresponding cursor.

[knutwannheden]

@sambsnyd Thanks for your review feedback! I did find and fix one more bug and added a few more tests. There are probably more corner cases which should be tested, but I can't think of them right now.

[timtebeek]

Over on this issue the question came up if Java 9+ recipes should go into rewrite-migrate-java rather than openrewrite/rewrite.

Intuitively I'd think any "new"-ish Java features would best fit into org.openrewrite.java.migrate, and be aggregated in that module in yaml migration recipes to complete a migration. That way there's also no question what Java source level is required for openrewrite/rewrite: 8 (for now), to best fit with what we're seeing in the wild.

What are your thoughts on that @sambsnyd ?

[sambsnyd]

@timtebeek I see rewrite-migrate-java as being for recipes about migrating to new versions of java. I don't think we need to put all recipes applicable only to newer versions of java there. This feels like a cleanup recipe more than a migration recipe, so putting it into rewrite-java feels reasonable to me

[knutwannheden]

This feels like a cleanup recipe more than a migration recipe, so putting it into rewrite-java feels reasonable to me

@sambsnyd Should the recipe be moved to the cleanup subpackage?