diff --git a/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java b/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java index f4e6c612a0..226cbc8a6d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java +++ b/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java @@ -289,11 +289,11 @@ public boolean isEmpty() { return replacements.isEmpty() && variablesToDelete.isEmpty(); } - public J.InstanceOf processInstanceOf(J.InstanceOf instanceOf, Cursor cursor) { + public J.InstanceOf processInstanceOf(J.InstanceOf instanceOf, Cursor cursor, Set usedNames) { if (!contextScopes.containsKey(instanceOf)) { return instanceOf; } - String name = patternVariableName(instanceOf, cursor); + String name = patternVariableName(instanceOf, cursor, usedNames); TypedTree typeCastTypeTree = computeTypeTreeFromTypeCasts(instanceOf); TypedTree currentTypeTree = (TypedTree) instanceOf.getClazz(); @@ -341,7 +341,7 @@ private TypeTree computeTypeTreeFromTypeCasts(J.InstanceOf instanceOf) { return typeCastTypeTree; } - private String patternVariableName(J.InstanceOf instanceOf, Cursor cursor) { + private String patternVariableName(J.InstanceOf instanceOf, Cursor cursor, Set usedNames) { VariableNameStrategy strategy; JavaType type = ((TypeTree) instanceOf.getClazz()).getType(); if (root instanceof J.If) { @@ -358,6 +358,13 @@ private String patternVariableName(J.InstanceOf instanceOf, Cursor cursor) { if (root instanceof J.If) { J.If enclosingIf = cursor.firstEnclosing(J.If.class); String nameInIfScope = VariableNameUtils.generateVariableName(baseName, new Cursor(cursor, enclosingIf), INCREMENT_NUMBER); + + // Also check against other pattern variables being introduced in the same expression + while (usedNames.contains(nameInIfScope)) { + String numStr = nameInIfScope.substring(baseName.length()); + nameInIfScope = baseName + (numStr.isEmpty() ? 1 : Integer.parseInt(numStr) + 1); + } + String nameInCursorScope = VariableNameUtils.generateVariableName(baseName, cursor, INCREMENT_NUMBER); return nameInIfScope.compareTo(nameInCursorScope) >= 0 ? nameInIfScope : nameInCursorScope; } @@ -407,13 +414,22 @@ public J visitBinary(J.Binary binary, Integer p) { // The left side changed, so the right side should see any introduced variable names Cursor widenedCursor = updateCursor(b); + // Collect names from the left side if it's an instanceof with a pattern + Set usedNames = new HashSet<>(); + if (b.getLeft() instanceof J.InstanceOf) { + J.InstanceOf leftInstanceOf = (J.InstanceOf) b.getLeft(); + if (leftInstanceOf.getPattern() != null) { + usedNames.add(((J.Identifier) leftInstanceOf.getPattern()).getSimpleName()); + } + } + Expression newRight; if (binary.getRight() instanceof J.InstanceOf) { - newRight = replacements.processInstanceOf((J.InstanceOf) binary.getRight(), widenedCursor); + newRight = replacements.processInstanceOf((J.InstanceOf) binary.getRight(), widenedCursor, usedNames); } else if (binary.getRight() instanceof J.Parentheses && ((J.Parentheses) binary.getRight()).getTree() instanceof J.InstanceOf) { @SuppressWarnings("unchecked") J.Parentheses originalRight = (J.Parentheses) binary.getRight(); - newRight = originalRight.withTree(replacements.processInstanceOf(originalRight.getTree(), widenedCursor)); + newRight = originalRight.withTree(replacements.processInstanceOf(originalRight.getTree(), widenedCursor, usedNames)); } else { newRight = (Expression) visitNonNull(binary.getRight(), p, widenedCursor); } @@ -426,7 +442,7 @@ public J visitBinary(J.Binary binary, Integer p) { @Override public J.InstanceOf visitInstanceOf(J.InstanceOf instanceOf, Integer p) { instanceOf = (J.InstanceOf) super.visitInstanceOf(instanceOf, p); - return replacements.processInstanceOf(instanceOf, getCursor()); + return replacements.processInstanceOf(instanceOf, getCursor(), new HashSet<>()); } @Override diff --git a/src/main/resources/META-INF/rewrite/examples.yml b/src/main/resources/META-INF/rewrite/examples.yml index 1351b9f6b6..1a668ccb06 100644 --- a/src/main/resources/META-INF/rewrite/examples.yml +++ b/src/main/resources/META-INF/rewrite/examples.yml @@ -119,6 +119,7 @@ examples: - description: '' parameters: - 'null' + - 'null' sources: - before: | public class PersonBuilder { @@ -360,44 +361,44 @@ examples: - description: '' sources: - before: | + import java.io.FileReader; + import java.io.IOException; + class A { - void foo() throws IllegalAccessException { + void foo() throws IOException { try { - throw new IllegalAccessException(); - } catch (Exception e) { - throw e; // `e` is removed below + new FileReader("").read(); + } catch (IOException e) { + throw e; } } } after: | + import java.io.FileReader; + import java.io.IOException; + class A { - void foo() throws IllegalAccessException { - throw new IllegalAccessException(); + void foo() throws IOException { + new FileReader("").read(); } } language: java - description: '' sources: - before: | - import java.io.FileReader; - import java.io.IOException; - class A { - void foo() throws IOException { + void foo() throws IllegalAccessException { try { - new FileReader("").read(); - } catch (IOException e) { - throw e; + throw new IllegalAccessException(); + } catch (Exception e) { + throw e; // `e` is removed below } } } after: | - import java.io.FileReader; - import java.io.IOException; - class A { - void foo() throws IOException { - new FileReader("").read(); + void foo() throws IllegalAccessException { + throw new IllegalAccessException(); } } language: java @@ -1212,6 +1213,61 @@ examples: language: java --- type: specs.openrewrite.org/v1beta/example +recipeName: org.openrewrite.staticanalysis.InstanceOfPatternMatch +examples: +- description: '' + sources: + - before: | + class LeftNode { + int bar() { + return 0; + } + } + class RightNode { + int bar() { + return 1; + } + } + + class Foo { + void bar(Object o1, Object o2) { + if (o1 instanceof LeftNode && o2 instanceof RightNode) { + ((LeftNode)o1).bar(); + ((RightNode)o2).bar(); + } + else if (o1 instanceof RightNode && o2 instanceof LeftNode) { + ((RightNode)o1).bar(); + ((LeftNode)o2).bar(); + } + } + } + after: | + class LeftNode { + int bar() { + return 0; + } + } + class RightNode { + int bar() { + return 1; + } + } + + class Foo { + void bar(Object o1, Object o2) { + if (o1 instanceof LeftNode node2 && o2 instanceof RightNode node3) { + node2.bar(); + node3.bar(); + } + else if (o1 instanceof RightNode node && o2 instanceof LeftNode node1) { + node.bar(); + node1.bar(); + } + } + } + language: java +--- +type: specs.openrewrite.org/v1beta/example recipeName: org.openrewrite.staticanalysis.IsEmptyCallOnCollections examples: - description: '' @@ -3039,18 +3095,6 @@ examples: type: specs.openrewrite.org/v1beta/example recipeName: org.openrewrite.staticanalysis.SimplifyBooleanExpression examples: -- description: '' - sources: - - before: | - fun getSymbol() : String? { - return null - } - language: kotlin - - before: | - val isPositive = getSymbol().equals("+") == true - after: | - val isPositive = getSymbol().equals("+") - language: kotlin - description: '' sources: - before: | @@ -3070,6 +3114,18 @@ examples: } } language: java +- description: '' + sources: + - before: | + fun getSymbol() : String? { + return null + } + language: kotlin + - before: | + val isPositive = getSymbol().equals("+") == true + after: | + val isPositive = getSymbol().equals("+") + language: kotlin --- type: specs.openrewrite.org/v1beta/example recipeName: org.openrewrite.staticanalysis.SimplifyBooleanExpressionWithDeMorgan diff --git a/src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java b/src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java index 31e9b17a94..4d209c29b1 100644 --- a/src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java @@ -17,6 +17,7 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; import org.openrewrite.Issue; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; @@ -33,6 +34,66 @@ public void defaults(RecipeSpec spec) { .allSources(sourceSpec -> version(sourceSpec, 17)); } + @DocumentExample + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/619") + @Test + void variableNamingConflictWithMultipleInstanceOf() { + rewriteRun( + //language=java + java( + """ + class LeftNode { + int bar() { + return 0; + } + } + class RightNode { + int bar() { + return 1; + } + } + + class Foo { + void bar(Object o1, Object o2) { + if (o1 instanceof LeftNode && o2 instanceof RightNode) { + ((LeftNode)o1).bar(); + ((RightNode)o2).bar(); + } + else if (o1 instanceof RightNode && o2 instanceof LeftNode) { + ((RightNode)o1).bar(); + ((LeftNode)o2).bar(); + } + } + } + """, + """ + class LeftNode { + int bar() { + return 0; + } + } + class RightNode { + int bar() { + return 1; + } + } + + class Foo { + void bar(Object o1, Object o2) { + if (o1 instanceof LeftNode node2 && o2 instanceof RightNode node3) { + node2.bar(); + node3.bar(); + } + else if (o1 instanceof RightNode node && o2 instanceof LeftNode node1) { + node.bar(); + node1.bar(); + } + } + } + """ + ) + ); + } @Nested class If {