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-constructable Concrete Ruby Classes #6422

Merged
merged 55 commits into from May 25, 2021
Merged

Conversation

byteit101
Copy link
Member

@byteit101 byteit101 commented Oct 2, 2020

Work on #449, duplicate #5270, and related #2369. See #449 for latest discussion.

Missing: Working code, Tests, and a good design.

Right now this is very hacky.

Notes to watch out for:

https://github.com/jruby/jruby/wiki/FAQs#when-i-implement-a-java-interface-and-provide-my-own-initialize-method-i-cant-construct-it-anymore

interface methods must be in :methods (vs all)

@byteit101
Copy link
Member Author

I'm now moving the concrete extension code into the reification code. So far I have the following pattern implement:
Normal Ruby class:

public class NormalReify2 extends RubyObject implements Reified { // this is unchanged, and we are a IRubyObject
   private static Ruby ruby;
   private static RubyClass rubyClass;

   public static void clinit(Ruby var0, RubyClass var1) {
      ruby = var0;
      rubyClass = var1;
   }

   public NormalReify2(Ruby var1, RubyClass var2) { //initialized via the RubyAlllocator, which is a change from before to allow java/init calls to not collide
      super(var1, var2);
// ruby called us, we will be initialized later
   }

   public NormalReify2() { //java-called
      super(ruby, rubyClass);
      this.callMethod("initialize"); // initialize as java called us
   }

   public int mymethod(String var1) {
      Ruby var2 = ruby;
      return ((Number)this.callMethod("mymethod", new IRubyObject[]{JavaUtil.convertJavaToUsableRubyObject(var2, var1)}).toJava(Integer.TYPE)).intValue();
   }

Concrete Extension:

public class RubyTest extends AbstractTest implements Reified { // Unsure of which interfaces should be implemented
   private static Ruby ruby;
   private static RubyClass rubyClass;
   private RubyBasicObject rubyObject; // in theory should be IRubyObject, but not worrying about the threadcontext is nice :-)

   public static void clinit(Ruby var0, RubyClass var1) { // to be replaced by static{} block
      ruby = var0;
      rubyClass = var1;
   }

   public RubyTest(Ruby var1, RubyClass var2) { // ruby facing ctor
      if (this.rubyObject == null) {
         this.rubyObject = new ConcreteJavaProxy(var1, var2, this); //note this pulls allocation out of an Allocator object, is that fine? what references, doc should there be?
      }
// no init, as ruby will call it later. this is part of the alloc
   }

   public RubyTest() { // java-facing ctor
      if (this.rubyObject == null) { // eh, lazy codegen that should be cleaned up
         (this.rubyObject = new ConcreteJavaProxy(ruby, rubyClass, this)).callMethod("initialize"); // init here, as java called us
      }
   }

   public List myAbstractFunc() { // an overridden function
      if (this.rubyObject == null) { // I think this could use some optimization, but this is here in case the parent calls methods in our super
         this.rubyObject = new ConcreteJavaProxy(ruby, rubyClass, this);
      }

      Ruby var1 = ruby;
      return (List)this.rubyObject.callMethod("myAbstractFunc", IRubyObject.NULL_ARRAY).toJava(List.class); // Should the method lookup be there every time?
   }

   public static IRubyObject __allocate__(Ruby var0, RubyClass var1) { //uses StaticRubyAllocator
      return (new RubyTest(var0, var1)).rubyObject; // Note: rubyObject has a ref to `this`, so no GC worries
   }

Ruby API:

class MyClass
   configure_java_class methods: :explicit/:all, call_init: true/false, java_constructable: true/false
   # sets JavaClassConfiguration
end

@byteit101
Copy link
Member Author

Of note, given this java class:

class MyClass {
  MyClass(String s){
    doAbstract();
  }
  protected abstract doAbstract();
}

and this JRuby class:

class MyRuby < MyClass
  def initialize(s)
    puts "in init"
  end
  def doAbstract
    puts "in abstract"
  end
end

the resulting output, with my current code is:

in abstract
in init

which is weird.

Also, super() is a no-op in ctors on concrete classes now, which also strikes me as weird

@byteit101
Copy link
Member Author

I have three sets of questions remaining: Architecture, Design, and Implementation, in order of importance

Architecture

Java Init Sequence == Ruby Init Sequence?

As of the way I've implemented it right now, a ruby init sequence is .reify() unless reified + a call to the java init sequence. This is:

ReifiedClass.<init>(*args)
  super(*args)
  this.rubyObject = new ConcreteJavaProxy(this)
  rubyObject#initialize(*args)
    # super() calls are *SILENTLY* no-ops

Alloc no longer exists [1]
I am unsure if __jreate! still exists in a useable form

I debated making super NOT a no-op, but that would involve a lot of code gen + super-splitting that I wanted to ask about before I went down that rabbit hole. I also didn't like only making it a no-op in java init sequences, as it seemed too context sensitive and prone to confusion. Note this is a behavioral change, and I don't like it.

In theory, we could separate args == 0, as super is a no-op there (probably[2]), but I feel like it could lead to a confusing mental model of args == 0 being different from args > 0.

1: I have no idea if this is enforced/rumps are still around
2: Weird static init stuff before super that the super depends on makes this not 100% true

Super-splitting

This is an idea I had, but am unsure of how to go about this or if it's an ok idea.
in essence, to allow #initialize to call super, we need to dispatch the types in code-gen. this isn't usable before super.

The general format of generated code will turn into:

ReifiedClass.<init>(*args)
  this.rubyObject = new ConcreteJavaProxy()
  superargs, continuation = rubyObject#initialize_with_split_super(*args)
  switch(guessSignatureFromArgs(*superargs))
    when V -> super()
    when LString -> super(*convert(superargs))
    when LString;Z -> super(*convert(superargs))
    etc...
   end
  rubyObject.setJavaObject(this)
  continuation.call

There are 3 ways I though of implementing this:

  1. AST/IR transforms as super is a keyword and can't be hidden in other methods (I think?)
  2. Explicit new method/api (see comments in support newInstance() with arguments #449 )
  3. Fibers

I think #3 is out due to overhead, though it would work today and is the most minimally invasive. Howerver ThreadLocal Storage would be disrupted.
2 is the most user-hostile in terms of backwards compatibility and extra stuff to think about, but is the simplest to implement
Despite some poking around the parser, I am unsure of the ability of 1, and it needs to be re-computable in case someone changes the definition of #initialize. It would work roughly by transforming as async/await do:

def initialize(*args)
     variable = OtherClass.call_static(*args)
     super(variable, keep_me = args[0] + 5)
     self.to_java.java_method(keep_me, variable) # values retained
end
def initialize_transformed(*args) # No need to change the return type, as initialize doesn't return values
     variable = OtherClass.call_static(*args)
     return [ [variable, keep_me = args[0] + 5], lambda {
         self.to_java.java_method(keep_me, variable) # values retained
    }
end

New Method names => new generated class

The old concrete extension code (now deleted in this PR) used to regenerate when a new method was added. The reify code doesn't appear to do this. Why the difference?

Design

Reify options

Right now things are configured as such:

class MyClass
   configure_java_class methods: :explicit/:all, call_init: true/false, java_constructable: true/false
end

I was also thinking of supporting the same options (to override the ones set there) in become_java!(methods: ...etc)

Where should the options be configured?

Also, are those reasonable options? (for all: https://github.com/jruby/jruby/pull/6422/files#diff-fa6783cb1e5bbf966f341330a90265a3aa04682625c103500fe61d5e8602ef96 and for what they do (this area of the code): https://github.com/jruby/jruby/pull/6422/files#diff-4823dd97065ce1b80e235b191b03497a85a35a0acd1e8089e09eef1365b699ecR1564-R2165)

Implementation

new method

I was having issues setting up the correct new method, which leads to double dispatch. Is there a way to preemptively separate these concerns: https://github.com/jruby/jruby/pull/6422/files#diff-ee5b3e6e296d6bf1315743db55b99867762ade5bdfedec34c19bef47ffc8ae98R113

Performance

Where should I worry about it? I see some places use DynamicMethods, others CachingCallSites. Should I support InDy in any way?

Not mentioned: the smaller and less important issues and todo's scattered around the code right now. I want to get the big questions answered first :-)

@byteit101
Copy link
Member Author

byteit101 commented Nov 1, 2020

Added super-splitting proposal code-gen, and some tests on how option 1 holds up. Answer: super sketchy with it being enforced to be just like java for that method to work (no super in if's, etc). I though about maybe an alternative to fibers, but that would require us to implement a continuation in super, precluding java calls on the stack frame between and the super call.

Super-splitter constructors will be rather large (decompiled from bytecode):

   public TestClass(int var1, String var2) {
      Ruby var3 = ruby;
      IRubyObject[] var7;
      this.$rubyInitArgs = var7 = new IRubyObject[]{JavaUtil.convertJavaToRuby(var3, var1), JavaUtil.convertJavaToUsableRubyObject(var3, var2)};
// returns [[super args], [lambda]]
      RubyArray var10001 = (this.$rubyObject = new ConcreteJavaProxy(var3, rubyClass)).callMethod("j_initialize", var7).convertToArray();
      IRubyObject var6 = var10001.entry(1);
      IRubyObject var8 = var10001.entry(0);
      RubyArray var5;
      switch(JCreateMethod.forTypes($rubyCtors, var8)) {
      case -1:
         super(var1, var2); // super (no args) called
         break;
      case 0: // disambiguate the ctors for this class for super (with args)
         var5 = var8.convertToArray();
         super((String)var5.entry(0).toJava(String.class), (Boolean)var5.entry(1).toJava(Boolean.TYPE));
         break;
      case 1:
         var5 = var8.convertToArray();
         super(((Number)var5.entry(0).toJava(Integer.TYPE)).intValue(), (String)var5.entry(1).toJava(String.class));
         break;
// case n, as appropriate
      default:
         throw new IllegalStateException("No available superconstructors match that type signature");
      }

      this.$rubyInitArgs = null;
      ((ConcreteJavaProxy)this.$rubyObject).setObject(this);
      var6.callMethod(var3.getCurrentContext(), "call"); // complete the lambda/initialize
   }

@byteit101
Copy link
Member Author

A summary of matrix conversation with @headius:

  • Init sequence in Java and Ruby should be the same
  • Use split-super + AST/IR transforms (option 1)
    • Talk to @enebo about doing IR transforms cleanly for this
  • in the future, look into hard-jitting initialize into the java
  • "hook-in" methods as proposed in support newInstance() with arguments #449 are mostly off the table as the break the initialize API (likely used a lot)

Of note this:

  • Doesn't break API for super() in ruby
  • Doesn't break API for #initialize allowing super() calls in ruby
  • Allows dynamic method changing of #initialize
  • Is a lot more work than other options
  • Is necessary because you can't pass uninitializedThis from <init> outside of the current frame in bytecode

@byteit101
Copy link
Member Author

byteit101 commented Nov 17, 2020

Things to do before this is finished, roughly in order:

  • Fix AST/IR transforms with @enebo 's help
  • Fix broken tests (except excess)
  • Fix excess definition options
  • Decide on ruby API for configuration
  • Add new tests
  • Ensure inheritance isn't broken
  • Make sure JRubyFX & FXML are supported with the changes
  • Ensure error messages are correct
  • Remove debugging println's and printStackTrace
  • Reformat code
  • Squash history, to hide the above crimes and prior experiments

@enebo
Copy link
Member

enebo commented Nov 18, 2020

From todays chat:

We have an AST solution but AST is not desirable as a finished product. It is good enough in WIP/PR form to debug and figure out what other problems we may encounter
The basic problem is we want to execute a front and back portion of some initialize methods and in IR we need to demarcate and split those apart and be able to execute them separately
The basic outline of what your generated code does now would likely be very similar but we might need to return temp array and maybe a couple of other things
so we can pass them to however we execute the second half
We need to consider error reporting and if we can make that descriptive or generic
As for splitting I think we can use a special instr (or add to runtimehelperinstr) to mark the super during IRBuild if the method is named initialize
Then the split itself should not be difficult we use startup instrs and just cleave it into two
I think that is the current state from this conversation and most of it is just considering an IR version
I think also we can initially do this as interp-only to reduce the scope
byteit101
Things I just realized would be really hard to support: super in a loop
But I will need to think through try/catch semantics
enebo (@enebo:matrix.org)
heh impossible in Java so there is that
byteit101
yea, if you try to compile the decompiled bytecode in javac, it doesn't like having code before the super() :-)
^ from the balloon example I pasted above
enebo (@enebo:matrix.org)
It may be that IR helps us a little bit that if we can detect a branch instr which jumps past the super instr or we see a jump instr which targets something above a super instr we can error

@headius
Copy link
Member

headius commented Mar 5, 2021

I realized that exceptions are wrapped & stacks tossed through a java ctor

As in other places we call through Java code, we should allow all exceptions to proceed unwrapped. This should restore the Ruby trace for your first example.

We used to post-process Java exceptions coming from Java integration calls but it required us to be catching every one and rewriting it, which then broke Java consumers of those exceptions further up the stack.

@headius
Copy link
Member

headius commented Mar 5, 2021

Here are the Windows GHA failures as of this moment:

 4) Error: test_interface_methods_seen(TestHigherJavasupport): TypeError: failed to coerce rubyobj.TestHigherJavasupport.A to org.jruby.javasupport.test.Interface2
D:/a/jruby/jruby/test/jruby/test_higher_javasupport.rb:1366:in `test_interface_methods_seen'
     1363:      ci = org.jruby.javasupport.test.ConsumeInterfaces.new
     1364:      ci.addInterface1(A.new)
     1365:      ci.addInterface1(B.new)
  => 1366:      ci.addInterface2(B.new)
     1367:      ci.addInterface1(C.new)
     1368:      ci.addInterface2(C.new)
     1369: 

 6) Error: test_lowercase_colon_package_syntax(TestHigherJavasupport): TypeError: failed to coerce rubyobj.TestHigherJavasupport.LCTestA to org.jruby.javasupport.test.Interface2
D:/a/jruby/jruby/test/jruby/test_higher_javasupport.rb:1403:in `test_lowercase_colon_package_syntax'
     1400:     assert_equal('boo!', LCTestB.new.boo)
     1401:     ci.addInterface1(LCTestA.new)
     1402:     ci.addInterface1(LCTestB.new)
  => 1403:     ci.addInterface2(LCTestB.new)
     1404:   end
     1405: 
     1406:   def test_marsal_java_object_fails

 7) Error: test_varargs_constructor_in_ruby_subclass(TestHigherJavasupport): ArgumentError: Constructor invocation failed: (TypeError) cannot convert instance of class org.jruby.RubyString to class [Ljava.lang.String;
D:/a/jruby/jruby/test/jruby/test_higher_javasupport.rb:203:in `initialize'
D:/a/jruby/jruby/test/jruby/test_higher_javasupport.rb:203:in `test_varargs_constructor_in_ruby_subclass'
     200:   end
     201: 
     202:   def test_varargs_constructor_in_ruby_subclass
  => 203:     var_args = RubyVarArgsCtor1.new 'foo', 'bar'
     204:     assert_equal 'foo', var_args.constants[0]
     205:     assert_equal 'bar', var_args.constants[1]
     206: 

 8) Error: test_varargs_overloaded_method(TestHigherJavasupport): ArgumentError: Constructor invocation failed: java.lang.reflect.InvocationTargetException
D:/a/jruby/jruby/test/jruby/test_higher_javasupport.rb:255:in `test_varargs_overloaded_method'
     252:     assert_equal 'foo', var_args.constants[0]
     253:     assert_equal 'bar', var_args.constants[1]
     254: 
  => 255:     var_args = RubyVarArgsOnlyCtor2.new
     256:     var_args.setConstants 'foo', 'bar' # (String, String)
     257:     assert_equal 'foo', var_args.constants[0]
     258:     assert_equal 'bar', var_args.constants[1]

I will try to confirm these should be passing. Not sure why Windows would have any differing behavior here.

@headius
Copy link
Member

headius commented Mar 5, 2021

Running rake test:jruby locally I get five failures that are similar to the Windows GHA build: https://gist.github.com/headius/13324fcf3b4ada1985eac7262388e77c

JI specs have three failures for me locally:

  1) JRuby class reification supports reification of java classes with interfaces
     Failure/Error: DEFAULT_FAILURE_NOTIFIER = lambda { |failure, _opts| raise failure }
     
       expected: [:success]
            got: [:failSO, :fail1, "fail3"]
     
       (compared using eql?)
     
       Diff:
       @@ -1 +1 @@
       -[:success]
       +[:failSO, :fail1, "fail3"]

  2) JRuby class reification supports reification of ruby classes with interfaces
     Failure/Error: DEFAULT_FAILURE_NOTIFIER = lambda { |failure, _opts| raise failure }
     
       expected: [:success]
            got: [:failSO, :fail1, "fail3"]
     
       (compared using eql?)
     
       Diff:
       @@ -1 +1 @@
       -[:success]
       +[:failSO, :fail1, "fail3"]

  3) A Ruby subclass of a Java concrete class with custom methods generates new methods usable by ruby & java
     Failure/Error: DEFAULT_FAILURE_NOTIFIER = lambda { |failure, _opts| raise failure }
     
       expected: 1
            got: 2
     
       (compared using ==)

This is MacOS, Java 11.

@headius headius added this to the JRuby 9.3.0.0 milestone Mar 5, 2021
@byteit101
Copy link
Member Author

What remains to be done?

@byteit101 byteit101 marked this pull request as ready for review March 15, 2021 22:16
@enebo enebo merged commit 2d8b505 into jruby:master May 25, 2021
byteit101 added a commit to byteit101/JRubyFXML that referenced this pull request Jun 1, 2021
Some more cleanup required, but jruby/jruby#6422 has been merged to
close jruby/jruby#449 which means that finally,
we can use the native FXMLLoader support by generating classes at runtime
based on the fxml files that the loader knows how to deal with, and we
can finally drop all the garbage to work around this. Yay!
enebo added a commit that referenced this pull request Jun 9, 2021
kares added a commit to kares/jruby that referenced this pull request Jun 17, 2021
* master:
  add --dev stage, fix JRUBY_OPTS
  Fix method visibility for define method
  doclint stinks
  getsockopt crash with a --dev flag
  Minor style tweaks after landing of jruby#6422
  Update for next development cycle
  Update for 9.2.18.0
  Update for newest jruby-launcher
@headius headius mentioned this pull request Sep 14, 2021
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

3 participants