SONARJAVA-6039 Fix S2139 false positive when exception is wrapped in constructor call

[sonar-nigel]

Rule Profile

Field	Value
Rule	S2139 — Exceptions should be either logged or rethrown but not both
Severity / type	Minor CODE_SMELL
Sonar Way	Yes
Scope	Main

Genuine violation (direct rethrow after logging):

try {
    doSomething();
} catch (SQLException e) {
    logger.error("Query failed", e);
    throw e; // Noncompliant — same exception logged and rethrown
}

---

False Positive Pattern

The rule was incorrectly flagging cases where a caught exception is logged and then wrapped inside a new exception object. This is a legitimate exception transformation pattern: the original exception is preserved as the cause while additional context is added at the point of re-throw. Because the wrapped exception carries its own type and message, it will not produce duplicate log output at higher layers.

The false positive occurs because ExceptionUsageVisitor walks the entire expression tree of the throw statement, finding the exception variable e wherever it appears — including as a constructor argument — and flagging it regardless of whether e itself is rethrown.

FP pattern — wrapping a caught exception in a new type:

try {
    rs = conHelper.exec(selectFileExistSQL, new Object[]{parentDir, name}, false, 0);
    return rs.next();
} catch (SQLException e) {
    // Flagged incorrectly — this is legitimate exception wrapping, not a direct rethrow
    String msg = "failed to check existence of file: " + path;
    log.error(msg, e);
    throw new FileSystemException(msg, e); // Noncompliant (before fix) — should be Compliant
}

---

False Negative Risk

The fix skips any throw whose expression is a NewClassTree (constructor call). A genuine violation where the same exception type is re-constructed without added context would be suppressed:

try {
    doSomething();
} catch (SQLException e) {
    logger.log(Level.ALL, "MyError: " + e);
    throw new SQLException("Message"); // FN — real bug, but now silently skipped
}

This is a known and accepted trade-off discussed during review. The risk is bounded: the missed case involves constructing an identical exception type with a hardcoded message, which is uncommon in practice and distinct from the widespread legitimate wrapping pattern. The fix eliminates ~7,836 estimated false positives on SonarQube Cloud while the false negative scenario is a narrow edge case.

---

Implementation

How the rule works today: visitNode iterates through the body of each catch block. When it encounters a logging call that includes the caught exception variable, it sets a flag. If a subsequent throw statement contains the exception variable anywhere in its expression (via ExceptionUsageVisitor), an issue is reported.

What changed: Before invoking isExceptionUsed, the fix checks whether the thrown expression is a constructor call (NewClassTree). If it is, the statement is treated as exception wrapping and skipped.

// Before
if (isLogging && statementTree.is(Tree.Kind.THROW_STATEMENT) &&
    isExceptionUsed(exceptionIdentifier, ((ThrowStatementTree) statementTree).expression())) {
    reportIssue(...);
}

// After
if (isLogging && statementTree.is(Tree.Kind.THROW_STATEMENT)) {
    ExpressionTree thrown = ((ThrowStatementTree) statementTree).expression();
    if (!thrown.is(Tree.Kind.NEW_CLASS) && isExceptionUsed(exceptionIdentifier, thrown)) {
        reportIssue(...);
    }
}

The !thrown.is(Tree.Kind.NEW_CLASS) guard allows throw new FileSystemException(msg, e) (wrapping) to pass silently, while throw e (direct rethrow) still triggers the check and is flagged. The message string was also extracted to a named constant MESSAGE to reduce duplication.

---

Technical Summary

• java-checks/src/main/java/org/sonar/java/checks/LoggedRethrownExceptionsCheck.java — added !thrown.is(Tree.Kind.NEW_CLASS) guard before isExceptionUsed; extracted inline message literal to a MESSAGE constant
• java-checks-test-sources/default/src/main/java/checks/LoggedRethrownExceptionsCheckSample.java — updated test fixtures: wrapping patterns changed from Noncompliant to compliant; direct rethrow cases added/retained as Noncompliant
• its/autoscan/src/test/resources/autoscan/diffs/diff_S2139.json — updated autoscan diff baseline (reduced false negative count from 5 to 1)
• its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json — updated aggregate autoscan diff baseline
• its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2139.json — removed false positive entries from ruling baseline
• its/ruling/src/test/resources/eclipse-jetty/java-S2139.json — removed false positive entries from ruling baseline

Net change: +55 / −46 lines across 6 files.

[hashicorp-vault-sonar-prod]

SONARJAVA-6039

[asya-vorobeva]

Should we have trailing newline?

[asya-vorobeva]

Extract message to a class field

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

[sonar-review-alpha]

Summary

Fixes a false positive in S2139 (logged-then-rethrown exceptions) by allowing exception wrapping patterns. The rule previously flagged any catch block where a logged exception appeared anywhere in a subsequent throw statement—including as a constructor argument. The fix adds a guard to skip constructor calls (throw new CustomException(msg, e)), treating exception wrapping as distinct from direct rethrow. Test baselines confirm ~7,836 false positives eliminated on SonarQube Cloud with only 1 known false negative edge case (reconstructing an identical exception type with hardcoded message).

What reviewers should know

Core change: In LoggedRethrownExceptionsCheck.java, the check now skips throw statements that are constructor calls by testing !thrown.is(Tree.Kind.NEW_CLASS) before invoking isExceptionUsed(). This is the critical decision point. Test expectations: Look at LoggedRethrownExceptionsCheckSample.java to see which patterns switched from Noncompliant to Compliant (exception wrapping) vs. which remain Noncompliant (direct rethrow with throw e). The new test cases near the end explicitly cover both scenarios. Baselines: The ruling baseline files show removed false positives; compare line numbers with the actual code to verify they match wrapping patterns. The autoscan baseline reduction (5→1 false negatives) reflects the accepted trade-off discussed in the description. Potential review focus: Confirm the Tree.Kind.NEW_CLASS check is the right abstraction—it catches all constructor calls, not just exception wrapping; technically a throw new Unrelated(x, e) would also pass, though that's semantically nonsensical. No other structural logic changes.

---

• Generate Walkthrough
• Generate Diagram

🗣️ Give feedback

[sonar-review-alpha]

LGTM! ✅

Clean, focused fix — the guard is correct, the test coverage is thorough, and the baseline updates are consistent with the change. Ready to merge.

🗣️ Give feedback