diff --git a/src/main/java/org/openrewrite/staticanalysis/UnwrapElseAfterReturn.java b/src/main/java/org/openrewrite/staticanalysis/UnwrapElseAfterReturn.java index 322819ab7..b1f807ff7 100644 --- a/src/main/java/org/openrewrite/staticanalysis/UnwrapElseAfterReturn.java +++ b/src/main/java/org/openrewrite/staticanalysis/UnwrapElseAfterReturn.java @@ -22,6 +22,7 @@ import org.openrewrite.Repeat; import org.openrewrite.TreeVisitor; import org.openrewrite.internal.ListUtils; +import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.Comment; import org.openrewrite.java.tree.J; @@ -30,9 +31,14 @@ import java.time.Duration; import java.util.Arrays; +import java.util.HashSet; import java.util.List; +import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; +import static java.util.Collections.singletonList; + public class UnwrapElseAfterReturn extends Recipe { @Getter @@ -52,10 +58,11 @@ public TreeVisitor getVisitor() { public J.Block visitBlock(J.Block block, ExecutionContext ctx) { J.Block b = visitAndCast(block, ctx, super::visitBlock); AtomicReference<@Nullable Space> endWhitespace = new AtomicReference<>(null); - J.Block alteredBlock = b.withStatements(ListUtils.flatMap(b.getStatements(), statement -> { + J.Block alteredBlock = b.withStatements(ListUtils.flatMap(b.getStatements(), (index, statement) -> { if (statement instanceof J.If) { J.If ifStatement = (J.If) statement; if (ifStatement.getElsePart() != null && endsWithReturnOrThrow(ifStatement.getThenPart())) { + List laterStatements = b.getStatements().subList(index + 1, b.getStatements().size()); Statement elsePart = ifStatement.getElsePart().getBody(); if (elsePart instanceof J.If) { // Else-if chain: find and unwrap the innermost else @@ -65,11 +72,13 @@ public J.Block visitBlock(J.Block block, ExecutionContext ctx) { endsWithReturnOrThrow(innermost.getThenPart()) && !(innermost.getElsePart().getBody() instanceof J.If)) { // Unwrap the innermost else - J.If modifiedChain = removeInnermostElse(ifStatement); Statement innermostElseBody = innermost.getElsePart().getBody(); - return flatten(innermost, innermostElseBody, endWhitespace, modifiedChain); + if (!collidesWithLaterScope(innermostElseBody, laterStatements)) { + J.If modifiedChain = removeInnermostElse(ifStatement); + return flatten(innermost, innermostElseBody, endWhitespace, modifiedChain); + } } - } else { + } else if (!collidesWithLaterScope(elsePart, laterStatements)) { // Plain else block: unwrap directly J.If newIf = ifStatement.withElsePart(null); return flatten(ifStatement, elsePart, endWhitespace, newIf); @@ -105,6 +114,83 @@ private List flatten(J.If tailIf, Statement tailElse, AtomicReference return Arrays.asList(ifWithoutElse, tailElse.withPrefix(tailIf.getElsePart().getPrefix())); } + /** + * Hoisting the else block widens the scope of the names it declares to the end of the enclosing block, + * so unwrapping is skipped where that could change how a later name resolves: a later redeclaration no + * longer compiles (JLS 6.4), and a later unqualified use of a field or statically imported member would + * be captured instead. Names a local cannot capture, such as invocation names and labels, are exempt. + * Every {@code instanceof} pattern variable counts as hoisted, since flow scoping (JLS 6.3.2) can carry + * one past its own statement; not all of them do, so this errs towards keeping the else block. + */ + private boolean collidesWithLaterScope(Statement elseBody, List laterStatements) { + if (laterStatements.isEmpty()) { + return false; + } + Set hoistedNames = new HashSet<>(); + JavaIsoVisitor> patternVariableCollector = new JavaIsoVisitor>() { + @Override + public J.InstanceOf visitInstanceOf(J.InstanceOf instanceOf, Set names) { + if (instanceOf.getPattern() instanceof J.Identifier) { + names.add(((J.Identifier) instanceOf.getPattern()).getSimpleName()); + } + return super.visitInstanceOf(instanceOf, names); + } + + @Override + public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, Set names) { + // A record deconstruction pattern nests its bindings as variable declarations + if (getCursor().firstEnclosing(J.DeconstructionPattern.class) != null) { + names.add(variable.getSimpleName()); + } + return super.visitVariable(variable, names); + } + }; + List hoistedStatements = elseBody instanceof J.Block ? ((J.Block) elseBody).getStatements() : singletonList(elseBody); + for (Statement hoisted : hoistedStatements) { + if (hoisted instanceof J.VariableDeclarations) { + for (J.VariableDeclarations.NamedVariable variable : ((J.VariableDeclarations) hoisted).getVariables()) { + hoistedNames.add(variable.getSimpleName()); + } + } else if (hoisted instanceof J.ClassDeclaration) { + hoistedNames.add(((J.ClassDeclaration) hoisted).getSimpleName()); + } + patternVariableCollector.visit(hoisted, hoistedNames); + } + if (hoistedNames.isEmpty()) { + return false; + } + + AtomicBoolean collides = new AtomicBoolean(false); + JavaIsoVisitor nameScanner = new JavaIsoVisitor() { + @Override + public J.Identifier visitIdentifier(J.Identifier identifier, AtomicBoolean found) { + if (hoistedNames.contains(identifier.getSimpleName())) { + // Declarations and unqualified uses are both identifiers; only another namespace or a + // qualifier makes one safe + Object parent = getCursor().getParentTreeCursor().getValue(); + boolean unaffected = parent instanceof J.MethodInvocation && identifier == ((J.MethodInvocation) parent).getName() || + parent instanceof J.FieldAccess && identifier == ((J.FieldAccess) parent).getName() || + parent instanceof J.MemberReference && identifier == ((J.MemberReference) parent).getReference() || + parent instanceof J.MethodDeclaration && identifier == ((J.MethodDeclaration) parent).getName() || + parent instanceof J.Label || + parent instanceof J.Break || + parent instanceof J.Continue; + if (!unaffected) { + found.set(true); + } + } + return super.visitIdentifier(identifier, found); + } + }; + for (Statement laterStatement : laterStatements) { + nameScanner.visit(laterStatement, collides); + if (collides.get()) { + return true; + } + } + return false; + } + private J.@Nullable If findInnermostIfWithElse(J.If ifStatement) { if (ifStatement.getElsePart() == null) { return null; diff --git a/src/test/java/org/openrewrite/staticanalysis/UnwrapElseAfterReturnTest.java b/src/test/java/org/openrewrite/staticanalysis/UnwrapElseAfterReturnTest.java index 909d75139..dc5c85aa4 100644 --- a/src/test/java/org/openrewrite/staticanalysis/UnwrapElseAfterReturnTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/UnwrapElseAfterReturnTest.java @@ -16,13 +16,16 @@ package org.openrewrite.staticanalysis; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledForJreRange; +import org.junit.jupiter.api.condition.JRE; import org.openrewrite.DocumentExample; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; import static org.openrewrite.java.Assertions.java; +import static org.openrewrite.java.Assertions.version; -@SuppressWarnings("ConstantConditions") +@SuppressWarnings({"ConstantConditions", "unused"}) class UnwrapElseAfterReturnTest implements RewriteTest { @Override @@ -650,6 +653,551 @@ int foo(boolean condition) { ); } + @Test + void doNotUnwrapWhenElseDeclarationCollidesWithLaterLocalVariable() { + rewriteRun( + //language=java + java( + """ + class Test { + void plain(boolean stop) { + if (stop) { + return; + } else { + int value = 1; + System.out.println(value); + } + int value = 2; + System.out.println(value); + } + + void chain(boolean first, boolean second) { + if (first) { + return; + } else if (second) { + return; + } else { + int value = 1; + System.out.println(value); + } + int value = 2; + System.out.println(value); + } + + void afterThrow(boolean stop) { + if (stop) { + throw new IllegalStateException(); + } else { + String value = "1"; + System.out.println(value); + } + long value = 2; + System.out.println(value); + } + } + """ + ) + ); + } + + @Test + void doNotUnwrapWhenElseDeclarationCollidesWithNestedScope() { + rewriteRun( + //language=java + java( + """ + import java.io.IOException; + import java.io.StringReader; + import java.util.List; + + class Test { + void catchVariable(boolean stop) { + if (stop) { + return; + } else { + int value = 1; + System.out.println(value); + } + try { + System.out.println("try"); + } catch (RuntimeException value) { + System.out.println(value); + } + } + + void resourceVariable(boolean stop) throws IOException { + if (stop) { + return; + } else { + int reader = 1; + System.out.println(reader); + } + try (StringReader reader = new StringReader("")) { + System.out.println(reader.read()); + } + } + + void lambdaParameter(boolean stop, List values) { + if (stop) { + return; + } else { + int element = 1; + System.out.println(element); + } + values.forEach(element -> System.out.println(element)); + } + + void loopVariable(boolean stop) { + if (stop) { + return; + } else { + int index = 1; + System.out.println(index); + } + for (int index = 0; index < 2; index++) { + System.out.println(index); + } + } + } + """ + ) + ); + } + + @Test + void doNotUnwrapWhenElseLocalClassCollidesWithLaterLocalClass() { + rewriteRun( + //language=java + java( + """ + class Test { + void foo(boolean stop) { + if (stop) { + return; + } else { + class Helper { + } + System.out.println(new Helper()); + } + class Helper { + } + System.out.println(new Helper()); + } + } + """ + ) + ); + } + + @Test + void doNotUnwrapWhenElseDeclarationCollidesWithLaterPatternVariable() { + rewriteRun( + version( + //language=java + java( + """ + class Test { + void foo(boolean stop, Object o) { + if (stop) { + return; + } else { + String text = "1"; + System.out.println(text); + } + if (o instanceof String text) { + System.out.println(text); + } + } + } + """ + ), 17 + ) + ); + } + + @Test + void doNotUnwrapWhenEscapedPatternVariableCollidesWithLaterDeclaration() { + rewriteRun( + version( + //language=java + java( + """ + class Test { + void plain(Object o, boolean stop) { + if (stop) { + return; + } else { + if (!(o instanceof String s)) { + return; + } + System.out.println(s); + } + String s = "later"; + System.out.println(s); + } + + void chain(Object o, boolean first, boolean second) { + if (first) { + return; + } else if (second) { + return; + } else { + if (!(o instanceof String s)) { + return; + } + System.out.println(s); + } + String s = "later"; + System.out.println(s); + } + + void singleStatementElse(Object o, boolean stop) { + if (stop) { + return; + } else + while (!(o instanceof String s)) + o = o.toString(); + String s = "later"; + System.out.println(s); + } + } + """ + ), 17 + ) + ); + } + + @Test + void doNotUnwrapWhenElseDeclarationShadowsNameUsedLater() { + rewriteRun( + //language=java + java( + """ + class Test { + int value; + + void plain(boolean stop) { + if (stop) { + return; + } else { + String value = "1"; + System.out.println(value); + } + int doubled = value * 2; + System.out.println(doubled); + } + + void chain(boolean first, boolean second) { + if (first) { + return; + } else if (second) { + return; + } else { + String value = "1"; + System.out.println(value); + } + int doubled = value * 2; + System.out.println(doubled); + } + + int sameType(boolean stop) { + if (stop) { + return -1; + } else { + int value = 1; + System.out.println(value); + } + return value; + } + } + """ + ) + ); + } + + @Test + void unwrapWhenLaterUsesAreNotCapturedByHoistedNames() { + rewriteRun( + //language=java + java( + """ + class Test { + int value; + + int value() { + return 42; + } + + void qualifiedFieldUse(boolean stop) { + if (stop) { + return; + } else { + String value = "1"; + System.out.println(value); + } + System.out.println(this.value); + } + + void methodNameUse(boolean stop) { + if (stop) { + return; + } else { + String value = "1"; + System.out.println(value); + } + System.out.println(value()); + } + + void labelUse(boolean stop) { + if (stop) { + return; + } else { + String value = "1"; + System.out.println(value); + } + value: + for (int i = 0; i < 2; i++) { + if (i == 1) { + break value; + } + continue value; + } + } + } + """, + """ + class Test { + int value; + + int value() { + return 42; + } + + void qualifiedFieldUse(boolean stop) { + if (stop) { + return; + } + String value = "1"; + System.out.println(value); + System.out.println(this.value); + } + + void methodNameUse(boolean stop) { + if (stop) { + return; + } + String value = "1"; + System.out.println(value); + System.out.println(value()); + } + + void labelUse(boolean stop) { + if (stop) { + return; + } + String value = "1"; + System.out.println(value); + value: + for (int i = 0; i < 2; i++) { + if (i == 1) { + break value; + } + continue value; + } + } + } + """ + ) + ); + } + + @Test + void unwrapWhenEscapedPatternVariableDoesNotCollide() { + rewriteRun( + version( + //language=java + java( + """ + class Test { + void foo(Object o, boolean stop) { + if (stop) { + return; + } else { + if (!(o instanceof String s)) { + return; + } + System.out.println(s); + } + System.out.println("done"); + } + } + """, + """ + class Test { + void foo(Object o, boolean stop) { + if (stop) { + return; + } + if (!(o instanceof String s)) { + return; + } + System.out.println(s); + System.out.println("done"); + } + } + """ + ), 17 + ) + ); + } + + @EnabledForJreRange(min = JRE.JAVA_21) + @Test + void unwrapOnlyWhenDeconstructionPatternBindingsDoNotCollide() { + rewriteRun( + version( + //language=java + java( + """ + class Test { + record Point(int x, int y) {} + + void collides(Object o, boolean stop) { + if (stop) { + return; + } else { + if (!(o instanceof Point(int x, int y))) { + return; + } + System.out.println(x + y); + } + int x = 5; + System.out.println(x); + } + + void doesNotCollide(Object o, boolean stop) { + if (stop) { + return; + } else { + if (!(o instanceof Point(int x, int y))) { + return; + } + System.out.println(x + y); + } + Point p = new Point(1, 2); + System.out.println(p); + } + } + """, + """ + class Test { + record Point(int x, int y) {} + + void collides(Object o, boolean stop) { + if (stop) { + return; + } else { + if (!(o instanceof Point(int x, int y))) { + return; + } + System.out.println(x + y); + } + int x = 5; + System.out.println(x); + } + + void doesNotCollide(Object o, boolean stop) { + if (stop) { + return; + } + if (!(o instanceof Point(int x, int y))) { + return; + } + System.out.println(x + y); + Point p = new Point(1, 2); + System.out.println(p); + } + } + """ + ), 21 + ) + ); + } + + @Test + void unwrapWhenElseDeclarationsDoNotCollide() { + rewriteRun( + //language=java + java( + """ + class Test { + void foo(boolean stop) { + if (stop) { + return; + } else { + int value = 1; + System.out.println(value); + } + int other = 2; + System.out.println(other); + } + } + """, + """ + class Test { + void foo(boolean stop) { + if (stop) { + return; + } + int value = 1; + System.out.println(value); + int other = 2; + System.out.println(other); + } + } + """ + ) + ); + } + + @Test + void unwrapWhenCollidingDeclarationRemainsNestedInTheElseBlock() { + rewriteRun( + //language=java + java( + """ + class Test { + void foo(boolean stop) { + if (stop) { + return; + } else { + for (int value = 0; value < 2; value++) { + System.out.println(value); + } + } + int value = 2; + System.out.println(value); + } + } + """, + """ + class Test { + void foo(boolean stop) { + if (stop) { + return; + } + for (int value = 0; value < 2; value++) { + System.out.println(value); + } + int value = 2; + System.out.println(value); + } + } + """ + ) + ); + } + @Test void commentsOnlyInBlocksWithNewLine() { rewriteRun(