Request: Update rule java:S1166 (either log or rethrow) for cases using unnamed varliables

Rules and Languages
,
1

Rule java:S1166 “Exception handlers should preserve the original exceptions”

states that
”When handling a caught exception, the original exception’s message and stack trace should be logged or passed forward.”

and gives a noncompliant code example

try {
  /* ... */
} catch (Exception e) {   // Noncompliant - exception is lost
  LOGGER.info("context");
}

Now

JEP 456: Unnamed Variables & Patterns (Java 22)
explicitly suggests the use of unnamed variables in catch clauses:

String s = ...
try {
    int i = Integer.parseInt(s);
    ... i ...
} catch (NumberFormatException _) {        // Unnamed variable
    System.out.println("Bad number: " + s);
}

https://openjdk.org/jeps/456

The implementaion of the sonar rule already allows exceptions for common patterns e.g. for NumberFormatException etc.

I suggest that allowing a similar handling for all unnamed variables would be consistent with the development of Java and giving the developer the freedom how to handle exceptions at their own discression.

3

In addition this rule seems to be in conflict with

Rule java:S7467
Unused exception parameter should use the unnamed variable pattern

This rule S1166 fails even though rule java:S7467 seems to explicitly state that unnamed variables should be allowed or prefered.

Both rules make sense at the same time, once the unnamed variables are allowed in rule S1166.

7

Thank you for the suggestion! As Java evolves, we update our rules accordingly, but occasionally some are overlooked. I’ve created a ticket, SONARJAVA-6095 based on your report. The team will discuss it and decide on the next steps for implementation.

9

Excellent paraphrase, thank you.

Maybe you want to include a reference to the similar rule (that does not work because of conflict with java:S1166)

Rule java:S7467 Unused exception parameter should use the unnamed variable pattern

And apart from that I am a but confused:

In addition to the contribution in the existing PR for an implementation (I signed the contributor agreement)

there is now a similar (identical ?) issue:

Doen’t really matter to me.

I just hope the change lands in sonar, soon one way or the other.

1 Like
10

I am documenting the use of unnamed variables in S1166 and adding a reference to S7467 to the rule description in a PR, which may not be publicly visible.

Regarding the second implementation, I missed your contribution and implemented the change myself. Internal PRs are easier to merge because they do not require additional legal processes. Since you mentioned that this is fine with you, I will proceed with my implementation.

Thank you for contributing to our community!

1 Like
12

Sorry for being a pain.

Your implementation is most likely wrong or at least not what you – or the language team –intended for this rule.

Your implementation does not catch the non-compliant inner catch (and your code lacks this test) in the below example. This is already the case for the existing excluded trivial cases where this might be ok to stop checking for further cases, but it is most likely not generally correct or the intention of this rule to omit further tests.

try {
} catch (Exception _) {                     // Compliant
  try {
  } catch (Exception z) { // Noncompliant {{Either log or rethrow this exception.}}
  }
}

As I said I signed the contributor agreement, so do with my implementation and especially my tests (and me commit message explaning this) what you want.

The current impl might come back as an error issue.

13

Thank you for catching this! I’ve created SONARJAVA-6126 to track this work. We will review your contribution soon.

1 Like