From 5dfd9f88506e44b680d7f92f84c713e88d27d07d Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Thu, 7 May 2026 18:46:57 -0400 Subject: [PATCH] Handle visibility in hover --- lib/ruby_lsp/listeners/definition.rb | 37 +----- lib/ruby_lsp/listeners/hover.rb | 2 + lib/ruby_lsp/requests/support/common.rb | 40 +++++++ test/requests/hover_expectations_test.rb | 139 ++++++++++++++++++++++- 4 files changed, 181 insertions(+), 37 deletions(-) diff --git a/lib/ruby_lsp/listeners/definition.rb b/lib/ruby_lsp/listeners/definition.rb index 3cd3c3074..5e2c5fd6d 100644 --- a/lib/ruby_lsp/listeners/definition.rb +++ b/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 diff --git a/lib/ruby_lsp/listeners/hover.rb b/lib/ruby_lsp/listeners/hover.rb index 6e3a49f86..c65bb673b 100644 --- a/lib/ruby_lsp/listeners/hover.rb +++ b/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) diff --git a/lib/ruby_lsp/requests/support/common.rb b/lib/ruby_lsp/requests/support/common.rb index 74edde32f..f719d3f5e 100644 --- a/lib/ruby_lsp/requests/support/common.rb +++ b/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```" diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index d7de2d385..ab1b773d7 100644 --- a/test/requests/hover_expectations_test.rb +++ b/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