SONARJAVA-6673 S1602: Fix false positives

java-checks/src/test/files/checks/LambdaSingleExpressionCheck_no_version.java → java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckNoVersionSample.java

@@ -1,13 +1,16 @@
-class A {
+package checks;
+
+import java.util.stream.IntStream;
+
+public class LambdaSingleExpressionCheckNoVersionSample {
   public void method() {
     IntStream.range(1, 5).map(x -> x * x - 1).forEach(x -> System.out.println(x));
     IntStream.range(1, 5).map(x -> {return x * x - 1;}) // Noncompliant {{Remove useless curly braces around statement and then remove useless return keyword (sonar.java.source not set. Assuming 8 or greater.)}}
 //                                 ^
-        .forEach(x -> { // Noncompliant {{Remove useless curly braces around statement (sonar.java.source not set. Assuming 8 or greater.)}}
+        .forEach(x -> { // Compliant - lambda body spans multiple lines, block form is kept for readability
           System.out.println(x + 11);
         });
-    //Non-Expression statement :
-    IntStream.range(1, 5).map(x -> {
+    IntStream.range(1, 5).map(x -> { // Compliant - non-expression statement
       if (x % 2 == 0) return 0;
       else return 1;
     });
@@ -22,15 +25,6 @@ public void method() {
       while(true) {
       }
     });
-
-    //Nested blocks
-    IntStream.range(1, 5).map(x -> { // Noncompliant {{Remove useless curly braces around statement (sonar.java.source not set. Assuming 8 or greater.)}}
-      {
-        {
-          return x + 1;
-        }
-      }
-    });
   }
 
 }

java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckSample.java

@@ -0,0 +1,63 @@
+package checks;
+
+import java.lang.invoke.MethodHandle;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.IntStream;
+import org.springframework.jdbc.core.JdbcTemplate;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
+
+public class LambdaSingleExpressionCheckSample {
+  public void method() {
+    IntStream.range(1, 5).map(x -> x * x - 1).forEach(x -> System.out.println(x));
+    IntStream.range(1, 5).map(x -> {return x * x - 1;}) // Noncompliant {{Remove useless curly braces around statement and then remove useless return keyword}}
+        .forEach(x -> { // Compliant - lambda body spans multiple lines, block form is kept for readability
+          System.out.println(x + 11);
+        });
+    IntStream.range(1, 5).map(x -> { // Compliant - non-expression statement
+      if (x % 2 == 0) return 0;
+      else return 1;
+    });
+    IntStream.range(1, 5).forEach(x -> {
+      try {
+        x = x/0;
+      } catch (Exception e) {
+        System.out.println(x);
+      }
+    });
+    IntStream.range(1, 5).forEach(x -> {
+      while(true) {
+      }
+    });
+    // Nested block
+    IntStream.range(1, 5).map(x -> { { { return x + 1; } } }); // Noncompliant {{Remove useless curly braces around statement}}
+  }
+
+  // Block lambda binds to RowCallbackHandler (void); simplifying to expression lambda
+  // would be ambiguous with ResultSetExtractor since merge() returns a value
+  void springJdbcQuery(JdbcTemplate jdbc, NamedParameterJdbcTemplate namedJdbc) {
+    Map<Long, Long> countByRecipient = new HashMap<>();
+    jdbc.query("SELECT recipient_id, cnt FROM t", rs -> { countByRecipient.merge(rs.getLong("recipient_id"), rs.getLong("cnt"), Long::sum); }); // Compliant
+    namedJdbc.query("SELECT recipient_id, cnt FROM t", Collections.emptyMap(),
+      rs -> { countByRecipient.merge(rs.getLong("recipient_id"), rs.getLong("cnt"), Long::sum); }); // Compliant
+    jdbc.query("SELECT name FROM t",
+      (rs, rowNum) -> { return rs.getString("name"); }); // Noncompliant {{Remove useless curly braces around statement and then remove useless return keyword}}
+  }
+
+  @FunctionalInterface
+  interface ThrowingRunnable {
+    void run() throws Throwable;
+  }
+
+  void process(ThrowingRunnable r) throws Throwable {
+    r.run();
+  }
+
+  // MethodHandle.invokeExact() and MethodHandle.invoke() are signature-polymorphic
+  void methodHandleInvocations(MethodHandle handle) throws Throwable {
+    process(() -> { handle.invokeExact(); });
+    process(() -> { handle.invoke(); });
+  }
+
+}

java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckSampleWithoutSemantic.java

@@ -0,0 +1,33 @@
+package checks;
+
+import java.lang.invoke.MethodHandle;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import org.springframework.jdbc.core.JdbcTemplate;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
+
+public class LambdaSingleExpressionCheckSampleWithoutSemantic {
+
+  @FunctionalInterface
+  interface ThrowingRunnable {
+    void run() throws Throwable;
+  }
+
+  void process(ThrowingRunnable r) throws Throwable {
+    r.run();
+  }
+
+  void springJdbcQuery(JdbcTemplate jdbc, NamedParameterJdbcTemplate namedJdbc) {
+    Map<Long, Long> countByRecipient = new HashMap<>();
+    jdbc.query("SELECT recipient_id, cnt FROM t", rs -> { countByRecipient.merge(rs.getLong("recipient_id"), rs.getLong("cnt"), Long::sum); }); // Noncompliant
+    namedJdbc.query("SELECT recipient_id, cnt FROM t", Collections.emptyMap(),
+      rs -> { countByRecipient.merge(rs.getLong("recipient_id"), rs.getLong("cnt"), Long::sum); }); // Noncompliant
+  }
+
+  void methodHandleInvocations(MethodHandle handle) throws Throwable {
+    process(() -> { handle.invokeExact(); });
+    process(() -> { handle.invoke(); });
+  }
+
+}

java-checks/src/main/java/org/sonar/java/checks/LambdaSingleExpressionCheck.java

@@ -17,11 +17,17 @@
 package org.sonar.java.checks;
 
 import org.sonar.check.Rule;
+import org.sonar.java.model.LineUtils;
 import org.sonar.plugins.java.api.JavaVersionAwareVisitor;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
 import org.sonar.plugins.java.api.JavaVersion;
+import org.sonar.plugins.java.api.semantic.MethodMatchers;
 import org.sonar.plugins.java.api.tree.BlockTree;
+import org.sonar.plugins.java.api.tree.ExpressionStatementTree;
+import org.sonar.plugins.java.api.tree.ExpressionTree;
 import org.sonar.plugins.java.api.tree.LambdaExpressionTree;
+import org.sonar.plugins.java.api.tree.MethodInvocationTree;
+import org.sonar.plugins.java.api.tree.ReturnStatementTree;
 import org.sonar.plugins.java.api.tree.StatementTree;
 import org.sonar.plugins.java.api.tree.Tree;
 
@@ -31,6 +37,20 @@
 @Rule(key = "S1602")
 public class LambdaSingleExpressionCheck extends IssuableSubscriptionVisitor implements JavaVersionAwareVisitor {
 
+  private static final MethodMatchers SPRING_JDBC_QUERY_MATCHER = MethodMatchers.create()
+    .ofSubTypes(
+      "org.springframework.jdbc.core.JdbcOperations",
+      "org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations")
+    .names("query")
+    .withAnyParameters()
+    .build();
+
+  private static final MethodMatchers METHOD_HANDLE_INVOKE_MATCHER = MethodMatchers.create()
+    .ofTypes("java.lang.invoke.MethodHandle")
+    .names("invoke", "invokeExact")
+    .withAnyParameters()
+    .build();
+
   @Override
   public boolean isCompatibleWithJavaVersion(JavaVersion version) {
     return version.isJava8Compatible();
@@ -45,7 +65,10 @@ public List<Tree.Kind> nodesToVisit() {
   public void visitNode(Tree tree) {
     LambdaExpressionTree lambdaExpressionTree = (LambdaExpressionTree) tree;
     Tree lambdaBody = lambdaExpressionTree.body();
-    if (isBlockWithOneStatement(lambdaBody)) {
+    if (isBlockWithOneStatement(lambdaBody)
+      && !hasMultilineBody(lambdaExpressionTree)
+      && !isInsideSpringJdbcQuery(lambdaExpressionTree)
+      && !isSingleMethodHandleInvocation(lambdaExpressionTree)) {
       String message = "Remove useless curly braces around statement";
       if (singleStatementIsReturn(lambdaExpressionTree)) {
         message += " and then remove useless return keyword";
@@ -74,4 +97,33 @@ private static boolean singleStatementIsReturn(LambdaExpressionTree lambdaExpres
   private static boolean isReturnStatement(Tree tree) {
     return tree.is(Tree.Kind.RETURN_STATEMENT);
   }
+
+  private static boolean hasMultilineBody(LambdaExpressionTree lambda) {
+    BlockTree block = (BlockTree) lambda.body();
+    return LineUtils.startLine(block.openBraceToken()) != LineUtils.startLine(block.closeBraceToken());
+  }
+
+  private static boolean isInsideSpringJdbcQuery(LambdaExpressionTree lambda) {
+    if (lambda.parameters().size() != 1) {
+      return false;
+    }
+    Tree parent = lambda.parent();
+    if (parent != null && parent.is(Tree.Kind.ARGUMENTS)) {
+      parent = parent.parent();
+    }
+    return parent != null && parent.is(Tree.Kind.METHOD_INVOCATION)
+      && SPRING_JDBC_QUERY_MATCHER.matches((MethodInvocationTree) parent);
+  }
+
+  private static boolean isSingleMethodHandleInvocation(LambdaExpressionTree lambda) {
+    StatementTree statement = ((BlockTree) lambda.body()).body().get(0);
+    ExpressionTree expression = null;
+    if (statement.is(Tree.Kind.EXPRESSION_STATEMENT)) {
+      expression = ((ExpressionStatementTree) statement).expression();
+    } else if (statement.is(Tree.Kind.RETURN_STATEMENT)) {
+      expression = ((ReturnStatementTree) statement).expression();
+    }
+    return expression != null && expression.is(Tree.Kind.METHOD_INVOCATION)
+      && METHOD_HANDLE_INVOKE_MATCHER.matches((MethodInvocationTree) expression);
+  }
 }

java-checks/src/test/java/org/sonar/java/checks/LambdaSingleExpressionCheckTest.java

@@ -19,20 +19,22 @@
 import org.junit.jupiter.api.Test;
 import org.sonar.java.checks.verifier.CheckVerifier;
 
+import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath;
+
 class LambdaSingleExpressionCheckTest {
 
   @Test
   void no_version() {
     CheckVerifier.newVerifier()
-      .onFile("src/test/files/checks/LambdaSingleExpressionCheck_no_version.java")
+      .onFile(mainCodeSourcesPath("checks/LambdaSingleExpressionCheckNoVersionSample.java"))
       .withCheck(new LambdaSingleExpressionCheck())
       .verifyIssues();
   }
 
   @Test
   void java_8() {
     CheckVerifier.newVerifier()
-      .onFile("src/test/files/checks/LambdaSingleExpressionCheck.java")
+      .onFile(mainCodeSourcesPath("checks/LambdaSingleExpressionCheckSample.java"))
       .withCheck(new LambdaSingleExpressionCheck())
       .withJavaVersion(8)
       .verifyIssues();
@@ -41,7 +43,7 @@ void java_8() {
   @Test
   void test_without_semantic() {
     CheckVerifier.newVerifier()
-      .onFile("src/test/files/checks/LambdaSingleExpressionCheck_no_version.java")
+      .onFile(mainCodeSourcesPath("checks/LambdaSingleExpressionCheckSampleWithoutSemantic.java"))
       .withCheck(new LambdaSingleExpressionCheck())
       .withoutSemantic()
       .verifyIssues();

sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1602.html

@@ -10,6 +10,27 @@ <h2>Why is this an issue?</h2>
 than one statement. However, when the code block consists of only one statement (which may or may not be a <code>return</code> statement), it can be
 rewritten using expression notation.</p>
 <p>This convention exists because expression notation has a cleaner, more concise, functional programming style and is regarded as more readable.</p>
+<h3>Exceptions</h3>
+<p>This rule does not flag block-form lambdas in the following cases:</p>
+<ul>
+  <li><strong>Multiline body</strong> — if the opening <code>{</code> and closing <code>}</code> are on different lines, a block form is kept for
+  readability.</li>
+  <li><strong>Spring JDBC <code>query()</code> with a single-parameter lambda</strong> — converting such a lambda to an expression form may silently
+  change overload resolution.</li>
+  <li><strong><code>MethodHandle.invoke()</code> / <code>MethodHandle.invokeExact()</code></strong> — these are signature-polymorphic methods.
+  Converting such lambdas to an expression form changes the inferred return type, which can cause a <code>WrongMethodTypeException</code> at
+  runtime.</li>
+</ul>
+<pre>
+entries.forEach(e -&gt; { // Compliant, multiline body
+  System.out.println(e.getKey() + ": " + e.getValue());
+});
+
+// Compliant, expression form would resolve to ResultSetExtractor instead of RowCallbackHandler
+jdbcTemplate.query("SELECT id, cnt FROM t", rs -&gt; { map.merge(rs.getLong("id"), rs.getLong("cnt"), Long::sum); });
+
+process(() -&gt; { handle.invokeExact(); }); // Compliant, signature-polymorphic invocation stays void
+</pre>
 <h2>How to fix it</h2>
 <ul>
   <li>If the code block consists only of a <code>return</code> statement, replace the code block with the argument expression from the
@@ -27,7 +48,7 @@ <h4>Compliant solution</h4>
 </pre>
 <h4>Noncompliant code example</h4>
 <pre data-diff-id="2" data-diff-type="noncompliant">
-x -&gt; {System.out.println(x+1);} // Noncompliant, replace code block with statement
+x -&gt; { System.out.println(x+1); } // Noncompliant, replace code block with statement
 </pre>
 <h4>Compliant solution</h4>
 <pre data-diff-id="2" data-diff-type="compliant">