Shopify / ruby-lsp

An opinionated language server for Ruby
https://shopify.github.io/ruby-lsp/
MIT License
1.33k stars 118 forks source link

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

Closed vinistock closed 2 weeks ago

vinistock commented 2 weeks ago

Motivation

With the current experience, clicking to go to the definition of a method (or hovering) over any part of the CallNode will always succeed. This is not what we want, since people should be able to go to definition or hover separately for method arguments.

It also leads to incorrect behaviour. For example:

[].each(&:foo)
#         ^ go to definition here is currently taking you to `each`, which is not correct

Implementation

We still need to locate the method calls based on the entire node, but if we're processing a method call that has no special handling (i.e.: not requires), then we need to ensure that the position being clicked is covered by method's identifier.

Otherwise, it means the user is clicking on the arguments node or on the receiver (neither of which should take you to the method declaration).

Automated Tests

Added tests demonstrating the behaviour.

paracycle commented 2 weeks ago

I would like to see how this behaves with clicking foo in the example below, though:

bar(a: foo, 42)

It would be smart to add a test case to make sure that we still jump to foo and not ignore it because it was in the arguments to bar.

vinistock commented 2 weeks ago

I would like to see how this behaves with clicking foo in the example below, though:

Added a test to ensure we don't regress, but that already worked from the implementation of locate_node.