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

Generify IRubyObject.toJava to make it more pleasant to use. #5063

Merged
merged 5 commits into from Feb 28, 2018

Conversation

headius
Copy link
Member

@headius headius commented Feb 27, 2018

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

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).
@headius headius requested review from kares and enebo February 27, 2018 18:50
@headius headius added this to the JRuby 9.2.0.0 milestone Feb 27, 2018
@headius
Copy link
Member Author

headius commented Feb 27, 2018

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.

Copy link
Member

@kares kares left a 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)) {
Copy link
Member

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)) {
Copy link
Member

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 |

Copy link
Member Author

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.

@headius
Copy link
Member Author

headius commented Feb 28, 2018

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:

$ jruby -e 'p 1.to_java(:int).to_java(:char)'
#<Java::JavaLang::Integer:0x30a3107a>

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.

@headius headius merged commit 80b9665 into master Feb 28, 2018
@headius headius deleted the generify_tojava branch February 28, 2018 22:27
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