From 5ec5b4b76ffe96365ae8b0b12bdbd8763fb0c5f4 Mon Sep 17 00:00:00 2001 From: Justin Rabiller <38727166+snutij@users.noreply.github.com> Date: Wed, 16 Oct 2024 16:27:37 +0200 Subject: [PATCH] Add hover for global variables (#2691) * feat: add hover for global variables * feat: add hover for all global variable nodes * feat: apply hover target correction on global variable * test: remove useless test setup * chore: remove CallNode condition since it is fixed with adding SymbolNode & StringNode to Hover::ALLOWED_TARGET --- lib/ruby_lsp/listeners/hover.rb | 52 ++++++++++++++++++ lib/ruby_lsp/requests/hover.rb | 30 ++++++++--- test/requests/hover_expectations_test.rb | 69 +++++++++++++++++++++++- 3 files changed, 144 insertions(+), 7 deletions(-) diff --git a/lib/ruby_lsp/listeners/hover.rb b/lib/ruby_lsp/listeners/hover.rb index ecfda3a7e..5d1220f57 100644 --- a/lib/ruby_lsp/listeners/hover.rb +++ b/lib/ruby_lsp/listeners/hover.rb @@ -13,6 +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, @@ -62,6 +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, @@ -128,6 +140,36 @@ 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) + handle_global_variable_hover(node.name.to_s) + end + + sig { params(node: Prism::GlobalVariableTargetNode).void } + def on_global_variable_target_node_enter(node) + handle_global_variable_hover(node.name.to_s) + 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 } def on_instance_variable_read_node_enter(node) handle_instance_variable_hover(node.name.to_s) @@ -265,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/lib/ruby_lsp/requests/hover.rb b/lib/ruby_lsp/requests/hover.rb index a675f91be..06fe522f9 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,28 @@ 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::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 a5f10b9df..b42224a80 100644 --- a/test/requests/hover_expectations_test.rb +++ b/test/requests/hover_expectations_test.rb @@ -60,6 +60,73 @@ def test_hovering_on_erb end end + 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, 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| + index = server.instance_variable_get(:@global_state).index + RubyIndexer::RBSIndexer.new(index).index_ruby_core + + expectations.each do |expectation| + server.process_message( + id: 1, + method: "textDocument/hover", + params: { textDocument: { uri: uri }, position: { line: expectation[:line], character: 1 } }, + ) + + assert_match(expectation[:documentation], server.pop_response.response.contents.value) + end + end + end + + def test_hover_apply_target_correction + source = <<~RUBY + $bar &&= 1 + $baz += 1 + $qux ||= 1 + $foo = 1 + RUBY + + lines_with_target_correction = [1, 2, 3, 4] + + with_server(source) do |server, uri| + lines_with_target_correction.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 @@ -310,13 +377,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(