diff --git a/src/main/java/org/openrewrite/staticanalysis/UseLambdaForFunctionalInterface.java b/src/main/java/org/openrewrite/staticanalysis/UseLambdaForFunctionalInterface.java index f57df39ea..cd291be17 100644 --- a/src/main/java/org/openrewrite/staticanalysis/UseLambdaForFunctionalInterface.java +++ b/src/main/java/org/openrewrite/staticanalysis/UseLambdaForFunctionalInterface.java @@ -36,12 +36,16 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import static java.util.Arrays.asList; import static java.util.Collections.emptyList; import static java.util.Collections.singleton; import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; public class UseLambdaForFunctionalInterface extends Recipe { + private static final Set OBJECT_METHOD_NAMES = new HashSet<>(asList( + "clone", "equals", "finalize", "getClass", "hashCode", "notify", "notifyAll", "toString", "wait")); + @Getter final String displayName = "Use lambda expressions instead of anonymous classes"; @@ -489,6 +493,9 @@ private static boolean overridesObjectMethod(JavaType.Method method) { if (usesThis(cursor)) { return "references `this`"; } + if (callsMethodOnAnonymousInstance(n, cursor)) { + return "calls a method on the anonymous instance"; + } if (shadowsLocalVariable(cursor)) { return "shadows a local variable"; } @@ -554,6 +561,69 @@ public J visitIdentifier(J.Identifier ident, Integer integer) { return hasThis.get(); } + /** + * An unqualified call resolving to a method the anonymous class inherits or declares is dispatched on that + * class's own {@code this}, which a lambda does not have. There is no {@code this} token for {@link #usesThis} + * to see, so it is recognised by what it resolves to. Unattributed calls are blocked only when named after an + * {@code Object} method, the one case no enclosing class could have supplied. + */ + private static boolean callsMethodOnAnonymousInstance(J.NewClass n, Cursor cursor) { + assert n.getBody() != null; + JavaType anonymousType = n.getType(); + AtomicBoolean callsAnonymousInstance = new AtomicBoolean(false); + new JavaVisitor() { + @Override + public J visitClassDeclaration(J.ClassDeclaration classDecl, Integer integer) { + // A local or member class declares its own `this` + return classDecl; + } + + @Override + public J visitNewClass(J.NewClass newClass, Integer integer) { + if (newClass.getBody() == null) { + return super.visitNewClass(newClass, integer); + } + // Same for a nested anonymous class, but a qualified `new`'s enclosing expression and its + // arguments are still evaluated in this scope + if (newClass.getEnclosing() != null) { + visit(newClass.getEnclosing(), integer); + } + for (Expression argument : newClass.getArguments()) { + visit(argument, integer); + } + return newClass; + } + + @Override + public J visitMethodInvocation(J.MethodInvocation method, Integer integer) { + if (isBareSuper(method.getSelect()) || + method.getSelect() == null && dispatchedOnThis(method.getMethodType(), method.getSimpleName())) { + callsAnonymousInstance.set(true); + } + return super.visitMethodInvocation(method, integer); + } + + @Override + public J visitMemberReference(J.MemberReference memberRef, Integer integer) { + if (isBareSuper(memberRef.getContaining())) { + callsAnonymousInstance.set(true); + } + return super.visitMemberReference(memberRef, integer); + } + + private boolean dispatchedOnThis(JavaType.@Nullable Method methodType, String name) { + return methodType == null ? + OBJECT_METHOD_NAMES.contains(name) : + TypeUtils.isAssignableTo(methodType.getDeclaringType(), anonymousType); + } + + private boolean isBareSuper(@Nullable Expression expression) { + return expression instanceof J.Identifier && "super".equals(((J.Identifier) expression).getSimpleName()); + } + }.visit(n.getBody(), 0, cursor); + return callsAnonymousInstance.get(); + } + private static List parameterNames(J.MethodDeclaration method) { return method.getParameters().stream() .filter(J.VariableDeclarations.class::isInstance) diff --git a/src/test/java/org/openrewrite/staticanalysis/UseLambdaForFunctionalInterfaceTest.java b/src/test/java/org/openrewrite/staticanalysis/UseLambdaForFunctionalInterfaceTest.java index 3ddf0ae89..a439eb5aa 100644 --- a/src/test/java/org/openrewrite/staticanalysis/UseLambdaForFunctionalInterfaceTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/UseLambdaForFunctionalInterfaceTest.java @@ -479,6 +479,340 @@ public Integer apply(Integer n) { ); } + @Test + void dontUseLambdaWhenImplicitGetClass() { + // The anonymous class's own `this` makes `getClass()` return the implementation class, not the enclosing one + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + Supplier> supplier() { + return new Supplier>() { + @Override + public Class get() { + return getClass(); + } + }; + } + } + """ + ) + ); + } + + @Test + void dontUseLambdaWhenImplicitGetClassIsNested() { + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + Supplier supplier(boolean b) { + return new Supplier() { + @Override + public String get() { + Supplier> nested = () -> getClass(); + return b ? String.valueOf(nested.get()) : ""; + } + }; + } + } + """ + ) + ); + } + + @Test + void dontUseLambdaWhenSuperGetClass() { + // Bare `super` is the `Object` part of the anonymous instance; in a static method a lambda's would not compile + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + static Supplier> supplier() { + return new Supplier>() { + @Override + public Class get() { + return super.getClass(); + } + }; + } + } + """ + ) + ); + } + + @Test + void dontUseLambdaWhenSuperGetClassMethodReference() { + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + Supplier>> supplier() { + return new Supplier>>() { + @Override + public Supplier> get() { + return super::getClass; + } + }; + } + } + """ + ) + ); + } + + @Test + void dontUseLambdaWhenEnclosingExpressionOfQualifiedNewCallsGetClass() { + // The qualified `new`'s enclosing expression is evaluated in the outer anonymous class's scope + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + class Inner { + } + + Test tag(Class c) { + return this; + } + + Supplier supplier() { + return new Supplier() { + @Override + public Object get() { + return tag(getClass()).new Inner() { + }; + } + }; + } + } + """ + ) + ); + } + + @Test + void dontUseLambdaWhenGetClassCannotBeAttributed() { + // Without a method type the receiver cannot be proven, so the site is left alone + rewriteRun( + spec -> spec.typeValidationOptions(TypeValidation.none()), + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + Supplier s = new Supplier() { + @Override + public String get() { + return getClass(unknown); + } + }; + } + """ + ) + ); + } + + @Test + void useLambdaWhenGetClassIsCalledOnAnotherObject() { + rewriteRun( + //language=java + java( + """ + import java.util.function.Function; + + class Test { + Function> f = new Function>() { + @Override + public Class apply(Object o) { + return o.getClass(); + } + }; + } + """, + """ + import java.util.function.Function; + + class Test { + Function> f = o -> o.getClass(); + } + """ + ) + ); + } + + @Test + void useLambdaWhenOnlyANestedAnonymousClassCallsGetClass() { + // The nested anonymous class keeps its own `this` either way + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + Supplier>> supplier() { + return new Supplier>>() { + @Override + public Supplier> get() { + return new Supplier>() { + @Override + public Class get() { + return getClass(); + } + }; + } + }; + } + } + """, + """ + import java.util.function.Supplier; + + class Test { + Supplier>> supplier() { + return () -> new Supplier>() { + @Override + public Class get() { + return getClass(); + } + }; + } + } + """ + ) + ); + } + + @Test + void dontUseLambdaWhenImplicitObjectMethodOtherThanGetClass() { + // `hashCode()` is the anonymous instance's; in a lambda it would silently become the enclosing instance's + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + Supplier supplier() { + return new Supplier() { + @Override + public Integer get() { + return hashCode(); + } + }; + } + } + """ + ) + ); + } + + @Test + void dontUseLambdaWhenSuperMethodReferenceOtherThanGetClass() { + // Bare `super` names the `Object` part of the anonymous instance + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + static Supplier> supplier() { + return new Supplier>() { + @Override + public Supplier get() { + return super::toString; + } + }; + } + } + """ + ) + ); + } + + @Test + void dontUseLambdaWhenTheInterfaceMethodCallsItself() { + // A lambda has no name to recurse through, so the call would not resolve at all + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + Supplier supplier(boolean b) { + return new Supplier() { + @Override + public String get() { + return b ? get() : "x"; + } + }; + } + } + """ + ) + ); + } + + @Test + void useLambdaWhenCallingAMethodOfTheEnclosingClass() { + // A call resolving to the enclosing class keeps its receiver in a lambda + rewriteRun( + //language=java + java( + """ + import java.util.function.Supplier; + + class Test { + String name() { + return "x"; + } + + Supplier supplier() { + return new Supplier() { + @Override + public String get() { + return name(); + } + }; + } + } + """, + """ + import java.util.function.Supplier; + + class Test { + String name() { + return "x"; + } + + Supplier supplier() { + return () -> name(); + } + } + """ + ) + ); + } + @SuppressWarnings("UnnecessaryLocalVariable") @Test void dontUseLambdaWhenShadowsLocalVariable() { @@ -1176,6 +1510,31 @@ public Integer apply(Integer n) { ); } + @Test + void dataTableRecordsImplicitGetClass() { + rewriteRun( + spec -> spec.dataTable(AnonymousFunctionalInterfaceImplementations.Row.class, rows -> { + assertThat(rows).hasSize(1); + assertThat(rows.getFirst().isConvertible()).isFalse(); + assertThat(rows.getFirst().getReason()).isEqualTo("calls a method on the anonymous instance"); + }), + //language=java + java( + """ + import java.util.function.Supplier; + class Test { + Supplier> s = new Supplier>() { + @Override + public Class get() { + return getClass(); + } + }; + } + """ + ) + ); + } + @Test void dataTableRecordsAnonymousClassDeclaringMoreThanTheInterfaceMethod() { rewriteRun(