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

Unable to use UnboundMethod referencing super with define_singleton_method #3869

Closed
saturnflyer opened this Issue May 10, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@saturnflyer

saturnflyer commented May 10, 2016

I'm unable to transplant a method defined in a module to define a method on another object

Environment

jruby 9.1.0.0 (2.3.0) 2016-05-02 a633c63 Java HotSpot(TM) 64-Bit Server VM 25.77-b03 on 1.8.0_77-b03 +jit [darwin-x86_64]

Expected Behavior

I expect to be able to define methods on an object using define_singleton_method and instance methods from a module

Example code:

https://github.com/saturnflyer/behavioral/blob/00dae0f871ac921b77983f1535276c375f0baaa8/lib/behavioral.rb

Or using this sample in an irb session should work:

class P
  def name
    'name'
  end
end
module Mod
  def name
    "mod #{super}"
  end
end
p = P.new
p.define_singleton_method(:name, Mod.instance_method(:name))
p.send :name #=> "mod name"

Actual Behavior

NullPointerException

I'm still trying to track down the exact cause of this and will update this issue if I find the concrete trigger but for now I have failing tests at: https://travis-ci.org/saturnflyer/behavioral/jobs/128500121

I believe it has to do with using super inside an UnboundMethod used to define the singleton_class instance method.

If you remove the super call from the example code above, it works fine.

Example stack trace from the link to travis-ci above:

  1) Error:
Behavioral#test_0003_allows adding multiple behavior modules at once:
Java::JavaLang::NullPointerException: 
    org.jruby.ir.runtime.IRRuntimeHelpers.unresolvedSuper(IRRuntimeHelpers.java:991)
    org.jruby.ir.instructions.UnresolvedSuperInstr.interpret(UnresolvedSuperInstr.java:83)
    org.jruby.ir.interpreter.InterpreterEngine.processCall(InterpreterEngine.java:348)
    org.jruby.ir.interpreter.StartupInterpreterEngine.interpret(StartupInterpreterEngine.java:77)
    org.jruby.ir.interpreter.InterpreterEngine.interpret(InterpreterEngine.java:80)
    org.jruby.internal.runtime.methods.MixedModeIRMethod.INTERPRET_METHOD(MixedModeIRMethod.java:144)
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 10, 2016

Member

(edited your stack trace for brevity)

Heh, well you definitely shouldn't get a NullPointerException!

You are correct that it has something to do with super. In this case when we super we need to dig out where the method actually lives in the hierarchy, so we can start searching for the next method up from the right place. I'm believe that search is blowing up, because it tries to find the method's original owner (module Mod) somewhere in the singleton P's hierarchy. It doesn't exist, we have a null reference, and kaboomsky.

The fix for this is not trivial, unfortunately, and @enebo and @subbuss know about it. When we bind an UnboundMethod into a new class, we need to properly clone that method and re-home that clone to the new target class. This is the source of many other issues, like cloned modules not acting independently.

@subbuss @enebo I think we need to finally address this after the 9.1.1.0 release.

Member

headius commented May 10, 2016

(edited your stack trace for brevity)

Heh, well you definitely shouldn't get a NullPointerException!

You are correct that it has something to do with super. In this case when we super we need to dig out where the method actually lives in the hierarchy, so we can start searching for the next method up from the right place. I'm believe that search is blowing up, because it tries to find the method's original owner (module Mod) somewhere in the singleton P's hierarchy. It doesn't exist, we have a null reference, and kaboomsky.

The fix for this is not trivial, unfortunately, and @enebo and @subbuss know about it. When we bind an UnboundMethod into a new class, we need to properly clone that method and re-home that clone to the new target class. This is the source of many other issues, like cloned modules not acting independently.

@subbuss @enebo I think we need to finally address this after the 9.1.1.0 release.

@headius headius added this to the JRuby 9.1.2.0 milestone May 10, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 10, 2016

Member

tldr: I have a patch that seems to fix your specific case, and it might be good enough for now...but there's other issues.

Ok, here's a trivial patch that surprisingly works fine for the module case (transplanting a method out of a module):

diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
index d5867237..c4133c0 100644
--- a/core/src/main/java/org/jruby/RubyModule.java
+++ b/core/src/main/java/org/jruby/RubyModule.java
@@ -1933,7 +1933,8 @@ public class RubyModule extends RubyObject {

             checkValidBindTargetFrom(context, (RubyModule)method.owner(context));

-            newMethod = new WrapperMethod(this, method.getMethod(), visibility);
+            newMethod = method.getMethod().dup();
+            newMethod.setImplementationClass(this);
         } else {
             throw runtime.newTypeError("wrong argument type " + arg1.getType().getName() + " (expected Proc/Method)");
         }

This works because module-based methods with super calls are compiled as "unresolved super", an instruction that doesn't know at compile time from what class it will start supering. The new implementationClass on the dup'ed method is used for the "frame class", indicating where to start supering.

This solves only the issue of what "frame class" gets set for the method, however, and doesn't help with "cref" issues @subbuss and @enebo and I have talked about. That probably doesn't keep us from fixing this particular bug.

However there's a more problematic case: you can also transplant a method from a superclass into a subclass, and it must behave as though it lives in that subclass. Unfortunately, normal class-based methods don't use unresolved super.

Here's an example:

class X
  def name
    p "x"
  end
end

class Mod < X
  def name
    p "mod"
    super
  end
end
class P < Mod
end
p = P.new
p.define_singleton_method(:name, Mod.instance_method(:name))
p.send :name #=> "mod name"

In this case, Mod is a superclass of P and so we can transplant its method. That should result in P#name printing out "mod" and supering, followed by Mod#name and X#name.

$ ruby23 blah.rb
"mod"
"mod"
"x"

However, since we can't re-home "resolved super" (as found in normal class methods), we still begin supering from the Mod class:

$ jruby blah.rb
"mod"
"x"

My guess would be that we'd have to clone the entire IR for this method and replace resolved supers with unresolved supers, but I don't know what impact that would have on the rest of the IR.

Member

headius commented May 10, 2016

tldr: I have a patch that seems to fix your specific case, and it might be good enough for now...but there's other issues.

Ok, here's a trivial patch that surprisingly works fine for the module case (transplanting a method out of a module):

diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
index d5867237..c4133c0 100644
--- a/core/src/main/java/org/jruby/RubyModule.java
+++ b/core/src/main/java/org/jruby/RubyModule.java
@@ -1933,7 +1933,8 @@ public class RubyModule extends RubyObject {

             checkValidBindTargetFrom(context, (RubyModule)method.owner(context));

-            newMethod = new WrapperMethod(this, method.getMethod(), visibility);
+            newMethod = method.getMethod().dup();
+            newMethod.setImplementationClass(this);
         } else {
             throw runtime.newTypeError("wrong argument type " + arg1.getType().getName() + " (expected Proc/Method)");
         }

This works because module-based methods with super calls are compiled as "unresolved super", an instruction that doesn't know at compile time from what class it will start supering. The new implementationClass on the dup'ed method is used for the "frame class", indicating where to start supering.

This solves only the issue of what "frame class" gets set for the method, however, and doesn't help with "cref" issues @subbuss and @enebo and I have talked about. That probably doesn't keep us from fixing this particular bug.

However there's a more problematic case: you can also transplant a method from a superclass into a subclass, and it must behave as though it lives in that subclass. Unfortunately, normal class-based methods don't use unresolved super.

Here's an example:

class X
  def name
    p "x"
  end
end

class Mod < X
  def name
    p "mod"
    super
  end
end
class P < Mod
end
p = P.new
p.define_singleton_method(:name, Mod.instance_method(:name))
p.send :name #=> "mod name"

In this case, Mod is a superclass of P and so we can transplant its method. That should result in P#name printing out "mod" and supering, followed by Mod#name and X#name.

$ ruby23 blah.rb
"mod"
"mod"
"x"

However, since we can't re-home "resolved super" (as found in normal class methods), we still begin supering from the Mod class:

$ jruby blah.rb
"mod"
"x"

My guess would be that we'd have to clone the entire IR for this method and replace resolved supers with unresolved supers, but I don't know what impact that would have on the rest of the IR.

@enebo enebo modified the milestones: JRuby 9.1.2.0, JRuby 9.1.3.0 May 23, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 22, 2016

Member

The dup vs WrapperMethod change has been proven out by another bug, #3708. There, we fixed the problem by making the singleton method defined by module_method also dup a proper method rather than using WrapperMethod.

Based on the success of that fix, I think it's appropriate to do the same thing here.

Member

headius commented Aug 22, 2016

The dup vs WrapperMethod change has been proven out by another bug, #3708. There, we fixed the problem by making the singleton method defined by module_method also dup a proper method rather than using WrapperMethod.

Based on the success of that fix, I think it's appropriate to do the same thing here.

headius added a commit to headius/jruby that referenced this issue Aug 22, 2016

Make all IR methods cloneable and eliminate trivial dup impls.
This is part of fixes for #3869. Duping methods by reconstructing
them causes them to have a new serial number, which is what we are
using to determine method equality (and not much else). By
changing dup to use clone here, Ruby methods transplanted from
a module to the same module will still be == and eql?.

headius added a commit to headius/jruby that referenced this issue Aug 22, 2016

Stop using WrapperMethod, since it messes up super logic.
This is for #3869 and relates to the module_function change from
and redefine it with a new visibility and implementation class.
However the impl class never passed through to the contained
method, preventing it from being used in super. This affected, for
example, module_fuction singleton methods that need to super or
methods transplanted using defined_method with a Method instance.
The new logic always tries to dup the target method so it can
be truly populated with the altered fields. This change fixed

The previous commit, using cloning instead of construction for
IR methods, works around the fact that there's no semi-transparent
WrapperMethod to delegate its serial number to the wrapped method.
Since in that case and in this one, the method's serial number
was expected to be the same after duplication, the clone
technique seems acceptable.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 22, 2016

Member

Pending review of #4102.

Member

headius commented Aug 22, 2016

Pending review of #4102.

@headius headius closed this Aug 23, 2016

headius added a commit that referenced this issue Sep 14, 2016

Stop using WrapperMethod, since it messes up super logic.
This is for #3869 and relates to the module_function change from
and redefine it with a new visibility and implementation class.
However the impl class never passed through to the contained
method, preventing it from being used in super. This affected, for
example, module_fuction singleton methods that need to super or
methods transplanted using defined_method with a Method instance.
The new logic always tries to dup the target method so it can
be truly populated with the altered fields. This change fixed

The previous commit, using cloning instead of construction for
IR methods, works around the fact that there's no semi-transparent
WrapperMethod to delegate its serial number to the wrapped method.
Since in that case and in this one, the method's serial number
was expected to be the same after duplication, the clone
technique seems acceptable.

jruby-1_7: Fixes #4155

headius added a commit that referenced this issue Nov 9, 2016

Reinstate WrapperMethod for visibility changes. Fixes #4272.
This was introduced via a fix for #3869, to update impl class so
that transplanted methods would super correctly. However, this
logic - which does not want a new impl class - got caught up. The
fix here reverts to using WrapperMethod for now, until we start
managing visibilty more like MRI or come up with a better fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment