From baf7b11df427bdce3021916cba19866dbbbe11cc Mon Sep 17 00:00:00 2001 From: Jente Sondervorst Date: Tue, 5 Aug 2025 12:32:23 +0200 Subject: [PATCH 1/4] Fixed the provided failing test Fixes #687 --- .../staticanalysis/MinimumSwitchCases.java | 59 ++++++++-- .../MinimumSwitchCasesTest.java | 104 ++++++++++++++++++ 2 files changed, 156 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java b/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java index c2aa5a339c..abed838b19 100644 --- a/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java +++ b/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java @@ -31,10 +31,7 @@ import org.openrewrite.marker.Markers; import org.openrewrite.staticanalysis.csharp.CSharpFileChecker; -import java.util.ArrayList; -import java.util.List; -import java.util.Set; -import java.util.UUID; +import java.util.*; import static java.util.Collections.singleton; import static java.util.Collections.singletonList; @@ -192,6 +189,31 @@ public J visitSwitch(J.Switch switch_, ExecutionContext ctx) { } } + Map firstCaseDeclarations = new JavaIsoVisitor>() { + @Override + public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, Map variableDeclarations) { + for (J.VariableDeclarations.NamedVariable var : multiVariable.getVariables()) { + if (multiVariable.getVariables().size() == 1) { + variableDeclarations.put(var.getSimpleName(), multiVariable); + } else { + variableDeclarations.put(var.getSimpleName(), multiVariable.withVariables(singletonList(var))); + } + } + return multiVariable; + } + + @Override + public J.Block visitBlock(J.Block block, Map variableDeclarations) { + for (Statement statement : block.getStatements()) { + if (statement instanceof J.VariableDeclarations) { + visitVariableDeclarations((J.VariableDeclarations) statement, variableDeclarations); + } + } + + return block; + } + }.reduce(cases[0], new HashMap<>()); + // move first case to "if" List thenStatements = getStatements(cases[0]); @@ -201,13 +223,37 @@ public J visitSwitch(J.Switch switch_, ExecutionContext ctx) { // move second case to "else" if (cases[1] != null) { assert generatedIf.getElsePart() != null; + JavaVisitor> firstAssignmentAsDeclarationVisitor = new JavaVisitor>() { + + @Override + public J visitStatement(Statement statement, Map stringVariableDeclarationsMap) { + if (statement instanceof J.Assignment) { + return visitAssignment((J.Assignment) statement, stringVariableDeclarationsMap); + } else { + return super.visitStatement(statement, stringVariableDeclarationsMap); + } + } + + @Override + public J visitAssignment(J.Assignment assignment, Map firstCaseDeclarations) { + if (assignment.getVariable() instanceof J.Identifier) { + String varName = ((J.Identifier) assignment.getVariable()).getSimpleName(); + if (firstCaseDeclarations.containsKey(varName)) { + J.VariableDeclarations originalDecl = firstCaseDeclarations.remove(varName); + return originalDecl.withVariables(ListUtils.map(originalDecl.getVariables(), v -> v == null ? null : v.withInitializer(assignment.getAssignment()))); + } + } + return super.visitAssignment(assignment, firstCaseDeclarations); + } + }; + if (isDefault(cases[1])) { generatedIf = generatedIf.withElsePart(generatedIf.getElsePart().withBody(((J.Block) generatedIf.getElsePart().getBody()).withStatements(ListUtils.map(getStatements(cases[1]), - s -> s instanceof J.Break ? null : s)))); + s -> s instanceof J.Break || s == null ? null : (Statement) firstAssignmentAsDeclarationVisitor.visitStatement(s, firstCaseDeclarations))))); } else { J.If elseIf = (J.If) generatedIf.getElsePart().getBody(); generatedIf = generatedIf.withElsePart(generatedIf.getElsePart().withBody(elseIf.withThenPart(((J.Block) elseIf.getThenPart()).withStatements(ListUtils.map(getStatements(cases[1]), - s -> s instanceof J.Break ? null : s))))); + s -> s instanceof J.Break || s == null ? null : (Statement) firstAssignmentAsDeclarationVisitor.visitStatement(s, firstCaseDeclarations)))))); } } @@ -250,7 +296,6 @@ private boolean switchesOnEnum(J.Switch switch_) { return selectorType instanceof JavaType.Class && ((JavaType.Class) selectorType).getKind() == JavaType.Class.Kind.Enum; } - }); } diff --git a/src/test/java/org/openrewrite/staticanalysis/MinimumSwitchCasesTest.java b/src/test/java/org/openrewrite/staticanalysis/MinimumSwitchCasesTest.java index 0a37ff4e6c..acc5808466 100644 --- a/src/test/java/org/openrewrite/staticanalysis/MinimumSwitchCasesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/MinimumSwitchCasesTest.java @@ -887,4 +887,108 @@ void doSomethingElse() {} ) ); } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/687") + @Test + void variableRedeclarationInNewBlocks() { + rewriteRun( + //language=java + java( + """ + class Test { + int someInt; + void test() { + switch (someInt) { + case 1: + String data = getSomeString(); + doThingOneWith(data); + break; + case 2: + data = getSomeOtherString(); + doThingTwoWith(data); + break; + } + } + String getSomeString() { return "one"; } + String getSomeOtherString() { return "two"; } + void doThingOneWith(String data) {} + void doThingTwoWith(String data) {} + } + """, + """ + class Test { + int someInt; + void test() { + if (someInt == 1) { + String data = getSomeString(); + doThingOneWith(data); + } else if (someInt == 2) { + String data = getSomeOtherString(); + doThingTwoWith(data); + } + } + String getSomeString() { return "one"; } + String getSomeOtherString() { return "two"; } + void doThingOneWith(String data) {} + void doThingTwoWith(String data) {} + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/687") + @Test + void MultipleVariableRedeclarationInNewBlocks() { + rewriteRun( + //language=java + java( + """ + class Test { + int someInt; + void test() { + switch (someInt) { + case 1: + String data = getSomeString(), otherString = getSomeString(); + doThingOneWith(data); + doThingOneWith(otherString); + break; + case 2: + data = getSomeOtherString(); + otherString = getSomeOtherString(); + doThingTwoWith(data); + doThingTwoWith(otherString); + break; + } + } + String getSomeString() { return "one"; } + String getSomeOtherString() { return "two"; } + void doThingOneWith(String data) {} + void doThingTwoWith(String data) {} + } + """, + """ + class Test { + int someInt; + void test() { + if (someInt == 1) { + String data = getSomeString(), otherString = getSomeString(); + doThingOneWith(data); + doThingOneWith(otherString); + } else if (someInt == 2) { + String data = getSomeOtherString(); + String otherString = getSomeOtherString(); + doThingTwoWith(data); + doThingTwoWith(otherString); + } + } + String getSomeString() { return "one"; } + String getSomeOtherString() { return "two"; } + void doThingOneWith(String data) {} + void doThingTwoWith(String data) {} + } + """ + ) + ); + } } From 55e7840570c76fc5c1cce2f853f2ce865eee59a3 Mon Sep 17 00:00:00 2001 From: Jente Sondervorst Date: Tue, 5 Aug 2025 12:39:26 +0200 Subject: [PATCH 2/4] Update src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../org/openrewrite/staticanalysis/MinimumSwitchCases.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java b/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java index abed838b19..6da67a360f 100644 --- a/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java +++ b/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java @@ -229,8 +229,7 @@ public J.Block visitBlock(J.Block block, Map var public J visitStatement(Statement statement, Map stringVariableDeclarationsMap) { if (statement instanceof J.Assignment) { return visitAssignment((J.Assignment) statement, stringVariableDeclarationsMap); - } else { - return super.visitStatement(statement, stringVariableDeclarationsMap); + return super.visitStatement(statement, stringVariableDeclarationsMap); } } From ca643b8f0098483f7c2e30bb025715d1274b69a7 Mon Sep 17 00:00:00 2001 From: Jente Sondervorst Date: Tue, 5 Aug 2025 12:41:49 +0200 Subject: [PATCH 3/4] Fixed the provided failing test Fixes #687 --- .../java/org/openrewrite/staticanalysis/MinimumSwitchCases.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java b/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java index 6da67a360f..efe0074103 100644 --- a/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java +++ b/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java @@ -229,8 +229,8 @@ public J.Block visitBlock(J.Block block, Map var public J visitStatement(Statement statement, Map stringVariableDeclarationsMap) { if (statement instanceof J.Assignment) { return visitAssignment((J.Assignment) statement, stringVariableDeclarationsMap); - return super.visitStatement(statement, stringVariableDeclarationsMap); } + return super.visitStatement(statement, stringVariableDeclarationsMap); } @Override From 3269a39431a55adfadac772353296149e01c64ab Mon Sep 17 00:00:00 2001 From: Jente Sondervorst Date: Wed, 6 Aug 2025 10:44:04 +0200 Subject: [PATCH 4/4] Review comments --- .../staticanalysis/MinimumSwitchCases.java | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java b/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java index efe0074103..7c18c0a0fd 100644 --- a/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java +++ b/src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java @@ -192,12 +192,8 @@ public J visitSwitch(J.Switch switch_, ExecutionContext ctx) { Map firstCaseDeclarations = new JavaIsoVisitor>() { @Override public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, Map variableDeclarations) { - for (J.VariableDeclarations.NamedVariable var : multiVariable.getVariables()) { - if (multiVariable.getVariables().size() == 1) { - variableDeclarations.put(var.getSimpleName(), multiVariable); - } else { - variableDeclarations.put(var.getSimpleName(), multiVariable.withVariables(singletonList(var))); - } + for (J.VariableDeclarations.NamedVariable var_ : multiVariable.getVariables()) { + variableDeclarations.put(var_.getSimpleName(), multiVariable.withVariables(ListUtils.filter(multiVariable.getVariables(), v -> v == var_))); } return multiVariable; } @@ -209,7 +205,6 @@ public J.Block visitBlock(J.Block block, Map var visitVariableDeclarations((J.VariableDeclarations) statement, variableDeclarations); } } - return block; } }.reduce(cases[0], new HashMap<>()); @@ -239,7 +234,7 @@ public J visitAssignment(J.Assignment assignment, Map v == null ? null : v.withInitializer(assignment.getAssignment()))); + return originalDecl.withVariables(ListUtils.map(originalDecl.getVariables(), v -> v.withInitializer(assignment.getAssignment()))); } } return super.visitAssignment(assignment, firstCaseDeclarations);