From a6c13bd55726fc6759da6d3fb3de4f409abaad59 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Fri, 6 Mar 2026 12:21:48 -0500 Subject: [PATCH] Migrate go to definition to use Rubydex --- lib/ruby_lsp/internal.rb | 3 + lib/ruby_lsp/listeners/definition.rb | 133 +++++++---------- lib/ruby_lsp/rubydex/definition.rb | 54 +++++++ .../definition/class_reference.exp.json | 30 ++-- .../definition/constant_reference.exp.json | 30 ++-- test/fixtures/class_reference.rb | 5 +- test/fixtures/class_reference_target.rb | 9 -- test/fixtures/constant_reference.rb | 4 +- test/fixtures/constant_reference_target.rb | 5 - test/requests/definition_expectations_test.rb | 138 ++++++++---------- test/test_helper.rb | 8 + 11 files changed, 212 insertions(+), 207 deletions(-) create mode 100644 lib/ruby_lsp/rubydex/definition.rb delete mode 100644 test/fixtures/class_reference_target.rb delete mode 100644 test/fixtures/constant_reference_target.rb diff --git a/lib/ruby_lsp/internal.rb b/lib/ruby_lsp/internal.rb index 7b3a04a000..cbff9bb3ce 100644 --- a/lib/ruby_lsp/internal.rb +++ b/lib/ruby_lsp/internal.rb @@ -31,6 +31,9 @@ require "shellwords" require "set" +# Rubydex LSP additions +require "ruby_lsp/rubydex/definition" + require "ruby-lsp" require "ruby_lsp/base_server" require "ruby_indexer/ruby_indexer" diff --git a/lib/ruby_lsp/listeners/definition.rb b/lib/ruby_lsp/listeners/definition.rb index 38478712cf..f9b14caece 100644 --- a/lib/ruby_lsp/listeners/definition.rb +++ b/lib/ruby_lsp/listeners/definition.rb @@ -13,6 +13,7 @@ def initialize(response_builder, global_state, language_id, uri, node_context, d @response_builder = response_builder @global_state = global_state @index = global_state.index #: RubyIndexer::Index + @graph = global_state.graph #: Rubydex::Graph @type_inferrer = global_state.type_inferrer #: TypeInferrer @language_id = language_id @uri = uri @@ -152,32 +153,32 @@ def on_global_variable_write_node_enter(node) #: (Prism::InstanceVariableReadNode node) -> void def on_instance_variable_read_node_enter(node) - handle_instance_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::InstanceVariableWriteNode node) -> void def on_instance_variable_write_node_enter(node) - handle_instance_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::InstanceVariableAndWriteNode node) -> void def on_instance_variable_and_write_node_enter(node) - handle_instance_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::InstanceVariableOperatorWriteNode node) -> void def on_instance_variable_operator_write_node_enter(node) - handle_instance_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::InstanceVariableOrWriteNode node) -> void def on_instance_variable_or_write_node_enter(node) - handle_instance_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::InstanceVariableTargetNode node) -> void def on_instance_variable_target_node_enter(node) - handle_instance_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::SuperNode node) -> void @@ -192,32 +193,32 @@ def on_forwarding_super_node_enter(node) #: (Prism::ClassVariableAndWriteNode node) -> void def on_class_variable_and_write_node_enter(node) - handle_class_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::ClassVariableOperatorWriteNode node) -> void def on_class_variable_operator_write_node_enter(node) - handle_class_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::ClassVariableOrWriteNode node) -> void def on_class_variable_or_write_node_enter(node) - handle_class_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::ClassVariableTargetNode node) -> void def on_class_variable_target_node_enter(node) - handle_class_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::ClassVariableReadNode node) -> void def on_class_variable_read_node_enter(node) - handle_class_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end #: (Prism::ClassVariableWriteNode node) -> void def on_class_variable_write_node_enter(node) - handle_class_variable_definition(node.name.to_s) + handle_variable_definition(node.name.to_s) end private @@ -257,93 +258,63 @@ def handle_super_node_definition #: (String name) -> void def handle_global_variable_definition(name) - entries = @index[name] + declaration = @graph[name] + return unless declaration - return unless entries - - entries.each do |entry| - location = entry.location - - @response_builder << Interface::Location.new( - uri: entry.uri.to_s, - range: Interface::Range.new( - start: Interface::Position.new(line: location.start_line - 1, character: location.start_column), - end: Interface::Position.new(line: location.end_line - 1, character: location.end_column), - ), - ) - end + declaration.definitions.each { |definition| @response_builder << definition.to_lsp_selection_location } end + # Handle class or instance variables. We collect all definitions across the ancestors of the type + # #: (String name) -> void - def handle_class_variable_definition(name) - type = @type_inferrer.infer_receiver_type(@node_context) - return unless type - - entries = @index.resolve_class_variable(name, type.name) - return unless entries - - entries.each do |entry| - @response_builder << Interface::Location.new( - uri: entry.uri.to_s, - range: range_from_location(entry.location), - ) - end - rescue RubyIndexer::Index::NonExistingNamespaceError - # If by any chance we haven't indexed the owner, then there's no way to find the right declaration - end - - #: (String name) -> void - def handle_instance_variable_definition(name) - # Sorbet enforces that all instance variables be declared on typed strict or higher, which means it will be able - # to provide all features for them + def handle_variable_definition(name) + # Sorbet enforces that all variables be declared on typed strict or higher, which means it will be able to + # provide all features for them return if @sorbet_level.strict? type = @type_inferrer.infer_receiver_type(@node_context) return unless type - entries = @index.resolve_instance_variable(name, type.name) - return unless entries + owner = @graph[type.name] + return unless owner.is_a?(Rubydex::Namespace) - entries.each do |entry| - location = entry.location + owner.ancestors.each do |ancestor| + member = ancestor.member(name) + next unless member - @response_builder << Interface::Location.new( - uri: entry.uri.to_s, - range: Interface::Range.new( - start: Interface::Position.new(line: location.start_line - 1, character: location.start_column), - end: Interface::Position.new(line: location.end_line - 1, character: location.end_column), - ), - ) + member.definitions.each { |definition| @response_builder << definition.to_lsp_selection_location } end - rescue RubyIndexer::Index::NonExistingNamespaceError - # If by any chance we haven't indexed the owner, then there's no way to find the right declaration end #: (String message, TypeInferrer::Type? receiver_type, ?inherited_only: bool) -> void def handle_method_definition(message, receiver_type, inherited_only: false) - methods = if receiver_type - @index.resolve_method(message, receiver_type.name, inherited_only: inherited_only) + declaration = if receiver_type + owner = @graph[receiver_type.name] + owner.find_member("#{message}()", only_inherited: inherited_only) if owner.is_a?(Rubydex::Namespace) end - # If the method doesn't have a receiver, or the guessed receiver doesn't have any matched candidates, - # then we provide a few candidates to jump to - # But we don't want to provide too many candidates, as it can be overwhelming - if receiver_type.nil? || (receiver_type.is_a?(TypeInferrer::GuessedType) && methods.nil?) - methods = @index[message]&.take(MAX_NUMBER_OF_DEFINITION_CANDIDATES_WITHOUT_RECEIVER) + # If the method doesn't have a receiver, or the guessed receiver doesn't have any matched candidates, then we + # provide a few candidates to jump to. However, we don't want to provide too many candidates, as it can be + # overwhelming + if receiver_type.nil? || (receiver_type.is_a?(TypeInferrer::GuessedType) && declaration.nil?) + declaration = @graph.search("##{message}()").take(MAX_NUMBER_OF_DEFINITION_CANDIDATES_WITHOUT_RECEIVER) end - return unless methods + return unless declaration - methods.each do |target_method| - uri = target_method.uri - full_path = uri.full_path - next if @sorbet_level.true_or_higher? && (!full_path || not_in_dependencies?(full_path)) + Array(declaration).each do |decl| + decl.definitions.each do |definition| + location = definition.location + uri = URI(location.uri) + full_path = uri.full_path + next if @sorbet_level.true_or_higher? && (!full_path || not_in_dependencies?(full_path)) - @response_builder << Interface::LocationLink.new( - target_uri: uri.to_s, - target_range: range_from_location(target_method.location), - target_selection_range: range_from_location(target_method.name_location), - ) + @response_builder << Interface::LocationLink.new( + target_uri: uri.to_s, + target_range: definition.to_lsp_selection_range, + target_selection_range: definition.to_lsp_name_range || definition.to_lsp_selection_range, + ) + end end end @@ -351,12 +322,10 @@ def handle_method_definition(message, receiver_type, inherited_only: false) def handle_require_definition(node, message) case message when :require - entry = @index.search_require_paths(node.content).find do |uri| - uri.require_path == node.content - end + document = @graph.resolve_require_path(node.content, $LOAD_PATH) - if entry - candidate = entry.full_path + if document + candidate = URI(document.uri).full_path if candidate @response_builder << Interface::Location.new( diff --git a/lib/ruby_lsp/rubydex/definition.rb b/lib/ruby_lsp/rubydex/definition.rb new file mode 100644 index 0000000000..3b89b29c2e --- /dev/null +++ b/lib/ruby_lsp/rubydex/definition.rb @@ -0,0 +1,54 @@ +# typed: strict +# frozen_string_literal: true + +module Rubydex + class Definition + #: () -> RubyLsp::Interface::Range + def to_lsp_selection_range + loc = location + + RubyLsp::Interface::Range.new( + start: RubyLsp::Interface::Position.new(line: loc.start_line, character: loc.start_column), + end: RubyLsp::Interface::Position.new(line: loc.end_line, character: loc.end_column), + ) + end + + #: () -> RubyLsp::Interface::Location + def to_lsp_selection_location + location = self.location + + RubyLsp::Interface::Location.new( + uri: location.uri, + range: RubyLsp::Interface::Range.new( + start: RubyLsp::Interface::Position.new(line: location.start_line, character: location.start_column), + end: RubyLsp::Interface::Position.new(line: location.end_line, character: location.end_column), + ), + ) + end + + #: () -> RubyLsp::Interface::Range? + def to_lsp_name_range + loc = name_location + return unless loc + + RubyLsp::Interface::Range.new( + start: RubyLsp::Interface::Position.new(line: loc.start_line, character: loc.start_column), + end: RubyLsp::Interface::Position.new(line: loc.end_line, character: loc.end_column), + ) + end + + #: () -> RubyLsp::Interface::Location? + def to_lsp_name_location + location = name_location + return unless location + + RubyLsp::Interface::Location.new( + uri: location.uri, + range: RubyLsp::Interface::Range.new( + start: RubyLsp::Interface::Position.new(line: location.start_line, character: location.start_column), + end: RubyLsp::Interface::Position.new(line: location.end_line, character: location.end_column), + ), + ) + end + end +end diff --git a/test/expectations/definition/class_reference.exp.json b/test/expectations/definition/class_reference.exp.json index 2acd501651..245cb01ad4 100644 --- a/test/expectations/definition/class_reference.exp.json +++ b/test/expectations/definition/class_reference.exp.json @@ -1,33 +1,33 @@ { + "params": [ + { + "line": 3, + "character": 10 + } + ], "result": [ { - "targetUri": "file:///fixtures/class_reference_target.rb", + "targetUri": "file:////fake.rb", "targetSelectionRange": { "start": { - "line": 4, - "character": 8 + "line": 0, + "character": 6 }, "end": { - "line": 4, - "character": 20 + "line": 0, + "character": 12 } }, "targetRange": { "start": { - "line": 4, - "character": 2 + "line": 0, + "character": 0 }, "end": { - "line": 7, - "character": 5 + "line": 1, + "character": 3 } } } - ], - "params": [ - { - "line": 0, - "character": 19 - } ] } diff --git a/test/expectations/definition/constant_reference.exp.json b/test/expectations/definition/constant_reference.exp.json index 4376a1af2a..994b215e0d 100644 --- a/test/expectations/definition/constant_reference.exp.json +++ b/test/expectations/definition/constant_reference.exp.json @@ -1,33 +1,33 @@ { + "params": [ + { + "line": 2, + "character": 10 + } + ], "result": [ { - "targetUri": "file:///fixtures/constant_reference_target.rb", - "targetSelectionRange": { + "targetUri": "file:////fake.rb", + "targetRange": { "start": { - "line": 3, - "character": 7 + "line": 0, + "character": 0 }, "end": { - "line": 3, + "line": 0, "character": 10 } }, - "targetRange": { + "targetSelectionRange": { "start": { - "line": 3, + "line": 0, "character": 0 }, "end": { - "line": 4, - "character": 3 + "line": 0, + "character": 10 } } } - ], - "params": [ - { - "line": 0, - "character": 12 - } ] } diff --git a/test/fixtures/class_reference.rb b/test/fixtures/class_reference.rb index b7db8e8b68..8fd48fae09 100644 --- a/test/fixtures/class_reference.rb +++ b/test/fixtures/class_reference.rb @@ -1 +1,4 @@ -example = RubyLsp::ExampleClass.new +class Target +end + +example = Target.new diff --git a/test/fixtures/class_reference_target.rb b/test/fixtures/class_reference_target.rb deleted file mode 100644 index e1aeb6caa1..0000000000 --- a/test/fixtures/class_reference_target.rb +++ /dev/null @@ -1,9 +0,0 @@ -# typed: strict -# frozen_string_literal: true - -module RubyLsp - class ExampleClass - def foo - end - end -end diff --git a/test/fixtures/constant_reference.rb b/test/fixtures/constant_reference.rb index be785de36a..1473e12e1d 100644 --- a/test/fixtures/constant_reference.rb +++ b/test/fixtures/constant_reference.rb @@ -1 +1,3 @@ -example = Foo +TARGET = 1 + +example = TARGET diff --git a/test/fixtures/constant_reference_target.rb b/test/fixtures/constant_reference_target.rb deleted file mode 100644 index d6937fd100..0000000000 --- a/test/fixtures/constant_reference_target.rb +++ /dev/null @@ -1,5 +0,0 @@ -# typed: strict -# frozen_string_literal: true - -module Foo -end diff --git a/test/requests/definition_expectations_test.rb b/test/requests/definition_expectations_test.rb index 5845300aac..0dbe7af57d 100644 --- a/test/requests/definition_expectations_test.rb +++ b/test/requests/definition_expectations_test.rb @@ -18,34 +18,9 @@ def run_expectations(source) with_server(source, stub_no_typechecker: true) do |server, uri| position = @__params&.first || { character: 0, line: 0 } - index = server.global_state.index - - index.index_file( - URI::Generic.from_path( - load_path_entry: "#{Dir.pwd}/lib", - path: File.expand_path( - "../../test/fixtures/class_reference_target.rb", - __dir__, - ), - ), - ) - index.index_file( - URI::Generic.from_path( - path: File.expand_path( - "../../test/fixtures/constant_reference_target.rb", - __dir__, - ), - ), - ) - index.index_file( - URI::Generic.from_path( - load_path_entry: "#{Dir.pwd}/lib", - path: File.expand_path( - "../../lib/ruby_lsp/server.rb", - __dir__, - ), - ), - ) + graph = server.global_state.graph + graph.index_all([File.expand_path("../../lib/ruby_lsp/server.rb", __dir__)]) + graph.resolve server.process_message( id: 1, @@ -161,26 +136,21 @@ class Baz end end - def test_jumping_to_default_require_of_a_gem - with_server("require \"bundler\"") do |server, uri| - index = server.global_state.index - - bundler_uri = URI::Generic.from_path( - path: "#{RbConfig::CONFIG["rubylibdir"]}/bundler.rb", - load_path_entry: RbConfig::CONFIG["rubylibdir"], - ) - index.index_file(bundler_uri) - - Dir.glob("#{RbConfig::CONFIG["rubylibdir"]}/bundler/*.rb").each do |path| - index.index_file(URI::Generic.from_path(load_path_entry: RbConfig::CONFIG["rubylibdir"], path: path)) - end + def test_jumping_to_a_gem_default_require + with_server("require \"minitest\"") do |server, uri| + graph = server.global_state.graph + minitest_paths = Dir.glob("#{gem_path("minitest")}/**/*.rb") + minitest_path = gem_path("minitest").join("lib").join("minitest.rb").to_s + minitest_paths << minitest_path + graph.index_all(minitest_paths) + graph.resolve server.process_message( id: 1, method: "textDocument/definition", params: { textDocument: { uri: uri }, position: { character: 10, line: 0 } }, ) - assert_equal(bundler_uri.to_s, server.pop_response.response.first.attributes[:uri]) + assert_equal(URI::Generic.from_path(path: minitest_path).to_s, server.pop_response.response.first.attributes[:uri]) end end @@ -229,31 +199,25 @@ class A def test_definition_addons source = <<~RUBY - RubyLsp + class Target + end + + Target RUBY begin create_definition_addon with_server(source, stub_no_typechecker: true, load_addons: true) do |server, uri| - server.global_state.index.index_file( - URI::Generic.from_path( - load_path_entry: "#{Dir.pwd}/lib", - path: File.expand_path( - "../../test/fixtures/class_reference_target.rb", - __dir__, - ), - ), - ) server.process_message( id: 1, method: "textDocument/definition", - params: { textDocument: { uri: uri }, position: { character: 0, line: 0 } }, + params: { textDocument: { uri: uri }, position: { character: 0, line: 3 } }, ) response = server.pop_response.response assert_equal(2, response.size) - assert_match("class_reference_target.rb", response[0].target_uri) + assert_match("fake.rb", response[0].target_uri) assert_match("generated_by_addon.rb", response[1].uri) end ensure @@ -317,9 +281,9 @@ def foo; end }, }, }) - index = server.global_state.index - path = second_uri.to_standardized_path #: as !nil - index.index_single(URI::Generic.from_path(path: path), second_source) + graph = server.global_state.graph + graph.index_source(second_uri.to_s, second_source, "ruby") + graph.resolve server.process_message( id: 1, @@ -461,6 +425,7 @@ def foo; end ) response = server.pop_response.response assert_equal(2, response.size) + response.sort_by! { |location| location.target_range.start.line } assert_equal(1, response[0].target_range.start.line) assert_equal(1, response[0].target_range.end.line) @@ -539,6 +504,7 @@ def foo; end ) response = server.pop_response.response assert_equal(2, response.size) + response.sort_by! { |location| location.target_range.start.line } assert_equal(1, response[0].target_range.start.line) assert_equal(1, response[0].target_range.end.line) @@ -589,7 +555,7 @@ class Foo end end - def test_methods_with_dynamic_namespace_is_also_suggested + def test_members_of_dynamic_namespaces_are_not_found source = <<~RUBY # typed: false @@ -610,11 +576,7 @@ def bar ) response = server.pop_response.response - assert_equal(1, response.size) - - range = response[0].attributes[:targetRange].attributes - range_hash = { start: range[:start].to_hash, end: range[:end].to_hash } - assert_equal({ start: { line: 3, character: 2 }, end: { line: 3, character: 14 } }, range_hash) + assert_empty(response) end end @@ -642,6 +604,7 @@ def foo; end response = server.pop_response.response assert_equal(2, response.size) + response.sort_by! { |location| location.target_range.start.line } range = response[0].attributes[:targetRange].attributes range_hash = { start: range[:start].to_hash, end: range[:end].to_hash } @@ -748,8 +711,9 @@ def test_definitions_are_listed_in_erb_files_as_unknown_receiver ERB with_server(source, URI("/fake.erb")) do |server, uri| - server.global_state.index.index_single( - URI::Generic.from_path(path: "/fake/path/foo.rb"), <<~RUBY + graph = server.global_state.graph + graph.index_source( + URI::Generic.from_path(path: "/fake/path/foo.rb").to_s, <<~RUBY, "ruby" class Bar def foo; end @@ -757,6 +721,7 @@ def bar; end end RUBY ) + graph.resolve server.process_message( id: 1, @@ -827,6 +792,32 @@ def baz end end + def test_inherited_class_variables + source = <<~RUBY + class Foo + @@hello = 123 + end + + class Bar < Foo + def self.hello + @@hello + end + end + RUBY + + with_server(source) do |server, uri| + server.process_message( + id: 1, + method: "textDocument/definition", + params: { textDocument: { uri: uri }, position: { line: 6, character: 4 } }, + ) + + response = server.pop_response.response + assert_equal(1, response.size) + assert_equal(1, response[0].range.start.line) + end + end + def test_definition_for_global_variables source = <<~RUBY $bar &&= 1 @@ -838,9 +829,6 @@ def test_definition_for_global_variables RUBY with_server(source) do |server, uri| - index = server.instance_variable_get(:@global_state).index - RubyIndexer::RBSIndexer.new(index).index_ruby_core - server.process_message( id: 1, method: "textDocument/definition", @@ -861,20 +849,11 @@ def test_definition_for_global_variables response = server.pop_response.response assert_equal(3, response.size) + + response.sort_by! { |location| location.range.start.line } assert_equal(2, response[0].range.start.line) assert_equal(3, response[1].range.start.line) assert_equal(4, response[2].range.start.line) - - server.process_message( - id: 1, - method: "textDocument/definition", - params: { textDocument: { uri: uri }, position: { character: 1, line: 5 } }, - ) - - response = server.pop_response.response.first - assert_match(%r{/gems/rbs-.*/core/global_variables.rbs}, response.uri) - assert_equal(response.range.start.line, response.range.end.line) - assert_operator(response.range.start.character, :<, response.range.end.character) end end @@ -1003,6 +982,7 @@ def self.baz params: { textDocument: { uri: uri }, position: { character: 4, line: 1 } }, ) response = server.pop_response.response + response.sort_by! { |location| location.range.start.line } assert_equal(1, response[0].range.start.line) assert_equal(4, response[1].range.start.line) diff --git a/test/test_helper.rb b/test/test_helper.rb index 11e27795cc..8d936a75de 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -24,5 +24,13 @@ class Test include RubyLsp::TestHelper Minitest::Test.make_my_diffs_pretty! + + # Returns full path to the requested gem + # + #: (String) -> Pathname + def gem_path(gem_name) + spec = Gem::Specification.find_by_name(gem_name) + Pathname.new(spec.full_gem_path) + end end end