Add recipe ReplaceClassIsInstanceWithInstanceof for SonarQube RSPEC-6202

[yurii-yu]

What's changed?

Add two new recipes for SonarQube Issue RSPEC-1170 and RSPEC-6202

What's your motivation?

Benefited a lot from the OpenRewrite project, it is our responsibility to make some contribution.

Anything in particular you'd like reviewers to focus on?

This is the first time I submit recipes, I tried my best to follow the convention of existing recipes.

Anyone you would like to review specifically?

maybe @timtebeek

Have you considered any alternatives or workarounds?

I do not think there are better alternatives.

Any additional context

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Much appreciated @yurii-yu ! Let's see if our bot has some automated suggestions already, before I dive in later today or over the weekend.

[yurii-yu]

@timtebeek thank you for the insights. I overlooked that point before. I guess I can fix that and add the corresponding test on the weekend.

[timtebeek]

@timtebeek thank you for the insights. I overlooked that point before. I guess I can fix that and add the corresponding test on the weekend.

Thanks @yurii-yu ; I've already pushed up a few quick polishing commits with the changes I was expecting; let me know if there's anything you'd still like to add and otherwise we do a final review and merge early next week.

[timtebeek]

This might be too simplistic. As seen on for instance this class, making the instance fields static could drastically change behavior if there's more than one instance, if combined with mutable fields. This operation should only be safe for immutable types like primitives.

[timtebeek]

It might make sense to take out AddStaticModifierToPublicFinalConstantsAndFields and add that in a separate PR, as for now that perhaps has a design flaw around mutable fields we'd need to fix first, which would otherwise hold up ReplaceClassIsInstanceWithInstanceof unnecessarily.

[yurii-yu]

It might make sense to take out AddStaticModifierToPublicFinalConstantsAndFields and add that in a separate PR, as for now that perhaps has a design flaw around mutable fields we'd need to fix first, which would otherwise hold up ReplaceClassIsInstanceWithInstanceof unnecessarily.

Well, I agree with you. It is quite normal that we find things are more complicated than we thought before.

[yurii-yu]

@timtebeek thank you for the insights. I overlooked that point before. I guess I can fix that and add the corresponding test on the weekend.

Thanks @yurii-yu ; I've already pushed up a few quick polishing commits with the changes I was expecting; let me know if there's anything you'd still like to add and otherwise we do a final review and merge early next week.

Thank you for your great help. I need to admit that sometimes it's very frustrating to know "how to do it in the OpenRewrite" way, and I really appreciate your help. I always learn a lot from you.
Sorry I am quite busy this weekend because my son has Eishocky Turnier. I will come back to you in details by the end of Monday.

[yurii-yu]

@timtebeek I removed the Recipe AddStaticModifierToPublicFinalConstantsAndFields from this pull request and I have no more questions about the code. Please have a look at the latest code
Thank you for your great help, I always learn something new from you.

[timtebeek]

Thanks a lot @yurii-yu ! Great to see this come together. I've added a last polishing commit and with that we're good to merge. :)

[timtebeek]

Came across these cases we don't yet handle correctly in the wider OSS commnity by running through Moderne:
https://app.moderne.io/recipes/org.openrewrite.staticanalysis.ReplaceClassIsInstanceWithInstanceof

Needs wrapping parentheses

diff --git a/src/main/java/org/apache/maven/plugins/scripting/engine/JavaEngine.java b/src/main/java/org/apache/maven/plugins/scripting/engine/JavaEngine.java
index e9d06af..78a264e 100644
--- a/src/main/java/org/apache/maven/plugins/scripting/engine/JavaEngine.java
+++ b/src/main/java/org/apache/maven/plugins/scripting/engine/JavaEngine.java
@@ -321,7 +321,7 @@ org.openrewrite.staticanalysis.ReplaceClassIsInstanceWithInstanceof
     }
 
     private void doClose(final CompiledScript compile) {
-        if (!AutoCloseable.class.isInstance(compile)) {
+        if (!compile instanceof AutoCloseable) {
             return;
         }
         try {

Cause class cast exception

java.lang.ClassCastException: class org.openrewrite.java.tree.J$MethodInvocation cannot be cast to class org.openrewrite.java.tree.J$Identifier (org.openrewrite.java.tree.J$MethodInvocation and org.openrewrite.java.tree.J$Identifier are in unnamed module of loader 'app')
org.openrewrite.staticanalysis.ReplaceClassIsInstanceWithInstanceof$1.visitMethodInvocation(ReplaceClassIsInstanceWithInstanceof.java:63)
org.openrewrite.staticanalysis.ReplaceClassIsInstanceWithInstanceof$1.visitMethodInvocation(ReplaceClassIsInstanceWithInstanceof.java:54)
org.openrewrite.java.tree.J$MethodInvocation.acceptJava(J.java:3905)
org.openrewrite.java.tree.J.accept(J.java:59)
org.openrewrite.TreeVisitor.visit(TreeVisitor.java:250)
org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:320)
org.openrewrite.java.JavaVisitor.visitRightPadded(JavaVisitor.java:1367)
org.openrewrite.java.JavaVisitor.visitControlParentheses(JavaVisitor.java:524)
org.openrewrite.java.tree.J$ControlParentheses.acceptJava(J.java:4828)
org.openrewrite.java.tree.J.accept(J.java:59)
org.openrewrite.TreeVisitor.visit(TreeVisitor.java:250)
org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:320)
org.openrewrite.java.JavaVisitor.visitIf(JavaVisitor.java:714)
org.openrewrite.java.tree.J$If.acceptJava(J.java:2528)
org.openrewrite.java.tree.J.accept(J.java:59)
org.openrewrite.TreeVisitor.visit(TreeVisitor.java:250)
...