Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parallel exception hierarchy for Java #5065

Merged
merged 12 commits into from
Feb 28, 2018
Merged

Parallel exception hierarchy for Java #5065

merged 12 commits into from
Feb 28, 2018

Conversation

headius
Copy link
Member

@headius headius commented Feb 27, 2018

This is to address problems related to #4781.

No Ruby exception types have a real exception type in Java, which means in order to capture any of them you must catch all of them (catch (RaiseException)) and inspect or rethrow. This is inefficient and ugly.

This PR introduces a parallel Java exception hierarchy that maps 1:1 to the base Ruby exception hierarchy.

  • All RubyException objects now know how to construct their appropriate Throwable.
  • All base exception types will have their own java.lang.Throwable (subclass of RaiseException).

In addition, this makes it possible for JRuby ext authors to create the same parallel Throwable+RubyException within their libraries to improve exception handling from Java (think catch (ParserError) in the Psych extension).

This work is the start of producing a Java-domain Throwable
hierarchy that matches the Ruby hierarchy, so that Ruby exceptions
can be caught using their actual Throwable type rather than using
RaiseException and checking the exceptions type manually.

A few changes of note:

* RubyException has most logic moved up to new
  AbstractRubyException.
* All RubyException now hold a reference to their RaiseException,
  created lazily once as needed.
  * Throw a Ruby exception using the form
    `throw ex.getRaiseException()`.
  * Provide construction logic specific to a given exception by
    overriding createRaiseException.
* The nativeException parameter is eliminated from all exception
  construction and initialization paths. It was unused.
This forms the meat of wiring up the new parallel exception
hierarchy. All public constructors of RaiseException are now
deprecated and unused. RaiseException.from() forms take their
place and know how to properly construct a Ruby exception and
wrap it with an appropriate RaiseException subclass.

Three native exceptions are added, mapping to the Ruby exception
type of the same name: Exception, StandardError, and
SignalException.

One location in the code that caught RaiseException is now able
to catch StandardError (RubyNumeric coerce logic). There are
likely others.

See #4781.
@headius headius added this to the JRuby 9.2.0.0 milestone Feb 27, 2018
@headius headius requested review from enebo and kares February 27, 2018 23:01
@headius
Copy link
Member Author

headius commented Feb 27, 2018

More work coming, but mostly just filling out the hierarchy.

This includes everything in standard Ruby plus our additions but
minus the errno classes.

See #4781.
@headius
Copy link
Member Author

headius commented Feb 28, 2018

I've implemented this for all types of exceptions standard to Ruby, plus our few additions, but I have not done all the errno exceptions. Should I?

I have a minor concern about cluttering up org.jruby with more classes, but I'm not sure where else the new Ruby*Exception types should live. Should we move them somewhere?

All the new Throwables live under org.jruby.exceptions.

This is just about ready, pending questions above and fixing any regressions.

@kares
Copy link
Member

kares commented Feb 28, 2018

💯 very nice (and backwards compatible)!

minor thing: I know we use get-er style naming for conversions but still
public RaiseException getRaiseException() maybe its better (might be subjective) as :
public RaiseException toRaiseException() (or asRaiseException) so the 'conversion' intent is clear?

@headius
Copy link
Member Author

headius commented Feb 28, 2018

minor thing: I know we use get-er style naming for conversions but still
public RaiseException getRaiseException() maybe its better (might be subjective) as :
public RaiseException toRaiseException() (or asRaiseException) so the 'conversion' intent is clear?

Yeah I have also had concerns about some of these names being rather long. Maybe throwable or toThrowable? It would still return RaiseException.

I believe this is green other than things also failing on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants