javac warnings for multi-catch

[David Holmes]

Gernot Neppert said the following on 04/27/11 16:56:
> Interesting observation!
> One might argue that the compiler warning points to a flaw in the
> original code: even though it 'works' as intended,
> it's not really 'clean code'. The author obviously did not trust the
> official API of task.run().

We do not trust the programmers that implement run(). The code we right 
is robust in the face of user-defined code that does unexpected things. 
The code might "innocently" invoke Class.newInstance for a class where 
the constructor can throw a checked-exception.

> If he had, catching RuntimeException and Error would have sufficed.
> Rather, he wanted to catch 'sneaky' checked Exceptions, too.
> So he used 'catch Throwable' to cover for that, even though there are
> no Throwable-derivatives other than 'Error' and 'Exception'.

Users can define their own subclass of Throwable - which would be a 
checked exception of course.

> To sum it up, I'd suggest the following code as a better, more concise
> way of expressing the intent:
> 
> try {
>            task.run();
>      } catch (Exception x) {
>                   thrown = x; throw x;
>      } catch (Error x) {
>                   thrown = x; throw x;
>      } finally {
>                  afterExecute(task, thrown);
>      }
> 
> Does the Java-7 compiler issue a warning for this, too?

Don't know but it can't be used because the Exception is checked and so 
we can't rethrow it unless we declare it.

David


> 
> 2011/4/26 David Holmes <David.Holmes at oracle.com>:
>> I have a query regarding the warnings that javac now issues that are
>> related to the work on multi-catch that does more precise analysis of
>> the exceptions that can be thrown. Here's an example:
>>
>> ../../../src/share/classes/java/util/concurrent/ThreadPoolExecutor.java:1115:
>> warning: unreachable catch clause
>>                     } catch (Throwable x) {
>>                       ^
>>   thrown types RuntimeException,Error have already been caught
>>
>> and here's the code:
>>
>>                     try {
>>                         task.run();
>>                     } catch (RuntimeException x) {
>>                         thrown = x; throw x;
>>                     } catch (Error x) {
>>                         thrown = x; throw x;
>>                     } catch (Throwable x) {
>>                         thrown = x; throw new Error(x);
>>                     } finally {
>>                         afterExecute(task, thrown);
>>                     }
>>
>> This pattern is not uncommon. The final catch of Throwable is there to
>> close the gap that exists because of the API's (Class.newInstance) that
>> allow you to throw checked-exceptions from methods that don't declare
>> them. So the catch clause is not in fact unreachable.
>>
>> It seems to me that these warnings will encourage people to remove the
>> "offending" catch clause and actually end up with code that is not
>> robust because of these historical flaws in the API's (not to mention
>> native calls).
>>
>> Perhaps it would be better to restrict the warnings to the cases where a
>> previous clause will catch the exception expected by the later clause,
>> rather than incorrectly assuming which exceptions can be thrown from the
>> methods in the try-block.
>>
>> David
>>
>>