From 3f55bbb615a05db0417e26bf92a07368e5afd7b2 Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Fri, 26 Apr 2024 11:58:27 -0400 Subject: [PATCH] Discard method call target if position doesn't cover identifier --- lib/ruby_lsp/requests/definition.rb | 33 +++++++----- lib/ruby_lsp/requests/hover.rb | 18 ++++--- lib/ruby_lsp/requests/request.rb | 14 +++++ test/requests/definition_expectations_test.rb | 52 +++++++++++++++++++ test/requests/hover_expectations_test.rb | 30 +++++++++++ 5 files changed, 128 insertions(+), 19 deletions(-) diff --git a/lib/ruby_lsp/requests/definition.rb b/lib/ruby_lsp/requests/definition.rb index ff06d0ec2..a3b014a0b 100644 --- a/lib/ruby_lsp/requests/definition.rb +++ b/lib/ruby_lsp/requests/definition.rb @@ -43,6 +43,7 @@ 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, @@ -50,33 +51,41 @@ def initialize(document, global_state, position, dispatcher, typechecker_enabled ) 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 diff --git a/lib/ruby_lsp/requests/hover.rb b/lib/ruby_lsp/requests/hover.rb index 95b170a16..e75f2f804 100644 --- a/lib/ruby_lsp/requests/hover.rb +++ b/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) diff --git a/lib/ruby_lsp/requests/request.rb b/lib/ruby_lsp/requests/request.rb index 903763a15..b4c8b62c3 100644 --- a/lib/ruby_lsp/requests/request.rb +++ b/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 diff --git a/test/requests/definition_expectations_test.rb b/test/requests/definition_expectations_test.rb index 329658880..36ba75a34 100644 --- a/test/requests/definition_expectations_test.rb +++ b/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 diff --git a/test/requests/hover_expectations_test.rb b/test/requests/hover_expectations_test.rb index 0a24311da..c0baf1050 100644 --- a/test/requests/hover_expectations_test.rb +++ b/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