[GR-18163] Fix #define_method with module_function visibility

PullRequest: truffleruby/3918

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:
 

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,84 +249,131 @@ 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
     m.respond_to?(:test3).should == true
   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

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