fix(lsp): Improve lsp behaviour on goto definition #1893
Conversation
| | None => | ||
| if (depth == 0 && position.character > 0) { | ||
| find_definition( | ||
| ~depth=1, |
There was a problem hiding this comment.
You are using ~depth with an integer that is only ever 1 or 0. That seems feels like it means it should be bool but that is probably not clear enough to explain the purpose so maybe you should make variants for the states this function can handle.
There was a problem hiding this comment.
I fixed this by using a better variable name and adding a comment. as I didnt feel that a variant provided enough context as to what we were doing.
There was a problem hiding this comment.
Reading your comment helps, but I think this should be variants that wrap the Position.position and probably called Forwards and Backwards or something. Bool flags are almost always bad design.
There was a problem hiding this comment.
I switched to a variant, named Forwards, Backwards
| if (check_position == Forward && position.character > 0) { | ||
| // If a user selects from left to right, their pointer ends up after the identifier | ||
| // this trys to check if the identifier was selected. | ||
| find_definition( | ||
| ~check_position=Backward, | ||
| sourcetree, | ||
| {line: position.line, character: position.character - 1}, | ||
| ); |
There was a problem hiding this comment.
I wonder if this is actually just a "bug" in the sourcetree, as such the sourcetree seems to be position exclusive when it probably should be location inclusive. Is this a correct assessment?
There was a problem hiding this comment.
I don't think so the issue really seems to be from highlighting and where your cursor is. we do not know which way the user is highlighting left to right or right to left. as such we don't know where the cursor will be.
There was a problem hiding this comment.
The sourcetree is supposed to provide a correct node, no matter the position of the cursor. Whether the cursor is like: |foobar or foobar| or foo|bar or anything. And it seems that it doesn't.
There was a problem hiding this comment.
The sourcetree is supposed to provide a correct node, no matter the position of the cursor. Whether the cursor is like:
|foobarorfoobar|orfoo|baror anything. And it seems that it doesn't.
Digging into this a bit more If we want to fix this in sourceTree then I think we should just add 1 to pos_end.charater in loc_to_interval.
I still think this is an issue in the lsp spec. mainly because of the case of foo|+1 with the cursor location being represented by | its ambigous if the user is selecting over foo from left to right or selecting + from right to left.
Note the ambiguity here really comes from the fact that the user is selecting over a range.
There was a problem hiding this comment.
@ospencer does Jake's thoughts above sound correct here?
There was a problem hiding this comment.
If the LSP spec doesn't say that it's in the range, then I think we should close this PR.
There was a problem hiding this comment.
It's up to our interpretation. The editor will send the request with whatever the cursor position is. We don't consider a cursor after something as overlapping that range. That's the correct behavior, but in the case Jake is trying to address—highlighting an identifier from the beginning to the end—your cursor ends up after, and the goto fails.
We have to decide if we care. I think it's reasonable enough to try to improve the user experience because I imagine some users do this. I don't think we should change the sourcetree though, because it's correct.
There was a problem hiding this comment.
I agree about not changing the sourcetree. For context as to when this was a problem for me. I was double clicking on an identifier to select it and then clicking goto definition which I feel is common behavior.
There was a problem hiding this comment.
We don't consider a cursor after something as overlapping that range.
If we don't consider the cursor in the range, then one of our tools shouldn't either.
There was a problem hiding this comment.
I agree with the statement and when considering the cursor position I agree with the viewpoint. The issue in this case isn't the cursor position though it is the selection range. And the fact that when you double click on an identifier to select it, it puts the cursor at the end. Which seems like reasonable user behaviour.
Co-authored-by: Oscar Spencer <oscar.spen@gmail.com>
spotandjake
force-pushed
the
spotandjake-definition-fix
branch
from
c1d52ad
to
e58c390
Compare
The previous behaviour of the lsp with goto definition failed when you selected a variable from
left to right, we now try and account for this by trying to find again, but one char over if we fail the first check.