fix: Override method accessors to provide the correct signature (#223)

Override method accessors to provide the correct signature for hooked methods

Author: kgilpin

lib/appmap/hook/method.rb

@@ -7,6 +7,23 @@ module AppMap
   end
 
   class Hook
+    SIGNATURES = {}
+
+    LOOKUP_SIGNATURE = lambda do |id|
+      method = super(id)
+    
+      signature = SIGNATURES[[ method.owner, method.name ]]
+      if signature
+        method.singleton_class.module_eval do
+          define_method(:parameters) do
+            signature
+          end
+        end
+      end
+    
+      method
+    end
+    
     class Method
       attr_reader :hook_package, :hook_class, :hook_method
 
@@ -100,12 +117,10 @@ def activate
         hook_method_def = hook_method_def.ruby2_keywords if hook_method_def.respond_to?(:ruby2_keywords)
 
         hook_method_parameters = hook_method.parameters.dup.freeze
+        SIGNATURES[[ hook_class, hook_method.name ]] = hook_method_parameters
+
         hook_class.ancestors.first.tap do |cls|
           cls.define_method_with_arity(hook_method.name, hook_method.arity, hook_method_def)
-          redefined_method = cls.instance_method(hook_method.name)
-          redefined_method.singleton_class.module_eval do
-            define_method(:parameters) { hook_method_parameters }
-          end
         end
       end
 
@@ -136,4 +151,27 @@ def with_disabled_hook(&function)
       end
     end
   end
+
+  module ObjectMethods
+    define_method(:method, AppMap::Hook::LOOKUP_SIGNATURE)
+    define_method(:public_method, AppMap::Hook::LOOKUP_SIGNATURE)
+    define_method(:singleton_method, AppMap::Hook::LOOKUP_SIGNATURE)
+  end
+
+  module ModuleMethods
+    define_method(:instance_method, AppMap::Hook::LOOKUP_SIGNATURE)
+    define_method(:public_instance_method, AppMap::Hook::LOOKUP_SIGNATURE)
+  end
+end
+
+unless ENV['APPMAP_NO_PATCH_OBJECT'] == 'true'
+  class Object
+    prepend AppMap::ObjectMethods
+  end
+end
+
+unless ENV['APPMAP_NO_PATCH_MODULE'] == 'true'
+  class Module
+    prepend AppMap::ModuleMethods
+  end
 end

spec/fixtures/hook/report_parameters.rb

@@ -0,0 +1,8 @@
+
+class ReportParameters
+  def to_s; self.class.name; end
+
+  def report_parameters(*args, kw1:, kw2: 'kw2', **kws)
+    method(:report_parameters).parameters
+  end
+end

spec/hook_spec.rb

@@ -225,6 +225,12 @@ def test_hook_behavior(file, events_yaml, setup: nil, &block)
         :value: default
     YAML
     test_hook_behavior 'spec/fixtures/hook/instance_method.rb', events_yaml do
+      expect(InstanceMethod.instance_method(:say_default).parameters).to eq([])
+      expect(InstanceMethod.public_instance_method(:say_default).parameters).to eq([])
+      expect(InstanceMethod.new.method(:say_default).parameters).to eq([])
+      expect(InstanceMethod.new.public_method(:say_default).parameters).to eq([])
+      expect { InstanceMethod.new.singleton_method(:say_default) }.to raise_error(NameError)
+
       expect(InstanceMethod.new.say_default).to eq('default')
     end
   end
@@ -315,7 +321,22 @@ def test_hook_behavior(file, events_yaml, setup: nil, &block)
         :class: String
         :value: protected
     YAML
+    parameters = []
     test_hook_behavior 'spec/fixtures/hook/protected_method.rb', events_yaml do
+      expect(ProtectedMethod.singleton_method(:call_protected).parameters).to eq(parameters)
+      expect(ProtectedMethod.instance_method(:call_protected).parameters).to eq(parameters)
+      expect(ProtectedMethod.public_instance_method(:call_protected).parameters).to eq(parameters)
+      expect(ProtectedMethod.new.method(:call_protected).parameters).to eq(parameters)
+      expect(ProtectedMethod.new.public_method(:call_protected).parameters).to eq(parameters)
+      expect { ProtectedMethod.new.singleton_method(:call_protected) }.to raise_error(NameError)
+
+      expect(ProtectedMethod.singleton_method(:protected_method).parameters).to eq(parameters)
+      expect(ProtectedMethod.instance_method(:protected_method).parameters).to eq(parameters)
+      expect(ProtectedMethod.public_instance_method(:protected_method).parameters).to eq(parameters)
+      expect(ProtectedMethod.new.method(:protected_method).parameters).to eq(parameters)
+      expect(ProtectedMethod.new.public_method(:protected_method).parameters).to eq(parameters)
+      expect { ProtectedMethod.new.singleton_method(:protected_method) }.to raise_error(NameError)
+
       expect(ProtectedMethod.new.call_protected).to eq('protected')
     end
   end
@@ -358,6 +379,7 @@ def test_hook_behavior(file, events_yaml, setup: nil, &block)
         :class: String
         :value: self.protected
     YAML
+    parameters = []
     test_hook_behavior 'spec/fixtures/hook/protected_method.rb', events_yaml do
       expect(ProtectedMethod.call_protected).to eq('self.protected')
     end
@@ -1100,6 +1122,51 @@ def secure_compare(a, b)
     end
   end
 
+  it 'preserves the sighnature of hooked methods' do
+    parameters = []
+    events = <<~YAML
+    ---
+    - :id: 1
+      :event: :call
+      :defined_class: ReportParameters
+      :method_id: report_parameters
+      :path: spec/fixtures/hook/report_parameters.rb
+      :lineno: 5
+      :static: false
+      :parameters:
+      - :name: :args
+        :class: Array
+        :value: '["foo"]'
+        :kind: :rest
+      - :name: :kw1
+        :class: String
+        :value: kw1
+        :kind: :keyreq
+      - :name: :kw2
+        :class: NilClass
+        :value: null
+        :kind: :key
+      - :name: :kws
+        :class: Hash
+        :value: "{}"
+        :kind: :keyrest
+      :receiver:
+        :class: ReportParameters
+        :value: ReportParameters
+    - :id: 2
+      :event: :return
+      :parent_id: 1
+      :return_value:
+        :class: Array
+        :value: "[[:rest, :args], [:keyreq, :kw1], [:key, :kw2], [:keyrest, :kws]]"
+    YAML
+    parameters = [[:rest, :args], [:keyreq, :kw1], [:key, :kw2], [:keyrest, :kws]]
+    test_hook_behavior 'spec/fixtures/hook/report_parameters.rb', events do
+      expect(ReportParameters.instance_method(:report_parameters).parameters).to eq(parameters)
+      expect(ReportParameters.new.report_parameters('foo', kw1: 'kw1')).to eq(parameters)
+    end
+  end
+
   describe 'kwargs handling' do
     if ruby_2?
       # https://github.com/applandinc/appmap-ruby/issues/153