castwide / solargraph

A Ruby language server.
https://solargraph.org
MIT License
1.89k stars 158 forks source link

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

Closed grncdr closed 1 year ago

grncdr commented 2 years ago

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: https://github.com/castwide/solargraph/blob/18854d7577737cf9929063d96dd99b3d0a77f703/lib/solargraph/yard_map/mapper.rb#L26

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 https://github.com/iftheshoefritz/solargraph-rails/issues/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 ``` ```rb 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 } #=> [#, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #, #] ``` 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`: ```sh 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. https://github.com/castwide/solargraph/blob/87f627551941d50f58aba42d466e426ca606de75/lib/solargraph/yard_map.rb#L309 2. [yardoc_file_for_spec](https://github.com/castwide/solargraph/blob/87f627551941d50f58aba42d466e426ca606de75/lib/solargraph/yard_map.rb#L348-L356) 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 "/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: ```diff 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: ```rb 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.
iftheshoefritz commented 2 years ago

@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 https://github.com/iftheshoefritz/solargraph-rails/issues/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 commented 2 years ago

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 commented 1 year ago

Thanks!