Handle visibility in hover

Author: vinistock

lib/ruby_lsp/listeners/definition.rb

@@ -302,7 +302,8 @@ def handle_method_definition(message, receiver_type, inherited_only: false)
         return unless declaration
 
         Array(declaration).each do |decl|
-          next unless reachable_from_call_site?(decl, receiver_type)
+          next if decl.is_a?(Rubydex::Method) &&
+            !method_reachable_from_call_site?(decl, receiver_type, @graph, @node_context)
 
           decl.definitions.each do |definition|
             location = definition.location
@@ -319,31 +320,6 @@ def handle_method_definition(message, receiver_type, inherited_only: false)
         end
       end
 
-      # A method is reachable from the call site when:
-      # - it is public, or there's no concrete receiver type to compare against
-      # - the call site is inside the receiver's own namespace (implicit/self call)
-      # - it is protected and the call site's class is in the same hierarchy as the method's defining class
-      #
-      #: (Rubydex::Declaration, TypeInferrer::Type?) -> bool
-      def reachable_from_call_site?(decl, receiver_type)
-        return true unless receiver_type
-        return true unless decl.is_a?(Rubydex::Method)
-
-        caller_namespace = @node_context.fully_qualified_name
-        return true if caller_namespace == receiver_type.name
-
-        return true if decl.public?
-        return false if decl.private?
-
-        owner = decl.owner
-        return false unless owner.is_a?(Rubydex::Namespace)
-
-        caller_declaration = @graph[caller_namespace]
-        return false unless caller_declaration.is_a?(Rubydex::Namespace)
-
-        caller_declaration.ancestors.any? { |ancestor| ancestor.name == owner.name }
-      end
-
       #: (Prism::StringNode node, Symbol message) -> void
       def handle_require_definition(node, message)
         case message
@@ -394,14 +370,7 @@ def handle_autoload_definition(node)
       def handle_constant_definition(value)
         declaration = @graph.resolve_constant(value, @node_context.nesting)
         return unless declaration
-
-        # Only allow jumping to the definition of a private constant if the constant is defined in the same namespace
-        # as the reference. Not every declaration kind exposes visibility (e.g. unresolved namespaces), so we gate on
-        # the Visibility module
-        if declaration.is_a?(Rubydex::Visibility) && declaration.private? &&
-            declaration.name != "#{@node_context.fully_qualified_name}::#{value}"
-          return
-        end
+        return unless constant_reachable_from_call_site?(declaration, value, @node_context)
 
         declaration.definitions.each do |definition|
           # If the project has Sorbet, then we only want to handle go to definition for constants defined in gems, as an

lib/ruby_lsp/listeners/hover.rb

@@ -461,6 +461,7 @@ def handle_method_hover(message, inherited_only: false)
         return unless methods
 
         first_method = methods.first #: as !nil
+        return unless method_reachable_from_call_site?(first_method, type, @graph, @node_context)
 
         title = "#{message}#{first_method.decorated_parameters}"
         title << first_method.formatted_signatures
@@ -513,6 +514,7 @@ def handle_variable_hover(name)
       def generate_hover(name, location)
         declaration = @graph.resolve_constant(name, @node_context.nesting)
         return unless declaration
+        return unless constant_reachable_from_call_site?(declaration, name, @node_context)
 
         categorized_markdown_from_definitions(declaration.name, declaration.definitions).each do |category, content|
           @response_builder.push(content, category: category)

lib/ruby_lsp/requests/support/common.rb

@@ -77,6 +77,46 @@ def self_receiver?(node)
           receiver.nil? || receiver.is_a?(Prism::SelfNode)
         end
 
+        # Returns true when a constant declaration is reachable from the call site. Private constants are only
+        # reachable from within the namespace where they are defined.
+        #
+        #: (Rubydex::Declaration declaration, String value, NodeContext node_context) -> bool
+        def constant_reachable_from_call_site?(declaration, value, node_context)
+          return true unless declaration.is_a?(Rubydex::Visibility) && declaration.private?
+
+          declaration.name == "#{node_context.fully_qualified_name}::#{value}"
+        end
+
+        # Returns true when a method is reachable from the call site, considering visibility and receiver type.
+        # A method is reachable when:
+        # - there's no concrete receiver type to compare against
+        # - the call site is inside the receiver's own namespace (implicit/self call)
+        # - it is public
+        # - it is protected and the call site's class is in the same hierarchy as the method's defining class
+        #
+        # The `method_decl` is duck-typed to support `Rubydex::Method`, `RubyIndexer::Entry::Member` and
+        # `RubyIndexer::Entry::MethodAlias`. All respond to `public?`, `private?` and `owner` (an object with a
+        # `name` attribute).
+        #
+        #: ((Rubydex::Method | RubyIndexer::Entry::Member | RubyIndexer::Entry::MethodAlias) method_decl, TypeInferrer::Type? receiver_type, Rubydex::Graph graph, NodeContext node_context) -> bool
+        def method_reachable_from_call_site?(method_decl, receiver_type, graph, node_context)
+          return true unless receiver_type
+
+          caller_namespace = node_context.fully_qualified_name
+          return true if caller_namespace == receiver_type.name
+
+          return true if method_decl.public?
+          return false if method_decl.private?
+
+          owner_name = method_decl.owner&.name
+          return false unless owner_name
+
+          caller_declaration = graph[caller_namespace]
+          return false unless caller_declaration.is_a?(Rubydex::Namespace)
+
+          caller_declaration.ancestors.any? { |ancestor| ancestor.name == owner_name }
+        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```"

test/requests/hover_expectations_test.rb

@@ -267,7 +267,7 @@ class A
         private_constant(:CONST)
       end
 
-      A::CONST
+      A::CONST # invalid private reference
     RUBY
 
     with_server(source, stub_no_typechecker: true) do |server, uri|
@@ -277,8 +277,141 @@ class A
         params: { textDocument: { uri: uri }, position: { character: 3, line: 5 } },
       )
 
-      # 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)
+      assert_nil(server.pop_response.response)
+    end
+  end
+
+  def test_hovering_over_private_method_with_implicit_self
+    source = <<~RUBY
+      class A
+        def bar
+          foo
+        end
+
+        private
+
+        # foo docs
+        def foo; end
+      end
+    RUBY
+
+    with_server(source, stub_no_typechecker: true) do |server, uri|
+      server.process_message(
+        id: 1,
+        method: "textDocument/hover",
+        params: { textDocument: { uri: uri }, position: { character: 4, line: 2 } },
+      )
+
+      assert_match("foo docs", server.pop_response.response.contents.value)
+    end
+  end
+
+  def test_does_not_hover_over_private_method_called_with_explicit_external_receiver
+    source = <<~RUBY
+      class A
+        private
+
+        # foo docs
+        def foo; end
+      end
+
+      class B
+        def bar
+          a = A.new
+          a.foo
+        end
+      end
+    RUBY
+
+    with_server(source, stub_no_typechecker: true) do |server, uri|
+      server.process_message(
+        id: 1,
+        method: "textDocument/hover",
+        params: { textDocument: { uri: uri }, position: { character: 6, line: 10 } },
+      )
+
+      assert_nil(server.pop_response.response)
+    end
+  end
+
+  def test_hovers_protected_method_inside_same_class
+    source = <<~RUBY
+      class A
+        def bar
+          self.foo
+        end
+
+        protected
+
+        # foo docs
+        def foo; end
+      end
+    RUBY
+
+    with_server(source, stub_no_typechecker: true) do |server, uri|
+      server.process_message(
+        id: 1,
+        method: "textDocument/hover",
+        params: { textDocument: { uri: uri }, position: { character: 9, line: 2 } },
+      )
+
+      assert_match("foo docs", server.pop_response.response.contents.value)
+    end
+  end
+
+  def test_hovers_protected_method_called_from_subclass
+    source = <<~RUBY
+      class A
+        protected
+
+        # foo docs
+        def foo; end
+      end
+
+      class B < A
+        def bar
+          a = A.new
+          a.foo
+        end
+      end
+    RUBY
+
+    with_server(source, stub_no_typechecker: true) do |server, uri|
+      server.process_message(
+        id: 1,
+        method: "textDocument/hover",
+        params: { textDocument: { uri: uri }, position: { character: 6, line: 10 } },
+      )
+
+      assert_match("foo docs", server.pop_response.response.contents.value)
+    end
+  end
+
+  def test_does_not_hover_protected_method_from_unrelated_class
+    source = <<~RUBY
+      class A
+        protected
+
+        # foo docs
+        def foo; end
+      end
+
+      class C
+        def bar
+          a = A.new
+          a.foo
+        end
+      end
+    RUBY
+
+    with_server(source, stub_no_typechecker: true) do |server, uri|
+      server.process_message(
+        id: 1,
+        method: "textDocument/hover",
+        params: { textDocument: { uri: uri }, position: { character: 6, line: 10 } },
+      )
+
+      assert_nil(server.pop_response.response)
     end
   end