solargraph icon indicating copy to clipboard operation
solargraph copied to clipboard

RDocToYard overwrites previously defined YARD objects, even if they aren't defined in RDoc

Open iftheshoefritz opened this issue 2 years ago • 10 comments

EDIT: this issue is quite far from where it started, look here for the latest.

====

[api_map.get_complex_type_methods returns empty for ActiveRecord::Associations::CollectionProxy<MyModel>:

> type = Solargraph::ComplexType.parse('ActiveRecord::Associations::CollectionProxy<Book>')
#<Solargraph::ComplexType:0x0000000107607ba8 @items=[#<Solargraph::ComplexType::UniqueType:0x0000000107607ec8 @name="ActiveRecord::Associations::CollectionProxy", @rooted=false, @substring="<Book>", @tag="ActiveRecord::Associations::CollectionProxy<Book>", @key_types=[], @subtypes=[#<Solargraph::ComplexType::UniqueType:0x0000000107607c48 @name="Book", @rooted=false, @substring="", @tag="Book", @key_types=[], @subtypes=[], @duck_type=false>], @duck_type=false>]>
> type_methods = api_map.get_complex_type_methods(type).reject { |p| p.closure.name =~ /Object|Expectations/ }.map(&:name)
[]

I expect at least the methods described here: https://api.rubyonrails.org/classes/ActiveRecord/Associations/CollectionProxy.html like .any, .first etc.

compared to the same thing with an Array of MyType:

> type = Solargraph::ComplexType.parse('Array<Book>')
#<Solargraph::ComplexType:0x0000000118a11be8 @items=[#<Solargraph::ComplexType::UniqueType:0x0000000118a11f08 @name="Array", @rooted=false, @substring="<Book>", @tag="Array<Book>", @key_types=[], @subtypes=[#<Solargraph::ComplexType::UniqueType:0x0000000118a11c88 @name="Book", @rooted=false, @substring="", @tag="Book", @key_types=[], @subtypes=[]>], @duck_type=false, @nil_type=false, @scope=:instance, @namespace="Array">], @namespace="Array">
> type_methods = api_map.get_complex_type_methods(type).reject { |p| p.closure.name =~ /Object|Expectations/ }.map(&:name)
["&", "*", "+", "-", "<<", "<=>", "==", "[]", "[]=", "any?", "assoc", "at", "bsearch", "clear", "collect", "collect!", "combination", ...]

Steps to reproduce:

  1. start Rails console in a Rails app with Book model
  2. Create api_map: `api_map = Solargraph::ApiMap.load(Rails.root)
  3. type = ... (follow from above)

iftheshoefritz avatar Aug 14 '21 09:08 iftheshoefritz

Some more feedback:

I can see that ApiMap.load invokes yard_map.change, then stuffs yard_map.pins inside ApiMap::Store. When I try ask it about Enumerable, it has lots of pins for me:

> yard_map.pins.select { |p| p.namespace == 'Enumerable'}.count
51
> yard_map.pins.select { |p| p.namespace == 'ActiveRecord::Relation'}.count
2

So the YardMap (and by extension ApiMap) does know that the Rails classes exist, but it's not getting that ActiveRecord::Relation includes Enumerable and so should have all of its instance methods.

However it does know about all of the methods on Array:

> yard_map.pins.select { |p| p.namespace == 'Array'}.count
104
(yard_map.pins.select { |p| p.namespace == 'Array'}.map(&:name) - yard_map.pins.select {|p| p.namespace == 'Enumerable'}.map(&:name)).count
85

Something weird going on that Array is an Enumerable and includes its methods, but AR::Relation is an Enumerable but does not.

iftheshoefritz avatar Aug 20 '21 16:08 iftheshoefritz

I see that Array might not be a good comparison option, maybe it doesn't include Enumerable? Instead I looked at Set, which does include Enumerable and the plot thickens:

irb(main):008:0> yard_map.pins.select { |p| p.namespace == 'Set'}.count
=> 0

Perhaps I am missing something because this is even fewer pins than for ActiveRecord::Relation (where at least there are some constants).

Also it seems like several tests in yard_map_spec are broken:

Failed examples:

rspec ./spec/yard_map_spec.rb:17 # Solargraph::YardMap finds stdlib require paths
rspec ./spec/yard_map_spec.rb:42 # Solargraph::YardMap collects pins from gems
rspec ./spec/yard_map_spec.rb:57 # Solargraph::YardMap ignores duplicate requires
rspec ./spec/yard_map_spec.rb:64 # Solargraph::YardMap ignores multiple paths to the same gem
rspec ./spec/yard_map_spec.rb:98 # Solargraph::YardMap includes gem dependencies based on attribute
rspec ./spec/yard_map_spec.rb:108 # Solargraph::YardMap excludes gem dependencies based on attribute
rspec ./spec/yard_map_spec.rb:142 # Solargraph::YardMap adds overrides
rspec ./spec/yard_map_spec.rb:151 # Solargraph::YardMap maps YAML to Psych

... including some that reference Set:

  it "finds stdlib require paths" do
    yard_map = Solargraph::YardMap.new(required: ['set'])
    pin = yard_map.path_pin('Set#add')
    expect(pin).to be
  end

iftheshoefritz avatar Aug 21 '21 11:08 iftheshoefritz

I was able to get results for ActiveRecord::Associations::CollectionProxy<Book> like this in IRB from a Rails app:

2.7.1 :001 > require 'solargraph'
 => true 
2.7.1 :002 > api_map = Solargraph::ApiMap.load('.')
[WARN] Unable to load chive 0.2.0 specified by workspace, using 0.1.1 instead
[WARN] Empty cache for rails 6.0.3.6. Reloading
2.7.1 :003 > type = Solargraph::ComplexType.parse('ActiveRecord::Associations::CollectionProxy<Book>')
2.7.1 :009 > type_methods = api_map.get_complex_type_methods(type).reject { |p| p.closure.name =~ /Object|Expectations/ }
2.7.1 :010 > type_methods.length
 => 276

That's with Rails 6.0.3.6, Solargraph 0.43.0, and Ruby 2.7.1.

Based on your failing specs, you might need to run yard gems to install some missing documentation.


The issue you found with Set appears to be a legitimate bug. As of Ruby 2.5, you no longer need to require 'set' explicitly, but Solargraph still expects that behavior. A temporary workaround is to add require 'set' (or # @!parse require 'set') somewhere in your workspace.

It's also possible that you have a separate set gem installed that's missing the relevant documentation. There's an ongoing effort to move the stdlib into separate gems, and some of them are stubs that confuse the yard map.

castwide avatar Aug 21 '21 16:08 castwide

Running yard gems didn't make a difference to my results with api_map.get_complex_type_methods.

It fixed some of the tests I mentioned, but I still have these failures:

Failed examples:

rspec ./spec/yard_map_spec.rb:17 # Solargraph::YardMap finds stdlib require paths
rspec ./spec/yard_map_spec.rb:142 # Solargraph::YardMap adds overrides
rspec ./spec/yard_map_spec.rb:151 # Solargraph::YardMap maps YAML to Psych

Perhaps that's related to the separate Set gem possibility that you mentioned.

I am running:

⚡ solargraph -v 0.43.0 ⚡ ruby -v ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [arm64-darwin20] ⚡ rails -v Rails 6.1.3.2

I'll try a Rails 6.0 app later in case that's an important difference.

iftheshoefritz avatar Aug 23 '21 11:08 iftheshoefritz

I installed a fresh Rails 6.0 install with solargraph 0.43.0 as a development dependency. Same result:

irb(main):001:0> require 'solargraph'
=> false
irb(main):002:0> api_map = Solargraph::ApiMap.load('.')
=> #<Solargraph::ApiMap:0x00000001298b35b0 @source_map_hash={"./app/channels/application_cable/channel.rb"=>#<Solargraph::SourceMap:0x0000000129f5cda8 @source=#<Solargraph::...
irb(main):003:0> type = Solargraph::ComplexType.parse('ActiveRecord::Associations::CollectionProxy<Book>')
=> #<Solargraph::ComplexType:0x000000013bb452a8 @items=[#<Solargraph::ComplexType::UniqueType:0x000000013bb457f8 @name="ActiveRecord::Associations::CollectionProxy", @rooted...
irb(main):004:0> type_methods = api_map.get_complex_type_methods(type).reject { |p| p.closure.name =~ /Object|Expectations/ }
=> []

just being a littler clearer on my installation steps:

  1. rails new
  2. add solargraph to the gemfile
  3. bundle install
  4. yard gems
  5. solargraph bundle
  6. curl -s https://gist.githubusercontent.com/cmer/024991c85c4de01dab17632b2dc7f064/raw > .solargraph.yml
  7. curl -s https://gist.githubusercontent.com/castwide/28b349566a223dfb439a337aea29713e/raw > config/definitions.rb
  8. rails c ...

Do you have anything else that I'm missing?

Oh and the Book model isn't really important here, I'm just using Array<Object> and ActiveRecord::Associations::CollectionProxy<Object> now

iftheshoefritz avatar Aug 25 '21 08:08 iftheshoefritz

Over on the rails thread, this comment: https://github.com/castwide/solargraph/issues/87#issuecomment-854068741 helped me figure this out. Solargraph bundle is doing something to interfere:

> solargraph clear
> bundle exec yard gems --rebuild
>  solargraph bundle
Caching custom documentation for activesupport 6.0.4.1
Caching custom documentation for actionview 6.0.4.1
Caching custom documentation for actionpack 6.0.4.1
Caching custom documentation for actioncable 6.0.4.1
Caching custom documentation for activejob 6.0.4.1
Caching custom documentation for activemodel 6.0.4.1
Caching custom documentation for activerecord 6.0.4.1
Caching custom documentation for activestorage 6.0.4.1
Caching custom documentation for actionmailbox 6.0.4.1
Caching custom documentation for actionmailer 6.0.4.1
Caching custom documentation for actiontext 6.0.4.1
Caching custom documentation for railties 6.0.4.1
> solargraph scan -v | grep CollectionProxy
ActiveRecord::Associations::CollectionProxy (/Users/frederickmeissner/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/activerecord-6.0.4.1/lib/active_record/associations/collection_proxy.rb 28)
ActiveRecord::Associations::CollectionProxy | Object

Compared to the first two without solargraph bundle:

> solargraph clear
> bundle exec yard gems --rebuild
> ⚡  solargraph scan -v | grep CollectionProxy
ActiveRecord::Associations::CollectionProxy (/Users/frederickmeissner/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/activerecord-6.0.4.1/lib/active_record/associations/collection_proxy.rb 28)
ActiveRecord::Associations::CollectionProxy | ActiveRecord::Relation
ActiveRecord::Associations::CollectionProxy.new (/Users/frederickmeissner/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/activerecord-6.0.4.1/lib/active_record/associations/collection_proxy.rb 29)
ActiveRecord::Associations::CollectionProxy#initialize (/Users/frederickmeissner/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/activerecord-6.0.4.1/lib/active_record/associations/collection_proxy.rb 29)
ActiveRecord::Associations::CollectionProxy#target (/Users/frederickmeissner/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/activerecord-6.0.4.1/lib/active_record/associations/collection_proxy.rb 37)
ActiveRecord::Associations::CollectionProxy#load_target (/Users/frederickmeissner/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/activerecord-6.0.4.1/lib/active_record/associations/collection_proxy.rb 41)
ActiveRecord::Associations::CollectionProxy#loaded? (/Users/frederickmeissner/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/activerecord-6.0.4.1/lib/active_record/associations/collection_proxy.rb 50)
ActiveRecord::Associations::CollectionProxy#loaded (/Users/frederickmeissner/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/activerecord-6.0.4.1/lib/active_record/associations/collection_proxy.rb 53)
ActiveRecord::Associations::CollectionProxy#find (/Users/frederickmeissner/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/activerecord-6.0.4.1/lib/active_record/associations/collection_proxy.rb 135)
... lots more

and after doing the latter, from the Rails console:

> type_methods = api_map.get_complex_type_methods(type).reject { |p| p.closure.name =~ /Object|Expectations/ }.map(&:name)
=> ["<<", "==", "append", "build", "calculate", "clear", "concat", "create", "create!", "delete", "delete_all", "destroy", "destroy_all", "empty?", "find", "include?", "last...

So the next question is what solargraph bundle does that interferes with all of this.

iftheshoefritz avatar Sep 10 '21 11:09 iftheshoefritz

Update:

I can see in RdocToYard that when SG generates YARD from RDoc for a gem it does:

  1. load all the RDoc objects found into a cleared YARD::Registry
  2. clear out the old cache_dir for that gem
  3. save the newly constructed registry into the old location

Obviously this means we lose anything that was documented in YARD but not in RDoc.

I'm wondering if it's possible to capture the existing YARD objects just before this, and add any objects that are not defined by the RDoc translation back into what SG saves.

iftheshoefritz avatar Sep 17 '21 16:09 iftheshoefritz

Thanks for staying on top of this. Capturing existing YARD objects seems feasible. I'll try a couple experiments and see how it looks. Any other suggestions or feedback you have is appreciated.

castwide avatar Sep 19 '21 17:09 castwide

Some more untangling:

After a solargraph clear, YardMap#add_gem_dependencies falls back to a yardoc file under the ruby install, e.g. ~/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/doc/activerecord-6.0.4.1/.yardoc. This file contains all the YARD objects I want.

After a solargraph bundle, ApiMap uses a cache directory, e.g. ~/.solargraph/cache/gems/activerecord-6.0.4.1/yardoc. This file does not contain the needed YARD objects.

iftheshoefritz avatar Oct 29 '21 17:10 iftheshoefritz

I tried a naive approach to saving all the YARD that didn't seem to improve things:

# Solargraph::YardMap::RDocToYard#run
prior_objects = YARD::Registry.all # save the YARD

rake_yard(mapstore)
YARD::Registry.clear

# add YARD objects back to the code_object_map that 
# (in theory) seemed like it was being cleared:
prior_objects.each do |code_object|
  code_object_map[code_object.path] = code_object
end

code_object_map.values.each do |co|
  YARD::Registry.register(co)
end

Something must be clearing out the YARD::Registry after this, or saving it to the wrong place?

iftheshoefritz avatar Oct 29 '21 17:10 iftheshoefritz