Shopify / ruby-lsp

An opinionated language server for Ruby
https://shopify.github.io/ruby-lsp/
MIT License
1.54k stars 146 forks source link

Allow `Definition` listeners receive document as a constructor argument #2191

Closed st0012 closed 1 month ago

st0012 commented 3 months ago

Context

I plan to support go-to-definition for RSpec's subject calls in ruby-lsp-rspec. Given that RSpec's subject definition and callers usually sit in the same file, like:

RSpec.describe Foo do
  subject { described_class.new }
  it "does something" do
    subject.something
  end
end

Therefore, I don't plan to rely on index for this feature, but instead visiting the document when the request is triggered.

Proposal

I'm aware that for index-based implementations using Dispatcher#dispatch_once is the most efficient way, so I don't plan to change it. But instead, I want to let addon listeners receive document instead of document.uri, so it'll be possible to visit it with a child dispatcher, like:

module RubyLsp
  module RSpec
    class Definition
      def initialize(response_builder, global_state, document, node_context, dispatcher)
        @document = document
        # ...
      end

      def on_call_node_enter(node)
        return unless node == @node_context.node
        child_dispatcher = Prism::Dispatcher.new
        subject_locator = SubjectLocator.new(child_dispatcher, node)
        child_dispatcher.dispatch(@document.tree)
        @response_builder << build_location(subject_locator.subject)
      end
vinistock commented 3 months ago

We were very intentional in not allowing this to happen for performance reasons. The whole listener approach was designed to prevent addons from re-visiting the same AST multiple times.

The solution here would be indexing addons (which we are yet to explore). We should allow addons to register for handling their DSLs during indexing with the ability to mutate the entries.

The RSpec addon would register to handle the subject DSL during indexing, put entries for the subject in the index and then go to definition would simply work.

It's essentially the same case as handling belongs_to for the Rails addon. We need a way to tell the indexer that when belongs_to is invoked you'll get a bunch of extra declarations inserted in the index.