-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Comments
(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. |
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 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:
In this case,
However, since we can't re-home "resolved super" (as found in normal class methods), we still begin supering from the
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. |
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. |
This is part of fixes for jruby#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?.
This is for jruby#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.
Pending review of #4102. |
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
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.
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 moduleExample code:
https://github.com/saturnflyer/behavioral/blob/00dae0f871ac921b77983f1535276c375f0baaa8/lib/behavioral.rb
Or using this sample in an irb session should work:
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:
The text was updated successfully, but these errors were encountered: