UnwrapElseAfterReturn: only unwrap when else declarations do not collide

[martinfrancois]

Suggested review order: 23 of 52 (Score: 5)
Review first: openrewrite/rewrite-migrate-java#1193

What's changed?

UnwrapElseAfterReturn now checks, before it moves the statements of an else body out into the enclosing block, whether that move changes how a name resolves further down that block. It collects every name the moved statements would add to the enclosing block, then looks at the statements that follow the if as written. If any of those declares one of those names, or uses one of them in a position where a newly visible local variable or local class captures it, the if and its else are left exactly as they are. Otherwise the recipe unwraps the else as it did before. The check is a new private method of the visitor, collidesWithLaterScope.

What's your motivation?

Recipe: org.openrewrite.staticanalysis.UnwrapElseAfterReturn.

Moving an else body into its enclosing block widens the scope of every name declared in that body. This causes two distinct defects: the generated source fails to compile when a later local declaration has the same name, or it compiles with different behavior when the moved local captures a later field or statically imported member reference.

Case 1: later local declaration

Before

This source compiles because the two value variables are in separate scopes:

void plain(boolean stop) {
    if (stop) {
        return;
    } else {
        int value = 1;
        System.out.println(value);
    }
    int value = 2;
    System.out.println(value);
}

Actual after the recipe

The moved declaration and the later declaration now occupy the same scope:

void plain(boolean stop) {
    if (stop) {
        return;
    }
    int value = 1;
    System.out.println(value);
    int value = 2;
    System.out.println(value);
}

error: variable value is already defined in method plain(boolean)

Expected after the recipe

(unchanged)

The recipe turns compiling code into non-compiling code because Java forbids declaring a local variable or local class inside the scope of another local variable or local class with the same name (JLS 6.4).

Case 2: later field reference

Before

The final return value; reads the field:

class Test {
    int value;

    int sameType(boolean stop) {
        if (stop) {
            return -1;
        } else {
            int value = 1;
            System.out.println(value);
        }
        return value;
    }
}

Actual after the recipe

The moved local variable now captures the final reference, so the method returns the local value 1 instead of the field:

class Test {
    int value;

    int sameType(boolean stop) {
        if (stop) {
            return -1;
        }
        int value = 1;
        System.out.println(value);
        return value;
    }
}

Expected after the recipe

(unchanged)

The recipe silently changes the method to return the local value 1 instead of the field.

Before this change, the recipe did not inspect names declared by the moved statements or references after the if. Both plain else blocks and else if chains are affected. Reproduced on 2.40.0 and on current main.

Anything in particular you'd like reviewers to focus on?

No existing test expectation changed; the one line of existing test code this change edits is the class level @SuppressWarnings("ConstantConditions"), widened to @SuppressWarnings({"ConstantConditions", "unused"}), because the new tests declare locals that are never read. Everything else in the test file is added lines, the new tests and the imports they need.

Three limitations:

• The check compares names and does not work out which scope each later name belongs to, so it also blocks some safe moves. For example, a local class declared after the if with a field named like a variable declared in the else body: that field is a member of the local class and cannot be captured by the moved local variable, but the matching name blocks the move anyway.
• Every instanceof pattern variable declared anywhere inside the else body is treated as if its scope reached the end of the enclosing block. Under the flow scoping rules of JLS 6.3.2 that holds for the s in if (!(o instanceof String s)) { return; }, which stays in scope for the rest of the enclosing block, but not for the s in if (o instanceof String s) { ... }, which is in scope only inside that if statement. The check does not tell the two apart, so an else body of { if (o instanceof String s) { ... } } blocks the unwrap when a later statement in the same block declares String s, even though the move would have compiled.
• A comment written before the else keyword is still dropped. That happens on main too.

Have you considered any alternatives or workarounds?

The alternative to leaving the if and its else as written is to remove the else anyway and wrap the moved statements in a bare block, { ... }, placed after the if, which keeps their names out of the enclosing block. That would be a small change inside flatten, the private method that already exists in this recipe and that builds the list of statements replacing the if; it would decide when to add the bare block from the same collidesWithLaterScope check this branch adds. I did not pick it because a bare block adds nesting back, which is the opposite of what the recipe is for, but I will make that change if you prefer it.

Any additional context

This change adds 11 tests to UnwrapElseAfterReturnTest. Without the code change in this pull request, these 7 tests fail:

• doNotUnwrapWhenElseDeclarationCollidesWithLaterLocalVariable
• doNotUnwrapWhenElseDeclarationCollidesWithLaterPatternVariable
• doNotUnwrapWhenElseDeclarationCollidesWithNestedScope
• doNotUnwrapWhenElseDeclarationShadowsNameUsedLater
• doNotUnwrapWhenElseLocalClassCollidesWithLaterLocalClass
• doNotUnwrapWhenEscapedPatternVariableCollidesWithLaterDeclaration
• unwrapOnlyWhenDeconstructionPatternBindingsDoNotCollide

The other 4 tests pass either way. Each covers a safe move that this branch still performs, so the new check does not block it. They do not prove that the check is never too broad; the first 2 limitations above describe where it is.

This change was prepared with AI assistance (Claude Code). I reviewed the code, the tests and this description.

I ran the formatter with the repository's .editorconfig. It also wanted to re-indent lines that this change does not touch, so I left those alone and kept the diff limited to this change.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've run ./gradlew build locally, and committed any resulting changes to recipes.csv
• I've formatted the lines I changed, without reformatting code I didn't touch