From 164e7286baa9fe1de58e430a52494da8621e69e4 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Mon, 10 Aug 2026 16:02:34 +0200 Subject: [PATCH 1/3] ChangeMethodTargetToStatic: only drop receivers with no observable effect The recipe replaced an invocation's receiver with the target type name unconditionally. Java still evaluates that expression, before the arguments, and only then discards its value, so `receiver().value()` became `B.value()` and the call to `receiver()` was gone: evaluation, ordering and any exception it raised were all observable, and the recipe dropped them. A member reference is worse still, because it evaluates and null checks its qualifier when the reference is created and `B::value` does neither. A receiver is now dropped only when its evaluation cannot be observed: a type name, `this` (possibly qualified), a literal, or a non-volatile variable read. An instantiation is still dropped, since `new A().staticMethod()` is the shape this recipe exists to rewrite, but only when its own arguments are discardable as well. A member reference is retargeted only when its qualifier already names a type. Every other invocation is left alone, receiver evaluation intact. Chains the recipe rewrites itself still collapse: a qualifier that is itself a matching invocation with a discardable receiver is accepted recursively, so `legacy.value().value()` still becomes `B.value()` rather than `B.value().value()`, which does not compile once `value()` is static on the target type. The deliberate cost is that a bound member reference on a variable, `a::value`, is no longer retargeted, as the null check it performs at creation has no equivalent in the static form. No existing test expectation changed. --- .../java/ChangeMethodTargetToStaticTest.java | 560 ++++++++++++++++++ .../java/ChangeMethodTargetToStatic.java | 85 ++- 2 files changed, 643 insertions(+), 2 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java index 78fd3f41a9e..c77c2123165 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java @@ -185,6 +185,566 @@ public void test() { ); } + @Test + void receiverMethodCallIsNotDropped() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public void nonStatic() {} + } + """ + ), + java( + """ + package b; + public class B { + public static void nonStatic() {} + } + """ + ), + java( + """ + import a.A; + + class C { + int calls; + + A receiver() { + calls++; + return new A(); + } + + public void test() { + receiver().nonStatic(); + } + } + """ + ) + ); + } + + @Test + void receiverExpressionsThatCanThrowAreNotDropped() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public void nonStatic() {} + } + """ + ), + java( + """ + package b; + public class B { + public static void nonStatic() {} + } + """ + ), + java( + """ + import a.A; + + class C { + A field = new A(); + A[] array = new A[1]; + + public void test(C other, Object o) { + other.field.nonStatic(); + array[0].nonStatic(); + ((A) o).nonStatic(); + } + } + """ + ) + ); + } + + @Test + void volatileFieldReceiverIsNotDropped() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public void nonStatic() {} + } + """ + ), + java( + """ + package b; + public class B { + public static void nonStatic() {} + } + """ + ), + java( + """ + import a.A; + + class C { + volatile A shared = new A(); + + public void test() { + shared.nonStatic(); + } + } + """ + ) + ); + } + + @Test + void instantiationArgumentsAreNotDropped() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public A() {} + public A(String s) {} + public void nonStatic() {} + } + """ + ), + java( + """ + package b; + public class B { + public static void nonStatic() {} + } + """ + ), + java( + """ + import a.A; + + class C { + int calls; + + String argument() { + calls++; + return "a"; + } + + public void test() { + new A("a").nonStatic(); + new A(argument()).nonStatic(); + } + } + """, + """ + import a.A; + import b.B; + + class C { + int calls; + + String argument() { + calls++; + return "a"; + } + + public void test() { + B.nonStatic(); + new A(argument()).nonStatic(); + } + } + """ + ) + ); + } + + @Test + void variableReceiversAreDropped() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public void nonStatic() {} + } + """ + ), + java( + """ + package b; + public class B { + public static void nonStatic() {} + } + """ + ), + java( + """ + import a.A; + + class C { + A field = new A(); + + public void test(A parameter) { + A local = new A(); + local.nonStatic(); + parameter.nonStatic(); + field.nonStatic(); + } + } + """, + """ + import a.A; + import b.B; + + class C { + A field = new A(); + + public void test(A parameter) { + A local = new A(); + B.nonStatic(); + B.nonStatic(); + B.nonStatic(); + } + } + """ + ) + ); + } + + @Test + void chainedSelfCallsCollapseOntoTargetType() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A value()", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public A value() { return this; } + } + """ + ), + java( + """ + package b; + public class B { + public static String value() { return "b"; } + } + """ + ), + java( + """ + import a.A; + + class C { + public void test(A legacy) { + legacy.value().value(); + legacy.value().value().value(); + } + } + """, + """ + import a.A; + import b.B; + + class C { + public void test(A legacy) { + B.value(); + B.value(); + } + } + """ + ) + ); + } + + @Test + void chainedCallOnRewrittenStaticFactoryCollapses() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A *(..)", "b.B", "java.util.List", null, false)), + java( + """ + package a; + import java.util.List; + public class A { + public static A of(String s) { return new A(); } + public List reverse() { return null; } + } + """ + ), + java( + """ + package b; + import java.util.List; + public class B { + public static List of(String s) { return null; } + public static List reverse() { return null; } + } + """ + ), + java( + """ + import a.A; + + class C { + public void test() { + A.of("x").reverse(); + } + } + """, + """ + import b.B; + + class C { + public void test() { + B.reverse(); + } + } + """ + ) + ); + } + + @Test + void chainedCallOnUndiscardableReceiverIsNotChanged() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A value()", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public A value() { return this; } + } + """ + ), + java( + """ + package b; + public class B { + public static String value() { return "b"; } + } + """ + ), + java( + """ + import a.A; + + class C { + int calls; + + A receiver() { + calls++; + return new A(); + } + + public void test() { + receiver().value().value(); + } + } + """ + ) + ); + } + + @Test + void thisReceiversAreReplaced() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A value()", "b.B", null, null, false)), + java( + """ + package b; + public class B { + public static String value() { return "b"; } + } + """ + ), + java( + """ + package a; + + import java.util.function.Supplier; + + public class A { + public String value() { return "a"; } + + public Supplier direct() { + return this::value; + } + + class Inner { + public Supplier qualified() { + return A.this::value; + } + + public String call() { + return A.this.value(); + } + } + } + """, + """ + package a; + + import b.B; + + import java.util.function.Supplier; + + public class A { + public String value() { return "a"; } + + public Supplier direct() { + return B::value; + } + + class Inner { + public Supplier qualified() { + return B::value; + } + + public String call() { + return B.value(); + } + } + } + """ + ) + ); + } + + @Test + void memberReferenceOnMethodCallIsNotChanged() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A value()", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public String value() { return "a"; } + } + """ + ), + java( + """ + package b; + public class B { + public static String value() { return "b"; } + } + """ + ), + java( + """ + import a.A; + + import java.util.function.Supplier; + + class C { + int calls; + + A receiver() { + calls++; + return new A(); + } + + public Supplier test() { + return receiver()::value; + } + } + """ + ) + ); + } + + @Test + void memberReferenceOnVariableIsNotChanged() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A value()", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public String value() { return "a"; } + } + """ + ), + java( + """ + package b; + public class B { + public static String value() { return "b"; } + } + """ + ), + java( + """ + import a.A; + + import java.util.function.Supplier; + + class C { + public Supplier test(A a) { + return a::value; + } + } + """ + ) + ); + } + + @Test + void memberReferenceOnRewrittenCallCollapses() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A *(..)", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public A self() { return this; } + public String value() { return "a"; } + } + """ + ), + java( + """ + package b; + public class B { + public static B self() { return new B(); } + public static String value() { return "b"; } + } + """ + ), + java( + """ + import a.A; + + import java.util.function.Supplier; + + class C { + public Supplier test(A legacy) { + return legacy.self()::value; + } + } + """, + """ + import a.A; + import b.B; + + import java.util.function.Supplier; + + class C { + public Supplier test(A legacy) { + return B::value; + } + } + """ + ) + ); + } + @Test void memberReferenceTargetToStatic() { rewriteRun( diff --git a/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java b/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java index 9725a9f081e..485c4d748e3 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java @@ -118,6 +118,85 @@ private boolean isAlreadyStaticCallOnTargetType(@Nullable Expression target, Met return isStatic && isSameReceiverType && calledOnTargetType; } + /** + * Java evaluates the expression qualifying a static method invocation and then discards its value, + * without a null check. This only drops an expression whose evaluation cannot be observed: a type name, + * {@code this} (possibly qualified), or a simple non-volatile variable read (class initialization + * aside). As a deliberate exception that preserves the {@code new A().staticMethod()} shape this recipe + * exists to rewrite, an instantiation whose arguments are themselves discardable is also dropped, even + * though a constructor can run arbitrary code and throw. A method invocation, a dereference, an array + * access, a cast or any other expression may mutate state or throw, so the invocation is left alone + * instead. + */ + private boolean isSafeToDiscard(@Nullable Expression expression) { + Expression expr = Expression.unwrap(expression); + if (expr == null || expr instanceof J.Empty || expr instanceof J.Literal) { + return true; + } + if (expr instanceof J.Identifier) { + JavaType.Variable fieldType = ((J.Identifier) expr).getFieldType(); + return fieldType == null || !fieldType.hasFlags(Flag.Volatile); + } + if (expr instanceof J.FieldAccess) { + return isTypeReference(expr); + } + if (expr instanceof J.NewClass) { + // Instantiating a type only to call a static method on it is the case this recipe was written + // for, so the instantiation itself is still discarded even though a constructor can throw. + // Its arguments are not covered by that, so they have to be discardable in their own right. + J.NewClass newClass = (J.NewClass) expr; + if (newClass.getBody() != null || newClass.getEnclosing() != null) { + return false; + } + for (Expression argument : newClass.getArguments()) { + if (!isSafeToDiscard(argument)) { + return false; + } + } + return true; + } + return false; + } + + /** + * A qualifier that is itself an invocation this recipe rewrites needs no preservation: the recipe's + * long-standing treatment of such chains is to collapse them, so that with a fluent + * {@code a.Legacy value()} pattern {@code legacy.value().value()} becomes {@code Modern.value()}. + * Blocking that collapse would leave the rewritten qualifier behind as the receiver of the outer + * call, producing {@code Modern.value().value()}, which no longer resolves once the migrated method + * is static on the target type. The qualifier's own receiver is checked recursively, so a chain + * rooted in an expression that cannot be discarded is left entirely unchanged instead. + */ + private boolean collapsesOntoTargetType(@Nullable Expression expression) { + Expression expr = Expression.unwrap(expression); + if (!(expr instanceof J.MethodInvocation)) { + return false; + } + J.MethodInvocation qualifier = (J.MethodInvocation) expr; + return !isAlreadyStaticCallOnTargetType(qualifier.getSelect(), qualifier) && + methodMatcher.matches(qualifier, matchUnknownTypes) && + (isSafeToDiscard(qualifier.getSelect()) || collapsesOntoTargetType(qualifier.getSelect())); + } + + /** + * Check if the expression only names a type or is {@code this} (possibly qualified, as in + * {@code Outer.this}), so that evaluating it always succeeds without any observable effect. Unlike an + * invocation, a member reference evaluates and null checks its qualifier when the reference is created, + * so it only replaces a qualifier of that shape with the target type. + */ + private boolean isTypeReference(@Nullable Expression expression) { + if (expression instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) expression; + return identifier.getFieldType() == null || "this".equals(identifier.getSimpleName()); + } + if (expression instanceof J.FieldAccess) { + J.Identifier name = ((J.FieldAccess) expression).getName(); + return (name.getFieldType() == null || "this".equals(name.getSimpleName())) && + name.getType() instanceof JavaType.FullyQualified; + } + return false; + } + /** * Transform the method type to reflect the new declaring type and static flag. */ @@ -140,7 +219,8 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, ctx); Expression select = method.getSelect(); if (!isAlreadyStaticCallOnTargetType(select, method) && - methodMatcher.matches(method, matchUnknownTypes)) { + methodMatcher.matches(method, matchUnknownTypes) && + (isSafeToDiscard(select) || collapsesOntoTargetType(select))) { JavaType.Method transformedType = null; if (method.getMethodType() != null) { maybeRemoveImport(method.getMethodType().getDeclaringType()); @@ -174,7 +254,8 @@ public J visitMemberReference(J.MemberReference memberRef, ExecutionContext ctx) J.MemberReference m = (J.MemberReference) super.visitMemberReference(memberRef, ctx); Expression containing = memberRef.getContaining(); if (!isAlreadyStaticCallOnTargetType(containing, memberRef) && - methodMatcher.matches(memberRef)) { + methodMatcher.matches(memberRef) && + (isTypeReference(containing) || collapsesOntoTargetType(containing))) { JavaType.Method transformedType = null; if (memberRef.getMethodType() != null) { maybeRemoveImport(memberRef.getMethodType().getDeclaringType()); From c923bbb8617e88319c0459fa409aab80d9f3e454 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 22:57:15 +0200 Subject: [PATCH 2/3] Check collapsed qualifier arguments, keep recursed rewrites, trim commentary A matched qualifier was collapsed without checking its own arguments, so `A.of(argument()).reverse()` silently dropped `argument()`. Rebuilding from the unvisited node also discarded argument rewrites, costing a second cycle, and a field read qualified by a type name or `this` was needlessly skipped. --- .../java/ChangeMethodTargetToStaticTest.java | 155 ++++++++++++++++++ .../java/ChangeMethodTargetToStatic.java | 88 ++++++---- 2 files changed, 215 insertions(+), 28 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java index c77c2123165..c95a11d2ba9 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java @@ -417,6 +417,116 @@ public void test(A parameter) { ); } + @Test + void qualifiedFieldReceiversAreDropped() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public void nonStatic() {} + } + """ + ), + java( + """ + package b; + public class B { + public static void nonStatic() {} + } + """ + ), + java( + """ + import a.A; + + class C { + static A shared = new A(); + A field = new A(); + + class Inner { + public void test() { + C.shared.nonStatic(); + C.this.field.nonStatic(); + } + } + + public void test() { + this.field.nonStatic(); + } + } + """, + """ + import a.A; + import b.B; + + class C { + static A shared = new A(); + A field = new A(); + + class Inner { + public void test() { + B.nonStatic(); + B.nonStatic(); + } + } + + public void test() { + B.nonStatic(); + } + } + """ + ) + ); + } + + @Test + void nestedMatchesInArgumentsAreRewritten() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A value(..)", "b.B", null, null, false)), + java( + """ + package a; + public class A { + public String value() { return "a"; } + public String value(String s) { return s; } + } + """ + ), + java( + """ + package b; + public class B { + public static String value() { return "b"; } + public static String value(String s) { return s; } + } + """ + ), + java( + """ + import a.A; + + class C { + public void test(A x, A y) { + x.value(y.value()); + } + } + """, + """ + import a.A; + import b.B; + + class C { + public void test(A x, A y) { + B.value(B.value()); + } + } + """ + ) + ); + } + @Test void chainedSelfCallsCollapseOntoTargetType() { rewriteRun( @@ -510,6 +620,51 @@ public void test() { ); } + @Test + void chainedCallOnCollapsingReceiverWithSideEffectingArgumentsIsNotChanged() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A *(..)", "b.B", "java.util.List", null, false)), + java( + """ + package a; + import java.util.List; + public class A { + public static A of(String s) { return new A(); } + public List reverse() { return null; } + } + """ + ), + java( + """ + package b; + import java.util.List; + public class B { + public static List of(String s) { return null; } + public static List reverse() { return null; } + } + """ + ), + java( + """ + import a.A; + + class C { + int calls; + + String argument() { + calls++; + return "x"; + } + + public void test() { + A.of(argument()).reverse(); + } + } + """ + ) + ); + } + @Test void chainedCallOnUndiscardableReceiverIsNotChanged() { rewriteRun( diff --git a/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java b/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java index 485c4d748e3..ea36b7c366e 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java @@ -119,14 +119,10 @@ private boolean isAlreadyStaticCallOnTargetType(@Nullable Expression target, Met } /** - * Java evaluates the expression qualifying a static method invocation and then discards its value, - * without a null check. This only drops an expression whose evaluation cannot be observed: a type name, - * {@code this} (possibly qualified), or a simple non-volatile variable read (class initialization - * aside). As a deliberate exception that preserves the {@code new A().staticMethod()} shape this recipe - * exists to rewrite, an instantiation whose arguments are themselves discardable is also dropped, even - * though a constructor can run arbitrary code and throw. A method invocation, a dereference, an array - * access, a cast or any other expression may mutate state or throw, so the invocation is left alone - * instead. + * Whether the receiver can be replaced by the target type name without the running program noticing: + * a type name, {@code this}, a literal, or a non-volatile field read qualified by neither. An + * invocation, a dereference, an array access or a cast may mutate state or throw, so those leave the + * call alone instead. */ private boolean isSafeToDiscard(@Nullable Expression expression) { Expression expr = Expression.unwrap(expression); @@ -138,12 +134,17 @@ private boolean isSafeToDiscard(@Nullable Expression expression) { return fieldType == null || !fieldType.hasFlags(Flag.Volatile); } if (expr instanceof J.FieldAccess) { - return isTypeReference(expr); + J.FieldAccess fieldAccess = (J.FieldAccess) expr; + if (isTypeReference(fieldAccess)) { + return true; + } + JavaType.Variable fieldType = fieldAccess.getName().getFieldType(); + return isTypeReference(fieldAccess.getTarget()) && + (fieldType == null || !fieldType.hasFlags(Flag.Volatile)); } if (expr instanceof J.NewClass) { - // Instantiating a type only to call a static method on it is the case this recipe was written - // for, so the instantiation itself is still discarded even though a constructor can throw. - // Its arguments are not covered by that, so they have to be discardable in their own right. + // `new A().staticMethod()` is the shape this recipe exists to rewrite, so the instantiation is + // dropped even though a constructor can throw. Its arguments still have to stand on their own. J.NewClass newClass = (J.NewClass) expr; if (newClass.getBody() != null || newClass.getEnclosing() != null) { return false; @@ -159,13 +160,10 @@ private boolean isSafeToDiscard(@Nullable Expression expression) { } /** - * A qualifier that is itself an invocation this recipe rewrites needs no preservation: the recipe's - * long-standing treatment of such chains is to collapse them, so that with a fluent - * {@code a.Legacy value()} pattern {@code legacy.value().value()} becomes {@code Modern.value()}. - * Blocking that collapse would leave the rewritten qualifier behind as the receiver of the outer - * call, producing {@code Modern.value().value()}, which no longer resolves once the migrated method - * is static on the target type. The qualifier's own receiver is checked recursively, so a chain - * rooted in an expression that cannot be discarded is left entirely unchanged instead. + * A qualifier that is itself an invocation this recipe rewrites needs no preservation, so that + * {@code legacy.value().value()} collapses to {@code Modern.value()} rather than to the + * {@code Modern.value().value()} that no longer resolves. Its own receiver and arguments are checked + * recursively, leaving a chain rooted in something undiscardable entirely unchanged. */ private boolean collapsesOntoTargetType(@Nullable Expression expression) { Expression expr = Expression.unwrap(expression); @@ -173,16 +171,49 @@ private boolean collapsesOntoTargetType(@Nullable Expression expression) { return false; } J.MethodInvocation qualifier = (J.MethodInvocation) expr; - return !isAlreadyStaticCallOnTargetType(qualifier.getSelect(), qualifier) && - methodMatcher.matches(qualifier, matchUnknownTypes) && - (isSafeToDiscard(qualifier.getSelect()) || collapsesOntoTargetType(qualifier.getSelect())); + if (isAlreadyStaticCallOnTargetType(qualifier.getSelect(), qualifier) || + !methodMatcher.matches(qualifier, matchUnknownTypes) || + !(isSafeToDiscard(qualifier.getSelect()) || collapsesOntoTargetType(qualifier.getSelect()))) { + return false; + } + for (Expression argument : qualifier.getArguments()) { + if (!isSafeToDiscard(argument)) { + return false; + } + } + return true; + } + + /** + * An invocation qualifying another matched invocation defers to that outer one, which either collapses + * it away or is itself left alone. Rewriting it here instead would strand a call to the migrated + * static method on a receiver that no longer declares it. + */ + private boolean isQualifierOfMatchedCall(Expression expression) { + Cursor parent = getCursor().getParentTreeCursor(); + while (parent.getValue() instanceof J.Parentheses) { + parent = parent.getParentTreeCursor(); + } + Object value = parent.getValue(); + if (value instanceof J.MethodInvocation) { + J.MethodInvocation outer = (J.MethodInvocation) value; + return Expression.unwrap(outer.getSelect()) == expression && + !isAlreadyStaticCallOnTargetType(outer.getSelect(), outer) && + methodMatcher.matches(outer, matchUnknownTypes); + } + if (value instanceof J.MemberReference) { + J.MemberReference outer = (J.MemberReference) value; + return Expression.unwrap(outer.getContaining()) == expression && + !isAlreadyStaticCallOnTargetType(outer.getContaining(), outer) && + methodMatcher.matches(outer); + } + return false; } /** - * Check if the expression only names a type or is {@code this} (possibly qualified, as in - * {@code Outer.this}), so that evaluating it always succeeds without any observable effect. Unlike an - * invocation, a member reference evaluates and null checks its qualifier when the reference is created, - * so it only replaces a qualifier of that shape with the target type. + * Whether the expression only names a type or is {@code this}, possibly qualified as {@code Outer.this}. + * A member reference evaluates and null checks its qualifier when the reference is created, so unlike an + * invocation it accepts only a qualifier of that shape. */ private boolean isTypeReference(@Nullable Expression expression) { if (expression instanceof J.Identifier) { @@ -220,6 +251,7 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) Expression select = method.getSelect(); if (!isAlreadyStaticCallOnTargetType(select, method) && methodMatcher.matches(method, matchUnknownTypes) && + !isQualifierOfMatchedCall(method) && (isSafeToDiscard(select) || collapsesOntoTargetType(select))) { JavaType.Method transformedType = null; if (method.getMethodType() != null) { @@ -230,7 +262,7 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) maybeAddImport(fullyQualifiedTargetTypeName, m.getSimpleName(), !matchUnknownTypes); } else { maybeAddImport(fullyQualifiedTargetTypeName, !matchUnknownTypes); - m = method.withSelect( + m = m.withSelect( new J.Identifier(randomId(), select == null ? Space.EMPTY : @@ -262,7 +294,7 @@ public J visitMemberReference(J.MemberReference memberRef, ExecutionContext ctx) transformedType = transformMethodType(memberRef.getMethodType()); } maybeAddImport(fullyQualifiedTargetTypeName, !matchUnknownTypes); - m = memberRef.withContaining( + m = m.withContaining( new J.Identifier(randomId(), containing.getPrefix(), Markers.EMPTY, From 0cc77d314baa149204156582de8180496fa3ef5b Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 15:16:18 +0200 Subject: [PATCH 3/3] Consolidate ChangeMethodTargetToStatic test stubs --- .../java/ChangeMethodTargetToStaticTest.java | 222 +++++------------- .../java/ChangeMethodTargetToStatic.java | 42 ++-- 2 files changed, 78 insertions(+), 186 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java index c95a11d2ba9..24f1a4e6d9c 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeMethodTargetToStaticTest.java @@ -15,6 +15,7 @@ */ package org.openrewrite.java; +import org.intellij.lang.annotations.Language; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.openrewrite.Issue; @@ -24,6 +25,38 @@ class ChangeMethodTargetToStaticTest implements RewriteTest { + @Language("java") + private static final String A_NON_STATIC = """ + package a; + public class A { + public void nonStatic() {} + } + """; + + @Language("java") + private static final String B_STATIC = """ + package b; + public class B { + public static void nonStatic() {} + } + """; + + @Language("java") + private static final String A_VALUE = """ + package a; + public class A { + public String value() { return "a"; } + } + """; + + @Language("java") + private static final String B_VALUE = """ + package b; + public class B { + public static String value() { return "b"; } + } + """; + @Test void targetToStatic() { rewriteRun( @@ -31,14 +64,7 @@ void targetToStatic() { new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false), new ChangeMethodName("b.B nonStatic()", "foo", null, null) ), - java( - """ - package a; - public class A { - public void nonStatic() {} - } - """ - ), + java(A_NON_STATIC), java( """ package b; @@ -189,22 +215,8 @@ public void test() { void receiverMethodCallIsNotDropped() { rewriteRun( spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), - java( - """ - package a; - public class A { - public void nonStatic() {} - } - """ - ), - java( - """ - package b; - public class B { - public static void nonStatic() {} - } - """ - ), + java(A_NON_STATIC), + java(B_STATIC), java( """ import a.A; @@ -230,22 +242,8 @@ public void test() { void receiverExpressionsThatCanThrowAreNotDropped() { rewriteRun( spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), - java( - """ - package a; - public class A { - public void nonStatic() {} - } - """ - ), - java( - """ - package b; - public class B { - public static void nonStatic() {} - } - """ - ), + java(A_NON_STATIC), + java(B_STATIC), java( """ import a.A; @@ -254,10 +252,10 @@ class C { A field = new A(); A[] array = new A[1]; - public void test(C other, Object o) { + public void test(C other, Object value) { other.field.nonStatic(); array[0].nonStatic(); - ((A) o).nonStatic(); + ((A) value).nonStatic(); } } """ @@ -269,22 +267,8 @@ public void test(C other, Object o) { void volatileFieldReceiverIsNotDropped() { rewriteRun( spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), - java( - """ - package a; - public class A { - public void nonStatic() {} - } - """ - ), - java( - """ - package b; - public class B { - public static void nonStatic() {} - } - """ - ), + java(A_NON_STATIC), + java(B_STATIC), java( """ import a.A; @@ -315,14 +299,7 @@ public void nonStatic() {} } """ ), - java( - """ - package b; - public class B { - public static void nonStatic() {} - } - """ - ), + java(B_STATIC), java( """ import a.A; @@ -367,22 +344,8 @@ public void test() { void variableReceiversAreDropped() { rewriteRun( spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), - java( - """ - package a; - public class A { - public void nonStatic() {} - } - """ - ), - java( - """ - package b; - public class B { - public static void nonStatic() {} - } - """ - ), + java(A_NON_STATIC), + java(B_STATIC), java( """ import a.A; @@ -421,22 +384,8 @@ public void test(A parameter) { void qualifiedFieldReceiversAreDropped() { rewriteRun( spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), - java( - """ - package a; - public class A { - public void nonStatic() {} - } - """ - ), - java( - """ - package b; - public class B { - public static void nonStatic() {} - } - """ - ), + java(A_NON_STATIC), + java(B_STATIC), java( """ import a.A; @@ -490,7 +439,7 @@ void nestedMatchesInArgumentsAreRewritten() { package a; public class A { public String value() { return "a"; } - public String value(String s) { return s; } + public String value(String input) { return input; } } """ ), @@ -499,7 +448,7 @@ public class A { package b; public class B { public static String value() { return "b"; } - public static String value(String s) { return s; } + public static String value(String input) { return input; } } """ ), @@ -508,8 +457,8 @@ public class B { import a.A; class C { - public void test(A x, A y) { - x.value(y.value()); + public void test(A receiver, A argument) { + receiver.value(argument.value()); } } """, @@ -518,7 +467,7 @@ public void test(A x, A y) { import b.B; class C { - public void test(A x, A y) { + public void test(A receiver, A argument) { B.value(B.value()); } } @@ -539,14 +488,7 @@ public class A { } """ ), - java( - """ - package b; - public class B { - public static String value() { return "b"; } - } - """ - ), + java(B_VALUE), java( """ import a.A; @@ -677,14 +619,7 @@ public class A { } """ ), - java( - """ - package b; - public class B { - public static String value() { return "b"; } - } - """ - ), + java(B_VALUE), java( """ import a.A; @@ -710,14 +645,7 @@ public void test() { void thisReceiversAreReplaced() { rewriteRun( spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A value()", "b.B", null, null, false)), - java( - """ - package b; - public class B { - public static String value() { return "b"; } - } - """ - ), + java(B_VALUE), java( """ package a; @@ -775,22 +703,8 @@ public String call() { void memberReferenceOnMethodCallIsNotChanged() { rewriteRun( spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A value()", "b.B", null, null, false)), - java( - """ - package a; - public class A { - public String value() { return "a"; } - } - """ - ), - java( - """ - package b; - public class B { - public static String value() { return "b"; } - } - """ - ), + java(A_VALUE), + java(B_VALUE), java( """ import a.A; @@ -818,22 +732,8 @@ public Supplier test() { void memberReferenceOnVariableIsNotChanged() { rewriteRun( spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A value()", "b.B", null, null, false)), - java( - """ - package a; - public class A { - public String value() { return "a"; } - } - """ - ), - java( - """ - package b; - public class B { - public static String value() { return "b"; } - } - """ - ), + java(A_VALUE), + java(B_VALUE), java( """ import a.A; @@ -841,8 +741,8 @@ public class B { import java.util.function.Supplier; class C { - public Supplier test(A a) { - return a::value; + public Supplier test(A receiver) { + return receiver::value; } } """ diff --git a/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java b/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java index ea36b7c366e..16732ec7010 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodTargetToStatic.java @@ -119,22 +119,20 @@ private boolean isAlreadyStaticCallOnTargetType(@Nullable Expression target, Met } /** - * Whether the receiver can be replaced by the target type name without the running program noticing: - * a type name, {@code this}, a literal, or a non-volatile field read qualified by neither. An - * invocation, a dereference, an array access or a cast may mutate state or throw, so those leave the - * call alone instead. + * Returns true when dropping the receiver cannot change program behavior. Expressions that can have side + * effects or throw return false. */ private boolean isSafeToDiscard(@Nullable Expression expression) { - Expression expr = Expression.unwrap(expression); - if (expr == null || expr instanceof J.Empty || expr instanceof J.Literal) { + Expression unwrapped = Expression.unwrap(expression); + if (unwrapped == null || unwrapped instanceof J.Empty || unwrapped instanceof J.Literal) { return true; } - if (expr instanceof J.Identifier) { - JavaType.Variable fieldType = ((J.Identifier) expr).getFieldType(); + if (unwrapped instanceof J.Identifier) { + JavaType.Variable fieldType = ((J.Identifier) unwrapped).getFieldType(); return fieldType == null || !fieldType.hasFlags(Flag.Volatile); } - if (expr instanceof J.FieldAccess) { - J.FieldAccess fieldAccess = (J.FieldAccess) expr; + if (unwrapped instanceof J.FieldAccess) { + J.FieldAccess fieldAccess = (J.FieldAccess) unwrapped; if (isTypeReference(fieldAccess)) { return true; } @@ -142,10 +140,10 @@ private boolean isSafeToDiscard(@Nullable Expression expression) { return isTypeReference(fieldAccess.getTarget()) && (fieldType == null || !fieldType.hasFlags(Flag.Volatile)); } - if (expr instanceof J.NewClass) { + if (unwrapped instanceof J.NewClass) { // `new A().staticMethod()` is the shape this recipe exists to rewrite, so the instantiation is // dropped even though a constructor can throw. Its arguments still have to stand on their own. - J.NewClass newClass = (J.NewClass) expr; + J.NewClass newClass = (J.NewClass) unwrapped; if (newClass.getBody() != null || newClass.getEnclosing() != null) { return false; } @@ -160,17 +158,14 @@ private boolean isSafeToDiscard(@Nullable Expression expression) { } /** - * A qualifier that is itself an invocation this recipe rewrites needs no preservation, so that - * {@code legacy.value().value()} collapses to {@code Modern.value()} rather than to the - * {@code Modern.value().value()} that no longer resolves. Its own receiver and arguments are checked - * recursively, leaving a chain rooted in something undiscardable entirely unchanged. + * Returns true when every receiver and argument in a matched call chain is safe to drop. */ private boolean collapsesOntoTargetType(@Nullable Expression expression) { - Expression expr = Expression.unwrap(expression); - if (!(expr instanceof J.MethodInvocation)) { + Expression unwrapped = Expression.unwrap(expression); + if (!(unwrapped instanceof J.MethodInvocation)) { return false; } - J.MethodInvocation qualifier = (J.MethodInvocation) expr; + J.MethodInvocation qualifier = (J.MethodInvocation) unwrapped; if (isAlreadyStaticCallOnTargetType(qualifier.getSelect(), qualifier) || !methodMatcher.matches(qualifier, matchUnknownTypes) || !(isSafeToDiscard(qualifier.getSelect()) || collapsesOntoTargetType(qualifier.getSelect()))) { @@ -185,9 +180,8 @@ private boolean collapsesOntoTargetType(@Nullable Expression expression) { } /** - * An invocation qualifying another matched invocation defers to that outer one, which either collapses - * it away or is itself left alone. Rewriting it here instead would strand a call to the migrated - * static method on a receiver that no longer declares it. + * Returns true when this expression is the receiver of another matched call. The outer call then decides + * whether the whole chain is safe to rewrite. */ private boolean isQualifierOfMatchedCall(Expression expression) { Cursor parent = getCursor().getParentTreeCursor(); @@ -211,9 +205,7 @@ private boolean isQualifierOfMatchedCall(Expression expression) { } /** - * Whether the expression only names a type or is {@code this}, possibly qualified as {@code Outer.this}. - * A member reference evaluates and null checks its qualifier when the reference is created, so unlike an - * invocation it accepts only a qualifier of that shape. + * Returns true for a type name, {@code this}, or {@code Outer.this}. */ private boolean isTypeReference(@Nullable Expression expression) { if (expression instanceof J.Identifier) {