Community
Participate
Working Groups
Since upgrading to Eclipse 4.5 integration build ID I20140715-0800, the following code snippet now triggers a compiler warning/error: try { ... } catch (Exception e) { // do nothing } The error is: The value of the exception parameter e is not used Turning off the setting "Value of parameter is not used" in the compiler settings removes this error. We have that setting enabled, for example to warn of unused parameters in private methods. Is it a design decision to have this setting also apply to exception parameters, or is just a bug?
It is done on purpose. Please see Bug 412119.
Marking as resolved.
I'd like to reopen this one to discuss whether the same preference key should be used for unused parameters on methods - which can be removed by the user - and unused parameters in catch blocks - which are mandatory according to the Java syntax. I don't think that code like try { .. } catch(Exception e) { .. some comment or some fallback logic .. } is necessarily bad style. Some APIs mandate exceptions to be caught. Could you please reconsider to use a new preference for the new validation?
(In reply to Sebastian Zarnekow from comment #3) > I'd like to reopen this one to discuss whether the same preference key > should be used for unused parameters on methods - which can be removed by > the user Good point. Please note we have bug 439520 to cover the quick fix part of it. (In reply to Sebastian Zarnekow from comment #3) > I don't think that code like try { .. } > catch(Exception e) { .. some comment or some fallback logic .. } is > necessarily bad style. Some APIs mandate exceptions to be caught. Could you > please reconsider to use a new preference for the new validation? I agree with your point. Copying Markus, as he decides on the UI side of preference.
(In reply to comment #3) > I'd like to reopen this one to discuss whether the same preference key should be > used for unused parameters on methods - which can be removed by the user - and > unused parameters in catch blocks - which are mandatory according to the Java > syntax. They are not mandatory in the same way unused method arguments are not mandatory, i.e. if you can freely change the signature of the method to remove an unused argument you can also change it to add the exception to its throws specification. (In reply to comment #3) > I don't think that code like try { .. } catch(Exception e) { .. some > comment or some fallback logic .. } is necessarily bad style. Some APIs mandate > exceptions to be caught. Could you please reconsider to use a new preference for > the new validation? Yeah, it is bad style. Happily this works as expected and allows for the questionable code to be found easily later: try { Thread.sleep(10000); } catch (@SuppressWarnings("unused") InterruptedException e) { }
Jay, let us raise a new bug against UI, document it here and resolve the present entry as it does not call for any action in Core as we know it now.
Looking at the Eclipse codebase, I found quite a few places where swallowing the exception is OK. Example: try { storeUrl(new URL("http://www.eclipse.org")); } catch (MalformedURLException e) { } Or: try { // do something risky return result; } catch (IllegalStateException e) { // didn't work out } // fall back code / return null... We have diagnostics for "Undocumented empty block" and for "Unused parameter" (now including exception parameters), which both make sense. But it's not nice that I have to add two notes like this to make the warnings go away: } catch (@SuppressWarnings("unused") MalformedURLException e) { // can't happen } Or do something horrible like: } catch (MalformedURLException e) { e.hashCode(); } Can we consider the @SuppressWarnings("unused") as documentation for the empty block and at least avoid the second problem? Or we bite the bullet and add a separate compiler option for unused exception parameters.
(In reply to comment #7) > Looking at the Eclipse codebase, I found quite a few places where swallowing the > exception is OK. Neither of the examples are convincing. > Example: > > try { > storeUrl(new URL("http://www.eclipse.org")); > } catch (MalformedURLException e) { > } Should be rewritten as try { storeUrl(new URL("http://www.eclipse.org")); } catch(MalformedURLException shouldNeverHappen) { throw new AssertionError(shouldNeverHappen); } > try { > // do something risky > return result; > } catch (IllegalStateException e) { > // didn't work out > } > // fall back code / return null... Can be rewritten as private static Object test() { Object result; try { result = doSomethingRisky(); } catch (@SuppressWarnings("unused") IllegalStateException e) { result = emptyObject(); } return result; } As rewritten, both of the examples are more explicit about the assertions being made in the code and avoid the warnings about empty blocks (which I personally find to be of little value anyways). The point of warnings is to find questionable and buggy code; I'd say this warning is having a perfect track record so far.
Looks like we still have a discussion with no conclusion yet. Will take a call during M2.
I strongly disagree with unused catch parameter check being lumped together with the unused parameter setting. I'm not bothered at all whether the catch parameter is used or not -- it all depends on the context. I am, however, bothered about unused method parameters and do want Eclipse to tell me about those. There are numerous cases where ignoring the catch parameter makes perfect sense. You may or may not agree with some or all of those but for some users this is the preferred way of structuring their code. IMHO forcing the user to dot their code with @SuppressWarnings("unused") is a step backwards in usability as for some (many?) this will be just noise obscuring the actual, useful code. Obviously some people want those warnings, as indicated by bug 412119, so please implement a separate setting in the compiler warnings/errors section for the people who don't consider this a valuable information. A few of examples where not using the exception parameter seems to be reasonable: 1. The exception type is specific enough try { url = new URL(myUrl); } catch(MalformedURLException e) { log("Invalid URL: "+ myUrl); return false; } 2. The exception can be safely ignored if shorter delay doesn't change the behaviour of the app try { Thread.sleep(500L); } catch (final InterruptedException e) { // } 3. The exception type is specific enough try { packageInfo = activity.getPackageManager().getPackageInfo(name, flags); } catch (final NameNotFoundException e) { throw new IllegalArgumentException("Invalid name: "+ name); } (using Android API) 4. We've all seen libraries which just throw checked Exceptions, for odd reasons and with no messages whatsoever, didn't we? try { badlyDesignedLibraryThrowingUselessExceptions.doSomething(); } catch (Exception e) { throw new RuntimeException("Exception while calling BadLibrary"); } 5. If the connection fails the specific reason doesn't matter try { conn = (L2CAPConnection)Connector.open(address, Connector.WRITE, true); } catch (final IOException e) { return null; } (using JRS82) 6. As above -- all we care about is that the operation failed RemoteDevice d = ... try { name = d.getFriendlyName(false); } catch (IOException e) { throw new RuntimeException("Connection to device "+ d +" failed"); } (using JRS82) 7. The exception type is not specific at all but in this context it can be thrown for one reason only try { Display.create(pf, attr); } catch (final LWJGLException e) { throw new RuntimeException("Unable to create display: "+ pf); } (with LWJGL) 8. The exception type is specific enough try { myClass = Class.forName(myClassName); } catch (final ClassNotFoundException e) { throw new RuntimeException("Class not found:"+ myClassName); } 9. The exception type is specific enough try { return new FileInputStream(f); } catch (final FileNotFoundException e) { return null; }
I understand the concerns raised and see why people would like to keep these two warning/preference separate. I have raised UI bug 439750 to discuss this.
(In reply to Jayaprakash Arthanareeswaran from comment #11) > I have raised UI bug 439750 to discuss this. Correction, the UI part is discussed in bug 441933.
It looks like there's no general consensus on how useful this diagnostic is. The best we can do at this point is give the user a choice and create a separate compiler option (set to "ignore" by default).
(In reply to Markus Keller from comment #13) > It looks like there's no general consensus on how useful this diagnostic is. > The best we can do at this point is give the user a choice and create a > separate compiler option (set to "ignore" by default). Agree with this, we don't seem to have consensus on other options.
Gerrit patch uploaded here: https://git.eclipse.org/r/#/c/37453/
Fix pushed in master: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=f73fdb0a354f2f38e3faeb7d9eceabb3d85530d5
Verified for Eclipse Mars 4.5 M4 using build I20141208-0800
*** Bug 448738 has been marked as a duplicate of this bug. ***
