SONARJAVA RIS-493: Implement S8795 - Code after unconditional control flow statements should be removed#5680
SONARJAVA RIS-493: Implement S8795 - Code after unconditional control flow statements should be removed#5680romainbrenguier wants to merge 3 commits into
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| @@ -0,0 +1,309 @@ | |||
| package checks; | |||
There was a problem hiding this comment.
🚨 Bug: Test sample with unreachable code won't compile; breaks build
S8795CheckSample.java is placed in java-checks-test-sources/default/src/main/java/checks/, which is compiled by maven-compiler-plugin (release 25) during the module build. Per JLS 14.21, a statement that cannot be executed because it is unreachable is a compile-time error in Java. Almost every Noncompliant case in this sample places a statement directly after return;, throw, break;, or continue; in the same block (e.g. return i + a; i++; at lines 13-14, throw ...; cleanup(); at lines 40-41, break; System.out.println(...); at lines 55-56). javac will reject these as "unreachable statement", so the java-checks-test-sources/default module fails to compile and the whole build breaks.
The repository has a dedicated directory for exactly this situation: java-checks-test-sources/default/src/main/files/non-compiling/, resolved by TestUtils.nonCompilingTestSourcesPath(...). The sample must be moved there and the test updated accordingly (see HardCodedPasswordCheckTest for the pattern).
Note also a broader design implication: because the targeted patterns are themselves Java compile errors, this rule will essentially only ever fire on non-compiling code, unlike the S1763 ports in dynamically-typed languages.
Relocate the sample to the non-compiling sources tree and load it via nonCompilingTestSourcesPath so the test-sources module still compiles.:
// Move the sample to the non-compiling directory:
// java-checks-test-sources/default/src/main/files/non-compiling/checks/S8795CheckSample.java
// and update the test to load it from there:
import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath;
@Test
void test() {
CheckVerifier.newVerifier()
.onFile(nonCompilingTestSourcesPath("checks/S8795CheckSample.java"))
.withCheck(new S8795Check())
.verifyIssues();
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| private void visitNestedBlocks(StatementTree statement) { | ||
| switch (statement.kind()) { | ||
| case IF_STATEMENT: | ||
| IfStatementTree ifStmt = (IfStatementTree) statement; | ||
| checkBlock(ifStmt.thenStatement()); | ||
| if (ifStmt.elseStatement() != null) { | ||
| checkBlock(ifStmt.elseStatement()); | ||
| } | ||
| break; | ||
| case FOR_STATEMENT: | ||
| checkBlock(((ForStatementTree) statement).statement()); | ||
| break; | ||
| case FOR_EACH_STATEMENT: | ||
| checkBlock(((ForEachStatement) statement).statement()); | ||
| break; |
There was a problem hiding this comment.
💡 Edge Case: Bare blocks, switch expressions and lambdas not traversed
visitNestedBlocks dispatches on a fixed set of statement kinds but does not handle a standalone BLOCK ({ ... }), SWITCH_EXPRESSION, or lambda/local-class bodies. As a result, unreachable code inside a bare nested block, e.g.
void m() {
{
return;
foo(); // not detected
}
}
is silently missed (false negative). A standalone block is a common construct (e.g. scoping blocks). Consider adding a case BLOCK: checkStatements(((BlockTree) statement).body()); break; branch, and decide explicitly whether switch-expression case bodies should be analyzed.
Add a BLOCK case so unreachable statements inside bare nested blocks are detected.:
case BLOCK:
checkStatements(((BlockTree) statement).body());
break;
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| @@ -0,0 +1,14 @@ | |||
| { | |||
| "title": "TODO: Title for S8795", | |||
There was a problem hiding this comment.
💡 Quality: Rule metadata (html/json) and Sonar_way profile are placeholders
S8795.json has "title": "TODO: Title for S8795" and S8795.html contains only TODO placeholders, and S8795 is not added to Sonar_way_profile.json. While this is a draft PR, these must be filled in with the real rule description/title and the rule registered in the profile before merge, otherwise the rule ships with no user-facing documentation and is not activated in the default quality profile.
Was this helpful? React with 👍 / 👎
CI failed: The CI build failed due to missing source directories required for integration tests and transient network timeouts connecting to the Develocity server.OverviewThe build failed across multiple jobs due to two distinct issues: a missing directory structure for integration tests and intermittent network timeouts while reaching the Develocity build cache service. A total of 8 logs were analyzed across 7 error templates. FailuresMissing Integration Test Sources (confidence: high)
Develocity Connection Timeout (confidence: high)
Summary
Code Review 🚫 Blocked 0 resolved / 3 findingsImplements rule S8795 to detect unreachable code, but the build is currently broken due to compilation errors in the test sample. Further work is needed to handle nested blocks and provide actual rule metadata. 🚨 Bug: Test sample with unreachable code won't compile; breaks build📄 java-checks-test-sources/default/src/main/java/checks/S8795CheckSample.java:1 📄 java-checks-test-sources/default/src/main/java/checks/S8795CheckSample.java:13-14 📄 java-checks-test-sources/default/src/main/java/checks/S8795CheckSample.java:40-41 📄 java-checks-test-sources/default/src/main/java/checks/S8795CheckSample.java:55-56 📄 java-checks/src/test/java/org/sonar/java/checks/S8795CheckTest.java:22 📄 java-checks/src/test/java/org/sonar/java/checks/S8795CheckTest.java:29
The repository has a dedicated directory for exactly this situation: Note also a broader design implication: because the targeted patterns are themselves Java compile errors, this rule will essentially only ever fire on non-compiling code, unlike the S1763 ports in dynamically-typed languages. Relocate the sample to the non-compiling sources tree and load it via nonCompilingTestSourcesPath so the test-sources module still compiles.💡 Edge Case: Bare blocks, switch expressions and lambdas not traversed📄 java-checks/src/main/java/org/sonar/java/checks/S8795Check.java:98-112
is silently missed (false negative). A standalone block is a common construct (e.g. scoping blocks). Consider adding a Add a BLOCK case so unreachable statements inside bare nested blocks are detected.💡 Quality: Rule metadata (html/json) and Sonar_way profile are placeholders📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8795.json:2 📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8795.html:1-5
🤖 Prompt for agentsTip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
There was a problem hiding this comment.
Comment gitar unblock to override this block and allow merging.
Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.
1 participant
Summary
This PR implements rule S8795 to detect unreachable code that appears after unconditional jump statements (return, throw, break, continue).
Implementation
S8795Check.javaS8795CheckTest.javaS8795CheckSample.javadocs/java/rules/S8795.mdRelated
Test Plan
mvn test -Dtest=S8795CheckTest -pl java-checks🤖 Generated with RAD workflow