Skip to content

UnwrapElseAfterReturn: only unwrap when else declarations do not collide #979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
martinfrancois wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

UnwrapElseAfterReturn: only unwrap when else declarations do not collide #979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19e2484
UnwrapElseAfterReturn: only unwrap when else declarations do not collide
martinfrancois Aug 10, 2026
96450e8
Review fixes: lock in the label-namespace exemption with a test
timtebeek Aug 11, 2026
d3303f3
Trim commentary
timtebeek Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 90 additions & 4 deletions src/main/java/org/openrewrite/staticanalysis/UnwrapElseAfterReturn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -52,10 +58,11 @@ public TreeVisitor<?, ExecutionContext> 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<Statement> 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
Expand All @@ -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);
Expand Down Expand Up @@ -105,6 +114,83 @@ private List<Statement> 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<Statement> laterStatements) {
if (laterStatements.isEmpty()) {
return false;
}
Set<String> hoistedNames = new HashSet<>();
JavaIsoVisitor<Set<String>> patternVariableCollector = new JavaIsoVisitor<Set<String>>() {
@Override
public J.InstanceOf visitInstanceOf(J.InstanceOf instanceOf, Set<String> 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<String> 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<Statement> 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<AtomicBoolean> nameScanner = new JavaIsoVisitor<AtomicBoolean>() {
@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;
Expand Down
Loading
Loading