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

Strip 'file ' prefix from all filenames in RdocToYard #585

Merged
merged 1 commit into from
Dec 8, 2022

Conversation

grncdr
Copy link
Contributor

@grncdr grncdr commented Sep 1, 2022

This fixes #473 (at least for me, see extended notes below).

The core issue is that the obj.filename property would sometimes be prefixed with the string "file ", which then causes YardMap::Mapper#map to remove the pins:

@pins.keep_if { |pin| pin.location.nil? || File.file?(pin.location.filename) } if @spec

This change adds the prefix stripping regardless of whether we get a filename from in_files or filename.

lots of notes about debugging this, feel free to skip

Reviving this thread, as I'm trying to fix iftheshoefritz/solargraph-rails#34. Here is what I observe:

Preamble

=> bundle exec solargraph --version
0.46.0

=> bundle exec yard --version
yard 0.9.28

=> bundle exec rails --version
Rails 7.0.2.4

Part 1 - generating YARD objects

Starting from scratch:

bundle exec yard gems
bundle exec solargraph bundle
require 'yard'
yardoc_file = "#{ENV['HOME']}/.solargraph/cache/gems/activerecord-7.0.2.4/yardoc"
YARD::Registry.load_yardoc(yardoc_file)
YARD::Registry.all.select { |o| o.namespace.name == :QueryMethods }
#=> [#<yardoc class ActiveRecord::QueryMethods::WhereChain>,
 #<yardoc method ActiveRecord::QueryMethods#and>,
 #<yardoc method ActiveRecord::QueryMethods#annotate>,
 #<yardoc method ActiveRecord::QueryMethods#create_with>,
 #<yardoc method ActiveRecord::QueryMethods#distinct>,
 #<yardoc method ActiveRecord::QueryMethods#eager_load>,
 #<yardoc method ActiveRecord::QueryMethods#excluding>,
 #<yardoc method ActiveRecord::QueryMethods#extending>,
 #<yardoc method ActiveRecord::QueryMethods#extract_associated>,
 #<yardoc method ActiveRecord::QueryMethods#from>,
 #<yardoc method ActiveRecord::QueryMethods#group>,
 #<yardoc method ActiveRecord::QueryMethods#having>,
 #<yardoc method ActiveRecord::QueryMethods#in_order_of>,
 #<yardoc method ActiveRecord::QueryMethods#includes>,
 #<yardoc method ActiveRecord::QueryMethods#invert_where>,
 #<yardoc method ActiveRecord::QueryMethods#joins>,
 #<yardoc method ActiveRecord::QueryMethods#left_joins>,
 #<yardoc method ActiveRecord::QueryMethods#left_outer_joins>,
 #<yardoc method ActiveRecord::QueryMethods#limit>,
 #<yardoc method ActiveRecord::QueryMethods#lock>,
 #<yardoc method ActiveRecord::QueryMethods#none>,
 #<yardoc method ActiveRecord::QueryMethods#offset>,
 #<yardoc method ActiveRecord::QueryMethods#optimizer_hints>,
 #<yardoc method ActiveRecord::QueryMethods#or>,
 #<yardoc method ActiveRecord::QueryMethods#order>,
 #<yardoc method ActiveRecord::QueryMethods#preload>,
 #<yardoc method ActiveRecord::QueryMethods#readonly>,
 #<yardoc method ActiveRecord::QueryMethods#references>,
 #<yardoc method ActiveRecord::QueryMethods#reorder>,
 #<yardoc method ActiveRecord::QueryMethods#reselect>,
 #<yardoc method ActiveRecord::QueryMethods#reverse_order>,
 #<yardoc method ActiveRecord::QueryMethods#rewhere>,
 #<yardoc method ActiveRecord::QueryMethods#select>,
 #<yardoc method ActiveRecord::QueryMethods#strict_loading>,
 #<yardoc method ActiveRecord::QueryMethods#structurally_compatible?>,
 #<yardoc method ActiveRecord::QueryMethods#uniq!>,
 #<yardoc method ActiveRecord::QueryMethods#unscope>,
 #<yardoc method ActiveRecord::QueryMethods#where>,
 #<yardoc method ActiveRecord::QueryMethods#without>]

Good start, the cached documentation in ~/.solargraph/cache is contains the expected method definitions.

However, solargraph scan -v does not show e.g. QueryMethods#where:

bundle exec solargraph scan -v | grep 'QueryMethods#where'
# no output

So it seems as though somewhere the yardoc from ~/.solargraph/cache is either not loaded, or is unloaded.

Part 2 - when do we load the cache?

Usages of cache_dir

  1. ser = File.join(CoreDocs.cache_dir, 'gems', "#{spec.name}-#{spec.version}.ser")
  2. yardoc_file_for_spec

The correct (cached) yardoc is found, because it gets loaded when I break in YardMap#process_yardoc with the condition y =~ /activerecord/

The yardoc contains the method objects at the time the yardoc is loaded (checked by breaking in YardMap::Mapper#map when co.name == :where)

However the @pins.keep_if line is removing 1596 pins, because many of the pins have a location.filename that looks like "<correct_gem_path>/activerecord-7.0.2.4/file lib/active_record/" (emphasis on the "file " in there).

Eventually we call process_yardoc

So, I hacked up YardMap::Helpers#object_location to strip the "file " prefix from code_object.file like so:

       def object_location code_object, spec
         return nil if spec.nil? || code_object.nil? || code_object.file.nil? || code_object.line.nil?
-        file = File.join(spec.full_gem_path, code_object.file)
+        file = File.join(spec.full_gem_path, code_object.file.sub(/^file /, ''))
         Solargraph::Location.new(file, Solargraph::Range.from_to(code_object.line - 1, 0, code_object.line - 1, 0))
       end

After this solargraph scan -v shows the missing methods, and auto-complete works!

Part 3 - a real fix?

So it's pretty weird that yard code objects contain an invalid path like that. Especially considering that these code objects should have been produced by Solargraphs own RdocToYard conversion. Taking a peek at RdocToYard.find_file reveals the issue:

      def self.find_file obj
        if obj.respond_to?(:in_files) && !obj.in_files.empty?
          [obj.in_files.first.to_s.sub(/^file /, ''), obj.line]
        else
          [obj.file, obj.line]
        end
      end

Turns out there was already code to strip the "file " prefix, but it only exists in one of the two branches. Hence this PR.

@grncdr grncdr force-pushed the fix-rdoc-to-yard-filename branch 2 times, most recently from 6c480b5 to 2e9c186 Compare September 1, 2022 16:12
@grncdr grncdr force-pushed the fix-rdoc-to-yard-filename branch from 2e9c186 to e8e1e1a Compare September 1, 2022 16:22
@iftheshoefritz
Copy link

iftheshoefritz commented Sep 30, 2022

@grncdr this does seem to fix #473, and I'm excited to see it go in!

However I'm not seeing what I'd expect if iftheshoefritz/solargraph-rails#34 were fixed. It looks like in an ActiveRecord model, when I autocomplete my_associated_models.first, there is still no type on .first.

In the LSP server response:

    {
      "label": "first",
      "kind": 2,
      "detail": "(limit = nil)",
      "data": {
        "path": "ActiveRecord::Associations::CollectionProxy#first",
        "return_type": "undefined",
        "location": {
          "filename": "/Users/fritz/.asdf/installs/ruby/3.0.0/lib/ruby/gems/3.0.0/gems/activerecord-7.0.1/lib/active_record/associations/collection_proxy.rb",
          "range": {
            "start": {
              "line": 141,
              "character": 0
            },
            "end": {
              "line": 141,
              "character": 0
            }
          }
        },
        ...

What I expect is "return_type": "MyAssociatedModel" in the response from solargraph.

I'm not sure though if this change could solve that? What is the behaviour that you observe?

Again, I still hope this goes in on the strength of it fixing #473 .

@grncdr
Copy link
Contributor Author

grncdr commented Sep 30, 2022

I'm not sure though if this change could solve that? What is the behaviour that you observe?

I don't expect it to. As far as I know, the original Rdoc doesn't specify a return type and so it needs augmentation on the part of solargraph-rails.

I've fixed that (and loads of other stuff) in my "hacks" branch of solargraph-rails, but that branch depends on some of my other changes to solargraph (which are in my "hacks" branch for solargraph).

If you switch to using those branches in your Gemfile, you'll get a very improved solargraph-rails experience, but it all needs more specs and especially some feedback on whether my Pin::DelegatedMethod PR (#602, #591) is on the right track.

@castwide
Copy link
Owner

castwide commented Dec 8, 2022

Thanks!

@castwide castwide merged commit b8484cb into castwide:master Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants