Module#define_method: MRI 4.0.6 arity, visibility, hooks, and bind checks

internal/vm/builtins.go

@@ -1362,43 +1362,55 @@ func (vm *VM) bootstrap() {
 	// Module#module_exec is Module#class_exec (the block runs with the module as
 	// self and receives the given arguments).
 	vm.cModule.define("module_exec", classExec)
-	vm.cModule.define("define_method", func(_ *VM, self object.Value, args []object.Value, blk *Proc) object.Value {
+	vm.cModule.define("define_method", func(vm *VM, self object.Value, args []object.Value, blk *Proc) object.Value {
 		cls := self.(*RClass)
-		name := nameArg(args[0])
+		if isFrozen(cls) {
+			vm.raiseFrozen(cls)
+		}
+		name := vm.defineMethodName(args[0])
+		vis := vm.defineMethodVis(cls, name)
 		// A Method / UnboundMethod second argument transplants that method's body
 		// under the new name and owner (Ruby allows define_method(:m, other_method)).
+		// The target class must be compatible with the source method's owner.
 		if len(args) > 1 {
 			switch src := args[1].(type) {
 			case *BoundMethod:
+				vm.checkTransplantBindable(cls, src.m.owner)
 				cm := *src.m
 				cm.origName = methodOriginalName(src.m)
-				cm.name, cm.owner = name, cls
+				cm.name, cm.owner, cm.vis = name, cls, vis
 				cls.methods[name] = &cm
 				bumpMethodSerial()
+				vm.fireMethodAdded(cls, name)
 				return object.Symbol(name)
 			case *UnboundMethod:
+				vm.checkTransplantBindable(cls, src.owner)
 				cm := *src.m
 				cm.origName = methodOriginalName(src.m)
-				cm.name, cm.owner = name, cls
+				cm.name, cm.owner, cm.vis = name, cls, vis
 				cls.methods[name] = &cm
 				bumpMethodSerial()
+				vm.fireMethodAdded(cls, name)
 				return object.Symbol(name)
 			}
 		}
+		// A Proc second argument is the body and takes precedence over any block
+		// also passed at the call site (MRI). A non-Proc second argument that got
+		// past the Method/UnboundMethod cases above is a type error.
 		body := blk
-		if body == nil {
-			if len(args) > 1 {
-				p, ok := args[1].(*Proc)
-				if !ok {
-					raise("TypeError", "wrong argument type %s (expected Proc)", classNameOf(args[1]))
-				}
-				body = p
-			} else {
-				raise("ArgumentError", "tried to create a method without a block")
+		if len(args) > 1 {
+			p, ok := args[1].(*Proc)
+			if !ok {
+				raise("TypeError", "wrong argument type %s (expected Proc/Method/UnboundMethod)", vm.classOf(args[1]).name)
 			}
+			body = p
+		}
+		if body == nil {
+			raise("ArgumentError", "tried to create a method without a block")
 		}
-		cls.methods[name] = &Method{name: name, proc: body, owner: cls}
+		cls.methods[name] = &Method{name: name, proc: body, owner: cls, vis: vis}
 		bumpMethodSerial()
+		vm.fireMethodAdded(cls, name)
 		return object.Symbol(name)
 	})
 

internal/vm/define_method_conformance_test.go

@@ -0,0 +1,194 @@
+package vm_test
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestDefineMethodArityConformance covers the lambda argument-arity semantics a
+// method created by define_method enforces: a count mismatch raises ArgumentError
+// and a lone Array argument is never auto-splat destructured (MRI 4.0.6).
+func TestDefineMethodArityConformance(t *testing.T) {
+	arityErr := []struct{ name, src string }{
+		{"zero_arg_required", `Class.new { define_method(:m) { |a| a } }.new.m`},
+		{"two_args_for_one", `Class.new { define_method(:m) { |a| a } }.new.m(1, 2)`},
+		{"one_arg_for_zero", `Class.new { define_method(:m) { :x } }.new.m(1)`},
+		{"opt_default_zero", `Class.new { define_method(:m) { |a, b = 1| a } }.new.m`},
+		{"opt_default_three", `Class.new { define_method(:m) { |a, b = 1| a } }.new.m(1, 2, 3)`},
+		{"proc_zero_param", `Class.new { p = Proc.new { || true }; define_method(:m, &p) }.new.m(:x)`},
+	}
+	for _, tc := range arityErr {
+		t.Run(tc.name, func(t *testing.T) {
+			err := runErr(t, tc.src)
+			if err == nil || !strings.Contains(err.Error(), "ArgumentError") {
+				t.Errorf("src=%q: got err=%v, want ArgumentError", tc.src, err)
+			}
+		})
+	}
+
+	// A lone Array argument binds whole to the single parameter (no auto-splat).
+	if got := eval(t, `p Class.new { define_method(:m) { |a,| a } }.new.m([1, 2])`); got != "[1, 2]\n" {
+		t.Errorf("no-auto-splat: got %q, want %q", got, "[1, 2]\n")
+	}
+	// Optional and splat parameters still bind normally under the strict check.
+	if got := eval(t, `p Class.new { define_method(:m) { |a, b = 9| [a, b] } }.new.m(1)`); got != "[1, 9]\n" {
+		t.Errorf("optional default: got %q, want %q", got, "[1, 9]\n")
+	}
+	if got := eval(t, `p Class.new { define_method(:m) { |a, *b| [a, b] } }.new.m(1, 2, 3)`); got != "[1, [2, 3]]\n" {
+		t.Errorf("splat: got %q, want %q", got, "[1, [2, 3]]\n")
+	}
+}
+
+// TestDefineMethodVisibilityConformance covers the visibility a define_method-ed
+// method receives: the current default when the definee is the receiver, public
+// when defined from another module, and always-private for :initialize.
+func TestDefineMethodVisibilityConformance(t *testing.T) {
+	cases := []struct{ name, src, want string }{
+		{"target_module_block", `
+			k = Class.new do
+				define_method(:pub) {}
+				private
+				define_method(:priv) {}
+			end
+			p k.public_instance_methods(false).include?(:pub)
+			p k.private_instance_methods(false).include?(:priv)`, "true\ntrue\n"},
+		{"another_module_public", `
+			k = Class.new
+			Class.new do
+				k.send(:define_method, :a) {}
+				private
+				k.send(:define_method, :b) {}
+			end
+			p k.public_instance_methods(false).include?(:a)
+			p k.public_instance_methods(false).include?(:b)`, "true\ntrue\n"},
+		{"initialize_private_block", `
+			k = Class.new { define_method(:initialize) {} }
+			p k.private_instance_methods(false).include?(:initialize)`, "true\n"},
+		{"initialize_private_unbound", `
+			k = Class.new do
+				def t; end
+				define_method(:initialize, instance_method(:t))
+			end
+			p k.private_instance_methods(false).include?(:initialize)`, "true\n"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := eval(t, tc.src); got != tc.want {
+				t.Errorf("src=%q\n got=%q\nwant=%q", tc.src, got, tc.want)
+			}
+		})
+	}
+}
+
+// TestDefineMethodErrorsConformance covers define_method's error paths: name
+// coercion, the non-Proc/Method type error, frozen receivers, and the cross-class
+// bind checks for a transplanted Method/UnboundMethod.
+func TestDefineMethodErrorsConformance(t *testing.T) {
+	cases := []struct{ name, src, wantClass, wantMsg string }{
+		{"not_proc_string", `Class.new { define_method(:m, "s") }`,
+			"TypeError", "wrong argument type String (expected Proc/Method/UnboundMethod)"},
+		{"not_proc_nil", `Class.new { define_method(:m, nil) }`,
+			"TypeError", "wrong argument type NilClass (expected Proc/Method/UnboundMethod)"},
+		{"not_proc_int", `Class.new { define_method(:m, 1234) }`,
+			"TypeError", "wrong argument type Integer (expected Proc/Method/UnboundMethod)"},
+		{"no_block", `Class.new { define_method(:m) }`,
+			"ArgumentError", ""},
+		{"name_not_sym", `Class.new { define_method(1001, ->{}) }`,
+			"TypeError", "is not a symbol nor a string"},
+		{"to_str_non_string", `
+			o = Object.new
+			def o.to_str() [] end
+			Class.new { define_method(o, ->{}) }`,
+			"TypeError", "can't convert Object to String"},
+		{"frozen", `Class.new { freeze; define_method(:m) {} }`,
+			"FrozenError", ""},
+		{"unbound_unrelated_class", `
+			c = Class.new { def foo; :x; end }
+			Class.new { define_method(:bar, c.instance_method(:foo)) }`,
+			"TypeError", "bind argument must be a subclass of"},
+		{"unbound_child_on_parent", `
+			P = Class.new { define_method(:foo) { :bar } }
+			C = Class.new(P) { define_method(:foo) { :baz } }
+			P.send(:define_method, :foo, C.instance_method(:foo))`,
+			"TypeError", "bind argument must be a subclass of"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := runErr(t, tc.src)
+			if err == nil {
+				t.Fatalf("src=%q: expected %s, got nil", tc.src, tc.wantClass)
+			}
+			if !strings.Contains(err.Error(), tc.wantClass) {
+				t.Errorf("src=%q: got err=%v, want class %s", tc.src, err, tc.wantClass)
+			}
+			if tc.wantMsg != "" && !strings.Contains(err.Error(), tc.wantMsg) {
+				t.Errorf("src=%q: got err=%v, want message %q", tc.src, err, tc.wantMsg)
+			}
+		})
+	}
+}
+
+// TestDefineMethodBodyConformance covers the successful body forms: name coercion
+// via #to_str, a Method/UnboundMethod taking precedence over a passed block, and
+// the method_added hook firing once after the method is installed.
+func TestDefineMethodBodyConformance(t *testing.T) {
+	cases := []struct{ name, src, want string }{
+		{"proc_precedes_block", `
+			k = Class.new do
+				define_method(:m, -> { :from_method }) { :from_block }
+			end
+			p k.new.m`, ":from_method\n"},
+		{"to_str_name", `
+			o = Object.new
+			def o.to_str() "foo" end
+			k = Class.new { define_method(o, -> { :ok }) }
+			p k.new.foo`, ":ok\n"},
+		{"unbound_transplant", `
+			k = Class.new do
+				def orig(a, b); [a, b]; end
+				define_method(:copy, instance_method(:orig))
+			end
+			p k.new.copy(1, 2)`, "[1, 2]\n"},
+		{"method_added_fires", `
+			$added = []
+			k = Class.new do
+				def self.method_added(n); $added << n; end
+				define_method(:hooked) { true }
+			end
+			p $added`, "[:hooked]\n"},
+		{"returns_symbol", `p Class.new { }.send(:define_method, "n") { true }`, ":n\n"},
+		{"module_owner_permissive", `
+			m = Module.new { def foo; :from_mod; end }
+			k = Class.new { define_method(:bar, m.instance_method(:foo)) }
+			p k.new.bar`, ":from_mod\n"},
+		{"object_owner_permissive", `
+			k = Class.new(BasicObject) do
+				define_method(:iof, ::Object.instance_method(:instance_of?))
+			end
+			p k.new.iof(k)`, "true\n"},
+		{"initialize_copy_private", `
+			k = Class.new { define_method(:initialize_copy) { |o| } }
+			p k.private_instance_methods(false).include?(:initialize_copy)`, "true\n"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := eval(t, tc.src); got != tc.want {
+				t.Errorf("src=%q\n got=%q\nwant=%q", tc.src, got, tc.want)
+			}
+		})
+	}
+}
+
+// TestDefineMethodSingletonBind covers the singleton-owner branch of the
+// transplant bind check: a singleton method re-homed onto an unrelated class
+// raises MRI's distinct "different class" TypeError.
+func TestDefineMethodSingletonBind(t *testing.T) {
+	src := `
+		c = Class.new { class << self; def foo; end; end }
+		m = c.method(:foo)
+		Class.new { define_method(:bar, m) }`
+	err := runErr(t, src)
+	if err == nil || !strings.Contains(err.Error(), "can't bind singleton method to a different class") {
+		t.Errorf("got err=%v, want singleton bind TypeError", err)
+	}
+}

internal/vm/method_reflection_test.go

@@ -107,8 +107,9 @@ func TestMethodOriginalName(t *testing.T) {
 		{`class C; define_method(:mine, ::Kernel.instance_method(:is_a?)); end; p C.new.method(:mine).original_name`, ":is_a?\n"},
 		// A block-defined method's original name is the name it was defined under.
 		{`class C; define_method(:f){}; end; p C.new.method(:f).original_name`, ":f\n"},
-		// An instance-method transplant keeps the iseq's original name.
-		{`class S; def orig; end; end; class C; define_method(:copy, S.instance_method(:orig)); end; p C.new.method(:copy).original_name`, ":orig\n"},
+		// An instance-method transplant keeps the iseq's original name (the target is
+		// a subclass of the source, as MRI's bind-compatibility rule requires).
+		{`class S; def orig; end; end; class C < S; define_method(:copy, S.instance_method(:orig)); end; p C.new.method(:copy).original_name`, ":orig\n"},
 	}
 	for _, c := range cases {
 		if got := eval(t, c.src); got != c.want {

internal/vm/modulefunc.go

@@ -216,3 +216,75 @@ func nameArg(v object.Value) string {
 		return ""
 	}
 }
+
+// defineMethodName coerces define_method's name argument. A Symbol or String is
+// taken directly; any other object is converted through #to_str, and a #to_str
+// that returns a non-String raises TypeError with MRI's "can't convert" message.
+// An object without #to_str raises the "is not a symbol nor a string" TypeError.
+func (vm *VM) defineMethodName(v object.Value) string {
+	switch x := v.(type) {
+	case object.Symbol:
+		return string(x)
+	case *object.String:
+		return x.Str()
+	}
+	if vm.respondsToDynamic(v, "to_str") {
+		r := vm.send(v, "to_str", nil, nil)
+		if s, ok := r.(*object.String); ok {
+			return s.Str()
+		}
+		raise("TypeError", "can't convert %s to String (%s#to_str gives %s)",
+			classNameOf(v), classNameOf(v), classNameOf(r))
+	}
+	raise("TypeError", "%s is not a symbol nor a string", vm.inspectStr(v))
+	return ""
+}
+
+// defineMethodVis is the visibility a method created by define_method receives.
+// :initialize and :initialize_copy are always private. Otherwise the method takes
+// the receiver's current default visibility: MRI uses the caller frame's default
+// visibility only when the definee equals the receiver module, and public
+// otherwise — and a `private`/`public` directive lands on its frame's self, so the
+// receiver's own defaultVis carries the directive exactly in that "definee equals
+// receiver" case (a class/module body or a class_eval on the receiver) and stays
+// at the receiver's untouched default when the directive was issued elsewhere.
+func (vm *VM) defineMethodVis(cls *RClass, name string) visibility {
+	if name == "initialize" || name == "initialize_copy" {
+		return visPrivate
+	}
+	return cls.defaultVis
+}
+
+// checkTransplantBindable raises TypeError when a Method/UnboundMethod whose
+// method is owned by owner cannot be re-homed onto cls by define_method. A method
+// owned by a Class (or a singleton class) may only move onto that class or one of
+// its subclasses; a method owned by an ordinary Module may move anywhere. The
+// singleton case carries MRI's distinct "different class" message.
+func (vm *VM) checkTransplantBindable(cls, owner *RClass) {
+	if owner == nil || owner.isModule {
+		return
+	}
+	// rbgo models the Kernel methods (instance_of?, respond_to?, …) as Object
+	// instance methods, so an UnboundMethod pulled from Object reports Object as its
+	// owner where MRI reports Kernel (a Module). Treat the universal roots as
+	// permissive owners so such a method can be re-homed onto any class, including a
+	// BasicObject subclass — matching MRI's module-owner rule.
+	if owner == vm.cObject || owner == vm.cBasicObject {
+		return
+	}
+	if classIsA(cls, owner) {
+		return
+	}
+	if owner.isSingleton {
+		raise("TypeError", "can't bind singleton method to a different class")
+	}
+	raise("TypeError", "bind argument must be a subclass of %s", owner.name)
+}
+
+// fireMethodAdded invokes cls.method_added(:name) when cls defines that hook as a
+// singleton method, mirroring the OpDefineMethod path for `def`.
+func (vm *VM) fireMethodAdded(cls *RClass, name string) {
+	if hook := lookupSMethod(cls, "method_added"); hook != nil {
+		vm.invoke(hook, cls, []object.Value{object.SymVal(name)}, nil)
+	}
+}

internal/vm/object_model.go

@@ -1982,10 +1982,16 @@ func (vm *VM) callProcMethod(p *Proc, self object.Value, args []object.Value, bl
 	anchored.superName, anchored.superDefinee, anchored.superArgs = name, owner, args
 	anchored.dmBody = true
 	anchored.dmDirect = true // this frame IS the method body: its `return` is a return target
+	// A define_method body always binds its arguments with method (lambda) arity
+	// semantics, regardless of whether the source was a block or a non-lambda Proc:
+	// MRI raises ArgumentError on a count mismatch and never auto-splats a lone Array.
+	// Marking the anchored copy as a lambda routes bindBlockArgs down that path so
+	// exec's own arity check fires. break/redo inside also take lambda semantics.
+	anchored.isLambda = true
 	// The define_method body IS the method: __method__ / __callee__ both report the
 	// method name it was defined under (and a block nested in the body inherits it).
 	anchored.methodCtx = frameMethod{orig: name, callee: name}
-	return vm.exec(p.iseq, self, vm.bindBlockArgs(p, args), vm.blockDefinee(p), "", p.env, body, &anchored, body, nil)
+	return vm.exec(p.iseq, self, vm.bindBlockArgs(&anchored, args), vm.blockDefinee(p), "", p.env, body, &anchored, body, nil)
 }
 
 // classEval runs a block as class_eval/module_eval would: self and the method

internal/vm/tier0_test.go

@@ -179,7 +179,9 @@ func TestReflection(t *testing.T) {
 		{`class C; def foo; 11; end; end; m = C.new.method(:foo); p m.unbind.bind(C.new).call`, "11\n"},
 		// define_method from an (Unbound)Method.
 		{`class C; def foo; 3; end; end; class C; define_method(:bar, instance_method(:foo)); end; p C.new.bar`, "3\n"},
-		{`class C; def foo; 7; end; end; m = C.new.method(:foo); class D; end; D.define_method(:g, m); p D.new.g`, "7\n"},
+		// A bound Method transplants onto the owner or a subclass of it (MRI rejects an
+		// unrelated target with "bind argument must be a subclass of ...").
+		{`class C; def foo; 7; end; end; m = C.new.method(:foo); class D < C; end; D.define_method(:g, m); p D.new.g`, "7\n"},
 		// method_defined?.
 		{`class C; def foo; end; end; p [C.method_defined?(:foo), C.method_defined?(:nope)]`, "[true, false]\n"},
 	})