Clojure

catch receives a RuntimeException rather than the expected checked exception

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.3
  • Fix Version/s: Release 1.4
  • Component/s: None
  • Labels:
    None
  • 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
Introduced: https://github.com/clojure/clojure/commit/8fda34e4c77cac079b711da59d5fe49b74605553

  1. checked-exception-regression-test.diff
    11/Oct/11 8:44 PM
    2 kB
    Paul Michael Bauer
  2. CLJ-855_throw_undeclared_checked.patch
    12/Oct/11 9:27 PM
    5 kB
    Herwig Hochleitner
  3. CLJ-855-sneaky-throw.patch
    14/Oct/11 2:34 PM
    22 kB
    Ben Smith-Mannschott
  4. CLJ-855-try-unwraps.patch
    14/Oct/11 12:04 AM
    17 kB
    Ben Smith-Mannschott

Activity

Hide
Rich Hickey added a comment -

CLJ-855-sneaky-throw.patch applied. Thanks Ben!

Show
Rich Hickey added a comment - CLJ-855-sneaky-throw.patch applied. Thanks Ben!
Hide
Paul Stadig added a comment -

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"?

Show
Paul Stadig added a comment - 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"?
Hide
Ben Smith-Mannschott added a comment - - edited

CLJ-855-sneaky-throw.patch

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.)

Show
Ben Smith-Mannschott added a comment - - edited CLJ-855-sneaky-throw.patch 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.)
Hide
Ben Smith-Mannschott added a comment -

CLJ-855-try-unwraps.patch:

  1. Teaches the Java portion of Clojure core to use WrappedException (a RuntimeException) when wrapping checked exceptions for re-throwing.
  2. Changes try to transparently unwrap (only!) WrappedException, passing the cause on to be handled by one of its catch clauses. This appears to solve CLJ-855.
  3. The original behavior of the try special form (no transparent unwrapping) is still available under the name try*.
Show
Ben Smith-Mannschott added a comment - CLJ-855-try-unwraps.patch:
  1. Teaches the Java portion of Clojure core to use WrappedException (a RuntimeException) when wrapping checked exceptions for re-throwing.
  2. Changes try to transparently unwrap (only!) WrappedException, passing the cause on to be handled by one of its catch clauses. This appears to solve CLJ-855.
  3. The original behavior of the try special form (no transparent unwrapping) is still available under the name try*.
Hide
Ben Smith-Mannschott added a comment - - edited

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. My experimentation with Herwig's patch showed that it will require investigating some failing unit tests. I believe it will prove a more invasive change than "try unwraps", but may be worth it. A nice challenge.

Show
Ben Smith-Mannschott added a comment - - edited 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. My experimentation with Herwig's patch showed that it will require investigating some failing unit tests. I believe it will prove a more invasive change than "try unwraps", but may be worth it. A nice challenge.
Hide
Herwig Hochleitner added a comment - - edited

Oh, it seems I was too fast with the second commit. That's kind of embarrassing, sorry about that.
Please only consider the first commit of the patch, where I implemented the chucked exception helpers.

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)

Show
Herwig Hochleitner added a comment - - edited Oh, it seems I was too fast with the second commit. That's kind of embarrassing, sorry about that. Please only consider the first commit of the patch, where I implemented the chucked exception helpers. 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)
Hide
Ben Smith-Mannschott added a comment -

The wrapping RuntimeException that causes try_catch.clj to fail is actually coming out of Compiler.eval(Object,boolean) without involving Reflector at all.

CLJ-855 appears to be about more than just oddness in Reflector.java. My guess is it requires a more general solution.

Show
Ben Smith-Mannschott added a comment - The wrapping RuntimeException that causes try_catch.clj to fail is actually coming out of Compiler.eval(Object,boolean) without involving Reflector at all. CLJ-855 appears to be about more than just oddness in Reflector.java. My guess is it requires a more general solution.
Hide
Paul Michael Bauer added a comment -

@Herwig thanks.
A few issues with the patch:
1. doesn't build. Use "return Util.throwCause(...)" instead of just "Util.throwCause(...)" Otherwise the compiler complains about a missing return statement.
2. could you observe the whitespace conventions in the rest of the Java source files (tabs for indentation)?
3. The semantics of throwCause are different than before. Previously, we threw the Throwable only if t wasnt 1) null 2) an instance of Exception or 3) instance of Error. Subtle, and maybe it's a non-issue?

Show
Paul Michael Bauer added a comment - @Herwig thanks. A few issues with the patch: 1. doesn't build. Use "return Util.throwCause(...)" instead of just "Util.throwCause(...)" Otherwise the compiler complains about a missing return statement. 2. could you observe the whitespace conventions in the rest of the Java source files (tabs for indentation)? 3. The semantics of throwCause are different than before. Previously, we threw the Throwable only if t wasnt 1) null 2) an instance of Exception or 3) instance of Error. Subtle, and maybe it's a non-issue?
Hide
Ben Smith-Mannschott added a comment - - edited

Applying the patch CLJ-855_throw_undeclared_checked.patch produces java that can not be compiled in four places:

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...

Show
Ben Smith-Mannschott added a comment - - edited Applying the patch CLJ-855_throw_undeclared_checked.patch produces java that can not be compiled in four places:
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...
Hide
Herwig Hochleitner added a comment - - edited

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
I've used this to fix the invoke* methods in Reflector. Please review.

There are a lot of other instances in the code, where checked exceptions are runtime-ified, but that's a separate issue.

Show
Herwig Hochleitner added a comment - - edited 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 I've used this to fix the invoke* methods in Reflector. Please review. There are a lot of other instances in the code, where checked exceptions are runtime-ified, but that's a separate issue.
Hide
Paul Michael Bauer added a comment - - edited

(edit: missed Kevin's post)
http://james-iry.blogspot.com/2010/08/on-removing-java-checked-exceptions-by.html

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)

Show
Paul Michael Bauer added a comment - - edited (edit: missed Kevin's post) http://james-iry.blogspot.com/2010/08/on-removing-java-checked-exceptions-by.html 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)
Hide
Kevin Downey added a comment -

> http://james-iry.blogspot.com/2010/08/on-removing-java-checked-exceptions-by.html

seems like refactoring Reflector.java to rethrow using this approach
is preferable to some wrapping/unwrapping scheme

Show
Kevin Downey added a comment - > http://james-iry.blogspot.com/2010/08/on-removing-java-checked-exceptions-by.html seems like refactoring Reflector.java to rethrow using this approach is preferable to some wrapping/unwrapping scheme
Hide
Ben Smith-Mannschott added a comment -

(This incorporates a slightly modified version of Paul Michael Bauer's
checked-exception-regression-test patch.)

ClojureException is an RTE. RTEs that originate from Clojure all are
of this type. This isn't strictly necessary for this proof of concept,
but it seemed prudent since it seems that the lack of specificity resulting
from using plain RTE everywhere got us into this mess in the first place.

WrappedException is a ClojureException. It is used when Clojure is
wrapping the underlying cause to work around Java's obsession with
checked exceptions. It's always the underlying cause of
WrappedException that is relevant to catchers.

Util.runtimeException(s) now returns a ClojureException
This is not strictly necessary for this patch to work

Util.runtimeException(s,e) now returns a WrappedException.

Util.runtimeException(e) now retuns a WrappedException when e is not
already a RuntimeException.

clojure.core/treye is a macro built on top of the compiler-provided
special form try. (A real solution to this problem would involve
altering the try special form itself, but this ist just a trial balloon
to figure out if this is the right way to go.)

Paul Michael Bauer's try_catch.clj uses treye in place of try and passes.

Show
Ben Smith-Mannschott added a comment - (This incorporates a slightly modified version of Paul Michael Bauer's checked-exception-regression-test patch.) ClojureException is an RTE. RTEs that originate from Clojure all are of this type. This isn't strictly necessary for this proof of concept, but it seemed prudent since it seems that the lack of specificity resulting from using plain RTE everywhere got us into this mess in the first place. WrappedException is a ClojureException. It is used when Clojure is wrapping the underlying cause to work around Java's obsession with checked exceptions. It's always the underlying cause of WrappedException that is relevant to catchers. Util.runtimeException(s) now returns a ClojureException This is not strictly necessary for this patch to work Util.runtimeException(s,e) now returns a WrappedException. Util.runtimeException(e) now retuns a WrappedException when e is not already a RuntimeException. clojure.core/treye is a macro built on top of the compiler-provided special form try. (A real solution to this problem would involve altering the try special form itself, but this ist just a trial balloon to figure out if this is the right way to go.) Paul Michael Bauer's try_catch.clj uses treye in place of try and passes.
Hide
Ben Smith-Mannschott added a comment -

(marked up code snippet in description to display as code.)

Show
Ben Smith-Mannschott added a comment - (marked up code snippet in description to display as code.)

People

Vote (2)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved: