Tie become_java! and concrete extension (fixes #4165) by byteit101 · Pull Request #6141 · jruby/jruby

byteit101 · 2020-03-26T22:04:58Z

Still in progress, but allow concrete class extension to define non-extended fields and signatures (required for jrubyfx to use the built in FXMLLoader that uses reflection on passed in classes)

Notable questions:

ClassJavaAddons: how to check for java proxies vs failed reification
Is adding two arguments everywhere the best way to send them down to the asm layer?
MethodData generalization: are the LocalMethodData constants reasonable?
MethodData.getDeclaringClass ???

headius · 2020-03-27T20:32:43Z

Putting this on my todo list to review... perhaps early next week? Maybe you have some thoughts @kares?

byteit101 · 2020-04-06T22:40:32Z

While investigating annotations, it appears that jrubyc and become_java! have different syntaxes:

java_annotation 'Deprecated'
java_signature 'void my_method()'

vs

java_signature '@Deprecated void my_method()'

jrubyc uses at least the former and become_java! only saves the latter. As such, I've implemented parameters for the latter, but that brings up what to do with fields. Should java_field support annotation parsing or should java_annotation be used instead, and probably expanded for methods too?

… work

byteit101 · 2020-04-08T22:18:57Z

As per conversation on matrix with @enebo, This PR now has the following API changes:
java_annotation is deprecated and warns on useless calls. https://github.com/jruby/jruby/wiki/JRuby-Reference#java_annotation should be updated
java_signature now supports annotations that have parameters (previously only supported unnamed parameters)
java_field now supports annotations (previously didn't support annotations)

kares · 2020-04-11T13:26:14Z

lib/ruby/stdlib/jruby/core_ext/class.rb

  end

+  def java_annotation(anno)
+    STDERR.puts "java_annotation is deprecated. Use java_signature '@#{anno} ...' instead. Called from: #{caller.first}"


minor: we probably just want to use warn or some kind of warnings on the native end.

Yeah use Kernel#warn or Warnings#warn so it can be silenced if necessary.

kares · 2020-04-11T13:34:04Z

Great work so far Patrick!

Think it makes sense to have as its going, I did like the previous java_annotation 'Deprecated' wout the @ prefix but for java_field etc it's more reasonable to have Java-like syntax @Deprecated ....

Not sure if current scope imports are respected (or whether they worked using java_annotation) but it would be nice to not have to always use fully-qualified Java class names in annotations.

headius

Just a few small changes. I will address your original questions in a separate comment.

headius · 2020-09-01T20:13:30Z

core/src/main/java/org/jruby/ast/java_signature/AnnotationVisitor.java

+ * Copyright (C) 2002 Jan Arne Petersen <jpetersen@uni-bonn.de>
+ * Copyright (C) 2002-2004 Anders Bengtsson <ndrsbngtssn@yahoo.se>
+ * Copyright (C) 2004 Thomas E Enebo <enebo@acm.org>
+ * Copyright (C) 2004 Stefan Matthias Aust <sma@3plus4.de>


Remove attributions unrelated to this file. If you really want to add yourself, go ahead, but since the audit trail is in git I don't believe we need to have these.

headius · 2020-09-01T20:13:52Z

core/src/main/java/org/jruby/ast/java_signature/BoolLiteral.java

+/*
+ * To change this template, choose Tools | Templates
+ * and open the template in the editor.
+ */


Fix license header

headius · 2020-09-01T20:14:04Z

core/src/main/java/org/jruby/ast/java_signature/NumberLiteral.java

+/*
+ * To change this template, choose Tools | Templates
+ * and open the template in the editor.
+ */


Fix license header

headius · 2020-09-01T20:15:01Z

core/src/main/java/org/jruby/ast/java_signature/NumberLiteral.java

+
+/**
+ *
+ * @author enebo


Author @enebo?

headius · 2020-09-01T20:17:48Z

core/src/main/java/org/jruby/javasupport/proxy/JavaProxyClassFactory.java

+
+        }
+        else 
+        {


Style issues here and other places... close brace and open brace should be on same line as else, if's open brace should be on same line, etc.

headius · 2020-09-01T20:23:26Z

lib/ruby/stdlib/jruby/core_ext/class.rb

  end

+  def java_annotation(anno)
+    STDERR.puts "java_annotation is deprecated. Use java_signature '@#{anno} ...' instead. Called from: #{caller.first}"


Yeah use Kernel#warn or Warnings#warn so it can be silenced if necessary.

headius · 2020-09-01T20:41:21Z

Addressing the original 4 question bullets:

ClassJavaAddons: how to check for java proxies vs failed reification

I'm not sure exactly what you mean. What you have seems fine to me, maybe?

Is adding two arguments everywhere the best way to send them down to the asm layer?

Usually if I feel like I'm passing around the same arguments too many places, it might be time for a data object that contains all of them together. It's not a huge deal either way for this PR, but perhaps we can refactor this to be more OO and not have to pass so many params all over.

MethodData generalization: are the LocalMethodData constants reasonable?

I think these are fine as-is.

MethodData.getDeclaringClass ???

If this is no longer being used then it's fine to remove.

byteit101 · 2020-09-01T20:49:48Z

core/src/main/java/org/jruby/java/addons/ClassJavaAddons.java

+        }
+        if (reifiedClass == null) {
+            throw context.runtime.newTypeError("requested class " + klass.getName() + " was not reifiable");
+        }


@headius This bit of code does not distinguish between Failed Reified Ruby classes and Concretized Classes. getReified is null if either it's a failure to reify or a java proxy class. Should the logic be split between those two cases, and if so, how?

Aha, so you think maybe we need another way to know a given class is a java proxy class. Perhaps we add another boolean flag to RubyClass like isJavaSubclass or isJavaProxy?

so you think maybe we need...

No, I am asking if we do. I have no strong opinion on these internals, but I do think the error message could be a misleading if we care to tell users about the difference between concrete vs reified, so I'm wondering if you think we should disambiguate.
Secondarily, I think it's safe for this code path to be retried for failed reified and failed concrete multiple times, but that might have implications I'm not aware of

byteit101 · 2020-09-01T22:35:56Z

Usually if I feel like I'm passing around the same arguments too many places, it might be time for a data object that contains all of them together. It's not a huge deal either way for this PR, but perhaps we can refactor this to be more OO and not have to pass so many params all over.

The reason I hesitated was because all of the items came from the ruby class, and so the obvious thing was to just pass the ruby class down, but I wasn't sure if that would break existing encapsulation design too much. Creating a small mirror of RubyClass things also seemed kind of silly if we could just use RubyClass

headius · 2020-09-03T22:24:27Z

I am fine merging this once review comments are addressed and CI is green. Thank you for your help and patience @byteit101!

headius · 2020-09-03T22:48:05Z

This might help the failures (annotation API is no longer part of JDK):

https://gist.github.com/headius/cde16ee9fe14767549ed0036ec0a04dc

WIP to tie become_java! and concrete extension jruby#4165

4e46e90

headius requested review from enebo, headius and kares March 27, 2020 20:32

headius added this to the JRuby 9.3.0.0 milestone Mar 27, 2020

byteit101 added 9 commits April 2, 2020 23:42

Fix NoMethodError as annotation is a hash, not an array

9e87fd7

Support saving annotations in java_signature, and add a visitor pattern

eac9813

Support the rest of the types that can be in annotations

f2de196

Save mostly the correct number type (except chars)

da7b87f

Method annotations for concrete extension

a03c1f8

Special case char to avoid reunboxing

04f503e

chars for special characters in parser

d5cafaa

Adding specs for annotation parameters

880e01b

Remove TODO

d16ded9

byteit101 added 5 commits April 6, 2020 19:24

Copy annotation test to java proxies

46291a2

Visit and generate field annotations

d5bb210

Update java_field to mirror java_signature and java_annotation to not…

bb9309c

… work

Add spec for java_field annotations

1a74e74

Warn, don't error

d1ab682

kares reviewed Apr 11, 2020

View reviewed changes

byteit101 added 3 commits April 11, 2020 15:51

Support java_import's when resolving names

7abe7eb

Swap to warn from STDERR as per PR

77cfcf5

Merge branch 'master' into concrete_java

6936849

headius reviewed Sep 1, 2020

View reviewed changes

byteit101 commented Sep 1, 2020

View reviewed changes

Fix license blocks & remove bad commented out things

e8e219d

byteit101 changed the title ~~WIP: tie become_java! and concrete extension (fixes #4165)~~ Tie become_java! and concrete extension (fixes #4165) Sep 3, 2020

byteit101 added 2 commits September 3, 2020 18:49

Patch from @headius to add annotation api to classpath for ji tests

705f73c

add annotation api to the ji classpath

0b899fa

headius merged commit 763be8b into jruby:master Sep 9, 2020

byteit101 mentioned this pull request Sep 28, 2020

support newInstance() with arguments #449

Closed

Uh oh!

Conversation

byteit101 commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

headius commented Mar 27, 2020

Uh oh!

byteit101 commented Apr 6, 2020

Uh oh!

byteit101 commented Apr 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kares commented Apr 11, 2020

Uh oh!

headius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

headius commented Sep 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byteit101 Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byteit101 commented Sep 1, 2020

Uh oh!

headius commented Sep 3, 2020

Uh oh!

headius commented Sep 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

byteit101 commented Mar 26, 2020 •

edited

Loading

byteit101 Sep 1, 2020 •

edited

Loading