KengoTODA / findbugs-slf4j

A SpotBugs/FindBugs plugin to verify usage of SLF4J
http://kengotoda.github.io/findbugs-slf4j/
Apache License 2.0
75 stars 11 forks source link

RFE: New SLF4J_LOST_EXCEPTION #74

Open vorburger opened 6 years ago

vorburger commented 6 years ago

Proposal for a new check which flags this up as wrong:

} catch (SomeException e) {
    LOG.error("...");
}

but this is, typically, what you want people to do instead of course:

} catch (SomeException e) {
    LOG.error("...", e);
}

I'm well aware that there are exceptions to this general rule, but in a large code base, typically this is what you do want, so in my experience enforcing this helps not having lost exceptions in production logs; and experienced senior developers would @SuppressFBWarnings for the exceptional cases when it's not wanted.

Or does a check similar to this already exist in core FindBugs or other quality control tools?

vorburger commented 6 years ago

@KengoTODA would you welcome (and merge) a PR for this?

seanf commented 6 years ago

As far as I'm concerned, if you catch a Throwable, in general you should either log it or rethrow it (perhaps wrapped in another Throwable). But either one is fine.

Is the idea here this? "If there's a log statement, and it's inside a catch block, it must log the exception." So we're not checking every catch block, just the ones which contain logging? And, more generally, we're not checking for rethrows?

If so, that definitely sounds like a good check, but it doesn't seem like it covers everything suggested in https://github.com/spotbugs/discuss/issues/49.

vorburger commented 6 years ago

@seanf just FYI I'm holding further comments & contributions to this project until my #72 moves. I can only encourage you to raise a PR yourself for a check like this to cover everything suggested in https://github.com/spotbugs/discuss/issues/49. PS: I like your (Commodore) logo!