-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Generify IRubyObject.toJava to make it more pleasant to use. #5063
Conversation
This change allow callers to skip the cast when calling toJava on a Ruby object. It was initially done to assist work on #4781 by allowing a simple "throw rubyException.toJava(Throwable.class). I tested binary compatibility in two ways: * A full recompile with all generification in place plus testing standard JRuby commands. This confirms that classes recompiled against the API but without other use of generics still function properly. * A partial recompile with only the generified classes rebuilt. This tests that classes not compiled against the API still function properly. Of course any external JRuby extensions (readline, openssl) will have been tested by running those standard commands (e.g. irb, gem install).
Bit of a snag...it appears that Class.cast does not work when the desired target is a primitive. This is noted in the toJava for RubyBoolean, since that logic wants to return a boolean but you can't return or cast to boolean from a generified method. This may not have a generic fix since there does not appear to be any way to programmatically cast primitives. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great stuff - found one minor copy-pasta
*/ | ||
@Override | ||
public Object toJava(Class target) { | ||
if (target.isAssignableFrom(Boolean.class) | target.equals(boolean.class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that one |
is pbly problematic - also you can skip equals for terminal types and primitives:
target == Boolean.class || target == boolean.class
*/ | ||
@Override | ||
public Object toJava(Class target) { | ||
if (target.isAssignableFrom(Boolean.class) | target.equals(boolean.class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here ||
got lost as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok it turns out isAssignableFrom is required here because the contract of Ruby Boolean is that it will coerce to a Boolean object for java.lang.Boolean or any supertype up to java.lang.Object.
I'll make the other changes along with my fixes.
I have additional fixes and no longer believe the casting issue is a big problem. After inspecting all our toJava implementations, only a handful can toJava into primitive types. Those cases will be fixed to just do manual casting to the container type. The generic toJava in JavaProxy could be a problem, but it does not appear to break even when specifying primitive target types for a boxed numeric, probably because it doesn't really coerce to a different type:
This may constitute a bug and get fixed at some point, so I'll make sure the blind casting here is at least smart about primitives. |
This change allow callers to skip the cast when calling toJava
on a Ruby object. It was initially done to assist work on #4781
by allowing a simple "throw rubyException.toJava(Throwable.class).
I tested binary compatibility in two ways:
standard JRuby commands. This confirms that classes recompiled
against the API but without other use of generics still function
properly.
This tests that classes not compiled against the API still
function properly.
Of course any external JRuby extensions (readline, openssl) will
have been tested by running those standard commands (e.g. irb,
gem install).