Mrefs without expr qualifier cannot NPE

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LambdaCanBeMethodReferenceRule.java

@@ -58,30 +58,32 @@ public class LambdaCanBeMethodReferenceRule extends AbstractJavaRulechainRule {
         return null;
     }
 
-    private void processLambdaWithBody(ASTLambdaExpression node, RuleContext data, ASTExpression expression) {
+    private void processLambdaWithBody(ASTLambdaExpression lambda, RuleContext data, ASTExpression expression) {
         if (expression instanceof ASTMethodCall) {
             ASTMethodCall call = (ASTMethodCall) expression;
-            if (canBeTransformed(call) && argumentsListMatches(call, node.getParameters())) {
-                data.addViolation(node, buildMethodRefString(call));
+            if (canBeTransformed(lambda, call) && argumentsListMatches(call, lambda.getParameters())) {
+                data.addViolation(lambda, buildMethodRefString(lambda, call));
             }
         }
     }
 
-    private String buildMethodRefString(ASTMethodCall call) {
+    private String buildMethodRefString(ASTLambdaExpression lambda, ASTMethodCall call) {
         StringBuilder sb = new StringBuilder();
         ASTExpression qualifier = call.getQualifier();
-        if (qualifier == null) {
-            OverloadSelectionResult info = call.getOverloadSelectionInfo();
-            assert !info.isFailed() : "should not be failed: " + call;
-            boolean isStatic = info.getMethodType().isStatic();
-            if (isStatic) {
-                JTypeDeclSymbol symbol = info.getMethodType().getDeclaringType().getSymbol();
-                assert symbol != null
-                    : "null symbol for " + info.getMethodType().getDeclaringType() + ", method " + info.getMethodType();
-                sb.append(symbol.getSimpleName());
-            } else {
-                sb.append("this");
-            }
+        OverloadSelectionResult info = call.getOverloadSelectionInfo();
+        assert !info.isFailed() : "should not be failed: " + call;
+
+        if (qualifier == null && info.getMethodType().isStatic()
+            || lambda.getParameters().size() != call.getArguments().size()) {
+            // this second condition corresponds to the case the first lambda
+            // param is the receiver of the method call
+
+            JTypeDeclSymbol symbol = info.getMethodType().getDeclaringType().getSymbol();
+            assert symbol != null
+                : "null symbol for " + info.getMethodType().getDeclaringType() + ", method " + info.getMethodType();
+            sb.append(symbol.getSimpleName());
+        } else if (qualifier == null) {
+            sb.append("this");
         } else {
             sb.append(PrettyPrintingUtil.prettyPrint(qualifier));
         }
@@ -92,7 +94,7 @@ public class LambdaCanBeMethodReferenceRule extends AbstractJavaRulechainRule {
     private boolean argumentsListMatches(ASTMethodCall call, ASTLambdaParameterList params) {
         ASTArgumentList args = call.getArguments();
         int start;
-        if (args.size() == params.size() - 1) {
+        if (params.size() == args.size() + 1) {
             // first parameter for the method call may be its receiver
             start = 1;
             JVariableSymbol firstParam = params.get(0).getVarId().getSymbol();
@@ -115,7 +117,8 @@ public class LambdaCanBeMethodReferenceRule extends AbstractJavaRulechainRule {
         return true;
     }
 
-    private boolean canBeTransformed(ASTMethodCall call) {
+    // coarse check to filter out some stuff before checking call arguments
+    private boolean canBeTransformed(ASTLambdaExpression lambda, ASTMethodCall call) {
         ASTExpression qualifier = call.getQualifier();
         if (call.getOverloadSelectionInfo().isFailed()) {
             // err on the side of FNs
@@ -135,6 +138,10 @@ public class LambdaCanBeMethodReferenceRule extends AbstractJavaRulechainRule {
             return true;
         }
 
+        if (lambda.getParameters().size() == call.getArguments().size() + 1) {
+            return true;
+        }
+
         return getProperty(REPORT_IF_MAY_NPE);
     }
 

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LambdaCanBeMethodReference.xml

@@ -109,7 +109,14 @@
 
     <test-code>
         <description>can rewrite many different qualifier types without NPE risk</description>
+        <rule-property name="reportEvenIfMayNPE">false</rule-property>
         <expected-problems>4</expected-problems>
+        <expected-messages>
+            <message>Lambda expression could be written as a method reference: `"abc"::indexOf`</message>
+            <message>Lambda expression could be written as a method reference: `this::correct`</message>
+            <message>Lambda expression could be written as a method reference: `Integer::valueOf`</message>
+            <message>Lambda expression could be written as a method reference: `String.class::isAssignableFrom`</message>
+        </expected-messages>
         <code><![CDATA[
             public final class Foo {
 
@@ -156,6 +163,10 @@
     <test-code>
         <description>Test when method ref is on receiver</description>
         <expected-problems>2</expected-problems>
+        <expected-messages>
+            <message>Lambda expression could be written as a method reference: `String::isEmpty`</message>
+            <message>Lambda expression could be written as a method reference: `String::contains`</message>
+        </expected-messages>
         <code><![CDATA[
             public final class Foo {
 
@@ -180,6 +191,59 @@
             ]]></code>
     </test-code>
 
+    <code-fragment id="riskOfNPE">
+        <![CDATA[
+            public final class Foo {
+
+                public static void iTakeLambda(ALambda l) {}
+                public static void iTakeLambda(ALambda2 l) {}
+                static {
+                    // parseInt(int) does not exist
+                    iTakeLambda(x -> foo().bar(x));
+                    iTakeLambda(x -> fooField.bar(x));
+                    // this also may not NPE because it will be converted to String::contains
+                    iTakeLambda((x,c) -> x.contains(c));
+                }
+                public boolean correct(int a) {
+                    return false;
+                }
+                static Foo foo() { return new Foo(); }
+                boolean bar(String x) { return true; }
+                static Foo fooField;
+            }
+
+            interface ALambda {
+                boolean doSomething(String a, CharSequence c);
+            }
+            interface ALambda2 {
+                boolean doSomething(String a);
+            }
+            ]]>
+    </code-fragment>
+
+
+    <test-code>
+        <description>Risk of NPE (report == false)</description>
+        <rule-property name="reportEvenIfMayNPE">false</rule-property>
+        <expected-problems>1</expected-problems>
+        <expected-messages>
+            <message>Lambda expression could be written as a method reference: `String::contains`</message>
+        </expected-messages>
+        <code-ref id="riskOfNPE"/>
+    </test-code>
+
+    <test-code>
+        <description>Risk of NPE (report == true)</description>
+        <rule-property name="reportEvenIfMayNPE">true</rule-property>
+        <expected-problems>3</expected-problems>
+        <expected-messages>
+            <message>Lambda expression could be written as a method reference: `foo()::bar`</message>
+            <message>Lambda expression could be written as a method reference: `fooField::bar`</message>
+            <message>Lambda expression could be written as a method reference: `String::contains`</message>
+        </expected-messages>
+        <code-ref id="riskOfNPE"/>
+    </test-code>
+