Skip to content

Commit

Permalink
Discard method call target if position doesn't cover identifier
Browse files Browse the repository at this point in the history
  • Loading branch information
vinistock committed Apr 26, 2024
1 parent 9b31275 commit acb8ff0
Show file tree
Hide file tree
Showing 5 changed files with 105 additions and 19 deletions.
33 changes: 21 additions & 12 deletions lib/ruby_lsp/requests/definition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 11 additions & 7 deletions lib/ruby_lsp/requests/hover.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
16 changes: 16 additions & 0 deletions lib/ruby_lsp/requests/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,22 @@ 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

start_covered = start_line < position[:line] ||
(start_line == position[:line] && location.start_column <= position[:character])
end_covered = end_line > position[:line] ||
(end_line == position[:line] && location.end_column >= position[:character])

start_covered && end_covered
end
end
end
end
27 changes: 27 additions & 0 deletions test/requests/definition_expectations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,33 @@ 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

private

def create_definition_addon
Expand Down
30 changes: 30 additions & 0 deletions test/requests/hover_expectations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit acb8ff0

Please sign in to comment.