diff --git a/lib/ruby_lsp/listeners/hover.rb b/lib/ruby_lsp/listeners/hover.rb index b06e118052..8aadae7c44 100644 --- a/lib/ruby_lsp/listeners/hover.rb +++ b/lib/ruby_lsp/listeners/hover.rb @@ -48,6 +48,7 @@ def initialize(response_builder, global_state, uri, node_context, dispatcher, so @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 @path = uri.to_standardized_path #: String? @node_context = node_context @@ -178,32 +179,32 @@ def on_global_variable_write_node_enter(node) #: (Prism::InstanceVariableReadNode node) -> void def on_instance_variable_read_node_enter(node) - handle_instance_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::InstanceVariableWriteNode node) -> void def on_instance_variable_write_node_enter(node) - handle_instance_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::InstanceVariableAndWriteNode node) -> void def on_instance_variable_and_write_node_enter(node) - handle_instance_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::InstanceVariableOperatorWriteNode node) -> void def on_instance_variable_operator_write_node_enter(node) - handle_instance_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::InstanceVariableOrWriteNode node) -> void def on_instance_variable_or_write_node_enter(node) - handle_instance_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::InstanceVariableTargetNode node) -> void def on_instance_variable_target_node_enter(node) - handle_instance_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::SuperNode node) -> void @@ -223,32 +224,32 @@ def on_yield_node_enter(node) #: (Prism::ClassVariableAndWriteNode node) -> void def on_class_variable_and_write_node_enter(node) - handle_class_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::ClassVariableOperatorWriteNode node) -> void def on_class_variable_operator_write_node_enter(node) - handle_class_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::ClassVariableOrWriteNode node) -> void def on_class_variable_or_write_node_enter(node) - handle_class_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::ClassVariableTargetNode node) -> void def on_class_variable_target_node_enter(node) - handle_class_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::ClassVariableReadNode node) -> void def on_class_variable_read_node_enter(node) - handle_class_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end #: (Prism::ClassVariableWriteNode node) -> void def on_class_variable_write_node_enter(node) - handle_class_variable_hover(node.name.to_s) + handle_variable_hover(node.name.to_s) end private @@ -324,62 +325,46 @@ def handle_method_hover(message, inherited_only: false) end end - #: (String name) -> void - def handle_instance_variable_hover(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 - 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 - - categorized_markdown_from_index_entries(name, entries).each do |category, content| - @response_builder.push(content, category: category) - 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_global_variable_hover(name) - entries = @index[name] - return unless entries + declaration = @graph[name] + return unless declaration - categorized_markdown_from_index_entries(name, entries).each do |category, content| + categorized_markdown_from_definitions(name, declaration.definitions).each do |category, content| @response_builder.push(content, category: category) end end + # Handle class or instance variables. We collect all definitions across the ancestors of the type + # #: (String name) -> void - def handle_class_variable_hover(name) + def handle_variable_hover(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_class_variable(name, type.name) - return unless entries + owner = @graph[type.name] + return unless owner.is_a?(Rubydex::Namespace) - categorized_markdown_from_index_entries(name, entries).each do |category, content| - @response_builder.push(content, category: category) + owner.ancestors.each do |ancestor| + member = ancestor.member(name) + next unless member + + categorized_markdown_from_definitions(member.name, member.definitions).each do |category, content| + @response_builder.push(content, category: category) + end 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, Prism::Location location) -> void def generate_hover(name, location) - entries = @index.resolve(name, @node_context.nesting) - return unless entries - - # We should only show hover for private constants if the constant is defined in the same namespace as the - # reference - first_entry = entries.first #: as !nil - full_name = first_entry.name - return if first_entry.private? && full_name != "#{@node_context.fully_qualified_name}::#{name}" + declaration = @graph.resolve_constant(name, @node_context.nesting) + return unless declaration - categorized_markdown_from_index_entries(full_name, entries).each do |category, content| + categorized_markdown_from_definitions(declaration.name, declaration.definitions).each do |category, content| @response_builder.push(content, category: category) end end diff --git a/lib/ruby_lsp/requests/support/common.rb b/lib/ruby_lsp/requests/support/common.rb index 010dd38509..8c391711cc 100644 --- a/lib/ruby_lsp/requests/support/common.rb +++ b/lib/ruby_lsp/requests/support/common.rb @@ -64,6 +64,46 @@ def self_receiver?(node) receiver.nil? || receiver.is_a?(Prism::SelfNode) end + #: (String, Enumerable[Rubydex::Definition], ?Integer?) -> Hash[Symbol, String] + def categorized_markdown_from_definitions(title, definitions, max_entries = nil) + markdown_title = "```ruby\n#{title}\n```" + file_links = [] + content = +"" + defs = max_entries ? definitions.take(max_entries) : definitions + defs.each do |definition| + # For Markdown links, we need 1 based display locations + loc = definition.location.to_display + uri = URI(loc.uri) + file_name = if uri.scheme == "untitled" + uri.opaque #: as !nil + else + File.basename( + uri.full_path, #: as !nil + ) + end + + # The format for VS Code file URIs is `file:///path/to/file.rb#Lstart_line,start_column-end_line,end_column` + string_uri = "#{loc.uri}#L#{loc.start_line},#{loc.start_column}-#{loc.end_line},#{loc.end_column}" + file_links << "[#{file_name}](#{string_uri})" + content << "\n\n#{definition.comments.map { |comment| comment.string.delete_prefix("# ") }.join("\n")}" unless definition.comments.empty? + end + + total_definitions = definitions.count + + additional_entries_text = if max_entries && total_definitions > max_entries + additional = total_definitions - max_entries + " | #{additional} other#{additional > 1 ? "s" : ""}" + else + "" + end + + { + title: markdown_title, + links: "**Definitions**: #{file_links.join(" | ")}#{additional_entries_text}", + documentation: content, + } + end + #: (String title, (Array[RubyIndexer::Entry] | RubyIndexer::Entry) entries, ?Integer? max_entries) -> Hash[Symbol, String] def categorized_markdown_from_index_entries(title, entries, max_entries = nil) markdown_title = "```ruby\n#{title}\n```" diff --git a/test/expectations/hover/documented_constant.exp.json b/test/expectations/hover/documented_constant.exp.json index cbbe5b37fe..d9483c231c 100644 --- a/test/expectations/hover/documented_constant.exp.json +++ b/test/expectations/hover/documented_constant.exp.json @@ -8,7 +8,7 @@ "result": { "contents": { "kind": "markdown", - "value": "```ruby\nBAZ\n```\n\n**Definitions**: [fake.rb](file:///fake.rb#L2,1-2,10)\n\n\n\nThis is the documentation for Baz" + "value": "```ruby\nBAZ\n```\n\n**Definitions**: [fake.rb](file:///fake.rb#L2,1-2,4)\n\n\n\nThis is the documentation for Baz" } } } diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index aff6fd9d49..511bfa4d32 100644 --- a/test/requests/hover_expectations_test.rb +++ b/test/requests/hover_expectations_test.rb @@ -51,18 +51,25 @@ class A def test_hovering_on_erb source = <<~ERB - <% String %> + <% Person %> ERB with_server(source, Kernel.URI("file:///fake.erb"), stub_no_typechecker: true) do |server, uri| - RubyIndexer::RBSIndexer.new(server.global_state.index).index_ruby_core + graph = server.global_state.graph + graph.index_source(URI::Generic.from_path(path: "/person.rb").to_s, <<~RUBY, "ruby") + # Hello from person.rb + class Person + end + RUBY + graph.resolve + server.process_message( id: 1, method: "textDocument/hover", params: { textDocument: { uri: uri }, position: { line: 0, character: 4 } }, ) response = server.pop_response - assert_match(/String\b/, response.response.contents.value) + assert_match(/Hello from person\.rb/, response.response.contents.value) end end @@ -76,10 +83,9 @@ def test_hovering_for_global_variables $qux ||= 1 # target write node $quux, $corge = 1 - # write node + # foo docs $foo = 1 - # read node - $DEBUG + $foo RUBY expectations = [ @@ -87,14 +93,11 @@ def test_hovering_for_global_variables { 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" }, + { line: 9, documentation: "foo docs" }, + { line: 10, documentation: "foo docs" }, ] 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, @@ -264,10 +267,9 @@ class A private_constant(:CONST) end - A::CONST # invalid private reference + A::CONST RUBY - # We need to pretend that Sorbet is not a dependency or else we can't properly test with_server(source, stub_no_typechecker: true) do |server, uri| server.process_message( id: 1, @@ -275,7 +277,8 @@ class A params: { textDocument: { uri: uri }, position: { character: 3, line: 5 } }, ) - assert_nil(server.pop_response.response) + # TODO: once we have visibility exposed from Rubydex, let's show that the constant is private + assert_match("A::CONST", server.pop_response.response.contents.value) end end