[GR-19220] method_added trigger fixes (#2875)

PullRequest: truffleruby/3659

Author: eregon

CHANGELOG.md

@@ -21,6 +21,7 @@ Bug fixes:
 * Fix `Array#pack` and accept `Numeric` values when `Float` is expected (#2815, @andrykonchin).
 * Fix `\P{}` matching in regular expressions (#2798, @andrykonchin).
 * Fix constants lookup when `BasicObject#instance_eval` method is called with a String (#2810, @andrykonchin).
+* Don't trigger the `method_added` event when changing a method's visibility or calling `module_function` (@paracycle, @nirvdrum).
 
 Compatibility:
 

spec/ruby/core/module/method_added_spec.rb

@@ -2,6 +2,10 @@
 require_relative 'fixtures/classes'
 
 describe "Module#method_added" do
+  before :each do
+    ScratchPad.record []
+  end
+
   it "is a private instance method" do
     Module.should have_private_instance_method(:method_added)
   end
@@ -13,8 +17,6 @@
   end
 
   it "is called when a new instance method is defined in self" do
-    ScratchPad.record []
-
     Module.new do
       def self.method_added(name)
         ScratchPad << name
@@ -32,8 +34,6 @@ def test() end
 
   it "is not called when a singleton method is added" do
     # obj.singleton_method_added is called instead
-    ScratchPad.record []
-
     klass = Class.new
     def klass.method_added(name)
       ScratchPad << name
@@ -60,8 +60,71 @@ def self.method_added(name)
     m.should_not have_method(:method_to_undef)
   end
 
+  it "is not called when a method changes visibility" do
+    Module.new do
+      def public_method
+      end
+
+      def private_method
+      end
+
+      def self.method_added(name)
+        ScratchPad << name
+      end
+
+      public :public_method
+      private :public_method
+
+      private :private_method
+      public :private_method
+    end
+
+    ScratchPad.recorded.should == []
+  end
+
+  it "is called when using #private in a subclass" do
+    parent = Class.new do
+      def foo
+      end
+    end
+
+    Class.new(parent) do
+      def self.method_added(name)
+        ScratchPad << name
+      end
+
+      # Create an instance as that might initialize some method lookup caches, which is interesting to test
+      self.new.foo
+
+      private :foo
+      public :foo
+    end
+
+    ScratchPad.recorded.should == [:foo]
+  end
+
+  it "is not called when a method is copied via module_function, rather #singleton_method_added is called" do
+    Module.new do
+      def mod_function
+      end
+
+      def self.method_added(name)
+        ScratchPad << [:method_added, name]
+      end
+
+      def self.singleton_method_added(name)
+        ScratchPad << [:singleton_method_added, name]
+      end
+
+      ScratchPad.record []
+
+      module_function :mod_function
+    end
+
+    ScratchPad.recorded.should == [[:singleton_method_added, :mod_function]]
+  end
+
   it "is called with a precise caller location with the line of the 'def'" do
-    ScratchPad.record []
     line = nil
 
     Module.new do

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

@@ -533,12 +533,19 @@ public void addMethod(RubyContext context, Node currentNode, InternalMethod meth
         }
 
         if (context.getCoreLibrary().isLoaded() && !method.isUndefined()) {
-            final RubySymbol methodSymbol = context.getLanguageSlow().getSymbol(method.getName());
-            if (RubyGuards.isSingletonClass(rubyModule)) {
-                RubyDynamicObject receiver = ((RubyClass) rubyModule).attached;
-                RubyContext.send(currentNode, receiver, "singleton_method_added", methodSymbol);
-            } else {
-                RubyContext.send(currentNode, rubyModule, "method_added", methodSymbol);
+            /* method_added/singleton_method_added should only be called if there wasn't already the same method
+             * definition for the given name on this rubyModule. Otherwise, it's just a change of visibility or so, not
+             * "a new method", and that should not call method_added/singleton_method_added. */
+            if (previousMethodEntry == null || previousMethodEntry.getMethod() == null ||
+                    previousMethodEntry.getMethod().getSharedMethodInfo() != method.getSharedMethodInfo()) {
+
+                final RubySymbol methodSymbol = context.getLanguageSlow().getSymbol(method.getName());
+                if (RubyGuards.isSingletonClass(rubyModule)) {
+                    RubyDynamicObject receiver = ((RubyClass) rubyModule).attached;
+                    RubyContext.send(currentNode, receiver, "singleton_method_added", methodSymbol);
+                } else {
+                    RubyContext.send(currentNode, rubyModule, "method_added", methodSymbol);
+                }
             }
         }
     }