Discard method call target if position doesn't cover identifier

[vinistock]

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.

[st0012]

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

[vinistock]

@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.

[kddnewton]

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).

[vinistock]

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

[andyw8]

(nit) could this be an early return?

[vinistock]

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.

[paracycle]

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]

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.