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..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; @@ -185,6 +211,595 @@ public void test() { ); } + @Test + void receiverMethodCallIsNotDropped() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), + java(A_NON_STATIC), + java(B_STATIC), + 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(A_NON_STATIC), + java(B_STATIC), + java( + """ + import a.A; + + class C { + A field = new A(); + A[] array = new A[1]; + + public void test(C other, Object value) { + other.field.nonStatic(); + array[0].nonStatic(); + ((A) value).nonStatic(); + } + } + """ + ) + ); + } + + @Test + void volatileFieldReceiverIsNotDropped() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), + java(A_NON_STATIC), + java(B_STATIC), + 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(B_STATIC), + 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(A_NON_STATIC), + java(B_STATIC), + 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 qualifiedFieldReceiversAreDropped() { + rewriteRun( + spec -> spec.recipe(new ChangeMethodTargetToStatic("a.A nonStatic()", "b.B", null, null, false)), + java(A_NON_STATIC), + java(B_STATIC), + 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 input) { return input; } + } + """ + ), + java( + """ + package b; + public class B { + public static String value() { return "b"; } + public static String value(String input) { return input; } + } + """ + ), + java( + """ + import a.A; + + class C { + public void test(A receiver, A argument) { + receiver.value(argument.value()); + } + } + """, + """ + import a.A; + import b.B; + + class C { + public void test(A receiver, A argument) { + B.value(B.value()); + } + } + """ + ) + ); + } + + @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(B_VALUE), + 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 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( + 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(B_VALUE), + 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(B_VALUE), + 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(A_VALUE), + java(B_VALUE), + 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(A_VALUE), + java(B_VALUE), + java( + """ + import a.A; + + import java.util.function.Supplier; + + class C { + public Supplier test(A receiver) { + return receiver::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..16732ec7010 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,108 @@ private boolean isAlreadyStaticCallOnTargetType(@Nullable Expression target, Met return isStatic && isSameReceiverType && calledOnTargetType; } + /** + * 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 unwrapped = Expression.unwrap(expression); + if (unwrapped == null || unwrapped instanceof J.Empty || unwrapped instanceof J.Literal) { + return true; + } + if (unwrapped instanceof J.Identifier) { + JavaType.Variable fieldType = ((J.Identifier) unwrapped).getFieldType(); + return fieldType == null || !fieldType.hasFlags(Flag.Volatile); + } + if (unwrapped instanceof J.FieldAccess) { + J.FieldAccess fieldAccess = (J.FieldAccess) unwrapped; + if (isTypeReference(fieldAccess)) { + return true; + } + JavaType.Variable fieldType = fieldAccess.getName().getFieldType(); + return isTypeReference(fieldAccess.getTarget()) && + (fieldType == null || !fieldType.hasFlags(Flag.Volatile)); + } + 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) unwrapped; + if (newClass.getBody() != null || newClass.getEnclosing() != null) { + return false; + } + for (Expression argument : newClass.getArguments()) { + if (!isSafeToDiscard(argument)) { + return false; + } + } + return true; + } + return false; + } + + /** + * Returns true when every receiver and argument in a matched call chain is safe to drop. + */ + private boolean collapsesOntoTargetType(@Nullable Expression expression) { + Expression unwrapped = Expression.unwrap(expression); + if (!(unwrapped instanceof J.MethodInvocation)) { + return false; + } + J.MethodInvocation qualifier = (J.MethodInvocation) unwrapped; + 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; + } + + /** + * 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(); + 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; + } + + /** + * Returns true for a type name, {@code this}, or {@code Outer.this}. + */ + 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 +242,9 @@ 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) && + !isQualifierOfMatchedCall(method) && + (isSafeToDiscard(select) || collapsesOntoTargetType(select))) { JavaType.Method transformedType = null; if (method.getMethodType() != null) { maybeRemoveImport(method.getMethodType().getDeclaringType()); @@ -150,7 +254,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 : @@ -174,14 +278,15 @@ 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()); transformedType = transformMethodType(memberRef.getMethodType()); } maybeAddImport(fullyQualifiedTargetTypeName, !matchUnknownTypes); - m = memberRef.withContaining( + m = m.withContaining( new J.Identifier(randomId(), containing.getPrefix(), Markers.EMPTY,