Clojure

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

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

> 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

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

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.

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

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

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.

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)

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.

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

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

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

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