From 6609b531e7a0ab123bb69039525a8fe532edfd99 Mon Sep 17 00:00:00 2001 From: snutij Date: Tue, 22 Oct 2024 21:33:08 +0200 Subject: [PATCH 1/5] feat: handle all kind of global variable nodes in definition request --- lib/ruby_lsp/listeners/definition.rb | 61 +++++++++++++++---- lib/ruby_lsp/requests/definition.rb | 5 ++ test/requests/definition_expectations_test.rb | 30 +++++++++ 3 files changed, 83 insertions(+), 13 deletions(-) diff --git a/lib/ruby_lsp/listeners/definition.rb b/lib/ruby_lsp/listeners/definition.rb index 8f4e28d49..6413cdabe 100644 --- a/lib/ruby_lsp/listeners/definition.rb +++ b/lib/ruby_lsp/listeners/definition.rb @@ -39,7 +39,12 @@ def initialize(response_builder, global_state, language_id, uri, node_context, d :on_block_argument_node_enter, :on_constant_read_node_enter, :on_constant_path_node_enter, + :on_global_variable_and_write_node_enter, + :on_global_variable_operator_write_node_enter, + :on_global_variable_or_write_node_enter, :on_global_variable_read_node_enter, + :on_global_variable_target_node_enter, + :on_global_variable_write_node_enter, :on_instance_variable_read_node_enter, :on_instance_variable_write_node_enter, :on_instance_variable_and_write_node_enter, @@ -121,23 +126,34 @@ def on_constant_read_node_enter(node) find_in_index(name) end + sig { params(node: Prism::GlobalVariableAndWriteNode).void } + def on_global_variable_and_write_node_enter(node) + handle_global_variable_definition(node.name.to_s) + end + + sig { params(node: Prism::GlobalVariableOperatorWriteNode).void } + def on_global_variable_operator_write_node_enter(node) + handle_global_variable_definition(node.name.to_s) + end + + sig { params(node: Prism::GlobalVariableOrWriteNode).void } + def on_global_variable_or_write_node_enter(node) + handle_global_variable_definition(node.name.to_s) + end + sig { params(node: Prism::GlobalVariableReadNode).void } def on_global_variable_read_node_enter(node) - entries = @index[node.name.to_s] - - return unless entries + handle_global_variable_definition(node.name.to_s) + end - entries.each do |entry| - location = entry.location + sig { params(node: Prism::GlobalVariableTargetNode).void } + def on_global_variable_target_node_enter(node) + handle_global_variable_definition(node.name.to_s) + end - @response_builder << Interface::Location.new( - uri: URI::Generic.from_path(path: entry.file_path).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 + sig { params(node: Prism::GlobalVariableWriteNode).void } + def on_global_variable_write_node_enter(node) + handle_global_variable_definition(node.name.to_s) end sig { params(node: Prism::InstanceVariableReadNode).void } @@ -197,6 +213,25 @@ def handle_super_node_definition ) end + sig { params(name: String).void } + def handle_global_variable_definition(name) + entries = @index[name] + + return unless entries + + entries.each do |entry| + location = entry.location + + @response_builder << Interface::Location.new( + uri: URI::Generic.from_path(path: entry.file_path).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 + end + sig { params(name: String).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 diff --git a/lib/ruby_lsp/requests/definition.rb b/lib/ruby_lsp/requests/definition.rb index 59f3592c3..e53f8d46d 100644 --- a/lib/ruby_lsp/requests/definition.rb +++ b/lib/ruby_lsp/requests/definition.rb @@ -46,7 +46,12 @@ def initialize(document, global_state, position, dispatcher, sorbet_level) Prism::ConstantReadNode, Prism::ConstantPathNode, Prism::BlockArgumentNode, + Prism::GlobalVariableAndWriteNode, + Prism::GlobalVariableOperatorWriteNode, + Prism::GlobalVariableOrWriteNode, Prism::GlobalVariableReadNode, + Prism::GlobalVariableTargetNode, + Prism::GlobalVariableWriteNode, Prism::InstanceVariableReadNode, Prism::InstanceVariableAndWriteNode, Prism::InstanceVariableOperatorWriteNode, diff --git a/test/requests/definition_expectations_test.rb b/test/requests/definition_expectations_test.rb index 17c78f3ab..64a795157 100644 --- a/test/requests/definition_expectations_test.rb +++ b/test/requests/definition_expectations_test.rb @@ -673,6 +673,11 @@ def baz def test_definition_for_global_variables source = <<~RUBY + $bar &&= 1 + $bar += 1 + $foo ||= 1 + $bar, $foo = 1 + $foo = 1 $DEBUG RUBY @@ -685,6 +690,31 @@ def test_definition_for_global_variables method: "textDocument/definition", params: { textDocument: { uri: uri }, position: { character: 1, line: 0 } }, ) + + response = server.pop_response.response + assert_equal(3, response.size) + assert_equal(0, response[0].range.start.line) + assert_equal(1, response[1].range.start.line) + assert_equal(3, response[2].range.start.line) + + server.process_message( + id: 1, + method: "textDocument/definition", + params: { textDocument: { uri: uri }, position: { character: 1, line: 2 } }, + ) + + response = server.pop_response.response + assert_equal(3, response.size) + 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) From 6673cdc57a4e6ebf28bdb9819ec0e7e0c57cb137 Mon Sep 17 00:00:00 2001 From: snutij Date: Tue, 22 Oct 2024 22:41:26 +0200 Subject: [PATCH 2/5] feat: apply target correction in case of position not exactly on target --- lib/ruby_lsp/requests/definition.rb | 25 +++++++++++++++---- test/requests/definition_expectations_test.rb | 23 +++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/lib/ruby_lsp/requests/definition.rb b/lib/ruby_lsp/requests/definition.rb index e53f8d46d..cd7b9d22a 100644 --- a/lib/ruby_lsp/requests/definition.rb +++ b/lib/ruby_lsp/requests/definition.rb @@ -77,11 +77,9 @@ def initialize(document, global_state, position, dispatcher, sorbet_level) parent, position, ) - elsif target.is_a?(Prism::CallNode) && !SPECIAL_METHOD_CALLS.include?(target.message) && !covers_position?( - target.message_loc, position - ) - # If the target is a method call, we need to ensure that the requested position is exactly on top of the - # method identifier. Otherwise, we risk showing definitions for unrelated things + elsif position_outside_target?(position, target) + # We need to ensure that the requested position is exactly on top of the + # target in dedicated cases like method calls. Otherwise, we risk showing definitions for unrelated things target = nil # For methods with block arguments using symbol-to-proc elsif target.is_a?(Prism::SymbolNode) && parent.is_a?(Prism::BlockArgumentNode) @@ -112,6 +110,23 @@ def perform @dispatcher.dispatch_once(@target) if @target @response_builder.response end + + private + + sig { params(position: T::Hash[Symbol, T.untyped], target: T.nilable(Prism::Node)).returns(T::Boolean) } + def position_outside_target?(position, target) + case target + when Prism::CallNode + !SPECIAL_METHOD_CALLS.include?(target.message) && !covers_position?(target.message_loc, position) + when Prism::GlobalVariableAndWriteNode, + Prism::GlobalVariableOperatorWriteNode, + Prism::GlobalVariableOrWriteNode, + Prism::GlobalVariableWriteNode + !covers_position?(target.name_loc, position) + else + false + end + end end end end diff --git a/test/requests/definition_expectations_test.rb b/test/requests/definition_expectations_test.rb index 64a795157..f163504d5 100644 --- a/test/requests/definition_expectations_test.rb +++ b/test/requests/definition_expectations_test.rb @@ -722,6 +722,29 @@ def test_definition_for_global_variables end end + def test_definition_apply_target_correction + source = <<~RUBY + $foo &&= 1 + $foo += 1 + $foo ||= 1 + $foo = 1 + RUBY + + lines_with_target_correction = [0, 1, 2, 3] + + with_server(source) do |server, uri| + lines_with_target_correction.each do |line| + server.process_message( + id: 1, + method: "textDocument/definition", + params: { textDocument: { uri: uri }, position: { character: 5, line: line } }, + ) + + assert_empty(server.pop_response.response) + end + end + end + def test_definition_for_instance_variables source = <<~RUBY class Foo From 37ba0ab778c6dbae87460d0b1afff2ad746abd8e Mon Sep 17 00:00:00 2001 From: snutij Date: Tue, 22 Oct 2024 22:42:20 +0200 Subject: [PATCH 3/5] test: lines document start at 0 --- test/requests/hover_expectations_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index b42224a80..6b08c8d24 100644 --- a/test/requests/hover_expectations_test.rb +++ b/test/requests/hover_expectations_test.rb @@ -109,7 +109,7 @@ def test_hover_apply_target_correction $foo = 1 RUBY - lines_with_target_correction = [1, 2, 3, 4] + lines_with_target_correction = [0, 1, 2, 3] with_server(source) do |server, uri| lines_with_target_correction.each do |line| From 00c8916a37303f896a1f8d1e46c3079e706c7780 Mon Sep 17 00:00:00 2001 From: snutij Date: Tue, 22 Oct 2024 22:52:35 +0200 Subject: [PATCH 4/5] fix: apply target correction on instance variable --- lib/ruby_lsp/requests/definition.rb | 7 ++++++- test/requests/definition_expectations_test.rb | 10 ++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/ruby_lsp/requests/definition.rb b/lib/ruby_lsp/requests/definition.rb index cd7b9d22a..e547cfc22 100644 --- a/lib/ruby_lsp/requests/definition.rb +++ b/lib/ruby_lsp/requests/definition.rb @@ -121,7 +121,12 @@ def position_outside_target?(position, target) when Prism::GlobalVariableAndWriteNode, Prism::GlobalVariableOperatorWriteNode, Prism::GlobalVariableOrWriteNode, - Prism::GlobalVariableWriteNode + Prism::GlobalVariableWriteNode, + Prism::InstanceVariableAndWriteNode, + Prism::InstanceVariableOperatorWriteNode, + Prism::InstanceVariableOrWriteNode, + Prism::InstanceVariableWriteNode + !covers_position?(target.name_loc, position) else false diff --git a/test/requests/definition_expectations_test.rb b/test/requests/definition_expectations_test.rb index f163504d5..b74090c7b 100644 --- a/test/requests/definition_expectations_test.rb +++ b/test/requests/definition_expectations_test.rb @@ -728,16 +728,22 @@ def test_definition_apply_target_correction $foo += 1 $foo ||= 1 $foo = 1 + class Foo + @foo &&= 1 + @foo += 1 + @foo ||= 1 + @foo = 1 + end RUBY - lines_with_target_correction = [0, 1, 2, 3] + lines_with_target_correction = [0, 1, 2, 3, 5, 6, 7, 8] with_server(source) do |server, uri| lines_with_target_correction.each do |line| server.process_message( id: 1, method: "textDocument/definition", - params: { textDocument: { uri: uri }, position: { character: 5, line: line } }, + params: { textDocument: { uri: uri }, position: { character: 7, line: line } }, ) assert_empty(server.pop_response.response) From 30896b90aca8fc79c9a5b6a4ee832b981120257b Mon Sep 17 00:00:00 2001 From: snutij Date: Tue, 22 Oct 2024 22:58:31 +0200 Subject: [PATCH 5/5] chore: remove callNode target correction since StringNode and SymbolNode are now well targetted --- lib/ruby_lsp/requests/definition.rb | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/lib/ruby_lsp/requests/definition.rb b/lib/ruby_lsp/requests/definition.rb index e547cfc22..8305f0ec6 100644 --- a/lib/ruby_lsp/requests/definition.rb +++ b/lib/ruby_lsp/requests/definition.rb @@ -12,12 +12,6 @@ class Definition < Request extend T::Sig extend T::Generic - SPECIAL_METHOD_CALLS = [ - :require, - :require_relative, - :autoload, - ].freeze - sig do params( document: T.any(RubyDocument, ERBDocument), @@ -78,8 +72,6 @@ def initialize(document, global_state, position, dispatcher, sorbet_level) position, ) elsif position_outside_target?(position, target) - # We need to ensure that the requested position is exactly on top of the - # target in dedicated cases like method calls. Otherwise, we risk showing definitions for unrelated things target = nil # For methods with block arguments using symbol-to-proc elsif target.is_a?(Prism::SymbolNode) && parent.is_a?(Prism::BlockArgumentNode) @@ -116,8 +108,6 @@ def perform sig { params(position: T::Hash[Symbol, T.untyped], target: T.nilable(Prism::Node)).returns(T::Boolean) } def position_outside_target?(position, target) case target - when Prism::CallNode - !SPECIAL_METHOD_CALLS.include?(target.message) && !covers_position?(target.message_loc, position) when Prism::GlobalVariableAndWriteNode, Prism::GlobalVariableOperatorWriteNode, Prism::GlobalVariableOrWriteNode,