Keep MockUtil declarations used across files and switch labels - #1086
Draft
martinfrancois wants to merge 1 commit into
Draft
Keep MockUtil declarations used across files and switch labels#1086martinfrancois wants to merge 1 commit into
MockUtil declarations used across files and switch labels#1086martinfrancois wants to merge 1 commit into
Conversation
…e-label declaration doNotRemoveFieldUsedInAnotherClass pins the unsound single-file use analysis: a MockUtil field referenced only from another source file is removed, so that file stops compiling. mockUtilsVariableInSwitchCaseToStatic pins the declaration left behind under a case label after its uses are migrated. Both are marked @disabled as known-failing.
4 tasks
MockUtil declarations used across files and switch labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Suggested review order: 50 of 52 (Score: 1)
Review first: openrewrite/rewrite-migrate-java#1197
What's changed?
Adds 2 known-failing tests to
MockUtilsToStaticTestthat reproduce two defects inMockUtilsToStatic: the recipe removes aMockUtilfield that another source file still uses, and it never removes a fully migratedMockUtildeclaration that sits directly under acaselabel. No recipe code changes. The tests are marked@Disabledso the suite stays green; removing the annotation shows the failures. The class had 3 tests on main and has 5 with this change; the 3 pre-existing tests still pass.What's your motivation?
Recipe:
org.openrewrite.java.testing.mockito.MockUtilsToStatic.Case 1: cross-file field use
Before
The recipe decides per file whether a
MockUtilvariable is still needed. It never sees uses from other compilation units. Given these two files in one run:Actual after the recipe
Using current main. the first file becomes an empty class,
public class MockitoMockUtils {\n}, with the field and the import removed. The second file is unchanged and still readsutils.util. Compiling the output pair with javac fails with:After the recipe runs, the code should still compile, so the field should stay when another file uses it. Test:
doNotRemoveFieldUsedInAnotherClass.Expected after the recipe
(unchanged)Case 2: declaration under a case label
Before
Given an old-style switch:
Actual after the recipe
Using current main, the call is migrated to
MockUtil.isMock(value), but the now-unusedMockUtil util = new MockUtil();line stays. TheDeleteStatementscheduled fromvisitNewClassonly removes statements whose parent is aJ.Block; statements aftercase 1:hang off theJ.Caseinstead, so the dead declaration is left behind. Test:mockUtilsVariableInSwitchCaseToStatic.Expected after the recipe
The call MUST be migrated and the unused declaration MUST be removed.
Found while preparing #1084, which rewrites the removal logic in this recipe but deliberately does not fix these two behaviors, as its description discloses.
Anything in particular you'd like reviewers to focus on?
I think defect 1 is a genuine bug, because the recipe produces code that does not compile. Defect 2 is more of an improvement: the output compiles, it just keeps a dead statement. If you agree this should change, I would gladly prepare the fix. If this behavior is intended, feel free to close this and I know it is settled.
Any additional context
Pre-existing tests changed: None.
Related open PR of mine touching the same recipe: #1084. It does not fix either behavior here; its removal logic also handles only declarations whose parent is a block, so the case-label gap persists with it.
This reproduction was prepared with AI assistance (Claude Code). I reviewed the tests and this description.
The added reproduction tests and the existing suite together cover changed and unchanged behavior. The known-failing tests remain disabled until implementation. The formatter run was calibrated per file; untouched lines were not reformatted.
Checklist
./gradlew buildlocally, and committed any resulting changes torecipes.csv