diff --git a/internal/vm/builtins.go b/internal/vm/builtins.go index fbb5823b..7cc6839d 100644 --- a/internal/vm/builtins.go +++ b/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) }) diff --git a/internal/vm/define_method_conformance_test.go b/internal/vm/define_method_conformance_test.go new file mode 100644 index 00000000..f78e7fcd --- /dev/null +++ b/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) + } +} diff --git a/internal/vm/method_reflection_test.go b/internal/vm/method_reflection_test.go index cfd4969c..f79c33d0 100644 --- a/internal/vm/method_reflection_test.go +++ b/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 { diff --git a/internal/vm/modulefunc.go b/internal/vm/modulefunc.go index bc61223e..67f4256a 100644 --- a/internal/vm/modulefunc.go +++ b/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) + } +} diff --git a/internal/vm/object_model.go b/internal/vm/object_model.go index 4e3c77b0..ee7afa98 100644 --- a/internal/vm/object_model.go +++ b/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 diff --git a/internal/vm/tier0_test.go b/internal/vm/tier0_test.go index 46e99924..46d9f83b 100644 --- a/internal/vm/tier0_test.go +++ b/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"}, })