Handle visibility in go to definition

Author: vinistock

lib/ruby_lsp/listeners/definition.rb

@@ -302,6 +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)
+
           decl.definitions.each do |definition|
             location = definition.location
             uri = URI(location.uri)
@@ -317,6 +319,31 @@ 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
@@ -368,12 +395,13 @@ def handle_constant_definition(value)
         declaration = @graph.resolve_constant(value, @node_context.nesting)
         return unless declaration
 
-        # [RUBYDEX] TODO: temporarily commented out until we have the visibility API
-        #
-        # We should only allow jumping to the definition of private constants if the constant is defined in the same
-        # namespace as the reference
-        #
-        # return if declaration.private? && declaration.name != "#{@node_context.fully_qualified_name}::#{value}"
+        # 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
 
         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

test/requests/definition_expectations_test.rb

@@ -180,8 +180,6 @@ class A
   end
 
   def test_jumping_to_private_constant_from_different_namespace
-    skip("[RUBYDEX] Requires the visibility API")
-
     source = <<~RUBY
       class A
         CONST = 123
@@ -201,6 +199,136 @@ class A
     end
   end
 
+  def test_jumping_to_private_method_with_implicit_self
+    source = <<~RUBY
+      class A
+        def bar
+          foo
+        end
+
+        private
+
+        def foo; end
+      end
+    RUBY
+
+    with_server(source, stub_no_typechecker: true) do |server, uri|
+      server.process_message(
+        id: 1,
+        method: "textDocument/definition",
+        params: { textDocument: { uri: uri }, position: { character: 4, line: 2 } },
+      )
+      response = server.pop_response.response
+      refute_empty(response)
+      assert_equal(uri.to_s, response.first.attributes[:targetUri])
+    end
+  end
+
+  def test_does_not_jump_to_private_method_called_with_explicit_external_receiver
+    source = <<~RUBY
+      class A
+        private
+
+        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/definition",
+        params: { textDocument: { uri: uri }, position: { character: 6, line: 9 } },
+      )
+      assert_empty(server.pop_response.response)
+    end
+  end
+
+  def test_jumps_to_protected_method_inside_same_class
+    source = <<~RUBY
+      class A
+        def bar
+          self.foo
+        end
+
+        protected
+
+        def foo; end
+      end
+    RUBY
+
+    with_server(source, stub_no_typechecker: true) do |server, uri|
+      server.process_message(
+        id: 1,
+        method: "textDocument/definition",
+        params: { textDocument: { uri: uri }, position: { character: 9, line: 2 } },
+      )
+      response = server.pop_response.response
+      refute_empty(response)
+      assert_equal(uri.to_s, response.first.attributes[:targetUri])
+    end
+  end
+
+  def test_jumps_to_protected_method_called_from_subclass
+    source = <<~RUBY
+      class A
+        protected
+
+        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/definition",
+        params: { textDocument: { uri: uri }, position: { character: 6, line: 9 } },
+      )
+      response = server.pop_response.response
+      refute_empty(response)
+      assert_equal(uri.to_s, response.first.attributes[:targetUri])
+    end
+  end
+
+  def test_does_not_jump_to_protected_method_from_unrelated_class
+    source = <<~RUBY
+      class A
+        protected
+
+        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/definition",
+        params: { textDocument: { uri: uri }, position: { character: 6, line: 9 } },
+      )
+      assert_empty(server.pop_response.response)
+    end
+  end
+
   def test_definition_addons
     source = <<~RUBY
       class Target