LambdaMethodReference avoids suggestions for non-static methods

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LambdaMethodReference.java

@@ -91,25 +91,31 @@ private Description checkMethodInvocation(
                 || !methodInvocation.getTypeArguments().isEmpty()) {
             return Description.NO_MATCH;
         }
+        Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocation);
+        if (methodSymbol == null) {
+            // Should only ever occur if there are errors in the AST, allow the compiler to fail later
+            return Description.NO_MATCH;
+        }
+        if (!methodSymbol.isStatic()) {
+            // Only support static invocations for the time being to avoid erroneously
+            // rewriting '() -> foo()' to 'ClassName::foo' instead of 'this::foo'
+            // or suggesting '() -> foo.doWork().bar()' should be rewritten to 'foo.doWork()::bar',
+            // which executes 'doWork' eagerly, even when the supplier is not used.
+            return Description.NO_MATCH;
+        }
         return buildDescription(root)
                 .setMessage(MESSAGE)
-                .addFix(buildFix(methodInvocation, root, state))
+                .addFix(buildFix(methodSymbol, root, state))
                 .build();
     }
 
     private static Optional<SuggestedFix> buildFix(
-            MethodInvocationTree invocation,
+            Symbol.MethodSymbol symbol,
             LambdaExpressionTree root,
             VisitorState state) {
-        return Optional.ofNullable(ASTHelpers.getSymbol(invocation))
-                // Only support static invocations for the time being to avoid erroneously
-                // rewriting '() -> foo()' to 'ClassName::foo' instead of 'this::foo'
-                .filter(Symbol::isStatic)
-                .flatMap(symbol -> {
-                    SuggestedFix.Builder builder = SuggestedFix.builder();
-                    return toMethodReference(SuggestedFixes.qualifyType(state, builder, symbol))
-                            .map(qualified -> builder.replace(root, qualified).build());
-                });
+        SuggestedFix.Builder builder = SuggestedFix.builder();
+        return toMethodReference(SuggestedFixes.qualifyType(state, builder, symbol))
+                .map(qualified -> builder.replace(root, qualified).build());
     }
 
     private static Optional<String> toMethodReference(String qualifiedMethodName) {

baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LambdaMethodReferenceTest.java

@@ -89,15 +89,15 @@ public void testAutoFix_block() {
     }
 
     @Test
-    public void testPositive_block_localMethod() {
+    public void testNegative_block_localMethod() {
         compilationHelper.addSourceLines(
                 "Test.java",
                 "import " + ImmutableList.class.getName() + ';',
                 "import " + List.class.getName() + ';',
                 "import " + Optional.class.getName() + ';',
                 "class Test {",
                 "  public List<Object> foo(Optional<List<Object>> optional) {",
-                "    // BUG: Diagnostic contains: Lambda should be a method reference",
+                // A future improvement may rewrite the following to 'orElseGet(this::bar)'
                 "    return optional.orElseGet(() -> { return bar(); });",
                 "  }",
                 "  private List<Object> bar() {",
@@ -195,15 +195,15 @@ public void testAutoFix_expression() {
     }
 
     @Test
-    public void testPositive_expression_localMethod() {
+    public void testNegative_expression_localMethod() {
         compilationHelper.addSourceLines(
                 "Test.java",
                 "import " + ImmutableList.class.getName() + ';',
                 "import " + List.class.getName() + ';',
                 "import " + Optional.class.getName() + ';',
                 "class Test {",
                 "  public List<Object> foo(Optional<List<Object>> optional) {",
-                "    // BUG: Diagnostic contains: Lambda should be a method reference",
+                // A future improvement may rewrite the following to 'orElseGet(this::bar)'
                 "    return optional.orElseGet(() -> bar());",
                 "  }",
                 "  private List<Object> bar() {",
@@ -258,4 +258,19 @@ public void testNegative_expression() {
                 "  }",
                 "}").doTest();
     }
+
+    @Test
+    public void testNegative_expression_chain() {
+        compilationHelper.addSourceLines(
+                "Test.java",
+                "import " + ImmutableList.class.getName() + ';',
+                "import " + Optional.class.getName() + ';',
+                "class Test {",
+                "  public Object foo(Optional<Object> optional) {",
+                // It's important that this is not rewritten to 'optional.orElseGet(ImmutableList.of(1)::size)'
+                // which create a new list eagerly, and returns a supplier for the new instances 'size()' function.
+                "    return optional.orElseGet(() -> ImmutableList.of(1).size());",
+                "  }",
+                "}").doTest();
+    }
 }

changelog/@unreleased/pr-771.v2.yml

@@ -0,0 +1,5 @@
+type: fix
+fix:
+  description: LambdaMethodReference avoids suggestions for non-static methods
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/771