Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Apr 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 deletions lib/ruby_lsp/requests/definition.rb
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) could this be an early return?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be honest, I'm not a fan of early returns in the initialize method. If you feel strongly about it, I can change, but I wouldn't normally.

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
View file
Open in desktop
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
14 changes: 14 additions & 0 deletions lib/ruby_lsp/requests/request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like an API that Prism::Location can provide 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kddnewton how would you feel about that? It would mostly be for line + column information since when dealing offsets, checking if an index covers is quite trivial.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think there might be a couple of objects hiding here. The fact that a raw hash is being passed around here makes me really uncomfortable. I think the following objects would be useful instead:

Prism::Position::ByteOffset[:offset, :byte_length]
Prism::Position::LineByteColumn[:start_line, :start_byte_column, :end_line, :end_byte_column]
Prism::Position::LineCharacterColumn[:start_line, :start_character_column, :end_line, :end_character_column]
Prism::Position::LineCodeUnitColumn[:start_line, :start_code_unit_column, :end_line, :end_code_unit_column]

with APIs that create those various positions from a location and can compare against those positions for things like covers?. Unless we're explicit about which kind of column counts we're using, I wouldn't want to make an assumption (because people get burned by this too often with ripper).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, indeed it needs to account for all possible ways that you can refer to a location.

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
52 changes: 52 additions & 0 deletions test/requests/definition_expectations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 30 additions & 0 deletions test/requests/hover_expectations_test.rb
View file
Open in desktop
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
Loading