Warn for catching RuntimeException, Exception and Throwable

[baloghadamsoftware]

According to SEI CERT rule ERR08-J catching RuntimeException, Exception and Throwable is a bad practice because it may hide exceptions which must be handled or avoided. This PR extends detector RuntimeExceptionCapture to warn for these cases.

---

Make sure these boxes are checked before submitting your PR -- thank you!

• Added an entry into CHANGELOG.md if you have changed SpotBugs code

[iloveeclipse]

This PR will produce again a lot of false positives. The idea of catching exact the exception types that some code might throw and don’t catch others is purely academic. In most cases exception handlers that catch Exception or Throwable are written because they want catch all potential exceptions from the try block, not only some specific. I know lot of absolutely valid code that does that.

So this new checks should be disabled by default.

[baloghadamsoftware]

Thank you for your comment @iloveeclipse!

I agree that this PR produces several new bug reports. However, based on my experience most of these reports are true positives. Unfortunately, many programmers are lazy to properly handle exceptions. Beside the true positives there are of course some false positives because you are right, there are some cases where you must catch all the exceptions. The SEI CERT rule itself describes such cases.

Case ERR08-J-EX0 where the exception is rethrown (possibly after logging) is handled. Case ERR08-J-EX2 only affects some special software. Where we can reduce the false positives are the cases described in ERR08-J-EX1: we must check for methods called in the try block whether they are methods of a class received as a generic parameter of the current method or the class. It is a valid case to catch RuntimeException (and maybe the other two) when calling such methods. Could you please mention some more cases?

I think that setting a check to disabled by default is the last resort. First we should try fix it by recognizing corner cases and reducing the number of false positives. Only if that effort fails should we disable the checker by default.

[baloghadamsoftware]

This far the results:

0 findings on SpotBugs itself and on Jenkins

7 Exceptions and 2 Throwables in NanoHTTPD: I checked several of them, they seem to be true positives.

4 Exceptions and 7 Throwables in Ttorrent: the same as above.

36 Exceptions and 5 Throwables in Bt: I checked some of them and I do not see any reasons for not specifying the possible exceptions.

26 Exceptions, 2 RuntimeExceptions and 7 Throwables in OpenGrok: Could not find any obvious false positives.

59 Exceptions, 1 RuntimeException and 1 Throwable in MATSim-Libs: the Throwable may be questionable, perhaps the RuntimeException but there a lots of obvious true positives among the Exceptions.

[baloghadamsoftware]

@iloveeclipse Any comments after seeing the statistics?

[hazendaz]

e ERR08-J-EX0 where the exception is rethrown (possibly after logging) is handled. Case ERR08-J-EX2 only affects some special software. Where we can reduce the false positives are the cases described in ERR08-J-EX1: we must check for methods called in the try block whether they are methods of a class received as a generic parameter of the current method or the class. It is a valid case to catch RuntimeException (and maybe the other two) when calling such methods. Could you please mention some more cases?

I don't think the issue should be active by default either because sometimes its other libraries that abuse usage and that would be a false positive on wrong code bases. For example, Selenium is really bad about using Exception. Even worse with Selenium, is that they have unchecked exceptions that should actually be checked exceptions. Anyways point is that its not always in direct developer control. I agree we mark them but not as a default setting. Note: I didn't review this PR so I didn't check to see if that was modified, instead what I did was resolve most of the coding notes others left.

[ThrawnCA]

Loops should always use braces.

[hazendaz]

@baloghadamsoftware If you have this set as disabled by default, it can come in. I personally like this but I know many libraries abuse what they catch and then rethrow which would cause the false positive but I personally would enable this to get my staff to be smarter about exception handling. Its not like sonar already doesn't say same thing ;) But let users opt into this one.