Fix 1286 generic cross file parse

lib/solargraph/api_map.rb

@@ -425,7 +425,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>]
@@ -759,9 +759,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
@@ -787,6 +788,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
 
@@ -802,7 +814,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)
@@ -843,6 +855,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
@@ -856,6 +869,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,
@@ -304,6 +305,29 @@ 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|
+          # A pin indexed in more than one pinset recurs as the same object; combining it with itself reorders its signatures
+          distinct = group.uniq(&:object_id)
+          if distinct.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 = distinct[1..].reduce(distinct.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,23 @@ 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)
+        # RBS closes a block over its method, YARD over its signature; those cannot be combined
+        return block.combine_with(other.block) if block.arity == other.block.arity &&
+                                                  block.closure&.name == other.block.closure&.name
+
+        # @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>]

lib/solargraph/repo.rb

@@ -98,7 +98,7 @@ def build_from_directory
         cmd = ['ruby', '-e', bundle_script]
         o, e, s = Open3.capture3(*cmd, chdir: directory)
         if s.success?
-          json = o && !o.empty? ? JSON.parse(o.strip.split("\n").last, symbolize_names: true) : []
+          json = JSON.parse(o.strip.split("\n").last || '{"metagems":[],"groups":{}}', symbolize_names: true)
           @metagems = json[:metagems].map { |data| Metagem.new(**data) }
           @bundled_group_map = json[:groups].transform_values { |names| names.map { |name| bundled_metagem_name_map[name] } }
         else

lib/solargraph/workspace/require_paths.rb

@@ -79,7 +79,8 @@ def require_path_from_gemspec_file gemspec_file_path
         o, e, s = Open3.capture3(*cmd)
         if s.success?
           begin
-            hash = o && !o.empty? ? JSON.parse(o.split("\n").last) : {}
+            line = o.split("\n").last
+            hash = line ? JSON.parse(line) : {}
             return [] if hash.empty?
             hash['paths'].map { |path| File.join(base, path) }
           rescue StandardError => e

spec/api_map/store_spec.rb

@@ -49,6 +49,105 @@
     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 'returns a pin indexed in two pinsets as is, not combined with itself' do
+      source = Solargraph::SourceMap.load_string(%(
+        class Foo
+          # @overload bar(index)
+          #   @param index [Integer]
+          #   @return [String]
+          # @overload bar(start, length)
+          #   @param start [Integer]
+          #   @param length [Integer]
+          #   @return [Array<String>]
+          # @overload bar(range)
+          #   @param range [Range<Integer>]
+          #   @return [Array<String>]
+          def bar(*args); end
+        end
+      ), 'foo.rb')
+      store = described_class.new(source.pins, source.pins)
+
+      bar_pins = store.get_methods('Foo', scope: :instance).select { |p| p.name == 'bar' }
+      expect(bar_pins).to all(equal(source.pins.find { |p| p.name == 'bar' }))
+    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