Duplicate variable name in if statement with two different instanceof (…

…#175) * Added failing JUnit test for issue "Duplicate variable name in if statement with two different instanceof" * Add issue link * Move and update test; include #265 * Update expectations & handle binary separately * Demonstrate known failure --------- Co-authored-by: Ko Turk <[email protected]> Co-authored-by: Tim te Beek <[email protected]>
openrewrite · Jul 14, 2024 · ab9825a · ab9825a
1 parent 48c94b5
commit ab9825a
Show file tree

Hide file tree

Showing 2 changed files with 114 additions and 7 deletions.
diff --git a/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java b/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java
@@ -19,7 +19,6 @@
 import lombok.EqualsAndHashCode;
 import lombok.Value;
 import org.openrewrite.*;
-import org.openrewrite.internal.ListUtils;
 import org.openrewrite.internal.lang.Nullable;
 import org.openrewrite.java.JavaVisitor;
 import org.openrewrite.java.VariableNameUtils;
@@ -265,7 +264,7 @@ private String patternVariableName(J.InstanceOf instanceOf, Cursor cursor) {
             } else {
                 strategy = VariableNameStrategy.short_();
             }
-            String baseName = variableBaseName((TypeTree) instanceOf.getClazz(), strategy);
+            String baseName = strategy.variableName(((TypeTree) instanceOf.getClazz()).getType());
             return VariableNameUtils.generateVariableName(baseName, cursor, INCREMENT_NUMBER);
         }
 
@@ -295,10 +294,6 @@ public J processVariableDeclarations(J.VariableDeclarations multiVariable) {
         }
     }
 
-    private static String variableBaseName(TypeTree typeTree, VariableNameStrategy nameStrategy) {
-        return nameStrategy.variableName(typeTree.getType());
-    }
-
     private static class UseInstanceOfPatternMatching extends JavaVisitor<Integer> {
 
         private final InstanceOfPatternReplacements replacements;
@@ -312,6 +307,26 @@ static J refactor(@Nullable J tree, InstanceOfPatternReplacements replacements,
             return new UseInstanceOfPatternMatching(replacements).visit(tree, 0, cursor);
         }
 
+        @Override
+        public J visitBinary(J.Binary original, Integer integer) {
+            Expression newLeft = (Expression) super.visitNonNull(original.getLeft(), integer);
+            if (newLeft != original.getLeft()) {
+                // The left side changed, so the right side should see any introduced variable names
+                J.Binary replacement = original.withLeft(newLeft);
+                Cursor widenedCursor = updateCursor(replacement);
+
+                Expression newRight;
+                if (original.getRight() instanceof J.InstanceOf) {
+                    newRight = replacements.processInstanceOf((J.InstanceOf) original.getRight(), widenedCursor);
+                } else {
+                    newRight = (Expression) super.visitNonNull(original.getRight(), integer, widenedCursor);
+                }
+                return replacement.withRight(newRight);
+            }
+            // The left side didn't change, so the right side doesn't need to see any introduced variable names
+            return super.visitBinary(original, integer);
+        }
+
         @Override
         public J.InstanceOf visitInstanceOf(J.InstanceOf instanceOf, Integer executionContext) {
             instanceOf = (J.InstanceOf) super.visitInstanceOf(instanceOf, executionContext);

diff --git a/src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java b/src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java
@@ -37,7 +37,6 @@ public void defaults(RecipeSpec spec) {
     @SuppressWarnings({"ImplicitArrayToString", "PatternVariableCanBeUsed", "UnnecessaryLocalVariable"})
     @Nested
     class If {
-
         @Test
         void ifConditionWithoutPattern() {
             rewriteRun(
@@ -468,6 +467,71 @@ void test(Object o) {
               )
             );
         }
+
+        @Test
+        @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/174")
+        void ifTwoDifferentInstanceOf() {
+            rewriteRun(
+              version(
+                //language=java
+                java(
+                  """
+                    class A {
+                        int combinedLength(Object o, Object o2) {
+                            if (o instanceof String && o2 instanceof String) {
+                                return ((String) o).length() + ((String) o2).length();
+                            }
+                            return -1;
+                        }
+                    }
+                    """,
+                  """
+                    class A {
+                        int combinedLength(Object o, Object o2) {
+                            if (o instanceof String string && o2 instanceof String string1) {
+                                return string.length() + string1.length();
+                            }
+                            return -1;
+                        }
+                    }
+                    """
+                ), 17
+              )
+            );
+        }
+
+        @Disabled("Not handled correctly yet")
+        @Test
+        @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/174")
+        void ifTwoDifferentInstanceOfWithParentheses() {
+            rewriteRun(
+              version(
+                //language=java
+                java(
+                  """
+                    class A {
+                        int combinedLength(Object o, Object o2) {
+                            if (o instanceof String && (o2 instanceof String)) {
+                                return ((String) o).length() + ((String) o2).length();
+                            }
+                            return -1;
+                        }
+                    }
+                    """,
+                  """
+                    class A {
+                        int combinedLength(Object o, Object o2) {
+                            if (o instanceof String string && (o2 instanceof String string1)) {
+                                return string.length() + string1.length();
+                            }
+                            return -1;
+                        }
+                    }
+                    """
+                ), 17
+              )
+            );
+        }
     }
 
     @SuppressWarnings({"CastCanBeRemovedNarrowingVariableType", "ClassInitializerMayBeStatic"})
@@ -570,6 +634,34 @@ String test(Object o) {
               )
             );
         }
+
+        @Test
+        @Issue("https://github.com/openrewrite/rewrite-static-analysis/pull/265")
+        void multipleCastsInDifferentOperands() {
+            rewriteRun(
+              //language=java
+              java(
+                """
+                  import java.util.Comparator;
+                  public class A {
+                     Comparator<Object> comparator() {
+                       return (a, b) ->
+                           a instanceof String && b instanceof String ? ((String) a).compareTo((String) b) : 0;
+                     }
+                  }
+                  """,
+                """
+                  import java.util.Comparator;
+                  public class A {
+                     Comparator<Object> comparator() {
+                       return (a, b) ->
+                           a instanceof String s && b instanceof String s1 ? s.compareTo(s1) : 0;
+                     }
+                  }
+                  """
+              )
+            );
+        }
     }
 
     @Nested