diff --git a/CHANGELOG.md b/CHANGELOG.md index cc79f4550fe8..1664fdb3f15c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ Bug fixes: * Fix class lookup after an object's class has been replaced by `IO#reopen` (@itarato, @eregon). * Fix `Marshal.load` and raise `ArgumentError` when dump is broken and is too short (#3108, @andrykonchin). * Fix `super` method lookup for unbounded attached methods (#3131, @itarato). +* Fix `Module#define_method(name, Method)` to respect `module_function` visibility (#3181, @andrykonchin). Compatibility: diff --git a/spec/ruby/core/module/module_function_spec.rb b/spec/ruby/core/module/module_function_spec.rb index 0602e95ca9c0..1c3ec5471b12 100644 --- a/spec/ruby/core/module/module_function_spec.rb +++ b/spec/ruby/core/module/module_function_spec.rb @@ -155,15 +155,62 @@ def foo m.foo.should == ["m", "super_m"] end + + context "methods created with define_method" do + context "passed a block" do + it "creates duplicates of the given instance methods" do + m = Module.new do + define_method :test1 do; end + module_function :test1 + end + + m.respond_to?(:test1).should == true + end + end + + context "passed a method" do + it "creates duplicates of the given instance methods" do + module_with_method = Module.new do + def test1; end + end + + c = Class.new do + extend module_with_method + end + + m = Module.new do + define_method :test2, c.method(:test1) + module_function :test2 + end + + m.respond_to?(:test2).should == true + end + end + + context "passed an unbound method" do + it "creates duplicates of the given instance methods" do + module_with_method = Module.new do + def test1; end + end + + m = Module.new do + define_method :test2, module_with_method.instance_method(:test1) + module_function :test2 + end + + m.respond_to?(:test2).should == true + end + end + end end describe "Module#module_function as a toggle (no arguments) in a Module body" do it "makes any subsequently defined methods module functions with the normal semantics" do - m = Module.new { + m = Module.new do module_function def test1() end def test2() end - } + end m.respond_to?(:test1).should == true m.respond_to?(:test2).should == true @@ -187,13 +234,13 @@ def test2() end it "stops creating module functions if the body encounters another toggle " \ "like public/protected/private without arguments" do - m = Module.new { + m = Module.new do module_function def test1() end def test2() end public def test3() end - } + end m.respond_to?(:test1).should == true m.respond_to?(:test2).should == true @@ -202,14 +249,14 @@ def test3() end it "does not stop creating module functions if the body encounters " \ "public/protected/private WITH arguments" do - m = Module.new { + m = Module.new do def foo() end module_function def test1() end def test2() end public :foo def test3() end - } + end m.respond_to?(:test1).should == true m.respond_to?(:test2).should == true @@ -217,69 +264,116 @@ def test3() end end it "does not affect module_evaled method definitions also if outside the eval itself" do - m = Module.new { + m = Module.new do module_function module_eval { def test1() end } module_eval " def test2() end " - } + end m.respond_to?(:test1).should == false m.respond_to?(:test2).should == false end it "has no effect if inside a module_eval if the definitions are outside of it" do - m = Module.new { + m = Module.new do module_eval { module_function } def test1() end def test2() end - } + end m.respond_to?(:test1).should == false m.respond_to?(:test2).should == false end it "functions normally if both toggle and definitions inside a module_eval" do - m = Module.new { - module_eval { + m = Module.new do + module_eval do module_function def test1() end def test2() end - } - } + end + end m.respond_to?(:test1).should == true m.respond_to?(:test2).should == true end - it "affects evaled method definitions also even when outside the eval itself" do - m = Module.new { + it "affects eval'ed method definitions also even when outside the eval itself" do + m = Module.new do module_function eval "def test1() end" - } + end m.respond_to?(:test1).should == true end it "doesn't affect definitions when inside an eval even if the definitions are outside of it" do - m = Module.new { + m = Module.new do eval "module_function" def test1() end - } + end m.respond_to?(:test1).should == false end it "functions normally if both toggle and definitions inside a eval" do - m = Module.new { + m = Module.new do eval <<-CODE module_function def test1() end def test2() end CODE - } + end m.respond_to?(:test1).should == true m.respond_to?(:test2).should == true end + + context "methods are defined with define_method" do + context "passed a block" do + it "makes any subsequently defined methods module functions with the normal semantics" do + m = Module.new do + module_function + define_method :test1 do; end + end + + m.respond_to?(:test1).should == true + end + end + + context "passed a method" do + it "makes any subsequently defined methods module functions with the normal semantics" do + module_with_method = Module.new do + def test1; end + end + + c = Class.new do + extend module_with_method + end + + m = Module.new do + module_function + define_method :test2, c.method(:test1) + end + + m.respond_to?(:test2).should == true + end + end + + context "passed an unbound method" do + it "makes any subsequently defined methods module functions with the normal semantics" do + module_with_method = Module.new do + def test1; end + end + + m = Module.new do + module_function + define_method :test2, module_with_method.instance_method(:test1) + end + + m.respond_to?(:test2).should == true + end + end + end end diff --git a/src/main/java/org/truffleruby/core/module/ModuleNodes.java b/src/main/java/org/truffleruby/core/module/ModuleNodes.java index 2579ed9bea5e..96bdeef2d12c 100644 --- a/src/main/java/org/truffleruby/core/module/ModuleNodes.java +++ b/src/main/java/org/truffleruby/core/module/ModuleNodes.java @@ -1240,7 +1240,8 @@ protected RubySymbol defineMethodWithMethod( final String name = nameToJavaStringNode.execute(this, RubyArguments.getArgument(rubyArgs, 0)); final Object method = RubyArguments.getArgument(rubyArgs, 1); - return addMethod(module, name, (RubyMethod) method); + needCallerFrame(callerFrame, target); + return addMethod(module, name, (RubyMethod) method, callerFrame.materialize()); } @Specialization(guards = { "isMethodParameterProvided(rubyArgs)", "isRubyProc(getArgument(rubyArgs, 1))" }) @@ -1294,7 +1295,8 @@ protected RubySymbol defineMethodWithoutMethodAndBlock( } @TruffleBoundary - private RubySymbol addMethod(RubyModule module, String name, RubyMethod method) { + private RubySymbol addMethod(RubyModule module, String name, RubyMethod method, + MaterializedFrame callerFrame) { final InternalMethod internalMethod = method.method; if (!ModuleOperations.canBindMethodTo(internalMethod, module)) { @@ -1310,8 +1312,7 @@ private RubySymbol addMethod(RubyModule module, String name, RubyMethod method) } } - module.fields.addMethod(getContext(), this, internalMethod.withName(name)); - return getSymbol(name); + return addInternalMethod(module, name, internalMethod, callerFrame); } @TruffleBoundary