Checker for "unused catch clause variable"?

user_3900485 0
Viewed 208

TL;dr version: (Java) How to check for unused Exception variables in catch clauses to avoid unintentionally swallowing errors. Maybe using tools like checkstyle (through Maven build time plugin) or using a code pattern.

Quite often in your code you write a try/catch block where u catch an internal exception, wrap it in a more meaningful exception (chained exception) and throw upwards in the call stack.

At times, people tend to forget to chain the exception (typos) and throw the wrapper exception without the underlying cause. Missing this out obviously causes debugging nightmares. Is there a way through build time plugins (for Maven) that can detect and warn/error on such instances?

Valid:

try {
    ...
} catch (IOException e)  {
    throw new MyException("API failure", e);
}

Invalid:

try {
    ...
} catch (IOException e) {
    throw new MyException ("API Failure");
}

Also, in case the it is intended to ignore the exception, the checker should be able to allow for a comment to disable rule for that line or maybe name the variable as catch (IOException ignored).

I know checkstyle has a checker for empty catch block, but the above scenario is much broader. Unused local variables doesn't seem to solve it either.

Are there any check tools available to avoid this common mistake?

One dumb solution could be to make all constructors of MyException take Exception as an argument (when no chaining required, pass null explicitly). This doesn't feel like the best way to go about this, hence looking for some kind of checker...

lint checkstyle java
1 Answers

I'm not quite sure 'use the variable' is a check you'd want to enable. In the few catch blocks where you actually (gasp!) have an ability to properly handle the situation (vs. rethrowing it, or that old chestnut, the 'log it and keep going' silliness), then often all you needed to know is that it was thrown. Naming the exception variable ignored just to ensure the linter tool doesn't complain about it then leads to worse code, as ignored as a variable name is commonly understood to mean the entire exception as thrown is being ignored, and not merely the content of the throwable.

You could invent a new term for 'the exception is not being ignored, but all info the throwable object conveys is', of course.

to illustrate:

  public Config getUserConfig() {
      try {
        return Config.parse(Files.readString(Paths.get("myapp.conf")));
      } catch (NoSuchFileException e) {
        return Config.defaultConfig();
      }
  }
}

seems fine to me; either renaming that e to ignored, or having to shove a @SuppressWarnings on there is decidedly suboptimal.

Perhaps the rule could be adjusted to:

  1. If the catch body is completely empty, then it needs either a comment or the throwable varname needs to be ignored.
  2. Any and all throw new statements in any catch body (as in, a throw statement whose argument is a new X() expression) must necessarily have the throwable varname as first or second parameter, or the constructor must be the receiver of the AST-wise enveloping method call, to initCause. That's because not all exception types have constructors that take a cause (cause as a concept didn't exist until java 1.4 or so, I think? So, legacy reasons).

In other words:

throw new Foo(whatever, e); // okay
throw new Foo(whatever).initCause(e); // okay
throw new Foo(whatever); // not okay

Having said all that, no, I'm not aware of any linting tool that can do anything along these lines. It'd have to be a source-level inter, and it shouldn't be all that difficult to write a plugin; most linter tools make it easy to add your own rules. Any LST-based analyser (one that operates on the source, but not as a raw sack of characters, but as an Abstract Syntax Tree which has been fully attributed and linked up, turning it into a Logical Syntax Tree) should make it easy.

user_7686440
Related