Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
iftheshoefritz opened this issue Aug 14, 2021 · 10 comments · Fixed by #585

Comments

@iftheshoefritz
Copy link

iftheshoefritz commented Aug 14, 2021

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 iftheshoefritz changed the title api_map.get_complex_type_methods works for Array<> but not ActiveRecord::Associations::CollectionProxy api_map.get_complex_type_methods works for Array<> but returns empty for ActiveRecord::Associations::CollectionProxy Aug 16, 2021
@iftheshoefritz iftheshoefritz changed the title api_map.get_complex_type_methods works for Array<> but returns empty for ActiveRecord::Associations::CollectionProxy api_map.get_complex_type_methods works for Array<> but returns empty for ActiveRecord::Associations::CollectionProxy<> Aug 16, 2021
@iftheshoefritz
Copy link
Author

iftheshoefritz commented Aug 20, 2021

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
Copy link
Author

iftheshoefritz commented Aug 21, 2021

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

@castwide
Copy link
Owner

castwide commented Aug 21, 2021

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.

@iftheshoefritz
Copy link
Author

iftheshoefritz commented Aug 23, 2021

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
Copy link
Author

iftheshoefritz commented Aug 25, 2021

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
Copy link
Author

Over on the rails thread, this comment: #87 (comment) 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
Copy link
Author

iftheshoefritz commented Sep 17, 2021

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 iftheshoefritz changed the title api_map.get_complex_type_methods works for Array<> but returns empty for ActiveRecord::Associations::CollectionProxy<> RDocToYard overwrites previously defined YARD objects, even if they aren't defined in RDoc Sep 17, 2021
@castwide
Copy link
Owner

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.

@iftheshoefritz
Copy link
Author

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
Copy link
Author

iftheshoefritz commented Oct 29, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants