diff --git a/lib/solargraph/api_map.rb b/lib/solargraph/api_map.rb index 76763389a3..5625b9b380 100755 --- a/lib/solargraph/api_map.rb +++ b/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] @@ -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 diff --git a/lib/solargraph/api_map/store.rb b/lib/solargraph/api_map/store.rb index 4d77ebaab0..0e8bfba75f 100644 --- a/lib/solargraph/api_map/store.rb +++ b/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] - # @return [Enumerable] + # @return [Array] 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] + # @return [Array] + 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>] # # @return [true] diff --git a/lib/solargraph/pin/callable.rb b/lib/solargraph/pin/callable.rb index ed87b79e41..b92c01d97a 100644 --- a/lib/solargraph/pin/callable.rb +++ b/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] diff --git a/lib/solargraph/pin/method.rb b/lib/solargraph/pin/method.rb index 81cc94d284..f1b41ab389 100644 --- a/lib/solargraph/pin/method.rb +++ b/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] def combine_signatures_by_type_arity(*signature_pins) + merged_pins = merge_signatures_differing_only_by_block_informativeness(signature_pins) + # @type [Hash{Array => Array}] 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] + # @return [Array] + def merge_signatures_differing_only_by_block_informativeness signature_pins + # @param sig [Pin::Signature] + # @param result [Array] + 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] # # @return [Array] diff --git a/lib/solargraph/repo.rb b/lib/solargraph/repo.rb index cbff9baabf..e57496ff0e 100644 --- a/lib/solargraph/repo.rb +++ b/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 diff --git a/lib/solargraph/workspace/require_paths.rb b/lib/solargraph/workspace/require_paths.rb index d12364b07b..2bf1f3cdd9 100644 --- a/lib/solargraph/workspace/require_paths.rb +++ b/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 diff --git a/spec/api_map/store_spec.rb b/spec/api_map/store_spec.rb index 764b5c838f..817c2a530b 100644 --- a/spec/api_map/store_spec.rb +++ b/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] + # @overload bar(range) + # @param range [Range] + # @return [Array] + 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 diff --git a/spec/pin/method_spec.rb b/spec/pin/method_spec.rb index 11448972de..3c45e7b2e6 100644 --- a/spec/pin/method_spec.rb +++ b/spec/pin/method_spec.rb @@ -128,6 +128,82 @@ 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 'combines RBS and YARD pins whose blocks have differently named closures' do + old_asserts = ENV.fetch('SOLARGRAPH_ASSERTS', nil) + ENV['SOLARGRAPH_ASSERTS'] = 'on' + location = Solargraph::Location.new('dsl.rb', Solargraph::Range.from_to(0, 0, 0, 0)) + void = Solargraph::ComplexType.parse('void') + namespace = Solargraph::Pin::Namespace.new(name: 'Dsl', type: :module, source: :rbs, location: location) + # RBS conversions close a block over its method; YARD closes it over its signature + rbs_pin = described_class.new(name: 'file', closure: namespace, source: :rbs, location: location) + rbs_block = Solargraph::Pin::Signature.new(closure: rbs_pin, source: :rbs, location: location, return_type: void) + rbs_pin.signatures = [Solargraph::Pin::Signature.new(block: rbs_block, closure: rbs_pin, source: :rbs, + location: location, return_type: void)] + yard_pin = described_class.new(name: 'file', closure: namespace, source: :yardoc, location: location, + parameters: []) + yard_pin.parameters = [Solargraph::Pin::Parameter.new(name: 'block', decl: :blockarg, closure: yard_pin, + source: :yardoc, location: location)] + + combined = rbs_pin.combine_with(yard_pin) + expect(combined.signatures.map { |sig| sig.block&.parameters }).to eq([[]]) + ensure + ENV['SOLARGRAPH_ASSERTS'] = old_asserts + 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 +862,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 diff --git a/spec/repo_spec.rb b/spec/repo_spec.rb index ddb19daf38..e9705b0ec9 100644 --- a/spec/repo_spec.rb +++ b/spec/repo_spec.rb @@ -70,5 +70,10 @@ meta = repo.find_by_name('nokogiri') expect(meta.name).to eq('nokogiri') end + + it 'does not raise when the bundle script prints only blank lines' do + allow(Open3).to receive(:capture3).and_return(["\n", '', instance_double(Process::Status, success?: true)]) + expect { described_class.new(directory) }.not_to raise_error + end end end diff --git a/spec/source_map/clip_spec.rb b/spec/source_map/clip_spec.rb index 1a4af74e8b..6b69f82746 100644 --- a/spec/source_map/clip_spec.rb +++ b/spec/source_map/clip_spec.rb @@ -1917,6 +1917,95 @@ def bad_passthrough; yield; end expect(type.to_s).to eq('undefined') end + it 'binds generics through a cross-file @!parse stub that adds @generic to an existing class' do + # gem_source has no `@generic` tag or `@!parse` stub, and is mapped + # before the workspace's stub. + gem_source = Solargraph::SourceMap.load_string(%( + module Widgetbox + class Collection + def self.make + new + end + + def last + nil + end + end + + class Widget + # @return [String, nil] + def resource_subtype; end + end + end + ), 'widgetbox.rb') + # A `@!parse` stub in a separate file adds `@generic T` to the existing + # class and overrides the return types of its methods. + parse_stub = Solargraph::SourceMap.load_string(%( + # @!parse + # module Widgetbox + # # @generic T + # class Collection + # class << self + # # @return [Widgetbox::Collection] + # def make; end + # end + # # @return [generic] + # def last; end + # end + # end + ), 'annotations.rb') + caller_source = Solargraph::Source.load_string(%( + Widgetbox::Collection.make.last.resource_subtype + ), 'test.rb') + api_map = Solargraph::ApiMap.new + api_map.catalog Solargraph::Bench.new(source_maps: [gem_source, parse_stub, Solargraph::SourceMap.map(caller_source)]) + clip = api_map.clip_at('test.rb', [1, 40]) + type = clip.infer + expect(type.tag).to eq('String') + end + + it 'yields the block parameter type declared by a cross-file @!parse stub' do + # The plain implementation only documents that it takes a block (no + # @yieldparam), simulating a gem's pins loading before a workspace + # @!parse stub that adds the block's parameter type. + plain_impl = Solargraph::SourceMap.load_string(%( + module Widgetbox + class Collection + # @return [String] + def name + 'collection' + end + end + + class << self + # @return [Widgetbox::Collection] + def build(&block) + Collection.new + end + end + end + ), 'widgetbox.rb') + parse_stub = Solargraph::SourceMap.load_string(%( + # @!parse + # module Widgetbox + # class << self + # # @yieldparam config [Widgetbox::Collection] + # # @return [Widgetbox::Collection] + # def build(&block); end + # end + # end + ), 'annotations.rb') + caller_source = Solargraph::Source.load_string(%( + Widgetbox.build do |config| + config + end + ), 'test.rb') + api_map = Solargraph::ApiMap.new + api_map.catalog Solargraph::Bench.new(source_maps: [plain_impl, parse_stub, Solargraph::SourceMap.map(caller_source)]) + clip = api_map.clip_at('test.rb', [2, 14]) + expect(clip.infer.tag).to eq('Widgetbox::Collection') + end + it 'uses simple return value of block to infer return value of Enumerable#map' do source = Solargraph::Source.load_string(%( a = ['a'].map { 123 } diff --git a/spec/workspace/require_paths_spec.rb b/spec/workspace/require_paths_spec.rb index eb95d0c5ba..be00ac20fb 100644 --- a/spec/workspace/require_paths_spec.rb +++ b/spec/workspace/require_paths_spec.rb @@ -77,6 +77,21 @@ end end + context 'with a gemspec that prints only blank lines' do + let(:dir_path) { File.realpath(Dir.mktmpdir) } + + before do + File.write(File.join(dir_path, 'blank.gemspec'), 'Gem::Specification.new') + allow(Open3).to receive(:capture3).and_return(["\n", '', instance_double(Process::Status, success?: true)]) + allow(Solargraph.logger).to receive(:warn) + end + + it 'does not log an error' do + paths + expect(Solargraph.logger).not_to have_received(:warn) + end + end + context 'with no gemspec file' do let(:dir_path) { File.realpath(Dir.mktmpdir) }