castwide / solargraph

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

Request textDocument/documentHighlight failed #678

Open yoshinori-ma opened 10 months ago

yoshinori-ma commented 10 months ago

I am using the vscode-solargraph extension. When I hover over code in the editor, I receive the following error:

[Error - 15:18:20] Request textDocument/documentHighlight failed.
  Message: [ArgumentError] dependency name must be a String, was nil
  Code: -32603

I also encountered an error when running solargraph scan Upon investigating the error message, I traced it back to this line in the Solargraph codebase: yard_map.rb#L285. It appears that the name becomes nil and causes the program to crash. I've noticed that the issue occurs when requiring a path with a leading /, like so: require '/app/spec/hoge'.

environments

$ code -v 
1.81.1
6c3e3dba23e8fadc360aed75ce363ba185c49794
arm64
$ solargraph -v
0.49.0
r-glebov commented 8 months ago

Yeah exactly my case, fixed it locally with this updated line in a yard_map.rb:

name = path.split('/').first.empty? ? path.split('/')[1] : path.split('/').first

r-glebov commented 8 months ago

Hopefully we'll have a fix in upcoming versions, so we don't need to have a patched version of the gem.

castwide commented 8 months ago

Can anyone provide a reproducible example? I'm totally okay with making this change, but I'd like to have a better understanding of the root cause.

r-glebov commented 8 months ago

I can share an example code, which didn't work with current Solargraph version until I fixed that myself.

require 'pathname'

module SomeApp
  ROOT = Pathname.pwd.freeze

  class << self
    def path(*args)
      root.join(*args)
    end

    def root
      ROOT
    end
end

And then I require any file in any part of the project this way:

require SomeApp.path('path', 'to', 'file')

That doesn't work with Solargraph because generated path looks like /app/path/to/file, and path.split('/').first in this case returns empty string as a first result of a split.

castwide commented 7 months ago

Thanks, @r-glebov.

I took a slightly different approach here. YardMap should only get documentation for external gem dependencies, so absolute paths should get ignored.

The fix is on master and will be included in the next release.

(Slightly related: #675)

castwide commented 7 months ago

Released in v0.50.0.