Fix generic binding and block signature merging through a @!parse stub

lib/solargraph/api_map.rb

@@ -452,7 +452,7 @@ def get_block_pins
     def get_methods rooted_tag, scope: :instance, visibility: [:public], deep: true
       rooted_type = ComplexType.try_parse(rooted_tag)
       fqns = rooted_type.namespace
-      namespace_pin = store.get_path_pins(fqns).select { |p| p.is_a?(Pin::Namespace) }.first
+      namespace_pin = namespace_pin_for_generics(fqns)
       cached = cache.get_methods(rooted_tag, scope, visibility, deep)
       return cached.clone unless cached.nil?
       # @type [Array<Solargraph::Pin::Method>]
@@ -783,9 +783,10 @@ def inner_get_methods_from_reference fq_reference_tag, namespace_pin, type, scop
       # @todo Can inner_get_methods be cached?  Lots of lookups of base types going on.
       methods = inner_get_methods(resolved_reference_type.tag, scope, visibility, deep, skip, no_core)
       if namespace_pin && !resolved_reference_type.all_params.empty?
-        reference_pin = store.get_path_pins(resolved_reference_type.name).select { |p| p.is_a?(Pin::Namespace) }.first
+        reference_pin = namespace_pin_for_generics(resolved_reference_type.name)
         # logger.debug { "ApiMap#add_methods_from_reference(type=#{type}) - resolving generics with #{reference_pin.generics}, #{resolved_reference_type.rooted_tags}" }
         methods = methods.map do |method_pin|
+          # @sg-ignore Need to add nil check here
           method_pin.resolve_generics(reference_pin, resolved_reference_type)
         end
       end
@@ -811,6 +812,17 @@ def store
       @store ||= Store.new
     end
 
+    # The namespace pin for fqns that actually declares its generics,
+    # picked from any duplicate pins for the same namespace.
+    # @todo Consider trade-offs of merging namespace pins instead of trying to choose the best one for this use
+    # @param fqns [String]
+    # @return [Pin::Namespace, nil]
+    def namespace_pin_for_generics fqns
+      candidates = store.get_path_pins(fqns).select { |p| p.is_a?(Pin::Namespace) }
+      # @sg-ignore select with an is_a? block does not narrow the element type
+      candidates.find { |p| !p.generics.empty? } || candidates.first
+    end
+
     # @return [Solargraph::ApiMap::Cache]
     attr_reader :cache
 
@@ -826,7 +838,7 @@ def inner_get_methods rooted_tag, scope, visibility, deep, skip, no_core = false
       rooted_type = ComplexType.parse(rooted_tag).force_rooted
       fqns = rooted_type.namespace
       rooted_type.all_params
-      namespace_pin = store.get_path_pins(fqns).select { |p| p.is_a?(Pin::Namespace) }.first
+      namespace_pin = namespace_pin_for_generics(fqns)
       return [] if no_core && fqns =~ /^(Object|BasicObject|Class|Module)$/
       reqstr = "#{fqns}|#{scope}|#{visibility.sort}|#{deep}"
       return [] if skip.include?(reqstr)
@@ -867,6 +879,7 @@ def inner_get_methods rooted_tag, scope, visibility, deep, skip, no_core = false
           end
           rooted_sc_tag = qualify_superclass(rooted_tag)
           unless rooted_sc_tag.nil?
+            # @sg-ignore Need to add nil check here
             result.concat inner_get_methods_from_reference(rooted_sc_tag, namespace_pin, rooted_type, scope,
                                                            visibility, true, skip, no_core)
           end
@@ -880,6 +893,7 @@ def inner_get_methods rooted_tag, scope, visibility, deep, skip, no_core = false
           end
           rooted_sc_tag = qualify_superclass(rooted_tag)
           unless rooted_sc_tag.nil?
+            # @sg-ignore Need to add nil check here
             result.concat inner_get_methods_from_reference(rooted_sc_tag, namespace_pin, rooted_type, scope,
                                                            visibility, true, skip, true)
           end

lib/solargraph/api_map/store.rb

@@ -72,11 +72,12 @@ def get_constants fqns, visibility = [:public]
       # @param fqns [String]
       # @param scope [Symbol]
       # @param visibility [Array<Symbol>]
-      # @return [Enumerable<Solargraph::Pin::Method>]
+      # @return [Array<Solargraph::Pin::Method>]
       def get_methods fqns, scope: :instance, visibility: [:public]
-        namespace_children(fqns).select do |pin|
+        pins = namespace_children(fqns).select do |pin|
           pin.is_a?(Pin::Method) && pin.scope == scope && visibility.include?(pin.visibility)
         end
+        combine_duplicate_method_pins(pins)
       end
 
       BOOLEAN_SUPERCLASS_PIN = Pin::Reference::Superclass.new(name: 'Boolean', closure: Pin::ROOT_PIN,
@@ -296,6 +297,27 @@ def index
         @index ||= Index.new
       end
 
+      # Combines same-path pins into one. They arise when a method is
+      # documented in more than one file - a `@!parse` stub re-documenting
+      # a method the gem already defines, or a reopened class. Aliases and
+      # DelegatedMethod are skipped; neither survives a merge.
+      #
+      # @param pins [Array<Pin::Method>]
+      # @return [Array<Pin::Method>]
+      def combine_duplicate_method_pins pins
+        result = []
+        pins.group_by(&:path).each_value do |group|
+          if group.length == 1 || group.any? { |pin| pin.is_a?(Pin::MethodAlias) || pin.is_a?(Pin::DelegatedMethod) }
+            result.concat(group)
+          else
+            # @sg-ignore group is never empty here (group_by never yields an empty group)
+            combined = group[1..].reduce(group.first) { |memo, pin| memo.combine_with(pin) }
+            result.push(combined)
+          end
+        end
+        result
+      end
+
       # @param pinsets [Array<Array<Pin::Base>>]
       #
       # @return [true]

lib/solargraph/pin/callable.rb

@@ -34,18 +34,21 @@ def method_namespace
         closure.namespace
       end
 
+      # A block documented with fewer yielded parameters says less
+      # about what it yields than a sibling with more, so it loses to
+      # that sibling rather than being picked between arbitrarily.
+      #
       # @param other [self]
       #
       # @return [Pin::Signature, nil]
       def combine_blocks other
-        if block.nil?
-          other.block
-        elsif other.block.nil?
-          block
-        else
-          # @type [Pin::Signature, nil]
-          choose_pin_attr(other, :block)
-        end
+        return other.block if block.nil? ||
+                              (!other.block.nil? && block.parameters.length < other.block.parameters.length)
+        return block if other.block.nil? || (block.parameters.length > other.block.parameters.length)
+        return block.combine_with(other.block) if block.arity == other.block.arity
+
+        # @type [Pin::Signature, nil]
+        choose_pin_attr(other, :block)
       end
 
       # @param other [self]

lib/solargraph/pin/method.rb

@@ -295,8 +295,7 @@ def typify api_map
         type = see_reference(api_map) || typify_from_super(api_map)
         logger.debug { "Method#typify(self=#{self}) - type=#{type&.rooted_tags.inspect}" }
         unless type.nil?
-          # @sg-ignore Need to add nil check here
-          qualified = type.qualify(api_map, *closure.gates)
+          qualified = type.qualify(api_map, *(closure&.gates || ['']))
           logger.debug { "Method#typify(self=#{self}) => #{qualified.rooted_tags.inspect}" }
           return qualified
         end
@@ -493,9 +492,11 @@ def combine_signatures other
       #
       # @return [Array<Pin::Signature>]
       def combine_signatures_by_type_arity(*signature_pins)
+        merged_pins = merge_signatures_differing_only_by_block_informativeness(signature_pins)
+
         # @type [Hash{Array => Array<Pin::Signature>}]
         by_type_arity = {}
-        signature_pins.each do |signature_pin|
+        merged_pins.each do |signature_pin|
           by_type_arity[signature_pin.type_arity] ||= []
           by_type_arity[signature_pin.type_arity] << signature_pin
         end
@@ -506,6 +507,46 @@ def combine_signatures_by_type_arity(*signature_pins)
         by_type_arity.values.flatten
       end
 
+      # A block documented with fewer yielded parameters than a
+      # sibling signature's block (including zero, e.g. `&block` with
+      # no @yieldparam at all) differs in type_arity from it, so
+      # combine_signatures_by_type_arity's bucketing would otherwise
+      # keep them as separate overloads. They're the same overload,
+      # just documented with different completeness - merge them
+      # here, before that bucketing, so the merged signature carries
+      # the more complete block onward.
+      #
+      # @param signature_pins [Array<Pin::Signature>]
+      # @return [Array<Pin::Signature>]
+      def merge_signatures_differing_only_by_block_informativeness signature_pins
+        # @param sig [Pin::Signature]
+        # @param result [Array<Pin::Signature>]
+        signature_pins.each_with_object([]) do |sig, result|
+          # @type [Integer, nil]
+          match_index = result.find_index { |existing| combinable_by_block_informativeness?(existing, sig) }
+          if match_index.nil?
+            result << sig
+          else
+            existing = result.fetch(match_index)
+            # Bypass Callable#combine_with's default parameter zip: it
+            # compares the signatures' full arity, which folds in the
+            # block's own arity and would raise here even though the
+            # blockless parameters already match (that's what
+            # combinable_by_block_informativeness? just verified).
+            result[match_index] = existing.combine_with(sig, parameters: existing.parameters)
+          end
+        end
+      end
+
+      # @param sig1 [Pin::Signature]
+      # @param sig2 [Pin::Signature]
+      # @return [Boolean]
+      def combinable_by_block_informativeness? sig1, sig2
+        return false if sig1.block.nil? || sig2.block.nil?
+        return false if sig1.block.parameters.length == sig2.block.parameters.length
+        sig1.type_arity[0..-2] == sig2.type_arity[0..-2]
+      end
+
       # @param same_type_arity_signatures [Array<Pin::Signature>]
       #
       # @return [Array<Pin::Signature>]

spec/api_map/store_spec.rb

@@ -49,6 +49,83 @@
     expect(store.get_path_pins('Bar')).to eq([bar_pin])
   end
 
+  describe '#get_methods' do
+    it 'combines pins for the same method path from different sources' do
+      plain_impl = Solargraph::SourceMap.load_string(%(
+        class Foo
+          def bar; end
+        end
+      ), 'plain.rb')
+      override = Solargraph::SourceMap.load_string(%(
+        class Foo
+          # @return [String]
+          def bar; end
+        end
+      ), 'override.rb')
+      store = described_class.new(plain_impl.pins + override.pins)
+      pins = store.get_methods('Foo', scope: :instance).select { |p| p.name == 'bar' }
+      expect(pins.length).to eq(1)
+      expect(pins.first.return_type.tag).to eq('String')
+    end
+
+    it 'does not combine a method alias with a regular method sharing its path' do
+      # A combined alias pin can't be traced back to its original
+      # target, which #resolve_method_alias needs to work.
+      regular = Solargraph::SourceMap.load_string(%(
+        class Foo
+          def bar; end
+        end
+      ), 'regular.rb')
+      aliased = Solargraph::SourceMap.load_string(%(
+        class Foo
+          def baz; end
+          alias bar baz
+        end
+      ), 'aliased.rb')
+      store = described_class.new(regular.pins + aliased.pins)
+      pins = []
+      expect { pins = store.get_methods('Foo', scope: :instance) }.not_to raise_error
+      bar_pins = pins.select { |p| p.name == 'bar' }
+      expect(bar_pins.length).to eq(2)
+      expect(bar_pins).to include(an_instance_of(Solargraph::Pin::MethodAlias))
+    end
+
+    it 'does not combine two delegated methods sharing a path' do
+      # DelegatedMethod#initialize requires exactly one of :method /
+      # :receiver, so a merged pin can't hold both delegation targets.
+      closure = Solargraph::Pin::Namespace.new(name: 'Foo', closure: Solargraph::Pin::ROOT_PIN, type: :class)
+      delegated = lambda do |receiver_name|
+        chain = Solargraph::Source::Chain.new([Solargraph::Source::Chain::Call.new(receiver_name, nil)])
+        Solargraph::Pin::DelegatedMethod.new(closure: closure, scope: :instance, name: 'bar', receiver: chain)
+      end
+      store = described_class.new([closure, delegated.call('one'), delegated.call('two')])
+      pins = []
+      expect { pins = store.get_methods('Foo', scope: :instance) }.not_to raise_error
+      bar_pins = pins.select { |p| p.name == 'bar' }
+      expect(bar_pins.length).to eq(2)
+      expect(bar_pins).to all(be_an_instance_of(Solargraph::Pin::DelegatedMethod))
+    end
+
+    it 'combines many same-path pins into a single pin' do
+      maps = (1..30).map do |i|
+        Solargraph::SourceMap.load_string(%(
+          class Foo
+            # @param other [Type#{i}]
+            # @return [Type#{i}]
+            def bar(other); end
+          end
+        ), "source#{i}.rb")
+      end
+      store = described_class.new(maps.flat_map(&:pins))
+
+      bar_pins = store.get_methods('Foo', scope: :instance).select { |p| p.name == 'bar' }
+      expect(bar_pins.length).to eq(1)
+      # The regression is combinatorial blowup, not a specific merge
+      # outcome, so bound the size rather than assert exact merges.
+      expect(bar_pins.first.signatures.length).to be <= maps.length
+    end
+  end
+
   # @todo This will become #get_superclass
   describe '#get_superclass' do
     it 'returns simple superclasses' do

spec/pin/method_spec.rb

@@ -128,6 +128,60 @@ def bazzle; end
     expect(result.length).to eq(signatures.length)
   end
 
+  it 'merges a block signature that declares parameters with one that does not' do
+    plain_impl = Solargraph::SourceMap.load_string(%(
+      module Widgetbox
+        class << self
+          # @return [String]
+          def build(&block) = 'x'
+        end
+      end
+    ), 'widgetbox.rb')
+    parse_stub = Solargraph::SourceMap.load_string(%(
+      # @!parse
+      #   module Widgetbox
+      #     class << self
+      #       # @yieldparam config [String]
+      #       # @return [String]
+      #       def build(&block); end
+      #     end
+      #   end
+    ), 'annotations.rb')
+    api_map = Solargraph::ApiMap.new
+    api_map.catalog Solargraph::Bench.new(source_maps: [plain_impl, parse_stub])
+    method = api_map.get_method_stack('Widgetbox', 'build', scope: :class).first
+    expect(method.signatures.length).to eq(1)
+    expect(method.signatures.first.block.parameters.map(&:name)).to eq(['config'])
+  end
+
+  it 'merges a block signature with more declared parameters than one with fewer' do
+    fewer_params = Solargraph::SourceMap.load_string(%(
+      module Widgetbox
+        class << self
+          # @yieldparam a [String]
+          # @return [String]
+          def build(&block) = 'x'
+        end
+      end
+    ), 'widgetbox.rb')
+    more_params = Solargraph::SourceMap.load_string(%(
+      # @!parse
+      #   module Widgetbox
+      #     class << self
+      #       # @yieldparam a [String]
+      #       # @yieldparam b [Integer]
+      #       # @return [String]
+      #       def build(&block); end
+      #     end
+      #   end
+    ), 'annotations.rb')
+    api_map = Solargraph::ApiMap.new
+    api_map.catalog Solargraph::Bench.new(source_maps: [fewer_params, more_params])
+    method = api_map.get_method_stack('Widgetbox', 'build', scope: :class).first
+    expect(method.signatures.length).to eq(1)
+    expect(method.signatures.first.block.parameters.map(&:name)).to eq(%w[a b])
+  end
+
   it 'does not merge with changes in parameters' do
     # @todo Method pin parameters are pins now
     pin1 = described_class.new(name: 'bar', parameters: %w[one two])
@@ -786,4 +840,12 @@ def foo(**bar); end
       expect { pin.signatures }.not_to raise_error
     end
   end
+
+  it 'typifies a closureless DuckMethod pin as String via Object#to_s' do
+    api_map = Solargraph::ApiMap.new
+    pin = Solargraph::Pin::DuckMethod.new(name: 'to_s', source: :api_map)
+    expect(pin.closure).to be_nil
+    expect(pin.return_type).to be_undefined
+    expect(pin.typify(api_map).rooted_tags).to eq('::String')
+  end
 end