SONARJAVA-6673 S1602: Fix false positives

[asya-vorobeva]

Stop raising issues for Spring JDBC query overloads, MethodHandle signature-polymorphic invocations, and multiline lambda bodies

[hashicorp-vault-sonar-prod]

SONARJAVA-6673

[github-actions]

Ruling Diff Summary

Detected changes in 2 rule files: 2 issues removed, 0 issues added.

S1602 (java) on eclipse-jetty - 1 issues removed, 0 issues added - deleted ruling file

Removed jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java (line 164)

(source file not found at this revision: jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java)

S1602 (java) on eclipse-jetty-similar-to-main - 1 issues removed, 0 issues added - deleted ruling file

Removed jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java (line 164)

(source file not found at this revision: jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java)

[gitar-bot]

Code Review ✅ Approved 4 resolved / 4 findings

Fixes false positives in S1602 for Spring JDBC query overloads, MethodHandle signature-polymorphic invocations, and multiline lambda bodies. Multiline-body exemption contradicts the rule description and should be reconsidered, Spring JDBC exemption is broader than necessary for the ambiguity it addresses, and test coverage was reduced with removal of nested blocks and an untested return branch.

✅ 4 resolved

✅ Bug: Multiline-body exemption contradicts the S1602 description

📄 java-checks/src/main/java/org/sonar/java/checks/LambdaSingleExpressionCheck.java:101-104 📄 java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckNoVersionSample.java:10-12
hasMultilineBody silences the rule for every one-statement block lambda whose braces sit on different lines, while S1602.html still states that "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" with no formatting exception. A user writing the documented noncompliant example across two lines (x -> { System.out.println(x+1); }) now gets no issue, which is the common formatting in real code (the previously-expected jetty issue and the no_version sample expectation were both dropped for exactly that reason). Either restrict the exemption to cases the rule genuinely cannot simplify, or update the rule description/RSPEC so the documented behaviour matches the implementation.

✅ Bug: Spring JDBC exemption is broader than the ambiguity it fixes

📄 java-checks/src/main/java/org/sonar/java/checks/LambdaSingleExpressionCheck.java:106-113
isInsideSpringJdbcQuery suppresses every block lambda that is a direct argument of any query overload on JdbcOperations/NamedParameterJdbcOperations, not only the one-parameter, value-producing case that is ambiguous between RowCallbackHandler and ResultSetExtractor. Unambiguous targets are now silently missed, e.g. jdbc.query(sql, (rs, rowNum) -> { return rs.getString(1); }) (arity 2, only RowMapper applies) or jdbc.query(con -> { return con.prepareStatement(sql); }, mapper). Narrow the exemption to single-parameter lambdas whose single statement is an expression statement with a non-void type.

✅ Quality: Test coverage lost: nested blocks removed, return branch untested

📄 java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckNoVersionSample.java:27-28 📄 java-checks/src/main/java/org/sonar/java/checks/LambdaSingleExpressionCheck.java:115-125 📄 java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckSample.java:52-56
The renamed sample drops the "Nested blocks" case, so the recursive isBlockWithOneStatement/isRefactorizable path (still reachable, e.g. x -> { { { return x + 1; } } } on one line) is no longer exercised by any test. Likewise the Tree.Kind.RETURN_STATEMENT branch of the new isSingleMethodHandleInvocation has no sample (() -> { return handle.invoke(); }), so half of the new code is unverified. Re-add a single-line nested-block case and a MethodHandle invocation in return position.

✅ Bug: Without-semantic sample misses two expected issues (test fails)

📄 java-checks-test-sources/default/src/main/java/checks/LambdaSingleExpressionCheckSampleWithoutSemantic.java:28-31 📄 java-checks/src/main/java/org/sonar/java/checks/LambdaSingleExpressionCheck.java:115-125 📄 java-checks/src/test/java/org/sonar/java/checks/LambdaSingleExpressionCheckTest.java:43-50
In LambdaSingleExpressionCheckSampleWithoutSemantic.java the two MethodHandle lambdas on lines 29-30 are single-line one-statement blocks, and METHOD_HANDLE_INVOKE_MATCHER cannot match when symbols are unknown (the same reason lines 23/25 are annotated // Noncompliant). The check therefore raises an issue on both lines, but neither carries a // Noncompliant marker, so LambdaSingleExpressionCheckTest.test_without_semantic() fails with unexpected issues. Add the markers (or exclude the two calls from this sample).

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
93.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[lijun-chen-sonarsource]

LGTM!

Just suggested some cosmetic change to the code to use the pattern matching syntax. Feel free to ignore.

[lijun-chen-sonarsource]

Suggested change

	return parent != null && parent.is(Tree.Kind.METHOD_INVOCATION)
	&& SPRING_JDBC_QUERY_MATCHER.matches((MethodInvocationTree) parent);
	return parent instanceof MethodInvocationTree methodInvocation
	&& SPRING_JDBC_QUERY_MATCHER.matches(methodInvocation);

[lijun-chen-sonarsource]

Suggested change

	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);
	if (!(lambda.body() instanceof BlockTree block) || block.body().size() != 1) {
	return false;
	}
	StatementTree statement = block.body().get(0);
	ExpressionTree expression = null;
	if (statement instanceof ExpressionStatementTree expressionStatement) {
	expression = expressionStatement.expression();
	} else if (statement instanceof ReturnStatementTree returnStatement) {
	expression = returnStatement.expression();
	}
	return expression instanceof MethodInvocationTree methodInvocation
	&& METHOD_HANDLE_INVOKE_MATCHER.matches(methodInvocation);