From cb245bcfbed2bd0712e19c07fd4d87fd73a899c7 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Mon, 10 Aug 2026 16:02:28 +0200 Subject: [PATCH 1/4] MockUtilsToStatic: only remove declarations whose uses are all migrated MockUtilsToStatic scheduled DeleteStatement for the whole enclosing J.VariableDeclarations as soon as it found new MockUtil() as an initializer, without analysing how the declared variable is used. Uses that ChangeMethodTargetToStatic does not rewrite, such as arguments, returns, aliases and comparisons, were left referring to a name that no longer exists, and deleting the statement also dropped sibling declarators together with the evaluation of their initializers, so the recipe emitted source that does not compile. The declaration is now analysed before anything is removed: a declarator is obsolete only when every reference to it in the compilation unit is the receiver of a MockUtil call that becomes static, written bare or through this, as an invocation or as a method reference. Only that declarator is removed, so siblings keep their type, order and initializer evaluation. A declaration without complete symbol attribution is left alone, and the MockUtil import is removed with the last declaration. Two limits worth noting. The analysis covers a single compilation unit, so uses of a visible field from another source file are not considered. Side effects in a call qualifier are still lost inside ChangeMethodTargetToStatic; that is a separate defect and is not addressed here. No existing test expectation changed. --- .../testing/mockito/MockUtilsToStatic.java | 185 ++++- .../mockito/MockUtilsToStaticTest.java | 759 ++++++++++++++++++ 2 files changed, 928 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java b/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java index c09dad648..9be63c7e9 100644 --- a/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java +++ b/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java @@ -16,13 +16,23 @@ package org.openrewrite.java.testing.mockito; import lombok.Getter; +import lombok.RequiredArgsConstructor; +import org.jspecify.annotations.Nullable; import org.openrewrite.*; +import org.openrewrite.internal.ListUtils; import org.openrewrite.java.ChangeMethodTargetToStatic; -import org.openrewrite.java.DeleteStatement; +import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.UsesType; +import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JRightPadded; +import org.openrewrite.java.tree.JavaType; +import org.openrewrite.java.tree.Space; + +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; /** * In Mockito 1 you use a code snippet like: @@ -50,8 +60,11 @@ public TreeVisitor getVisitor() { } public static class MockUtilsToStaticVisitor extends JavaVisitor { - private static final MethodMatcher METHOD_MATCHER = new MethodMatcher("org.mockito.internal.util.MockUtil ()"); - private final ChangeMethodTargetToStatic changeMethodTargetToStatic = new ChangeMethodTargetToStatic("org.mockito.internal.util.MockUtil *(..)", "org.mockito.internal.util.MockUtil", null, null, false); + private static final String MOCK_UTIL = "org.mockito.internal.util.MockUtil"; + private static final String MOCK_UTIL_METHODS = MOCK_UTIL + " *(..)"; + private static final MethodMatcher METHOD_MATCHER = new MethodMatcher(MOCK_UTIL + " ()"); + private static final MethodMatcher MIGRATED_METHOD_MATCHER = new MethodMatcher(MOCK_UTIL_METHODS); + private final ChangeMethodTargetToStatic changeMethodTargetToStatic = new ChangeMethodTargetToStatic(MOCK_UTIL_METHODS, MOCK_UTIL, null, null, false); @Override public J visitCompilationUnit(J.CompilationUnit compilationUnit, ExecutionContext ctx) { @@ -60,21 +73,161 @@ public J visitCompilationUnit(J.CompilationUnit compilationUnit, ExecutionContex } @Override - public J visitNewClass(J.NewClass newClass, ExecutionContext ctx) { - if (METHOD_MATCHER.matches(newClass)) { - // Check to see if the new MockUtil() is being assigned to a variable or field, like - // MockUtil util = new MockUtil(); - // If it is, then we'll get rid of it - - Cursor parent = getCursor().dropParentUntil(J.class::isInstance); - if (parent.getValue() instanceof J.VariableDeclarations.NamedVariable) { - Object namedVar = parent.dropParentUntil(J.class::isInstance).getValue(); - if (namedVar instanceof J.VariableDeclarations) { - doAfterVisit(new DeleteStatement<>((J.VariableDeclarations) namedVar)); - } + public J visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { + J.VariableDeclarations vd = (J.VariableDeclarations) super.visitVariableDeclarations(multiVariable, ctx); + // Declarations of `new MockUtil()` are only obsolete once every use of the declared variable in this + // compilation unit is the receiver of a call that is migrated to its static form; every other use would + // be left undefined. Uses of a visible field from another source file are not analysed. + J.CompilationUnit scope = getCursor().firstEnclosing(J.CompilationUnit.class); + if (scope == null) { + return vd; + } + + List> variables = + ListUtils.map(vd.getPadding().getVariables(), v -> isObsoleteMockUtilInstance(v.getElement(), scope) ? null : v); + if (variables.size() == vd.getVariables().size()) { + return vd; + } + if (variables.isEmpty()) { + if (getCursor().getParentTreeCursor().getValue() instanceof J.Block) { + maybeRemoveImport(MOCK_UTIL); + //noinspection DataFlowIssue + return null; + } + return vd; + } + if (vd.getVariables().get(0) != variables.get(0).getElement()) { + // Removing the first declarator leaves the next one to carry the separation from the type expression + variables = ListUtils.mapFirst(variables, v -> v.getElement().getPrefix().isEmpty() ? + v.withElement(v.getElement().withPrefix(Space.SINGLE_SPACE)) : v); + } + return vd.getPadding().withVariables(variables); + } + + private static boolean isObsoleteMockUtilInstance(J.VariableDeclarations.NamedVariable variable, J.CompilationUnit scope) { + if (!(variable.getInitializer() instanceof J.NewClass) || !METHOD_MATCHER.matches((J.NewClass) variable.getInitializer())) { + return false; + } + JavaType.Variable variableType = variable.getVariableType(); + // Without symbol attribution the uses of the variable can not be proven obsolete + return variableType != null && + !new FindUnmigratedUses(variableType, variable.getSimpleName()).reduce(scope, new AtomicBoolean()).get(); + } + + @RequiredArgsConstructor + private static class FindUnmigratedUses extends JavaIsoVisitor { + private final JavaType.Variable variableType; + private final String name; + + @Override + public @Nullable J visit(@Nullable Tree tree, AtomicBoolean found) { + return found.get() ? (J) tree : super.visit(tree, found); + } + + @Override + public J.Package visitPackage(J.Package pkg, AtomicBoolean found) { + // Package and import name segments are not uses of the variable + return pkg; + } + + @Override + public J.Import visitImport(J.Import anImport, AtomicBoolean found) { + return anImport; + } + + @Override + public J.Identifier visitIdentifier(J.Identifier identifier, AtomicBoolean found) { + if (!name.equals(identifier.getSimpleName()) || + identifier.getFieldType() != null && !variableType.equals(identifier.getFieldType())) { + return identifier; + } + Cursor parent = getCursor().getParentTreeCursor(); + if (!isNeverVariableReference(identifier, parent) && !isMigratedUse(identifier, parent)) { + found.set(true); + } + return identifier; + } + + /** + * An identifier in these positions declares a variable or names a method, constructor, type, + * label, enum constant, or annotation element, or resolved to a type or a package or type + * segment of a qualified name, so it is never a reference to the variable under analysis. + */ + private static boolean isNeverVariableReference(J.Identifier identifier, Cursor parentCursor) { + if (identifier.getFieldType() == null && + (identifier.getType() instanceof JavaType.Class || identifier.getType() instanceof JavaType.GenericTypeVariable)) { + // Resolved to a type: a same-named class or type variable used as a static receiver or in a type position + return true; + } + Object parent = parentCursor.getValue(); + if (parent instanceof J.VariableDeclarations.NamedVariable) { + return ((J.VariableDeclarations.NamedVariable) parent).getName() == identifier; + } + if (parent instanceof J.MethodInvocation) { + return ((J.MethodInvocation) parent).getName() == identifier; + } + if (parent instanceof J.MethodDeclaration) { + return ((J.MethodDeclaration) parent).getName() == identifier; + } + if (parent instanceof J.ClassDeclaration) { + return ((J.ClassDeclaration) parent).getName() == identifier; + } + if (parent instanceof J.MemberReference) { + return ((J.MemberReference) parent).getReference() == identifier; + } + if (parent instanceof J.Label) { + return ((J.Label) parent).getLabel() == identifier; + } + if (parent instanceof J.Break) { + return ((J.Break) parent).getLabel() == identifier; + } + if (parent instanceof J.Continue) { + return ((J.Continue) parent).getLabel() == identifier; + } + if (parent instanceof J.EnumValue) { + return ((J.EnumValue) parent).getName() == identifier; + } + if (parent instanceof J.TypeParameter) { + return ((J.TypeParameter) parent).getName() == identifier; + } + if (parent instanceof J.Assignment) { + // The left side of an annotation element assignment names the element, not a variable + return ((J.Assignment) parent).getVariable() == identifier && + parentCursor.getParentTreeCursor().getValue() instanceof J.Annotation; + } + if (parent instanceof J.FieldAccess) { + // Package and type segments of a qualified name carry no field type; a reference to + // the analysed variable always does, because its declaration is attributed + return identifier.getFieldType() == null; + } + return false; + } + + /** + * True when the identifier is the receiver of a call that `ChangeMethodTargetToStatic` rewrites + * to its static form, either bare (`util.isMock(..)`, `util::isMock`) or as the final name of a + * field access receiver (`this.util.isMock(..)`, `this.util::isMock`). The rewrite replaces the + * whole receiver with the class name, so such a use no longer needs the instance. + */ + private static boolean isMigratedUse(J.Identifier identifier, Cursor parent) { + Object parentValue = parent.getValue(); + if (parentValue instanceof J.FieldAccess && ((J.FieldAccess) parentValue).getName() == identifier) { + return isMigratedReceiver((J.FieldAccess) parentValue, parent.getParentTreeCursor().getValue()); + } + return isMigratedReceiver(identifier, parentValue); + } + + private static boolean isMigratedReceiver(Expression receiver, Object parent) { + if (parent instanceof J.MethodInvocation) { + J.MethodInvocation method = (J.MethodInvocation) parent; + return method.getSelect() == receiver && MIGRATED_METHOD_MATCHER.matches(method); + } + if (parent instanceof J.MemberReference) { + J.MemberReference reference = (J.MemberReference) parent; + return reference.getContaining() == receiver && MIGRATED_METHOD_MATCHER.matches(reference); } + return false; } - return super.visitNewClass(newClass, ctx); } } } diff --git a/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java b/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java index dbe746cc3..20acec887 100644 --- a/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java +++ b/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java @@ -129,4 +129,763 @@ public void isMockExample() { ) ); } + + @Test + void retainLocalVariableUsedAsMethodArgument() { + //language=java + rewriteRun( + java( + """ + 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) { + } + } + """, + """ + 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) { + } + } + """ + ) + ); + } + + @Test + void retainFieldUsedAsReturnValue() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + private MockUtil util = new MockUtil(); + + MockUtil expose() { + return util; + } + + boolean test(Object value) { + return util.isMock(value); + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + private MockUtil util = new MockUtil(); + + MockUtil expose() { + return util; + } + + boolean test(Object value) { + return MockUtil.isMock(value); + } + } + """ + ) + ); + } + + @Test + void retainStaticFieldUsedInComparison() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + private static MockUtil util = new MockUtil(); + + static boolean isShared(MockUtil other) { + return other == util; + } + + static boolean test(Object value) { + return util.isMock(value); + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + private static MockUtil util = new MockUtil(); + + static boolean isShared(MockUtil other) { + return other == util; + } + + static boolean test(Object value) { + return MockUtil.isMock(value); + } + } + """ + ) + ); + } + + @Test + void retainVariableUsedToInitializeAnAlias() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil util = new MockUtil(); + MockUtil alias = util; + return alias.isMock(value); + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil util = new MockUtil(); + MockUtil alias = util; + return MockUtil.isMock(value); + } + } + """ + ) + ); + } + + @Test + void removeFirstDeclaratorOnlyPreservingSiblingEvaluation() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil util = new MockUtil(), observed = createObserved(); + observe(observed); + return util.isMock(value); + } + + MockUtil createObserved() { + return new MockUtil(); + } + + void observe(Object value) { + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil observed = createObserved(); + observe(observed); + return MockUtil.isMock(value); + } + + MockUtil createObserved() { + return new MockUtil(); + } + + void observe(Object value) { + } + } + """ + ) + ); + } + + @Test + void removeLastDeclaratorOnlyPreservingSiblingEvaluation() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil observed = createObserved(), util = new MockUtil(); + observe(observed); + return util.isMock(value); + } + + MockUtil createObserved() { + return new MockUtil(); + } + + void observe(Object value) { + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil observed = createObserved(); + observe(observed); + return MockUtil.isMock(value); + } + + MockUtil createObserved() { + return new MockUtil(); + } + + void observe(Object value) { + } + } + """ + ) + ); + } + + @Test + void removeFirstDeclaratorWithoutSpaceAfterComma() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil util = new MockUtil(),observed = createObserved(); + observe(observed); + return util.isMock(value); + } + + MockUtil createObserved() { + return new MockUtil(); + } + + void observe(Object value) { + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil observed = createObserved(); + observe(observed); + return MockUtil.isMock(value); + } + + MockUtil createObserved() { + return new MockUtil(); + } + + void observe(Object value) { + } + } + """ + ) + ); + } + + @Test + void removeFieldWhoseUsesAreMigratedThroughThisReceiver() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + private MockUtil util = new MockUtil(); + private MockUtil kept = new MockUtil(); + + MockUtil expose() { + return kept; + } + + boolean test(Object value) { + return this.util.isMock(value); + } + + boolean keptTest(Object value) { + return kept.isMock(value); + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + private MockUtil kept = new MockUtil(); + + MockUtil expose() { + return kept; + } + + boolean test(Object value) { + return MockUtil.isMock(value); + } + + boolean keptTest(Object value) { + return MockUtil.isMock(value); + } + } + """ + ) + ); + } + + @Test + void removeVariableWhoseNameCollidesWithMethodDeclarationTypeDeclarationAndLabelNames() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + static class util { + } + + boolean test(Object value) { + MockUtil util = new MockUtil(); + MockUtil kept = new MockUtil(); + observe(kept); + boolean result = util.isMock(value) && kept.isMock(value); + util(); + util: + while (result) { + break util; + } + return result; + } + + void util() { + } + + void observe(Object value) { + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + static class util { + } + + boolean test(Object value) { + MockUtil kept = new MockUtil(); + observe(kept); + boolean result = MockUtil.isMock(value) && MockUtil.isMock(value); + util(); + util: + while (result) { + break util; + } + return result; + } + + void util() { + } + + void observe(Object value) { + } + } + """ + ) + ); + } + + @Test + void removeVariableWhoseUseIsAMigratedMethodReference() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + import java.util.function.Predicate; + + class Test { + boolean test(Object value) { + MockUtil util = new MockUtil(); + MockUtil kept = new MockUtil(); + observe(kept); + Predicate isMock = util::isMock; + return isMock.test(value) && kept.isMock(value); + } + + void observe(Object value) { + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + import java.util.function.Predicate; + + class Test { + boolean test(Object value) { + MockUtil kept = new MockUtil(); + observe(kept); + Predicate isMock = MockUtil::isMock; + return isMock.test(value) && MockUtil.isMock(value); + } + + void observe(Object value) { + } + } + """ + ) + ); + } + + @Test + void removeFieldWhoseUseIsAMigratedMethodReferenceThroughThisReceiver() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + import java.util.function.Predicate; + + class Test { + private MockUtil util = new MockUtil(); + private MockUtil kept = new MockUtil(); + + MockUtil expose() { + return kept; + } + + Predicate checker() { + return this.util::isMock; + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + import java.util.function.Predicate; + + class Test { + private MockUtil kept = new MockUtil(); + + MockUtil expose() { + return kept; + } + + Predicate checker() { + return MockUtil::isMock; + } + } + """ + ) + ); + } + + @Test + void removeDeclarationDespiteSameNamedPackageSegment() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil util = new MockUtil(); + java.util.Date date = new java.util.Date(); + return util.isMock(value) && date.getTime() >= 0L; + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + java.util.Date date = new java.util.Date(); + return MockUtil.isMock(value) && date.getTime() >= 0L; + } + } + """ + ) + ); + } + + @Test + void removeFullyQualifiedDeclarationDespiteSameNamedPackageSegment() { + //language=java + rewriteRun( + java( + """ + class Test { + boolean test(Object value) { + org.mockito.internal.util.MockUtil util = new org.mockito.internal.util.MockUtil(); + return util.isMock(value); + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + return MockUtil.isMock(value); + } + } + """ + ) + ); + } + + @Test + void removeDeclarationDespiteSameNamedEnumConstant() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + enum Kind { util, other } + + boolean test(Object value) { + MockUtil util = new MockUtil(); + return util.isMock(value) && Kind.util != Kind.other; + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + enum Kind { util, other } + + boolean test(Object value) { + return MockUtil.isMock(value) && Kind.util != Kind.other; + } + } + """ + ) + ); + } + + @Test + void removeDeclarationDespiteSameNamedAnnotationAttribute() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + @interface Marker { + String util(); + } + + @Marker(util = "x") + boolean test(Object value) { + MockUtil util = new MockUtil(); + return util.isMock(value); + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + @interface Marker { + String util(); + } + + @Marker(util = "x") + boolean test(Object value) { + return MockUtil.isMock(value); + } + } + """ + ) + ); + } + + @Test + void removeDeclarationDespiteSameNamedNestedTypeUsedAsStaticReceiver() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + static class util { + static boolean flag() { + return true; + } + } + + boolean other() { + return util.flag(); + } + + boolean test(Object value) { + MockUtil util = new MockUtil(); + return util.isMock(value); + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + static class util { + static boolean flag() { + return true; + } + } + + boolean other() { + return util.flag(); + } + + boolean test(Object value) { + return MockUtil.isMock(value); + } + } + """ + ) + ); + } + + @Test + void removeDeclarationDespiteSameNamedTypeParameter() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + import java.util.List; + + class Test { + util pick(List items) { + return items.get(0); + } + + boolean test(Object value) { + MockUtil util = new MockUtil(); + return util.isMock(value); + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + import java.util.List; + + class Test { + util pick(List items) { + return items.get(0); + } + + boolean test(Object value) { + return MockUtil.isMock(value); + } + } + """ + ) + ); + } + + @Test + void removeImportWithLastDeclaration() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + void test() { + MockUtil util = new MockUtil(); + } + } + """, + """ + class Test { + void test() { + } + } + """ + ) + ); + } + + @Test + void doNotRemoveDeclarationsOfSimilarlyNamedTypes() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + static class FakeUtil { + boolean isMock(Object value) { + return true; + } + } + + boolean test(Object value) { + FakeUtil fake = new FakeUtil(); + MockUtil util = new MockUtil(); + return fake.isMock(value) && util.isMock(value); + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + static class FakeUtil { + boolean isMock(Object value) { + return true; + } + } + + boolean test(Object value) { + FakeUtil fake = new FakeUtil(); + return fake.isMock(value) && MockUtil.isMock(value); + } + } + """ + ) + ); + } } From 1cc8df2c184e8dd46f2cf3db69f10792d903d9e8 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 09:21:35 +0200 Subject: [PATCH 2/4] Preserve declarator spacing when removing the first or last declarator --- .../testing/mockito/MockUtilsToStatic.java | 38 ++++---- .../mockito/MockUtilsToStaticTest.java | 91 +++++++++++++++++++ 2 files changed, 111 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java b/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java index 9be63c7e9..d69a7d347 100644 --- a/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java +++ b/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java @@ -75,17 +75,16 @@ public J visitCompilationUnit(J.CompilationUnit compilationUnit, ExecutionContex @Override public J visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { J.VariableDeclarations vd = (J.VariableDeclarations) super.visitVariableDeclarations(multiVariable, ctx); - // Declarations of `new MockUtil()` are only obsolete once every use of the declared variable in this - // compilation unit is the receiver of a call that is migrated to its static form; every other use would - // be left undefined. Uses of a visible field from another source file are not analysed. + // Uses of a visible field from another source file are not analysed J.CompilationUnit scope = getCursor().firstEnclosing(J.CompilationUnit.class); if (scope == null) { return vd; } + List> original = vd.getPadding().getVariables(); List> variables = - ListUtils.map(vd.getPadding().getVariables(), v -> isObsoleteMockUtilInstance(v.getElement(), scope) ? null : v); - if (variables.size() == vd.getVariables().size()) { + ListUtils.map(original, v -> isObsoleteMockUtilInstance(v.getElement(), scope) ? null : v); + if (variables.size() == original.size()) { return vd; } if (variables.isEmpty()) { @@ -96,11 +95,16 @@ public J visitVariableDeclarations(J.VariableDeclarations multiVariable, Executi } return vd; } - if (vd.getVariables().get(0) != variables.get(0).getElement()) { - // Removing the first declarator leaves the next one to carry the separation from the type expression - variables = ListUtils.mapFirst(variables, v -> v.getElement().getPrefix().isEmpty() ? + if (original.get(0) != variables.get(0)) { + // The next declarator now carries the separation from the type expression + variables = ListUtils.mapFirst(variables, v -> v.getElement().getPrefix().getComments().isEmpty() ? v.withElement(v.getElement().withPrefix(Space.SINGLE_SPACE)) : v); } + JRightPadded last = original.get(original.size() - 1); + if (variables.get(variables.size() - 1) != last) { + // The previous declarator now carries the separation from the semicolon + variables = ListUtils.mapLast(variables, v -> v.withAfter(last.getAfter())); + } return vd.getPadding().withVariables(variables); } @@ -149,14 +153,13 @@ public J.Identifier visitIdentifier(J.Identifier identifier, AtomicBoolean found } /** - * An identifier in these positions declares a variable or names a method, constructor, type, - * label, enum constant, or annotation element, or resolved to a type or a package or type - * segment of a qualified name, so it is never a reference to the variable under analysis. + * @return whether the identifier declares a variable or names a method, type, label, enum constant or + * annotation element, or is a package or type segment of a qualified name, so that it can never be a + * reference to the variable under analysis. */ private static boolean isNeverVariableReference(J.Identifier identifier, Cursor parentCursor) { if (identifier.getFieldType() == null && (identifier.getType() instanceof JavaType.Class || identifier.getType() instanceof JavaType.GenericTypeVariable)) { - // Resolved to a type: a same-named class or type variable used as a static receiver or in a type position return true; } Object parent = parentCursor.getValue(); @@ -196,18 +199,17 @@ private static boolean isNeverVariableReference(J.Identifier identifier, Cursor parentCursor.getParentTreeCursor().getValue() instanceof J.Annotation; } if (parent instanceof J.FieldAccess) { - // Package and type segments of a qualified name carry no field type; a reference to - // the analysed variable always does, because its declaration is attributed + // Package and type segments of a qualified name carry no field type, unlike an attributed variable return identifier.getFieldType() == null; } return false; } /** - * True when the identifier is the receiver of a call that `ChangeMethodTargetToStatic` rewrites - * to its static form, either bare (`util.isMock(..)`, `util::isMock`) or as the final name of a - * field access receiver (`this.util.isMock(..)`, `this.util::isMock`). The rewrite replaces the - * whole receiver with the class name, so such a use no longer needs the instance. + * @return whether the identifier is the receiver of a call that {@link ChangeMethodTargetToStatic} + * rewrites to its static form, either bare (`util.isMock(..)`, `util::isMock`) or as the final name of + * a field access receiver (`this.util.isMock(..)`); the rewrite replaces the whole receiver with the + * class name, so such a use no longer needs the instance. */ private static boolean isMigratedUse(J.Identifier identifier, Cursor parent) { Object parentValue = parent.getValue(); diff --git a/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java b/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java index 20acec887..b3db44d11 100644 --- a/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java +++ b/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java @@ -366,6 +366,97 @@ void observe(Object value) { ); } + @Test + void removeFirstDeclaratorSpanningMultipleLines() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil util = new MockUtil(), + observed = createObserved(); + observe(observed); + return util.isMock(value); + } + + MockUtil createObserved() { + return new MockUtil(); + } + + void observe(Object value) { + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil observed = createObserved(); + observe(observed); + return MockUtil.isMock(value); + } + + MockUtil createObserved() { + return new MockUtil(); + } + + void observe(Object value) { + } + } + """ + ) + ); + } + + @Test + void removeLastDeclaratorWithSpaceBeforeComma() { + //language=java + rewriteRun( + java( + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil observed = createObserved() , util = new MockUtil(); + observe(observed); + return util.isMock(value); + } + + MockUtil createObserved() { + return new MockUtil(); + } + + void observe(Object value) { + } + } + """, + """ + import org.mockito.internal.util.MockUtil; + + class Test { + boolean test(Object value) { + MockUtil observed = createObserved(); + observe(observed); + return MockUtil.isMock(value); + } + + MockUtil createObserved() { + return new MockUtil(); + } + + void observe(Object value) { + } + } + """ + ) + ); + } + @Test void removeFirstDeclaratorWithoutSpaceAfterComma() { //language=java From 4f0f93e7aaed100cfa11e0685410a83bfdbc8a60 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 12 Aug 2026 00:35:13 +0200 Subject: [PATCH 3/4] Trim commentary --- .../java/testing/mockito/MockUtilsToStatic.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java b/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java index d69a7d347..91f14720b 100644 --- a/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java +++ b/src/main/java/org/openrewrite/java/testing/mockito/MockUtilsToStatic.java @@ -113,7 +113,7 @@ private static boolean isObsoleteMockUtilInstance(J.VariableDeclarations.NamedVa return false; } JavaType.Variable variableType = variable.getVariableType(); - // Without symbol attribution the uses of the variable can not be proven obsolete + // Without symbol attribution the uses cannot be proven obsolete return variableType != null && !new FindUnmigratedUses(variableType, variable.getSimpleName()).reduce(scope, new AtomicBoolean()).get(); } @@ -153,9 +153,8 @@ public J.Identifier visitIdentifier(J.Identifier identifier, AtomicBoolean found } /** - * @return whether the identifier declares a variable or names a method, type, label, enum constant or - * annotation element, or is a package or type segment of a qualified name, so that it can never be a - * reference to the variable under analysis. + * @return whether the identifier declares a variable or names something in another namespace, so that + * it can never reference the variable under analysis. */ private static boolean isNeverVariableReference(J.Identifier identifier, Cursor parentCursor) { if (identifier.getFieldType() == null && @@ -199,17 +198,15 @@ private static boolean isNeverVariableReference(J.Identifier identifier, Cursor parentCursor.getParentTreeCursor().getValue() instanceof J.Annotation; } if (parent instanceof J.FieldAccess) { - // Package and type segments of a qualified name carry no field type, unlike an attributed variable + // Package and type segments carry no field type, unlike an attributed variable return identifier.getFieldType() == null; } return false; } /** - * @return whether the identifier is the receiver of a call that {@link ChangeMethodTargetToStatic} - * rewrites to its static form, either bare (`util.isMock(..)`, `util::isMock`) or as the final name of - * a field access receiver (`this.util.isMock(..)`); the rewrite replaces the whole receiver with the - * class name, so such a use no longer needs the instance. + * @return whether the identifier is the receiver of a call {@link ChangeMethodTargetToStatic} makes + * static, bare or through a field access, in which case the rewrite drops it and the instance with it. */ private static boolean isMigratedUse(J.Identifier identifier, Cursor parent) { Object parentValue = parent.getValue(); From a3b586b03052e2e1fdbc59fe69dbee1db47e53e8 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 15:16:18 +0200 Subject: [PATCH 4/4] Parameterize MockUtil declarator tests --- .../mockito/MockUtilsToStaticTest.java | 240 ++---------------- 1 file changed, 27 insertions(+), 213 deletions(-) diff --git a/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java b/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java index b3db44d11..47bf7ee7f 100644 --- a/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java +++ b/src/test/java/org/openrewrite/java/testing/mockito/MockUtilsToStaticTest.java @@ -16,6 +16,8 @@ package org.openrewrite.java.testing.mockito; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.openrewrite.DocumentExample; import org.openrewrite.InMemoryExecutionContext; import org.openrewrite.java.JavaParser; @@ -276,228 +278,40 @@ boolean test(Object value) { ); } - @Test - void removeFirstDeclaratorOnlyPreservingSiblingEvaluation() { + @ParameterizedTest + @ValueSource(strings = { + "MockUtil util = new MockUtil(), observed = createObserved();", + "MockUtil observed = createObserved(), util = new MockUtil();", + """ + MockUtil util = new MockUtil(), + observed = createObserved();""", + "MockUtil observed = createObserved() , util = new MockUtil();", + "MockUtil util = new MockUtil(),observed = createObserved();" + }) + void removeOnlyTheMigratedDeclarator(String declaration) { //language=java - rewriteRun( - java( - """ - import org.mockito.internal.util.MockUtil; - - class Test { - boolean test(Object value) { - MockUtil util = new MockUtil(), observed = createObserved(); - observe(observed); - return util.isMock(value); - } - - MockUtil createObserved() { - return new MockUtil(); - } - - void observe(Object value) { - } - } - """, - """ - import org.mockito.internal.util.MockUtil; - - class Test { - boolean test(Object value) { - MockUtil observed = createObserved(); - observe(observed); - return MockUtil.isMock(value); - } + var source = """ + import org.mockito.internal.util.MockUtil; - MockUtil createObserved() { - return new MockUtil(); - } - - void observe(Object value) { - } + class Test { + boolean test(Object value) { + %s + observe(observed); + return %s; } - """ - ) - ); - } - - @Test - void removeLastDeclaratorOnlyPreservingSiblingEvaluation() { - //language=java - rewriteRun( - java( - """ - import org.mockito.internal.util.MockUtil; - - class Test { - boolean test(Object value) { - MockUtil observed = createObserved(), util = new MockUtil(); - observe(observed); - return util.isMock(value); - } - - MockUtil createObserved() { - return new MockUtil(); - } - void observe(Object value) { - } + MockUtil createObserved() { + return new MockUtil(); } - """, - """ - import org.mockito.internal.util.MockUtil; - class Test { - boolean test(Object value) { - MockUtil observed = createObserved(); - observe(observed); - return MockUtil.isMock(value); - } - - MockUtil createObserved() { - return new MockUtil(); - } - - void observe(Object value) { - } + void observe(Object value) { } - """ - ) - ); - } - - @Test - void removeFirstDeclaratorSpanningMultipleLines() { - //language=java + } + """; rewriteRun( java( - """ - import org.mockito.internal.util.MockUtil; - - class Test { - boolean test(Object value) { - MockUtil util = new MockUtil(), - observed = createObserved(); - observe(observed); - return util.isMock(value); - } - - MockUtil createObserved() { - return new MockUtil(); - } - - void observe(Object value) { - } - } - """, - """ - import org.mockito.internal.util.MockUtil; - - class Test { - boolean test(Object value) { - MockUtil observed = createObserved(); - observe(observed); - return MockUtil.isMock(value); - } - - MockUtil createObserved() { - return new MockUtil(); - } - - void observe(Object value) { - } - } - """ - ) - ); - } - - @Test - void removeLastDeclaratorWithSpaceBeforeComma() { - //language=java - rewriteRun( - java( - """ - import org.mockito.internal.util.MockUtil; - - class Test { - boolean test(Object value) { - MockUtil observed = createObserved() , util = new MockUtil(); - observe(observed); - return util.isMock(value); - } - - MockUtil createObserved() { - return new MockUtil(); - } - - void observe(Object value) { - } - } - """, - """ - import org.mockito.internal.util.MockUtil; - - class Test { - boolean test(Object value) { - MockUtil observed = createObserved(); - observe(observed); - return MockUtil.isMock(value); - } - - MockUtil createObserved() { - return new MockUtil(); - } - - void observe(Object value) { - } - } - """ - ) - ); - } - - @Test - void removeFirstDeclaratorWithoutSpaceAfterComma() { - //language=java - rewriteRun( - java( - """ - import org.mockito.internal.util.MockUtil; - - class Test { - boolean test(Object value) { - MockUtil util = new MockUtil(),observed = createObserved(); - observe(observed); - return util.isMock(value); - } - - MockUtil createObserved() { - return new MockUtil(); - } - - void observe(Object value) { - } - } - """, - """ - import org.mockito.internal.util.MockUtil; - - class Test { - boolean test(Object value) { - MockUtil observed = createObserved(); - observe(observed); - return MockUtil.isMock(value); - } - - MockUtil createObserved() { - return new MockUtil(); - } - - void observe(Object value) { - } - } - """ + source.formatted(declaration, "util.isMock(value)"), + source.formatted("MockUtil observed = createObserved();", "MockUtil.isMock(value)") ) ); }