From 20d2285288c3c38843bad9e426fb928ce3b3bb69 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 11 Apr 2026 14:20:31 +0200 Subject: [PATCH 1/3] Fix InstanceOfPatternMatch duplicate pattern variable names with flow scoping Track introduced pattern variable names across statements within a method to avoid generating duplicates when Java's flow scoping keeps an earlier pattern variable in scope (e.g., else branch with return/throw). Fixes https://github.com/moderneinc/customer-requests/issues/2202 --- .../InstanceOfPatternMatch.java | 35 ++++++++++++--- .../InstanceOfPatternMatchTest.java | 44 +++++++++++++++++++ 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java b/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java index b672e108e..9ac2f3a87 100644 --- a/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java +++ b/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java @@ -72,12 +72,20 @@ public TreeVisitor getVisitor() { ); return Preconditions.check(preconditions, new JavaVisitor() { + final Set introducedPatternVarNames = new HashSet<>(); + + @Override + public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { + introducedPatternVarNames.clear(); + return (J.MethodDeclaration) super.visitMethodDeclaration(method, ctx); + } + @Override public @Nullable J postVisit(J tree, ExecutionContext ctx) { J result = super.postVisit(tree, ctx); InstanceOfPatternReplacements original = getCursor().getMessage("flowTypeScope"); if (original != null && !original.isEmpty()) { - return UseInstanceOfPatternMatching.refactor(result, original, getCursor().getParentOrThrow()); + return UseInstanceOfPatternMatching.refactor(result, original, getCursor().getParentOrThrow(), introducedPatternVarNames); } return result; } @@ -476,9 +484,10 @@ private boolean isUnboundedWildcardParameterized(TypeTree typeTree) { private static class UseInstanceOfPatternMatching extends JavaVisitor { private final InstanceOfPatternReplacements replacements; + private final Set introducedNames; - static @Nullable J refactor(@Nullable J tree, InstanceOfPatternReplacements replacements, Cursor cursor) { - return new UseInstanceOfPatternMatching(replacements).visit(tree, 0, cursor); + static @Nullable J refactor(@Nullable J tree, InstanceOfPatternReplacements replacements, Cursor cursor, Set introducedNames) { + return new UseInstanceOfPatternMatching(replacements, introducedNames).visit(tree, 0, cursor); } @Override @@ -489,7 +498,7 @@ public J visitBinary(J.Binary binary, Integer p) { Cursor widenedCursor = updateCursor(b); // Collect names from the left side if it's an instanceof with a pattern - Set usedNames = new HashSet<>(); + Set usedNames = new HashSet<>(introducedNames); if (b.getLeft() instanceof J.InstanceOf) { J.InstanceOf leftInstanceOf = (J.InstanceOf) b.getLeft(); if (leftInstanceOf.getPattern() != null) { @@ -499,11 +508,19 @@ public J visitBinary(J.Binary binary, Integer p) { Expression newRight; if (binary.getRight() instanceof J.InstanceOf) { - newRight = replacements.processInstanceOf((J.InstanceOf) binary.getRight(), widenedCursor, usedNames); + J.InstanceOf rightResult = replacements.processInstanceOf((J.InstanceOf) binary.getRight(), widenedCursor, usedNames); + if (rightResult.getPattern() != null && ((J.InstanceOf) binary.getRight()).getPattern() == null) { + introducedNames.add(((J.Identifier) rightResult.getPattern()).getSimpleName()); + } + newRight = rightResult; } 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, usedNames)); + J.InstanceOf rightResult = replacements.processInstanceOf(originalRight.getTree(), widenedCursor, usedNames); + if (rightResult.getPattern() != null && originalRight.getTree().getPattern() == null) { + introducedNames.add(((J.Identifier) rightResult.getPattern()).getSimpleName()); + } + newRight = originalRight.withTree(rightResult); } else { newRight = (Expression) visitNonNull(binary.getRight(), p, widenedCursor); } @@ -516,7 +533,11 @@ 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(), new HashSet<>()); + J.InstanceOf result = replacements.processInstanceOf(instanceOf, getCursor(), new HashSet<>(introducedNames)); + if (result.getPattern() != null && instanceOf.getPattern() == null) { + introducedNames.add(((J.Identifier) result.getPattern()).getSimpleName()); + } + return result; } @Override diff --git a/src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java b/src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java index ad54b6a5c..1b2f8e580 100644 --- a/src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java @@ -1989,4 +1989,48 @@ void foo(Object object) { ) ); } + + @Issue("https://github.com/moderneinc/customer-requests/issues/2202") + @Test + void flowScopedPatternVariableConflictWithElseReturn() { + rewriteRun( + //language=java + java( + """ + class A { + void test(Object objNotOnOrAfter, Object objNotBefore) { + String strNotOnOrAfter = ""; + if (objNotOnOrAfter != null && objNotOnOrAfter instanceof String) { + strNotOnOrAfter = (String) objNotOnOrAfter; + } else { + return; + } + + String strNotBefore = ""; + if (objNotBefore != null && objNotBefore instanceof String) { + strNotBefore = (String) objNotBefore; + } + } + } + """, + """ + class A { + void test(Object objNotOnOrAfter, Object objNotBefore) { + String strNotOnOrAfter = ""; + if (objNotOnOrAfter != null && objNotOnOrAfter instanceof String string) { + strNotOnOrAfter = string; + } else { + return; + } + + String strNotBefore = ""; + if (objNotBefore != null && objNotBefore instanceof String string1) { + strNotBefore = string1; + } + } + } + """ + ) + ); + } } From 591a3ef1659859773e88832582694317bbed97e8 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 11 Apr 2026 14:26:24 +0200 Subject: [PATCH 2/3] Use cursor message instead of visitor field for introduced pattern names Store the set on the enclosing method/class cursor via computeMessageIfAbsent, which naturally scopes it without needing an explicit clear() in visitMethodDeclaration. --- .../staticanalysis/InstanceOfPatternMatch.java | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java b/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java index 9ac2f3a87..920887c51 100644 --- a/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java +++ b/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java @@ -72,20 +72,16 @@ public TreeVisitor getVisitor() { ); return Preconditions.check(preconditions, new JavaVisitor() { - final Set introducedPatternVarNames = new HashSet<>(); - - @Override - public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { - introducedPatternVarNames.clear(); - return (J.MethodDeclaration) super.visitMethodDeclaration(method, ctx); - } - @Override public @Nullable J postVisit(J tree, ExecutionContext ctx) { J result = super.postVisit(tree, ctx); InstanceOfPatternReplacements original = getCursor().getMessage("flowTypeScope"); if (original != null && !original.isEmpty()) { - return UseInstanceOfPatternMatching.refactor(result, original, getCursor().getParentOrThrow(), introducedPatternVarNames); + Cursor methodCursor = getCursor().dropParentUntil( + v -> v instanceof J.MethodDeclaration || v instanceof J.ClassDeclaration); + Set introducedNames = methodCursor.computeMessageIfAbsent( + "introducedPatternVarNames", k -> new HashSet<>()); + return UseInstanceOfPatternMatching.refactor(result, original, getCursor().getParentOrThrow(), introducedNames); } return result; } From 73cee8ab284bbd6c2679e868004abde586831b9a Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 11 Apr 2026 14:46:02 +0200 Subject: [PATCH 3/3] Add ROOT_VALUE guard and extract trackIntroducedName helper Add Cursor.ROOT_VALUE to dropParentUntil predicate for safety, and deduplicate the pattern variable tracking logic into a helper method. --- .../staticanalysis/InstanceOfPatternMatch.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java b/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java index 920887c51..b22ca2ba4 100644 --- a/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java +++ b/src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java @@ -78,7 +78,7 @@ public TreeVisitor getVisitor() { InstanceOfPatternReplacements original = getCursor().getMessage("flowTypeScope"); if (original != null && !original.isEmpty()) { Cursor methodCursor = getCursor().dropParentUntil( - v -> v instanceof J.MethodDeclaration || v instanceof J.ClassDeclaration); + v -> v instanceof J.MethodDeclaration || v instanceof J.ClassDeclaration || v == Cursor.ROOT_VALUE); Set introducedNames = methodCursor.computeMessageIfAbsent( "introducedPatternVarNames", k -> new HashSet<>()); return UseInstanceOfPatternMatching.refactor(result, original, getCursor().getParentOrThrow(), introducedNames); @@ -505,17 +505,13 @@ public J visitBinary(J.Binary binary, Integer p) { Expression newRight; if (binary.getRight() instanceof J.InstanceOf) { J.InstanceOf rightResult = replacements.processInstanceOf((J.InstanceOf) binary.getRight(), widenedCursor, usedNames); - if (rightResult.getPattern() != null && ((J.InstanceOf) binary.getRight()).getPattern() == null) { - introducedNames.add(((J.Identifier) rightResult.getPattern()).getSimpleName()); - } + trackIntroducedName((J.InstanceOf) binary.getRight(), rightResult); newRight = rightResult; } else if (binary.getRight() instanceof J.Parentheses && ((J.Parentheses) binary.getRight()).getTree() instanceof J.InstanceOf) { @SuppressWarnings("unchecked") J.Parentheses originalRight = (J.Parentheses) binary.getRight(); J.InstanceOf rightResult = replacements.processInstanceOf(originalRight.getTree(), widenedCursor, usedNames); - if (rightResult.getPattern() != null && originalRight.getTree().getPattern() == null) { - introducedNames.add(((J.Identifier) rightResult.getPattern()).getSimpleName()); - } + trackIntroducedName(originalRight.getTree(), rightResult); newRight = originalRight.withTree(rightResult); } else { newRight = (Expression) visitNonNull(binary.getRight(), p, widenedCursor); @@ -530,10 +526,14 @@ public J visitBinary(J.Binary binary, Integer p) { public J.InstanceOf visitInstanceOf(J.InstanceOf instanceOf, Integer p) { instanceOf = (J.InstanceOf) super.visitInstanceOf(instanceOf, p); J.InstanceOf result = replacements.processInstanceOf(instanceOf, getCursor(), new HashSet<>(introducedNames)); - if (result.getPattern() != null && instanceOf.getPattern() == null) { + trackIntroducedName(instanceOf, result); + return result; + } + + private void trackIntroducedName(J.InstanceOf original, J.InstanceOf result) { + if (result.getPattern() != null && original.getPattern() == null) { introducedNames.add(((J.Identifier) result.getPattern()).getSimpleName()); } - return result; } @Override