Partially migrate hover to use Rubydex

Author: vinistock

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

lib/ruby_lsp/requests/support/common.rb

@@ -64,6 +64,44 @@ 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
+
+          additional_entries_text = if max_entries && defs.length > max_entries
+            additional = defs.length - 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```"

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"
         }
     }
 }

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,25 +83,21 @@ 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 = [
       { 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" },
+      { 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,18 +267,18 @@ 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,
         method: "textDocument/hover",
         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