WorkspaceSymbol request are returning invalid response on method alias, unresolved method alias and constants and class variables

[pkondzior]

Description

Ruby LSP Information

Ruby LSP WorkspaceSymbol Responses are not generated according to spec if index contains unresolved alias methods, unresolved constant aliases or class variables. Additionally resolvable methods and constants are not being processed either in resolved method or alias form.

The spec for SymbolInformation stands that kind should be represented by one of SybolKind enum variants while RubyLSP. If we are going to look into how these are encoded by Ruby-LSP we can notice that neither RubyIndexer::Entry::ConstantAlias, RubyIndexer::Entry::MethodAlias nor RubyIndexer::Entry::ClassVariable have their when branch on the case. This leads to nil results. Additionally due to lazy nature of unresolved entries fuzzy_search is not able to work on RubyIndexer::Entry::ConstantAlias nor RubyIndexer::Entry::MethodAlias from index.

Reproduction steps

1. Start the Ruby LSP using a Zed Eeditor
2. Open a Ruby file with following content

OK = 'OK'
class Foo
  BOK = OK
  BAD = AD
  def test
  end
  alias whatever test
  alias_method :bar, :to_a
  alias_method "baz", "to_a"
  @@test = '123'
end

3. Go to New Agent Thread and try to open type @symbol
4. Observer nothing shows up and You have data did not match any variant of untagged enum WorkspaceSymbolResponse exception in the logs

Alternatively you can run this following code in IRB

Code snippet or error message

require 'ruby-lsp'
require "ruby_lsp/internal"
@global_state = RubyLsp::GlobalState.new
# @global_state.stubs(:has_type_checker).returns(false)
@index = @global_state.index
@index.index_single(URI::Generic.from_path(path: "/fake.rb"), <<~RUBY)
  OK = 'OK'
  class Foo
    BOK = OK
    BAD = AD
    def test
    end
    alias whatever test
    alias_method :bar, :to_a
    alias_method "baz", "to_a"
    @@test = '123'
  end
RUBY

RubyLsp::Requests::WorkspaceSymbol.new(@global_state, nil).perform

Which should result in following response:

[...,
#<LanguageServer::Protocol::Interface::WorkspaceSymbol:0x000000010646ee50
  @attributes=
   {:name=>"@@test",
    :kind=>nil,
    :containerName=>"",
    :location=>
     #<LanguageServer::Protocol::Interface::Location:0x000000010646ef40
      @attributes=
       {:uri=>"file:///fake.rb",
        :range=>
         #<LanguageServer::Protocol::Interface::Range:0x000000010646f058
          @attributes=
           {:start=>#<LanguageServer::Protocol::Interface::Position:0x000000010646f530 @attributes={:line=>9, :character=>2}>,
            :end=>#<LanguageServer::Protocol::Interface::Position:0x000000010646f0f8 @attributes={:line=>9, :character=>8}>}>}>}>]

You can notice that kind for class variable, constant alias and resolvable method alias is nil, additionally method aliases which are not resolvable are also included in the result.

[pkondzior]

There has been an attempt on kind of fixing this issue #3596 but I do not think that approach is complete.

It should be augmented with following steps:

1. Resolve alias methods and constant during fuzzy search that happens while response for workspaceSymbol message is processed
2. Then skip unresolvable method aliases and constants
3. Additionally we should encode properly variants of entries RubyIndexer::Entry::ClassVariable, RubyIndexer::Entry::ConstantAlias and RubyIndexer::Entry::MethodAlias for kind_for_entry

@vinistock What are your thoughts on this? I’d really appreciate your feedback, since I’m not sure whether constant and method resolution should happen during the fuzzy search. If not, how can we ensure that we’re not encoding unresolvable symbols during response generation? It seems that the current lazy approach to method and constant resolution is used in most places. Here’s the PR

BTW, I think there might be a bug in RubyIndexer::Index#constant_completion_candidates because it doesn’t attempt to resolve constants on UnresolvedConstantAlias entries — unlike RubyIndexer::Index#method_completion_candidates, which does.

Edit: I've checked and there is a bug in completion, but the response builder has a fallback and this is not as severe as in workspace response generation. for following code

 CONSTANT = 1
 module Original
   TARGET = CONSTANT
   T
 end

This request:

server.process_message(id: 1, method: "textDocument/completion", params: {
  textDocument: { uri: uri },
  position: { line: 4, character: 3 },
})

is going to respond with:

=> [#<LanguageServer::Protocol::Interface::CompletionItem:0x00000001202333d8
  @attributes=
   {label: "Original::TARGET",
    labelDetails: #<LanguageServer::Protocol::Interface::CompletionItemLabelDetails:0x0000000120233518 @attributes={description: "fake.rb"}>,
    kind: 18,
    filterText: "TARGET",
    textEdit:
     #<LanguageServer::Protocol::Interface::TextEdit:0x0000000120233450
      @attributes=
       {range:
         #<LanguageServer::Protocol::Interface::Range:0x00000001202350e8
          @attributes=
           {start: #<LanguageServer::Protocol::Interface::Position:0x00000001202351d8 @attributes={line: 4, character: 2}>,
            end: #<LanguageServer::Protocol::Interface::Position:0x0000000120235138 @attributes={line: 4, character: 3}>}>,
        newText: "TARGET"}>}>]

The completion is going to be represented through fallback as 18 - Reference , where it should be rather Class https://github.com/Shopify/ruby-lsp/blob/main/lib/ruby_lsp/listeners/completion.rb#L619

[janko]

Great find and happy to see that a fixed is already prepared. This breaks project symbol search in Zed when Ruby LSP matches a method alias or another symbol for which it returns kind as null, as Zed fails to handle it since it's malformed according to the LSP spec.

[pkondzior]

@vinistock what else I can do to make this PR being merged ? #3784 this issue really prevents Zed users from using ruby-lsp in their projects right now, there is a fallback available with solagraph, but the fix I think is pretty straightforward, ruby-lsp should just respect LSP spec and do not return nils for SymbolKind enums.

[vinistock]

I added #3803 to fix this, which is basically #3596.

@pkondzior regarding your PR, we cannot, with the current architecture of the indexer, have an eager resolution phase. We intentionally designed the resolution to be lazy for everything because just the indexing phase on our large codebases is already too slow - and resolution is quite expensive with all of the recursive algorithms.

Our team is in the process of redesigning indexing with a more performant implementation and a better resolution phase, which should address many issues in terms of correctness, speed and memory usage. While I truly appreciate the interest in this area and the PRs, let's please hold off on major changes to this current indexer as we will completely substitute it once our new work is complete.

[pkondzior]

@vinistock understood. Thank You for taking care of missing symbol kinds, this will at least produce valid response. Looking forward for the new indexer design 🤞