[CLJ-855] catch receives a RuntimeException rather than the expected checked exception Created: 11/Oct/11 Updated: 01/Mar/13 Resolved: 29/Feb/12 |
|
| Status: | Closed |
| Project: | Clojure |
| Component/s: | None |
| Affects Version/s: | Release 1.3 |
| Fix Version/s: | Release 1.4 |
| Type: | Defect | Priority: | Major |
| Reporter: | Paul Michael Bauer | Assignee: | Ben Smith-Mannschott |
| Resolution: | Completed | Votes: | 2 |
| Labels: | None | ||
| Attachments: |
|
| Patch: | Code and Test |
| Approval: | Ok |
| Description |
|
Expressions passed to try that trigger the use of clojure.lang.Reflector result in their thrown checked exceptions being wrapped in RuntimeException. As a result, the subsequent set of catches won't switch on the expected checked exception. Attached: patch for regression test that exposes the problem (defn- get-exception [expression]
(try (eval expression)
nil
(catch java.lang.Throwable t
t)))
(deftest catch-receives-checked-exception
(are [expression expected-exception] (= expected-exception
(type (get-exception expression)))
"Eh, I'm pretty safe" nil
'(java.io.FileReader. "CAFEBABEx0/idonotexist") java.io.FileNotFoundException)) ; fails
Clojure ML Thread: https://groups.google.com/forum/#!topic/clojure/I5l1YHVMgkI/discussion |
| Comments |
| Comment by Ben Smith-Mannschott [ 12/Oct/11 2:06 AM ] |
|
(marked up code snippet in description to display as code.) |
| Comment by Ben Smith-Mannschott [ 12/Oct/11 4:23 PM ] |
|
(This incorporates a slightly modified version of Paul Michael Bauer's ClojureException is an RTE. RTEs that originate from Clojure all are WrappedException is a ClojureException. It is used when Clojure is Util.runtimeException(s) now returns a ClojureException Util.runtimeException(s,e) now returns a WrappedException. Util.runtimeException(e) now retuns a WrappedException when e is not clojure.core/treye is a macro built on top of the compiler-provided Paul Michael Bauer's try_catch.clj uses treye in place of try and passes. |
| Comment by Kevin Downey [ 12/Oct/11 5:34 PM ] |
|
> http://james-iry.blogspot.com/2010/08/on-removing-java-checked-exceptions-by.html seems like refactoring Reflector.java to rethrow using this approach |
| Comment by Paul Michael Bauer [ 12/Oct/11 5:42 PM ] |
|
(edit: missed Kevin's post) I'd rather see Reflector.java modified to use this "chucking" technique than have a special try form that specially unwraps an exception and re-throws. Maybe there's a need for WrappedException, but since it's not immediately necessary if we fix Reflector, then I'd rather see it in a separate commit. (I'm swamped at work today and don't have time to fix Reflector myself right now) |
| Comment by Herwig Hochleitner [ 12/Oct/11 9:27 PM ] |
|
I added Util.throwUnchecked and Util.throwCause (also Util.declareThrows and Util.invokeThrowing) as justified in https://groups.google.com/d/msg/clojure-dev/4QynV81W0Qk/rIN9fYUK-AkJ There are a lot of other instances in the code, where checked exceptions are runtime-ified, but that's a separate issue. |
| Comment by Ben Smith-Mannschott [ 13/Oct/11 1:07 AM ] |
|
Applying the patch catch (Exception e)
{
Util.throwCause(e);
}
This leaves the compiler complaining about a missing return. catch (Exception e) { return Util.throwCause(e); } compiles. Also, the patch doesn't actually cause try_catch.clj to pass. I suspect the RuntimeException being caught by try_catch.clj is not, in fact, originating from one of the four points you have corrected. (Indeed, I discovered something similar when working on my patch, but it was late, and I ended up solving it in such a way that I didn't have to find the actual source of this particular wrapped exception.) I'm still investigating... |
| Comment by Paul Michael Bauer [ 13/Oct/11 1:11 AM ] |
|
@Herwig thanks. |
| Comment by Ben Smith-Mannschott [ 13/Oct/11 1:21 AM ] |
|
The wrapping RuntimeException that causes try_catch.clj to fail is actually coming out of Compiler.eval(Object,boolean) without involving Reflector at all.
|
| Comment by Herwig Hochleitner [ 13/Oct/11 10:06 AM ] |
|
Oh, it seems I was too fast with the second commit. That's kind of embarrassing, sorry about that. I had considered the issue with only rethrowing causes that are instances of Exception or Error. I dismissed it, because I considered it an artifact of not having chucked exceptions before. The throwCause is for unwrapping InvokationTargetExceptions and such. The only case where it could unwrap differently than the original code, is when having causes other than Exception or Error. i.e. a third subclass of Throwable, which is mostly unheard of (but should probably be unwrapped too). Rich, do you remember, if there are any further implications to the catch idiom in Reflector? catch(Exception e) { if(e.getCause() instanceof Exception) throw Util.runtimeException(e.getCause()); else if(e.getCause() instanceof Error) throw (Error) e.getCause(); throw Util.runtimeException(e); } Regarding the actual source of the wrapping (Compiler.eval): It showed up in my `rgrep catch(Exception`, but I considered it part of the "other issue" (of general refactoring). In fact, I think it won't show up, mostly, because people noticed this issue when having reflective calls in clojure code, which don't use eval. So the test case is probably testing another source of RuntimeException wrapping. This shows, IMO, that most occurrences of `throw Util.runtimeException` should be replaced by `return Util.throwUnchecked`, but probably not all. This will require some scrutiny and careful testing. We need to consider every `catch (Exception e)` in the clj code base. I'll take a stab at it, tonight (CEST), also testing properly this time (sorry again, had some troubles with my build system yesterday) |
| Comment by Ben Smith-Mannschott [ 13/Oct/11 4:21 PM ] |
|
I've added a unit test which provokes this issue in the Reflector, as opposed to in the Compiler by way of eval. You can browse it here: https://github.com/bpsm/clojure/commits/CLJ-855 I've refined my patch using the "try unwraps" strategy. The refined patch renames Clojure's try special form to try* and provides try with transparent unwrapping as a macro on top of try*. The curious may look here: https://github.com/bpsm/clojure/commits/CLJ-855-try-unwraps I'll post new patches after I've had some sleep. I intend to try my hand at the "sneaky throws" strategy that seems to be all the rage here. |
| Comment by Ben Smith-Mannschott [ 14/Oct/11 12:04 AM ] |
|
|
| Comment by Ben Smith-Mannschott [ 14/Oct/11 2:34 PM ] |
|
Using "sneaky throw" allows us to effectively rethrow checked exceptions without needing to declare or catch them. In effect, we're back to where we were before wrapping them all up in RTEs (8fda34e4c77...), but without needing to pollute seemingly every method signature in the Java portion of Clojure core with "throws Exception" This patch was less work than I expected (thanks be to sed) but I'm not as confident in its correctness as I am in that of the try-unwrap variant. I can't claim to have really groked Clojure's approach to exceptions. I don't understand why Reflector gives preference to rethrowing the cause of the exception it catches (especially now that we're no longer wrapping all over the place.) I also don't understand why Var.dissoc returns the exception it catches rather than throwing it. (I've left a big fat TODO comment there, which should go from any final version of this patch, should it be accepted.) |
| Comment by Paul Stadig [ 10/Feb/12 9:08 AM ] |
|
Another option, not to make things even more complicated, is to revert the original commit. Perhaps I'm missing the rationale, but... Checked exceptions are irrelevant to Clojure code, since they are a fabrication of the Java compiler. Using Clojure code from the Java side could be confusing if you call invoke on IFn and get a checked exception even though it is not declared with a throws clause. It is also confusing to get checked exceptions wrapped in one or more layers of RTEs. Was it not sufficient the way it was before having IFn declare "throws Exception"? |
| Comment by Rich Hickey [ 29/Feb/12 3:06 PM ] |
|
|