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

java_send/java_alias and perhaps others are not finding the proper method #5631

Open
enebo opened this Issue Feb 26, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@enebo
Copy link
Member

enebo commented Feb 26, 2019

This is largely an issue with Java 9+ issue but if we call something like:

java_import java.awt.Canvas;
java_import java.awt.Graphics;
java_import javax.swing.JFrame;

class Drawing < Canvas
  def paint(g)
    g.fillOval(100, 100, 200, 200)
    g.java_send :translate, [::Java.int, ::Java.int], 10, 10
  end
end

frame = JFrame.new("My Drawing");
canvas = Drawing.new
canvas.setSize(400, 400)
frame.add(canvas);
frame.pack
frame.setVisible(true)

The java_send to translate will find the method from the object it represents. So it asks for its class and finds:
METHOD public void sun.java2d.SunGraphics2D.translate(int,int)
and this will fail in Java 9+ beacuse it is not exported. It should in fact call:
METHOD public abstract void java.awt.Graphics.translate(int,int) which is fine.

Ordinary method bindings on Java proxy classes appear to call the right method so it is unclear if there is an issue there.

headius added a commit that referenced this issue Feb 26, 2019

Use modules to find the nearest accessible method.
Part of #5631. This only fixes java_send, I believe.
@headius

This comment has been minimized.

Copy link
Member

headius commented Feb 26, 2019

java_send is fixed by just using isExported and isPublic. Similar fixes can be done for java_alias and java_field.

@kares: this may need a more general solution though...Java integration currently skips methods it can't trySetAccessible (via Modulator), when all it really needs is for their package to be exported (added feature to Modulator today) and the class and method public. We may be cutting out more packages than we need with current logic. Have a look at my commit above and let me know what you think/

@enebo enebo added this to the JRuby 9.2.7.0 milestone Feb 26, 2019

@kares

This comment has been minimized.

Copy link
Member

kares commented Feb 27, 2019

nice, the exported look makes sense since we only look for public (accessible) methods.
wonder thought whether we shouldn't just do the 'normal' hierarchy search, but than users are doing java_send compared to a Ruby send for a reason.

@headius

This comment has been minimized.

Copy link
Member

headius commented Feb 27, 2019

The java_send changes were apparently more invasive than I expected, and some specs regressed. I'm fixing that now.

@enebo I have to change the logic to allow searching private classes because we have specs that expect that behavior. This may be a case where we need some Java 9-specific behavior above and beyond the module logic (i.e. be less aggressive on 9 because it yells at us).

@kares I know about the logic to setAccessible Java methods during setup, but do you know of anywhere we're explicitly backing just because we're on 9, rather than via module checks? (e.g. "if (!on_java_9) set_something_accessible")

@kares

This comment has been minimized.

Copy link
Member

kares commented Feb 27, 2019

@headius nope, do not recall we have any such checks in place - believe I did not add/see any such

headius added a commit that referenced this issue Feb 27, 2019

@headius

This comment has been minimized.

Copy link
Member

headius commented Feb 27, 2019

a09a866 also relates to this.

@headius headius modified the milestones: JRuby 9.2.7.0, JRuby 9.2.8.0 Apr 8, 2019

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 8, 2019

This is still outstanding; we have imbalanced fixes going into 9.2.7 due to only fixing java_send in 6cca3e4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.