This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 244608 - "The catch is too broad" warning is sometimes wrong
Summary: "The catch is too broad" warning is sometimes wrong
Status: RESOLVED FIXED
Alias: None
Product: java
Classification: Unclassified
Component: Hints (show other bugs)
Version: 8.0
Hardware: PC Windows 7
: P3 normal (vote)
Assignee: Svata Dedic
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-20 17:33 UTC by tln
Modified: 2014-06-19 02:35 UTC (History)
0 users

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description tln 2014-05-20 17:33:59 UTC
There is a specific line in my code where it says:

catch (Exception ex)

This line is marked by Netbeans with a warning saying:
"The catch(java.lang.Exception) is too broad, the actually caught exception is <something else>".

Unfortunately this warning is wrong, I specifically want my code that way, it makes sense, and it is not too broad.

It is probably possible for me to fix this warning using the configuration options in the Hints dialog, but since the problem is here with default settings in place, I thought I should file this bug, so you can consider tuning the defaults a little to accommodate my case properly.

Thank you.
Comment 1 tln 2014-05-20 17:36:13 UTC
Sorry, something went wrong. I meant to say the following, too:

The reason I'm catching java.lang.Exception and not the class suggested is that I want to handle RuntimeExceptions as well in my 'catch' block.
Comment 2 Svata Dedic 2014-05-20 19:10:28 UTC
Could you paste a snippet of the erorr handling code (leaving out details, if appropriate) ?

A construction like
try {
 // some code
} catch (RuntimeException ex) {
 // handle runtime
} catch (SpecificCheckedException ex2) {
 // handle specific
} catch (Umbrella1Exception | Umbrella2Exception ex3) {
 // handle umbrellas in an uniform way
}

should work well without any warnings.

You may also  use @SuppressWarnings("BroadCatchBlock") annotation on the enclosing method to disable the check just for the method's scope.
Comment 3 tln 2014-05-20 20:28:40 UTC
Thanks for your reply. I know there are ways around the warning. My point is slightly different. I'm not asking how to get rid of the warning. I'm trying to point out that this warning should not be a factory default. That's all. :)

Take me for instance. My software is usually strongly modularized (which I consider a very good practise), that's why I often have to wrap Exceptions from other modules to make sure they don't leak the code of the module at hand (which would violate encapsulation).

In other words: In my code you often see this kind of thing:

try
{
   ...
}
catch (Exception ex)
{
   throw new ModuleSpecificException(ex);
}

For instance assume there is a printing module, then it only throws PrintingExceptions, and if there is a file system module, it only emits FileSystemExceptions, etc. The printer module will explicetely never emit a FileSystemException as such (or let it fall through), even if it uses the file system module and the exception might occur. Instead the printing module will wrap this exception when caught, because the fact THAT it uses the file system module is an implementation detail that should under no circumstances be a part of the public API of the printing module if you know what I mean.

Now, because I have been doing things like this for years, I find it very unnatural to be warned about catching and wrapping all exceptions (catching java.lang.Exception). It is often the perfect thing to do (or to code) IMHO, and I don't want to be warned about it as if it were suspicious.

Furthermore, even if this weren't the case, from my point of view the question which exception class to catch is NOT primarily a question of which exceptions are currently thrown but it is more a logical question with respect to the (yet unknown) future of the code. The exception class you catch must logically match the reason why you're catching something. As such a super class of the exceptions actually thrown may be more correct than the specific class. Do you get what I'm trying to say? It's a little hard to explain.

In slightly other words: A catch block handles a set of certain error scenarios. This set can be larger than *currently* needed and could also right now already cover future cases that will not appear before the code grows. If a catch block is able to handle a set of scenarios (including future ones), the exception class caught should best reflect the set of scenarios covered, it should not be limited to the errors *currently* possible. That would be a coupling between different modules that is unbeneficial.

A rought example to illustrate the general idea:
Even if the code in the 'try' block might currently only throw FileNotFoundExceptions, it might still be more correct to catch a super class, like FileNotAccessibleException or FileSystemException.

That's one aspect what I'm trying to say.

---------------

Now, the case 'catch (java.lang.Exception ex)' is very special in another respect. While the above said only refers to reported exceptions, java.lang.Exception covers RuntimeExceptions as well.

As such I simply can't see a reason why

catch (RuntimeException ex)
{
   <code X>
}
catch (AnotherException ex)
{
   <code X again>
}

is supposed to be generally better than just

catch (Exception ex)
{
   <code X>
}

if (and only if) my coding idea was and is to catch everything, no matter what.

Having said this there are also cases where I'd argue the former should be preferred over the latter. It's not like I'm saying the warning is generally wrong. Again, it depends on the logical structure of the code, namely on whether my catch block is really made for covering *all* exceptions, also in the future.

All I'm trying to say is: A general warning should not be a factory default, at least not as it is right now.

If you insist to deliver it like this, then never mind, I'll just turn it off for me. I'm just trying to make you reconsider, but I can very well accept if you still have another opinion than me.

Thanks again, Netbeans is really great. I love it. :D
Comment 4 Svata Dedic 2014-05-20 20:47:30 UTC
The reasoning is sound, but there are many different coding styles. The issue can be seen just from the opposite way: if a totally generic type, like Exception is caught, then exceptions thrown in a future version of code, even those not meant to be wrapped, but rather handled specifically or directly propagate (see below), are caught and "handled". 

Some RTEs are plain programming errors - NPE, ConcurrentModEx, ... under certain coding schemes, these should not be wrapped at module boundaries since they indicate programming error rather than an exceptional state to be handled as module's function failure.

Re. duplicating branches - catching specific types, or common supertypes (but not that generic as an Exception) is actually recommended; in Java 7, multicatch can be used to eliminate catch handler code duplication.

In your case, you could even define j.l.Exception as an 'umbrella' exception, so the hint understands you are using Exception to cover all the hierarchy.


---

This is a second independent well-advocated request to change the default setting; scheduling for next release.
Comment 5 tln 2014-05-20 20:59:38 UTC
We totally agree that mindlessly catching java.lang.Exception out of laziness can cause a lot of damage and waste of time, especially in the long run or when there is a big development team with the usual dynamics. I'm in no way saying that generally catching high-level exceptions is good or even just tolerable. I'm only afraid the issue might be too complex for an automated warning engine, but if you could come up with something that really assists in identifying suspicious catches without causing too many false alarms, I'd really pay you a lot of respect. That'd definitely be a contribution to the robustness of everyone's code.
Comment 6 Svata Dedic 2014-06-06 15:19:10 UTC
Default enablement state changed in jet-main#6ae4162f5760
Comment 7 Quality Engineering 2014-06-19 02:35:28 UTC
Integrated into 'main-silver', will be available in build *201406190001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)

Changeset: http://hg.netbeans.org/main-silver/rev/6ae4162f5760
User: Svata Dedic <sdedic@netbeans.org>
Log: #244608: default enablement changed