From 8333120ac872337aa4e3d4900737b43515954288 Mon Sep 17 00:00:00 2001 From: snutij Date: Sat, 5 Oct 2024 10:54:30 +0200 Subject: [PATCH 1/5] feat: add hover for global variables --- lib/ruby_lsp/listeners/hover.rb | 14 ++++++++++++++ test/requests/hover_expectations_test.rb | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/lib/ruby_lsp/listeners/hover.rb b/lib/ruby_lsp/listeners/hover.rb index ecfda3a7e..61944cc99 100644 --- a/lib/ruby_lsp/listeners/hover.rb +++ b/lib/ruby_lsp/listeners/hover.rb @@ -13,6 +13,7 @@ class Hover Prism::ConstantReadNode, Prism::ConstantWriteNode, Prism::ConstantPathNode, + Prism::GlobalVariableReadNode, Prism::InstanceVariableReadNode, Prism::InstanceVariableAndWriteNode, Prism::InstanceVariableOperatorWriteNode, @@ -62,6 +63,7 @@ def initialize(response_builder, global_state, uri, node_context, dispatcher, so :on_constant_write_node_enter, :on_constant_path_node_enter, :on_call_node_enter, + :on_global_variable_read_node_enter, :on_instance_variable_read_node_enter, :on_instance_variable_write_node_enter, :on_instance_variable_and_write_node_enter, @@ -128,6 +130,18 @@ def on_call_node_enter(node) handle_method_hover(message) end + sig { params(node: Prism::GlobalVariableReadNode).void } + def on_global_variable_read_node_enter(node) + global_variable = node.name.to_s + entries = @index[global_variable] + + return unless entries + + categorized_markdown_from_index_entries(global_variable, entries).each do |category, content| + @response_builder.push(content, category: category) + end + end + sig { params(node: Prism::InstanceVariableReadNode).void } def on_instance_variable_read_node_enter(node) handle_instance_variable_hover(node.name.to_s) diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index a5f10b9df..c962175de 100644 --- a/test/requests/hover_expectations_test.rb +++ b/test/requests/hover_expectations_test.rb @@ -60,6 +60,25 @@ def test_hovering_on_erb end end + def test_hovering_for_global_variables + source = <<~RUBY + $DEBUG + 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/hover", + params: { textDocument: { uri: uri }, position: { line: 0, character: 1 } }, + ) + + assert_match("The debug flag", server.pop_response.response.contents.value) + end + end + def test_hovering_precision source = <<~RUBY module Foo From 9d177d0fa85414565717e6feb59b8cd5c21d4164 Mon Sep 17 00:00:00 2001 From: snutij Date: Mon, 14 Oct 2024 09:20:09 +0200 Subject: [PATCH 2/5] feat: add hover for all global variable nodes --- lib/ruby_lsp/listeners/hover.rb | 50 +++++++++++++++++++++--- test/requests/hover_expectations_test.rb | 35 ++++++++++++++--- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/lib/ruby_lsp/listeners/hover.rb b/lib/ruby_lsp/listeners/hover.rb index 61944cc99..5d1220f57 100644 --- a/lib/ruby_lsp/listeners/hover.rb +++ b/lib/ruby_lsp/listeners/hover.rb @@ -13,7 +13,12 @@ class Hover Prism::ConstantReadNode, Prism::ConstantWriteNode, Prism::ConstantPathNode, + Prism::GlobalVariableAndWriteNode, + Prism::GlobalVariableOperatorWriteNode, + Prism::GlobalVariableOrWriteNode, Prism::GlobalVariableReadNode, + Prism::GlobalVariableTargetNode, + Prism::GlobalVariableWriteNode, Prism::InstanceVariableReadNode, Prism::InstanceVariableAndWriteNode, Prism::InstanceVariableOperatorWriteNode, @@ -63,7 +68,12 @@ def initialize(response_builder, global_state, uri, node_context, dispatcher, so :on_constant_write_node_enter, :on_constant_path_node_enter, :on_call_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, @@ -130,16 +140,34 @@ def on_call_node_enter(node) handle_method_hover(message) end + sig { params(node: Prism::GlobalVariableAndWriteNode).void } + def on_global_variable_and_write_node_enter(node) + handle_global_variable_hover(node.name.to_s) + end + + sig { params(node: Prism::GlobalVariableOperatorWriteNode).void } + def on_global_variable_operator_write_node_enter(node) + handle_global_variable_hover(node.name.to_s) + end + + sig { params(node: Prism::GlobalVariableOrWriteNode).void } + def on_global_variable_or_write_node_enter(node) + handle_global_variable_hover(node.name.to_s) + end + sig { params(node: Prism::GlobalVariableReadNode).void } def on_global_variable_read_node_enter(node) - global_variable = node.name.to_s - entries = @index[global_variable] + handle_global_variable_hover(node.name.to_s) + end - return unless entries + sig { params(node: Prism::GlobalVariableTargetNode).void } + def on_global_variable_target_node_enter(node) + handle_global_variable_hover(node.name.to_s) + end - categorized_markdown_from_index_entries(global_variable, entries).each do |category, content| - @response_builder.push(content, category: category) - end + sig { params(node: Prism::GlobalVariableWriteNode).void } + def on_global_variable_write_node_enter(node) + handle_global_variable_hover(node.name.to_s) end sig { params(node: Prism::InstanceVariableReadNode).void } @@ -279,6 +307,16 @@ def handle_instance_variable_hover(name) # If by any chance we haven't indexed the owner, then there's no way to find the right declaration end + sig { params(name: String).void } + def handle_global_variable_hover(name) + entries = @index[name] + return unless entries + + categorized_markdown_from_index_entries(name, entries).each do |category, content| + @response_builder.push(content, category: category) + end + end + sig { params(name: String, location: Prism::Location).void } def generate_hover(name, location) entries = @index.resolve(name, @node_context.nesting) diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index c962175de..3ed85f8c2 100644 --- a/test/requests/hover_expectations_test.rb +++ b/test/requests/hover_expectations_test.rb @@ -62,20 +62,43 @@ def test_hovering_on_erb def test_hovering_for_global_variables source = <<~RUBY + # and write node + $bar &&= 1 + # operator write node + $baz += 1 + # or write node + $qux ||= 1 + # target write node + $quux, $corge = 1 + # write node + $foo = 1 + # read node $DEBUG RUBY + expectations = [ + { line: 1, character: 1, documentation: "and write node" }, + { line: 3, character: 1, documentation: "operator write node" }, + { line: 5, character: 1, documentation: "or write node" }, + { line: 7, character: 1, documentation: "target write node" }, + { line: 7, character: 10, documentation: "target write node" }, + { line: 9, character: 1, documentation: "write node" }, + { line: 11, character: 1, documentation: "The debug flag" }, + ] + 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/hover", - params: { textDocument: { uri: uri }, position: { line: 0, character: 1 } }, - ) + expectations.each do |expectation| + server.process_message( + id: 1, + method: "textDocument/hover", + params: { textDocument: { uri: uri }, position: { line: expectation[:line], character: 1 } }, + ) - assert_match("The debug flag", server.pop_response.response.contents.value) + assert_match(expectation[:documentation], server.pop_response.response.contents.value) + end end end From 75852a14a45eab9eddd3750327b55d38d96f0f00 Mon Sep 17 00:00:00 2001 From: snutij Date: Mon, 14 Oct 2024 09:21:19 +0200 Subject: [PATCH 3/5] feat: apply hover target correction on global variable --- lib/ruby_lsp/requests/hover.rb | 33 +++++++++++++++++++----- test/requests/hover_expectations_test.rb | 32 ++++++++++++++++++++++- 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/lib/ruby_lsp/requests/hover.rb b/lib/ruby_lsp/requests/hover.rb index a675f91be..a10619dbf 100644 --- a/lib/ruby_lsp/requests/hover.rb +++ b/lib/ruby_lsp/requests/hover.rb @@ -46,17 +46,13 @@ def initialize(document, global_state, position, dispatcher, sorbet_level) target = node_context.node parent = node_context.parent - if (Listeners::Hover::ALLOWED_TARGETS.include?(parent.class) && - !Listeners::Hover::ALLOWED_TARGETS.include?(target.class)) || - (parent.is_a?(Prism::ConstantPathNode) && target.is_a?(Prism::ConstantReadNode)) + if should_refine_target?(parent, target) target = determine_target( T.must(target), T.must(parent), position, ) - elsif target.is_a?(Prism::CallNode) && target.name != :require && target.name != :require_relative && - !covers_position?(target.message_loc, position) - + elsif position_outside_target?(position, target) target = nil end @@ -89,6 +85,31 @@ def perform ), ) end + + private + + sig { params(parent: T.nilable(Prism::Node), target: T.nilable(Prism::Node)).returns(T::Boolean) } + def should_refine_target?(parent, target) + (Listeners::Hover::ALLOWED_TARGETS.include?(parent.class) && + !Listeners::Hover::ALLOWED_TARGETS.include?(target.class)) || + (parent.is_a?(Prism::ConstantPathNode) && target.is_a?(Prism::ConstantReadNode)) + end + + 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 + target.name != :require && target.name != :require_relative && + !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/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index 3ed85f8c2..3cd930169 100644 --- a/test/requests/hover_expectations_test.rb +++ b/test/requests/hover_expectations_test.rb @@ -102,6 +102,36 @@ def test_hovering_for_global_variables end end + def test_hover_apply_target_correction + source = <<~RUBY + # and write node + $bar &&= 1 + # operator write node + $baz += 1 + # or write node + $qux ||= 1 + # write node + $foo = 1 + RUBY + + source_line_nodes = [1, 3, 5, 7] + + with_server(source) do |server, uri| + source_line_nodes.each do |line| + server.process_message( + id: 1, + method: "textDocument/hover", + params: { + textDocument: { uri: uri }, + position: { line: line, character: 5 }, + }, + ) + + assert_nil(server.pop_response.response) + end + end + end + def test_hovering_precision source = <<~RUBY module Foo @@ -352,13 +382,13 @@ def bar end RUBY - # Going to definition on `argument` should not take you to the `foo` method definition with_server(source) do |server, uri| server.process_message( id: 1, method: "textDocument/hover", params: { textDocument: { uri: uri }, position: { character: 12, line: 5 } }, ) + # Hover on `argument` should not show you the `foo` documentation assert_nil(server.pop_response.response) server.process_message( From 2289669f8bf54ec75a3f431662d115858bd4726d Mon Sep 17 00:00:00 2001 From: snutij Date: Tue, 15 Oct 2024 22:03:17 +0200 Subject: [PATCH 4/5] test: remove useless test setup --- test/requests/hover_expectations_test.rb | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index 3cd930169..b42224a80 100644 --- a/test/requests/hover_expectations_test.rb +++ b/test/requests/hover_expectations_test.rb @@ -77,13 +77,12 @@ def test_hovering_for_global_variables RUBY expectations = [ - { line: 1, character: 1, documentation: "and write node" }, - { line: 3, character: 1, documentation: "operator write node" }, - { line: 5, character: 1, documentation: "or write node" }, - { line: 7, character: 1, documentation: "target write node" }, - { line: 7, character: 10, documentation: "target write node" }, - { line: 9, character: 1, documentation: "write node" }, - { line: 11, character: 1, documentation: "The debug flag" }, + { line: 1, documentation: "and write node" }, + { line: 3, documentation: "operator write node" }, + { line: 5, documentation: "or write node" }, + { line: 7, documentation: "target write node" }, + { line: 9, documentation: "write node" }, + { line: 11, documentation: "The debug flag" }, ] with_server(source) do |server, uri| @@ -104,20 +103,16 @@ def test_hovering_for_global_variables def test_hover_apply_target_correction source = <<~RUBY - # and write node $bar &&= 1 - # operator write node $baz += 1 - # or write node $qux ||= 1 - # write node $foo = 1 RUBY - source_line_nodes = [1, 3, 5, 7] + lines_with_target_correction = [1, 2, 3, 4] with_server(source) do |server, uri| - source_line_nodes.each do |line| + lines_with_target_correction.each do |line| server.process_message( id: 1, method: "textDocument/hover", From a5f2253c9fdaa1f86efe3a00445eb3b9b9a1b741 Mon Sep 17 00:00:00 2001 From: snutij Date: Wed, 16 Oct 2024 09:25:14 +0200 Subject: [PATCH 5/5] chore: remove CallNode condition since it is fixed with adding SymbolNode & StringNode to Hover::ALLOWED_TARGET --- lib/ruby_lsp/requests/hover.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/ruby_lsp/requests/hover.rb b/lib/ruby_lsp/requests/hover.rb index a10619dbf..06fe522f9 100644 --- a/lib/ruby_lsp/requests/hover.rb +++ b/lib/ruby_lsp/requests/hover.rb @@ -98,9 +98,6 @@ def should_refine_target?(parent, target) 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 - target.name != :require && target.name != :require_relative && - !covers_position?(target.message_loc, position) when Prism::GlobalVariableAndWriteNode, Prism::GlobalVariableOperatorWriteNode, Prism::GlobalVariableOrWriteNode,