[GR-17126] Use the class where the method was defined to check if an UnboundMethod can be used for define_method.

PullRequest: truffleruby/951

Author: eregon

CHANGELOG.md

@@ -10,6 +10,7 @@ Bug fixes:
 * Fixed `Symbol#match` returning `MatchData` (#1706).
 * Allow `Time#strftime` to be called with binary format strings.
 * Do not modify the argument passed to `IO#write` when the encoding does not match (#1714).
+* Use the class where the method was defined to check if an `UnboundMethod` can be used for `#define_method` (#1710).
 
 Compatibility:
 

spec/ruby/core/module/define_method_spec.rb

@@ -281,7 +281,7 @@ def foo
 
     lambda {
       Class.new { define_method :bar, m }
-    }.should raise_error(TypeError)
+    }.should raise_error(TypeError, /can't bind singleton method to a different class/)
   end
 
   it "raises a TypeError when a Method from one class is defined on an unrelated class" do
@@ -396,20 +396,42 @@ class DefineMethodSpecClass
     klass.new.should respond_to(:bar)
   end
 
+
+  it "allows an UnboundMethod of a Kernel method retrieved from Object to defined on a BasicObject subclass" do
+    klass = Class.new(BasicObject) do
+      define_method :instance_of?, ::Object.instance_method(:instance_of?)
+    end
+    klass.new.instance_of?(klass).should == true
+  end
+
   it "raises a TypeError when an UnboundMethod from a child class is defined on a parent class" do
     lambda {
       ParentClass = Class.new { define_method(:foo) { :bar } }
       ChildClass = Class.new(ParentClass) { define_method(:foo) { :baz } }
       ParentClass.send :define_method, :foo, ChildClass.instance_method(:foo)
-    }.should raise_error(TypeError)
+    }.should raise_error(TypeError, /bind argument must be a subclass of ChildClass/)
   end
 
   it "raises a TypeError when an UnboundMethod from one class is defined on an unrelated class" do
     lambda {
       DestinationClass = Class.new {
         define_method :bar, ModuleSpecs::InstanceMeth.instance_method(:foo)
       }
-    }.should raise_error(TypeError)
+    }.should raise_error(TypeError, /bind argument must be a subclass of ModuleSpecs::InstanceMeth/)
+  end
+
+  it "raises a TypeError when an UnboundMethod from a singleton class is defined on another class" do
+    c = Class.new do
+      class << self
+        def foo
+        end
+      end
+    end
+    m = c.method(:foo).unbind
+
+    lambda {
+      Class.new { define_method :bar, m }
+    }.should raise_error(TypeError, /can't bind singleton method to a different class/)
   end
 end
 

spec/ruby/core/unboundmethod/bind_spec.rb

@@ -21,11 +21,21 @@
     UnboundMethodSpecs::Mod.instance_method(:from_mod).bind(Object.new).should be_kind_of(Method)
   end
 
-  it "returns Method returned for obj is equal to one directly returned by obj.method" do
+  it "the returned Method is equal to the one directly returned by obj.method" do
     obj = UnboundMethodSpecs::Methods.new
     @normal_um.bind(obj).should == obj.method(:foo)
   end
 
+  it "returns Method for any object kind_of? the Module the method is defined in" do
+    @parent_um.bind(UnboundMethodSpecs::Child1.new).should be_kind_of(Method)
+    @child1_um.bind(UnboundMethodSpecs::Parent.new).should be_kind_of(Method)
+    @child2_um.bind(UnboundMethodSpecs::Child1.new).should be_kind_of(Method)
+  end
+
+  it "allows binding a Kernel method retrieved from Object on BasicObject" do
+    Object.instance_method(:instance_of?).bind(BasicObject.new).call(BasicObject).should == true
+  end
+
   it "returns a callable method" do
     obj = UnboundMethodSpecs::Methods.new
     @normal_um.bind(obj).call.should == obj.foo

src/main/java/org/truffleruby/core/module/ModuleFields.java

@@ -363,7 +363,7 @@ public RubyConstant removeConstant(RubyContext context, Node currentNode, String
 
     @TruffleBoundary
     public void addMethod(RubyContext context, Node currentNode, InternalMethod method) {
-        assert ModuleOperations.canBindMethodTo(method.getDeclaringModule(), rubyModuleObject) ||
+        assert ModuleOperations.canBindMethodTo(method, rubyModuleObject) ||
                 ModuleOperations.assignableTo(context.getCoreLibrary().getObjectClass(), method.getDeclaringModule()) ||
                 // TODO (pitr-ch 24-Jul-2016): find out why undefined methods sometimes do not match above assertion
                 // e.g. "block in _routes route_set.rb:525" in rails/actionpack/lib/action_dispatch/routing/

src/main/java/org/truffleruby/core/module/ModuleNodes.java

@@ -1049,14 +1049,18 @@ public DynamicObject defineMethodMethod(DynamicObject module, String name, Dynam
         @TruffleBoundary
         @Specialization(guards = "isRubyUnboundMethod(method)")
         public DynamicObject defineMethod(DynamicObject module, String name, DynamicObject method, NotProvided block) {
-            final DynamicObject origin = Layouts.UNBOUND_METHOD.getOrigin(method);
-            if (!ModuleOperations.canBindMethodTo(origin, module)) {
-                throw new RaiseException(getContext(), coreExceptions().typeError("bind argument must be a subclass of " + Layouts.MODULE.getFields(origin).getName(), this));
+            final InternalMethod internalMethod = Layouts.UNBOUND_METHOD.getMethod(method);
+            if (!ModuleOperations.canBindMethodTo(internalMethod, module)) {
+                final DynamicObject declaringModule = internalMethod.getDeclaringModule();
+                if (RubyGuards.isSingletonClass(declaringModule)) {
+                    throw new RaiseException(getContext(), coreExceptions().typeError(
+                            "can't bind singleton method to a different class", this));
+                } else {
+                    throw new RaiseException(getContext(), coreExceptions().typeError("bind argument must be a subclass of " + Layouts.MODULE.getFields(declaringModule).getName(), this));
+                }
             }
 
-            // TODO CS 5-Apr-15 TypeError if the method came from a singleton
-
-            return addMethod(module, name, Layouts.UNBOUND_METHOD.getMethod(method));
+            return addMethod(module, name, internalMethod);
         }
 
         @TruffleBoundary

src/main/java/org/truffleruby/core/module/ModuleOperations.java

@@ -60,17 +60,17 @@ public static boolean inAncestorsOf(DynamicObject module, DynamicObject ancestor
     }
 
     @TruffleBoundary
-    public static boolean canBindMethodTo(DynamicObject origin, DynamicObject module) {
-        assert RubyGuards.isRubyModule(origin);
+    public static boolean canBindMethodTo(InternalMethod method, DynamicObject module) {
         assert RubyGuards.isRubyModule(module);
+        final DynamicObject origin = method.getDeclaringModule();
 
-        if (!(RubyGuards.isRubyClass(origin))) {
+        if (!RubyGuards.isRubyClass(origin)) { // Module (not Class) methods can always be bound
             return true;
         } else if (Layouts.MODULE.getFields(module).isRefinement()) {
             DynamicObject refinedClass = Layouts.MODULE.getFields(module).getRefinedClass();
-            return ((RubyGuards.isRubyClass(refinedClass)) && ModuleOperations.assignableTo(refinedClass, origin));
+            return RubyGuards.isRubyClass(refinedClass) && ModuleOperations.assignableTo(refinedClass, origin);
         } else {
-            return ((RubyGuards.isRubyClass(module)) && ModuleOperations.assignableTo(module, origin));
+            return RubyGuards.isRubyClass(module) && ModuleOperations.assignableTo(module, origin);
         }
     }
 

src/main/java/org/truffleruby/language/methods/CanBindMethodToModuleNode.java

@@ -32,18 +32,17 @@ public static CanBindMethodToModuleNode create() {
     protected boolean canBindMethodToCached(InternalMethod method, DynamicObject module,
             @Cached("method.getDeclaringModule()") DynamicObject declaringModule,
             @Cached("module") DynamicObject cachedModule,
-            @Cached("canBindMethodTo(declaringModule, cachedModule)") boolean canBindMethodTo) {
+            @Cached("canBindMethodTo(method, cachedModule)") boolean canBindMethodTo) {
         return canBindMethodTo;
     }
 
     @Specialization(guards = "isRubyModule(module)")
     protected boolean canBindMethodToUncached(InternalMethod method, DynamicObject module) {
-        final DynamicObject declaringModule = method.getDeclaringModule();
-        return canBindMethodTo(declaringModule, module);
+        return canBindMethodTo(method, module);
     }
 
-    protected boolean canBindMethodTo(DynamicObject declaringModule, DynamicObject module) {
-        return ModuleOperations.canBindMethodTo(declaringModule, module);
+    protected boolean canBindMethodTo(InternalMethod method, DynamicObject module) {
+        return ModuleOperations.canBindMethodTo(method, module);
     }
 
     protected int getCacheLimit() {