diff --git a/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S1602.json b/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S1602.json deleted file mode 100644 index a025176e325..00000000000 --- a/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S1602.json +++ /dev/null @@ -1,5 +0,0 @@ -{ -"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java": [ -164 -] -} diff --git a/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2139.json b/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2139.json deleted file mode 100644 index 0967ef424bc..00000000000 --- a/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2139.json +++ /dev/null @@ -1 +0,0 @@ -{} diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S1602.json b/its/ruling/src/test/resources/eclipse-jetty/java-S1602.json deleted file mode 100644 index a025176e325..00000000000 --- a/its/ruling/src/test/resources/eclipse-jetty/java-S1602.json +++ /dev/null @@ -1,5 +0,0 @@ -{ -"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java": [ -164 -] -} diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S9142.json b/its/ruling/src/test/resources/eclipse-jetty/java-S9142.json deleted file mode 100644 index 9e26dfeeb6e..00000000000 --- a/its/ruling/src/test/resources/eclipse-jetty/java-S9142.json +++ /dev/null @@ -1 +0,0 @@ -{} \ No newline at end of file diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S9346.json b/its/ruling/src/test/resources/eclipse-jetty/java-S9346.json deleted file mode 100644 index 0967ef424bc..00000000000 --- a/its/ruling/src/test/resources/eclipse-jetty/java-S9346.json +++ /dev/null @@ -1 +0,0 @@ -{} diff --git a/its/ruling/src/test/resources/sonar-server/java-S9142.json b/its/ruling/src/test/resources/sonar-server/java-S9142.json deleted file mode 100644 index 9e26dfeeb6e..00000000000 --- a/its/ruling/src/test/resources/sonar-server/java-S9142.json +++ /dev/null @@ -1 +0,0 @@ -{} \ No newline at end of file diff --git a/java-checks/src/test/files/checks/LambdaSingleExpressionCheck_no_version.java b/java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckNoVersionSample.java similarity index 60% rename from java-checks/src/test/files/checks/LambdaSingleExpressionCheck_no_version.java rename to java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckNoVersionSample.java index fee4fe24aa4..37b725383ed 100644 --- a/java-checks/src/test/files/checks/LambdaSingleExpressionCheck_no_version.java +++ b/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; - } - } - }); } } diff --git a/java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckSample.java new file mode 100644 index 00000000000..620af87f1ce --- /dev/null +++ b/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 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(); }); + } + +} diff --git a/java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckSampleWithoutSemantic.java b/java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckSampleWithoutSemantic.java new file mode 100644 index 00000000000..f753a1e7bda --- /dev/null +++ b/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 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(); }); + } + +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/LambdaSingleExpressionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/LambdaSingleExpressionCheck.java index 7bea2013ad2..e5112c2da32 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/LambdaSingleExpressionCheck.java +++ b/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 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); + } } diff --git a/java-checks/src/test/files/checks/LambdaSingleExpressionCheck.java b/java-checks/src/test/files/checks/LambdaSingleExpressionCheck.java deleted file mode 100644 index be9d920a23f..00000000000 --- a/java-checks/src/test/files/checks/LambdaSingleExpressionCheck.java +++ /dev/null @@ -1,35 +0,0 @@ -class A { - 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 -> { // Noncompliant {{Remove useless curly braces around statement}} - System.out.println(x + 11); - }); - //Non-Expression statement : - IntStream.range(1, 5).map(x -> { - 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 blocks - IntStream.range(1, 5).map(x -> { // Noncompliant {{Remove useless curly braces around statement}} - { - { - return x + 1; - } - } - }); - } - -} diff --git a/java-checks/src/test/java/org/sonar/java/checks/LambdaSingleExpressionCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/LambdaSingleExpressionCheckTest.java index bbe05d83d96..4c6a3925bfb 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/LambdaSingleExpressionCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/LambdaSingleExpressionCheckTest.java @@ -19,12 +19,14 @@ 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(); } @@ -32,7 +34,7 @@ void no_version() { @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(); diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1602.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1602.html index 90af7dad958..7efea65fa38 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1602.html +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1602.html @@ -10,6 +10,27 @@

Why is this an issue?

than one statement. However, when the code block consists of only one statement (which may or may not be a return statement), it can be rewritten using expression notation.

This convention exists because expression notation has a cleaner, more concise, functional programming style and is regarded as more readable.

+

Exceptions

+

This rule does not flag block-form lambdas in the following cases:

+
    +
  • Multiline body — if the opening { and closing } are on different lines, a block form is kept for + readability.
    • +
  • Spring JDBC query() with a single-parameter lambda — converting such a lambda to an expression form may silently + change overload resolution.
    • +
  • MethodHandle.invoke() / MethodHandle.invokeExact() — these are signature-polymorphic methods. + Converting such lambdas to an expression form changes the inferred return type, which can cause a WrongMethodTypeException at + runtime.
    • +
+
+entries.forEach(e -> { // 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 -> { map.merge(rs.getLong("id"), rs.getLong("cnt"), Long::sum); });
+
+process(() -> { handle.invokeExact(); }); // Compliant, signature-polymorphic invocation stays void
+

How to fix it

  • If the code block consists only of a return statement, replace the code block with the argument expression from the @@ -27,7 +48,7 @@

    Compliant solution

    Noncompliant code example

    -x -> {System.out.println(x+1);} // Noncompliant, replace code block with statement
+x -> { System.out.println(x+1); } // Noncompliant, replace code block with statement

    Compliant solution