Improve handling of class variables, aliases and constant aliases

[pkondzior]

Motivation

Closes #3781

Workspace symbol search was not properly handling class variables, method aliases, and constant aliases.
As a result, symbols such as @@test, alias whatever test, or BOK = OK were missing or incorrectly reported.
This change ensures that all symbol types are correctly resolved and returned, improving the completeness and accuracy of workspace symbol indexing.

Implementation

• Added resolve_entry and skip_unresolved_entries helpers in RubyIndexer::Index to properly resolve constant and method aliases before returning them in search results.
• Updated fuzzy_search to skip unresolved entries and include correctly resolved ones.
• Extended kind_for_entry to support:
  • Entry::ClassVariable (as FIELD)
  • Entry::ConstantAlias (as CONSTANT)
  • Entry::MethodAlias (as METHOD)
  • Fallback handling for unknown kinds.
• This ensures class variables, constant aliases, and method aliases appear in workspace symbol results with the correct symbol kind.

Automated Tests

Added new test cases in workspace_symbol_test.rb covering:

• Method aliases (alias whatever test, alias_method)
• Constant aliases (BOK = OK)
• Unresolved constants (e.g., BAD = AD)
• Class variables (@@test)

These tests verify that symbol kinds and resolutions behave correctly for all the above cases.

Manual Tests

1. Open a Ruby file in VS Code containing:

   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

2. Trigger “Go to Symbol in Workspace” (Ctrl+T or Cmd+T).
3. Search for:

• whatever → shows as method
• Foo::BOK → shows as constant
• @@test → shows as field
• BAD → not listed (unresolved constant)

4. Verify that symbol kinds and visibility match expected behaviour

[graphite-app]

How to use the Graphite Merge Queue

Add the label graphite-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[pkondzior]

During discussion in #2660 (comment) I've found few places where we could cut down performance profile for response generation on this call, I think these optimisations are going to affect also other response calls in the project:

• URI is platform aware now and skips unnecessary work on unix and windows platforms
• Entries are now caching file_path, file_name values as these are not going to change during entry lifetime
• Entries now are able to check and cache in_dependencies? so checking that is cheaper during response generation
• Constant resolution step has been added to index_all and index_single Index calls, this way we can now resolve constant and methods when they get loaded into index which shrinks performance profiles on workspace symbol generation
• workspace symbol response passes now only resolved entries using entry fast call check #resolved?

All this changes are optimising responsiveness of first call to workspace/Symbol:
on this branch:

workspace/symbol average: 0.019443 std_dev: 0.006428

on main:

workspace/symbol average: 0.053088 std_dev: 0.001863

We are going to be 2.7 times faster on a first call, I believe on the second call it will go even further due to cache population for the entries.

[pkondzior]

I've added one more commit which makes Sorbet happy and cuts down performance profile even further:

workspace/symbol average: 0.015083 std_dev: 0.001324

[janko]

@pkondzior Note that the optimization part of these changes slow down indexing significantly, to the point where it doesn't finish in reasonable time.

This is my benchmarking script:

require "ruby_lsp/internal"
require "benchmark"

global_state = RubyLsp::GlobalState.new
puts "====== indexing ======="
p Benchmark.realtime {
  global_state.index.index_all
}
puts "====================="

durations = 10.times.map {
  p Benchmark.realtime {
    RubyLsp::Requests::WorkspaceSymbol.new(global_state, "UserForm").perform
  }
}
puts "====================="
puts durations.sum / durations.count

Given how performance-sensitive indexing is, I don't think we can afford adding any additional overhead to it.

[pkondzior]

@pkondzior Note that the optimization part of these changes slow down indexing significantly, to the point where it doesn't finish in reasonable time.

You have to either resolve indexes when indexing is done or during building response call, you will slow down either of them, you cannot avoid it.

Given how performance-sensitive indexing is, I don't think we should be adding any unnecessary overhead to it.

What kind of metrics are you using saying current indexing is slow ?

I've did the benchmark vs main branch and this are the results:
this branch:

warmup average: 5.600879 std_dev: 0
workspace/symbol average: 0.015775 std_dev: 0.002079

main:

warmup average: 1.996108 std_dev: 0
workspace/symbol average: 0.048942 std_dev: 0.003455

So as You see workspace/Symbol is significantly faster if we are going to pay for constant and method resolution, uri parsing and dependency flagging upfront during indexing. If you want to defer this work to workspace/Symbol you will have to pay it anyway there. The resolution is necessary to provide proper information for the json Sybol resolution that has been the root cause of the issue we both had in the Zed editor.

I want to admit that due to nature of the changes on the Entry class level we are sharing that toll across all instances of resonses that are dealing with entry path, so definition resolution is also going to be faster.

That all being said I think we could still do the method resolution a bit differently ie. along the regular entries hash we build during indexing we could create Set with unresolved entries cache that we should process on demand (index/index_all) this way the processing of unresolved entries could be more robust, we would not have to iterate through the whole index to find unresolved entries.

I've already asked here #3781 (comment) @vinistock what shall be the right approach for the resolution of unresolved entries, this is the root cause of the whole nil kind issue we have here and without design decision which tradeoff we should take I am abandoning myself from continuing from working on this issue further. I can come back further and continue working on this, or switch back to solagraph.

[janko]

We absolutely cannot significantly slow down indexing for the sake of improving workspace/symbol performance, especially if there is an alternative implementation that achieves similar gains without any impact on indexing. Slower indexing means the developer needs to wait longer before they can use any LSP features, including "go to definition".

For our Rails app, it takes 15s for Ruby LSP to index the codebase and gems on my MacBook Air M1 (measured with the script I shared). I would like it to be faster, but it's already heavily optimized AFAICT. With current changes in this PR, indexing is still running after 3min. I'm also not seeing any performance gains in workspace/symbol searches 🤷🏻

That's why I recommend separating out any strictly performance improvements into a separate PR. If the optimization isn't valid / worthwhile, then the PR can't be merged, even if the bugfix itself is correct.

[pkondzior]

We absolutely cannot significantly slow down indexing for the sake of improving workspace/symbol performance, especially if there is an alternative implementation that achieves similar gains without any impact on indexing. Slower indexing means the developer needs to wait longer before they can use any LSP features, including "go to definition".

Not sure why you are talking this tone. Who is "we" here ? Not sure about what alternative implementation you are talking either related to this issue.

Anyway, with You all saying that, I've looked into my code again and there was an issue with indexing, I've applied same approach as with collecting_comments flag and looks like there is almost no impact on the indexing side and still huge win on the workspace symbol side.

this branch:

warmup average: 1.874377 std_dev: 0
workspace/symbol average: 0.014662 std_dev: 0.001566

main:

warmup average: 1.876204 std_dev: 0
workspace/symbol average: 0.042014 std_dev: 0.001491

Thanks for pointing that out.

[janko]

Not sure why you are talking this tone. Who is "we" here ?

Apologies for the tone. I just minded that you initially dismissed my concerns about the performance regression in indexing as a simple "we have to make this slower, so that we can make this other thing faster" type of situation. The "we" was us users of Ruby LSP.

Not sure about what alternative implementation you are talking either related to this issue.

In #2660 (comment), you presented this PR as containing a better approach to workspace symbol performance optimizations than #3792, even to the point of downvoting my comment.

Anyway, with You all saying that, I've looked into my code again and there was an issue with indexing,

Thanks, I can confirm the indexing time appears unimpacted now 👍🏻

[pkondzior]

There is stil stack level overflow constant alias resolution on Ruby 3.4 with mkmf.rb shipped on windows and ubuntu, the reasons why this happens only on these platforms and not on windows is probably that both are x86 are ruby distro is probably sharing that content of the file across both of the distros, on macos this issue does not exists but still it means there is a bug in constant resolution. Investigating

[pkondzior]

Apologies for the tone. I just minded that you initially dismissed my concerns about the performance regression in indexing as a simple "we have to make this slower, so that we can make this other thing faster" type of situation. The "we" was us users of Ruby LSP.

I Apologies for that too, I did not realised that the problem is that severe on bigger projects, I was expecting some impact because I knew there was impact due to constant resolution even without any performance improvements that happen today. But fixing alias method comments made things better and all the impact You experience was due to my error not the resolution processing.

In #2660 (comment), you presented this PR as containing a better approach to workspace symbol performance optimizations than #3792, even to the point of downvoting my comment.

I've downvoted because I do not support that approach, and that is my personal opinion on that, I am not claiming that my approach is the only one possible solution that should be applied, I've been trying on building technical and merithoric ground on why I think one approach is better than other but found that there is not much ground for the discussion on the issue you have mentioned so decided to focus on improving and finishing work here instead of loosing steam on arguing there.

Thank you again for point my error, it made the solution better.

If there is a need on moving URI and Entry dependency flag performance enhancements into separate PR I can do that. But I think comments alias caching should stay because they are going to affect indexing performance as there is coupling to resolution processing.

[pkondzior]

There is stil stack level overflow constant alias resolution on Ruby 3.4 with mkmf.rb shipped on windows and ubuntu, the reasons why this happens only on these platforms and not on windows is probably that both are x86 are ruby distro is probably sharing that content of the file across both of the distros, on macos this issue does not exists but still it means there is a bug in constant resolution. Investigating

Ok found the issue, it boils down to this code:

require 'rbconfig'

module MakeMakefile
  RbConfig = ::RbConfig

  CONFIG = RbConfig::MAKEFILE_CONFIG
end

I have partial fix, but I am still not completely sure about the MVP for test regression test case.

[pkondzior]

This was tricky, integration tests is is running ruby-lsp --doctor in isolation and doesn't perform RBSIndexer.new(self).index_ruby_core where regular harness for indexing does, this lead to false positive tests on this code snippet I've ran through tests but not through rbs-lsp --doctor

require 'rbconfig'

module MakeMakefile
  RbConfig = ::RbConfig

  CONFIG = RbConfig::MAKEFILE_CONFIG
end

I've come up with this test case to be able to get into stack level too deep on test harness:

module RealClass
  CONSTANT = {}
end

module Foo
  SomeClass = ::SomeClass
  RealClass = ::RealClass

  UNRESOLVED = SomeClass::CONSTANT
  CONSTANT = RealClass::CONSTANT
end

Because now resolution of constants happen at the end of indexing step rbs-lsp --doctor is able to expose mentioned issue due to its internal difference on index_ruby_core . I am leaving the code here so the tests can pass but I will create separate ticket to handle mentioned case there.

Edited: Here is the PR addressing the issue

[pkondzior]

Closing this as #3803 handled most of the issues regarding ZED editor