Discard method call target if position doesn't cover identifier (#1981)

Author: vinistock

lib/ruby_lsp/requests/definition.rb

@@ -43,40 +43,49 @@ def initialize(document, global_state, position, dispatcher, typechecker_enabled
           ResponseBuilders::CollectionResponseBuilder[Interface::Location].new,
           ResponseBuilders::CollectionResponseBuilder[Interface::Location],
         )
+        @dispatcher = dispatcher
 
         target, parent, nesting = document.locate_node(
           position,
           node_types: [Prism::CallNode, Prism::ConstantReadNode, Prism::ConstantPathNode],
         )
 
         if target.is_a?(Prism::ConstantReadNode) && parent.is_a?(Prism::ConstantPathNode)
+          # If the target is part of a constant path node, we need to find the exact portion of the constant that the
+          # user is requesting to go to definition for
           target = determine_target(
             target,
             parent,
             position,
           )
+        elsif target.is_a?(Prism::CallNode) && target.name != :require && target.name != :require_relative &&
+            !covers_position?(target.message_loc, position)
+          # If the target is a method call, we need to ensure that the requested position is exactly on top of the
+          # method identifier. Otherwise, we risk showing definitions for unrelated things
+          target = nil
         end
 
-        Listeners::Definition.new(
-          @response_builder,
-          global_state,
-          document.uri,
-          nesting,
-          dispatcher,
-          typechecker_enabled,
-        )
+        if target
+          Listeners::Definition.new(
+            @response_builder,
+            global_state,
+            document.uri,
+            nesting,
+            dispatcher,
+            typechecker_enabled,
+          )
 
-        Addon.addons.each do |addon|
-          addon.create_definition_listener(@response_builder, document.uri, nesting, dispatcher)
+          Addon.addons.each do |addon|
+            addon.create_definition_listener(@response_builder, document.uri, nesting, dispatcher)
+          end
         end
 
         @target = T.let(target, T.nilable(Prism::Node))
-        @dispatcher = dispatcher
       end
 
       sig { override.returns(T::Array[Interface::Location]) }
       def perform
-        @dispatcher.dispatch_once(@target)
+        @dispatcher.dispatch_once(@target) if @target
         @response_builder.response
       end
     end

lib/ruby_lsp/requests/hover.rb

@@ -41,25 +41,29 @@ def provider
       end
       def initialize(document, global_state, position, dispatcher, typechecker_enabled)
         super()
-        @target = T.let(nil, T.nilable(Prism::Node))
-        @target, parent, nesting = document.locate_node(
+        target, parent, nesting = document.locate_node(
           position,
           node_types: Listeners::Hover::ALLOWED_TARGETS,
         )
 
         if (Listeners::Hover::ALLOWED_TARGETS.include?(parent.class) &&
-            !Listeners::Hover::ALLOWED_TARGETS.include?(@target.class)) ||
-            (parent.is_a?(Prism::ConstantPathNode) && @target.is_a?(Prism::ConstantReadNode))
-          @target = determine_target(
-            T.must(@target),
+            !Listeners::Hover::ALLOWED_TARGETS.include?(target.class)) ||
+            (parent.is_a?(Prism::ConstantPathNode) && target.is_a?(Prism::ConstantReadNode))
+          target = determine_target(
+            T.must(target),
             T.must(parent),
             position,
           )
+        elsif target.is_a?(Prism::CallNode) && target.name != :require && target.name != :require_relative &&
+            !covers_position?(target.message_loc, position)
+
+          target = nil
         end
 
         # Don't need to instantiate any listeners if there's no target
-        return unless @target
+        return unless target
 
+        @target = T.let(target, T.nilable(Prism::Node))
         uri = document.uri
         @response_builder = T.let(ResponseBuilders::Hover.new, ResponseBuilders::Hover)
         Listeners::Hover.new(@response_builder, global_state, uri, nesting, dispatcher, typechecker_enabled)

lib/ruby_lsp/requests/request.rb

@@ -65,6 +65,20 @@ def determine_target(target, parent, position)
 
         target
       end
+
+      # Checks if a given location covers the position requested
+      sig { params(location: T.nilable(Prism::Location), position: T::Hash[Symbol, T.untyped]).returns(T::Boolean) }
+      def covers_position?(location, position)
+        return false unless location
+
+        start_line = location.start_line - 1
+        end_line = location.end_line - 1
+        line = position[:line]
+        character = position[:character]
+
+        (start_line < line || (start_line == line && location.start_column <= character)) &&
+          (end_line > line || (end_line == line && location.end_column >= character))
+      end
     end
   end
 end

test/requests/definition_expectations_test.rb

@@ -403,6 +403,58 @@ def foo; end
     end
   end
 
+  def test_definition_precision_for_methods_with_block_arguments
+    source = <<~RUBY
+      class Foo
+        def foo(&block); end
+      end
+
+      bar.foo(&:argument)
+    RUBY
+
+    # Going to definition on `argument` should not take you to the `foo` method definition
+    with_server(source) do |server, uri|
+      server.process_message(
+        id: 1,
+        method: "textDocument/definition",
+        params: { textDocument: { uri: uri }, position: { character: 12, line: 4 } },
+      )
+      assert_empty(server.pop_response.response)
+
+      server.process_message(
+        id: 1,
+        method: "textDocument/definition",
+        params: { textDocument: { uri: uri }, position: { character: 4, line: 4 } },
+      )
+      refute_empty(server.pop_response.response)
+    end
+  end
+
+  def test_definition_for_method_call_inside_arguments
+    source = <<~RUBY
+      class Foo
+        def foo; end
+
+        def bar(a:, b:); end
+
+        def baz
+          bar(a: foo, b: 42)
+        end
+      end
+    RUBY
+
+    with_server(source) do |server, uri|
+      server.process_message(
+        id: 1,
+        method: "textDocument/definition",
+        params: { textDocument: { uri: uri }, position: { character: 11, line: 6 } },
+      )
+      response = server.pop_response.response.first
+      assert_equal(1, response.range.start.line)
+      assert_equal(1, response.range.end.line)
+    end
+  end
+
   private
 
   def create_definition_addon

test/requests/hover_expectations_test.rb

@@ -281,6 +281,36 @@ class Post
     end
   end
 
+  def test_hover_precision_for_methods_with_block_arguments
+    source = <<~RUBY
+      class Foo
+        # Hello
+        def foo(&block); end
+
+        def bar
+          foo(&:argument)
+        end
+      end
+    RUBY
+
+    # Going to definition on `argument` should not take you to the `foo` method definition
+    with_server(source) do |server, uri|
+      server.process_message(
+        id: 1,
+        method: "textDocument/hover",
+        params: { textDocument: { uri: uri }, position: { character: 12, line: 5 } },
+      )
+      assert_nil(server.pop_response.response)
+
+      server.process_message(
+        id: 1,
+        method: "textDocument/hover",
+        params: { textDocument: { uri: uri }, position: { character: 4, line: 5 } },
+      )
+      assert_match("Hello", server.pop_response.response.contents.value)
+    end
+  end
+
   private
 
   def create_hover_addon