SONARJAVA-6473 Fix FP in S3626: return inside try-finally in a loop

[francois-mora-sonarsource]

Summary

• S3626 (RedundantJumpCheck) was incorrectly flagging return; statements inside try blocks as redundant when the try was wrapped in a finally clause inside a loop. Both the return path and the fall-through path share the same CFG successor (the finally block), causing the equality check to misidentify the jump as redundant.
• The fix adds isFinallyBlockWithDistinctContinuation: before reporting, check whether the common successor is a finally block that leads somewhere genuinely different for the return path vs. the fall-through path (i.e., it has a non-exit successor that is not a dead-loop condition block).
• Dead-loop conditions (do-while(false), while(false), and constant-false expressions like Boolean.FALSE or !true) are correctly excluded — returns inside them are redundant.

Related

• RSPEC update: https://github.com/SonarSource/rspec/pull/7186

Test plan

• Existing test cases continue to pass (no regression on // Noncompliant and compliant cases)
• New compliant cases: while/for/do-while loops with early return in try-finally — no issue raised
• New // Noncompliant cases: return at end of method in try-finally, do-while(false), while(false), do-while(Boolean.FALSE), do-while(!true) — issue correctly raised

🤖 Generated with Claude Code

[hashicorp-vault-sonar-prod]

SONARJAVA-6473

[gitar-bot]

Code Review ✅ Approved 4 resolved / 4 findings

Restricts RedundantJumpCheck to prevent false positives when return statements appear inside try-finally loops. Resolved findings include incorrect loop continue handling, alphabetical import order, and improved dead-loop detection.

✅ 4 resolved

✅ Edge Case: FP guard not applied to continue in try-finally loops

📄 java-checks/src/main/java/org/sonar/java/checks/RedundantJumpCheck.java:61 📄 java-checks/src/main/java/org/sonar/java/checks/RedundantJumpCheck.java:70 📄 java-checks/src/main/java/org/sonar/java/checks/RedundantJumpCheck.java:97-103
The new isFinallyBlockWithDistinctContinuation guard in checkBlock is only applied when the terminator is a RETURN_STATEMENT (line 70). However, checkBlock reports redundant jumps for both CONTINUE_STATEMENT and RETURN_STATEMENT (line 61). A continue; inside a try block with a finally clause in a loop produces the same CFG convergence on the shared finally block that caused the original return false positive. For example:

while (cond) {
  try {
    if (c2) continue; // not actually redundant: it skips foo()
    foo();
  } finally { bar(); }
}

Here the continue is wrongly flagged as redundant because both the jump path and the fall-through path converge on the finally block. The fix addresses returns but leaves the symmetric continue case unfixed (note the helper is keyed on exitBlock()/method-exit semantics specific to returns, so it cannot be naively reused for continue, which jumps to the loop header). Consider extending the fix to cover continue and adding a corresponding compliant test case.

✅ Quality: Imports not in alphabetical order (triggers S8445)

📄 java-checks/src/main/java/org/sonar/java/checks/RedundantJumpCheck.java:29-33
The newly added imports are out of alphabetical order: ReturnStatementTree (line 33) is placed after WhileStatementTree (line 32) within the org.sonar.plugins.java.api.tree group. sonar-java enforces import ordering via its own rule S8445 (ImportDeclarationOrderCheck), which is run against this codebase, so this will surface as a self-analysis/quality-gate violation even though it does not break mvn compile. Reorder so ReturnStatementTree precedes WhileStatementTree.

✅ Edge Case: Dead-loop detection omits constant-false for-loops

📄 java-checks/src/main/java/org/sonar/java/checks/RedundantJumpCheck.java:105-117
isDeadLoopExitingTo only recognizes dead loops written as do-while(false)/while(false) (DO_STATEMENT and WHILE_STATEMENT). A for loop with a constant-false condition (e.g. for (;false;) { try { ...; return; } finally {...} }) executes its body at most once just like while(false), so the return is redundant — but it will not be detected as a dead loop and could therefore be incorrectly suppressed by isFinallyBlockWithDistinctContinuation. This is an uncommon edge case, but if for-loop support is intended, handle Tree.Kind.FOR_STATEMENT (ForStatementTree.condition()) as well.

✅ Bug: Removed null guards can NPE when no enclosing try-finally/loop

📄 java-checks/src/main/java/org/sonar/java/checks/RedundantJumpCheck.java:119-133
This commit removed the defensive null checks from enclosingTryFinally (L119-125) and hasFollowingStatementBeforeLoopContinuation (L131-142).

• enclosingTryFinally previously walked while (current != null) and returned null if no try-finally was found. It now loops while (!current.is(Tree.Kind.TRY_STATEMENT) || ((TryStatementTree) current).finallyBlock() == null) with no null check. If current reaches the top of the AST without finding a try-finally with a finally block, current.parent() returns null and the next current.is(...) call throws a NullPointerException.
• hasFollowingStatementBeforeLoopContinuation previously guarded with while (current.parent() != null). It now loops while (!parent.is(WHILE/DO/FOR/FOR_EACH)) and unconditionally dereferences parent. If no enclosing loop is found, parent becomes null and parent.is(...) throws an NPE.

These paths are only reached for a CONTINUE_STATEMENT whose CFG successor is a finally block, so for well-formed code an enclosing try-finally and loop are normally present. However, sonar-java routinely analyzes incomplete or semantically broken source (error-recovery trees), and the previous code defended against exactly this. Removing the guards turns a previously-safe lookup into a potential check crash. The cause where this previously returned a graceful null/false is now an unhandled NPE.

Suggested fix: restore null-safety in both loops.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

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
91.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube