Only remove MockUtil declarations whose uses are all migrated

[martinfrancois]

Suggested review order: 48 of 52 (Score: 1)
Review first: #1088

What's changed?

MockUtilsToStatic now removes a MockUtil declaration only when every use of that variable in the same file is one the migration rewrites away: the variable is the receiver of a call to a MockUtil method (util.isMock(x), this.util.isMock(x)) or the qualifier of a method reference to one (util::isMock, this.util::isMock). Those uses become MockUtil.isMock(x) and MockUtil::isMock and no longer need the instance. Any other use of the variable anywhere in the file keeps the declaration.

The rule is applied per declared variable, so in MockUtil util = new MockUtil(), observed = createObserved(); only util is dropped, observed keeps its initializer and createObserved() is still evaluated; the statement itself goes only when no declared variable is left in it. The MockUtil import is removed only when nothing in the file still needs it. For example, the import remains when the recipe's output names the type in MockUtil.isMock(x).

Before

import org.mockito.internal.util.MockUtil;

class Test {
    boolean test(Object value) {
        MockUtil util = new MockUtil();
        observe(util);
        return util.isMock(value);
    }

    void observe(Object value) {
    }
}

Actual after the recipe

Using main today. The declaration of util has been deleted, so the observe(util) line no longer compiles:

import org.mockito.internal.util.MockUtil;

class Test {
    boolean test(Object value) {
        observe(util);
        return MockUtil.isMock(value);
    }

    void observe(Object value) {
    }
}

Expected after the recipe

The declaration stays, because observe(util) is a use that the migration does not rewrite away:

import org.mockito.internal.util.MockUtil;

class Test {
    boolean test(Object value) {
        MockUtil util = new MockUtil();
        observe(util);
        return MockUtil.isMock(value);
    }

    void observe(Object value) {
    }
}

Rewriting the calls themselves is still left to ChangeMethodTargetToStatic, exactly as before; MockUtilsToStatic only decides which declarations to remove. The two cannot disagree about which calls count as rewritten: both are driven by the same method pattern, "org.mockito.internal.util.MockUtil *(..)", held in a single constant that the recipe passes to ChangeMethodTargetToStatic and also uses to build its own MethodMatcher.

The removal has also moved. The visitNewClass override that handed the enclosing statement to DeleteStatement is deleted, and with it the last use and the import of DeleteStatement; a new visitVariableDeclarations override can drop a single declared variable out of a statement that declares several, preserving the spacing that the removed declarator carried around the comma and the semicolon.

What's your motivation?

Recipe: org.openrewrite.java.testing.mockito.MockUtilsToStatic.

Main deletes the whole declaration statement as soon as it sees new MockUtil() as an initializer, without checking whether the variable is still used anywhere, so the output above does not compile. With several declarators more than a name is lost: given MockUtil util = new MockUtil(), observed = createObserved(); the whole statement disappears, so createObserved() is never called. That output reaches users through the Mockito1to3Migration composite in mockito.yml as well as through a direct run of the recipe. Reproduced on 3.43.0 and on 3.44.0-SNAPSHOT built from main at 96ec8d6.

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

No existing test expectation changed: the three existing tests are untouched.

Three limits are worth knowing:

• The analysis reads one file at a time, so it cannot see uses of a field from other source files. A field whose only uses are elsewhere is therefore removed even though those uses stop compiling. That is unsound, and this change does not fix it: main removes such a field too, so the outcome there is unchanged rather than correct.
• A declaration statement whose parent is not a block is never removed as a whole. Statements written after case 1: in an old style switch are where this shows up: they hang off the case, not off a block, so a MockUtil declaration there stays even when every use of the variable is rewritten away, as it does on main. Where such a statement declares more than one variable, the MockUtil variable is still dropped from it.
• This one is new with this change, and it is the price of the fix. Where a use is not rewritten away, the declaration stays and so does its new MockUtil() initializer, which does not compile against Mockito 2 and later, where that constructor is private, and Mockito1to3Migration bumps Mockito to 3.x in the same run. Main deletes the declaration and with it that call, so this change trades one declaration to fix by hand for a name that was undefined at every use site.

Have you considered any alternatives or workarounds?

The analysis of uses lives in a private static nested class of this recipe's visitor, FindUnmigratedUses. The only thing it reads from this repository is the MethodMatcher constant of the enclosing visitor. Moving it as a shared helper into the rewrite-java module of openrewrite/rewrite requires making that matcher a constructor parameter, opening a separate pull request there first, and rebasing this change onto the helper. Say so in review and I will open it; otherwise FindUnmigratedUses stays where it is.

Any additional context

This change adds 17 test methods to MockUtilsToStaticTest. The parameterized removeOnlyTheMigratedDeclarator method supplies 5 declaration layouts, so the focused class reports 24 executions: 21 added executions plus the 3 existing tests. Without the code change, 13 added executions fail. Four check that a declaration is kept when some use of the variable is not rewritten away. The parameterized cases cover removal of the first or last declarator and preserve comma and line-break formatting. The remaining cases guard against keeping a declaration because the name util also appears as a package segment, enum constant, annotation attribute, nested type or type parameter.

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

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