Go to definition does not work at end of identifier

Question

Go to definition does not work at end of identifier

lovettchris opened this issue 3 years ago · 14 comments

Prerequisites

Put an X between the brackets on this line if you have done all of the following:
- Checked that your issue isn't already filed.
- Reduced the issue to a self-contained, reproducible test case.

Description

I have a little external math function called factorial and when I do goto definition with the cursor at the end of the word it does not work right.

Steps to Reproduce

double click the word "factorial" to select the whole word
press F12 to goto definition

Expected behavior: [What you expect to happen]
Should work and take my to my factorial function, but it only works if I move the cursor left 1 char so it is "inside" the word "factorial".

Actual behavior: [What actually happens]
Fails to find it.

Reproduces how often: [What percentage of the time does it reproduce?]
100%

Versions

commit 89ab3ca

Additional Information

Any additional information, configuration or data that might be necessary to reproduce the issue.

lovettchris commented 3 years ago

woot!

Answer 1 · 2021-11-05T07:24:52.000Z

This is problematic in the presence of custom notation. Should ⟨factorial|⟩, where | is the cursor position, go to factorial or to the definition of the anonymous constructor? We could make it work only when not followed directly by another token though.

Answer 2 · 2021-11-05T16:39:33.000Z

This is problematic in the presence of custom notation. Should ⟨factorial|⟩, where | is the cursor position, go to factorial or to the definition of the anonymous constructor? We could make it work only when not followed directly by another token though.

@Kha I think this is a good compromise.

Answer 3 · 2021-12-03T15:28:43.000Z

This is problematic in the presence of custom notation. Should ⟨factorial|⟩, where | is the cursor position, go to factorial or to the definition of the anonymous constructor? We could make it work only when not followed directly by another token though.

This is already inconsistent in the current implementation (using VSCode). When typing ⟨factorial|⟩ (with the cursor at the |) VSCode will highlight the word factorial, while the hover at the same position will show the info for ⟨⟩.

Answer 4 · 2021-12-03T15:45:26.000Z

Thanks, good to know. Alternatively we could always prefer identifiers over adjacent tokens.

Answer 5 · 2021-12-03T15:56:22.000Z

For the use case of selecting a word with a double-click, this is a bit weird in VSCode. With ⟨a, b⟩, when double-clicking anywhere in b⟩, both tokens will be selected. Also for identifiers, the cursor will be on the right side of the selection, but in, for example, <|, the cursor will be placed on the left side of the selection.

This makes it difficult to think of a solution that will work in all cases.

Answer 6 · 2021-12-03T16:23:39.000Z

Then maybe the current behavior isn't too bad since at least it is consistent. I don't think I've ever used go-to-definition with a selection to be honest (if your hand is already on the mouse, why not use Ctrl + click?).

Answer 7 · 2021-12-03T16:27:32.000Z

Ah, I forgot about your comment that it is already inconsistent right now in VS Code. I would be okay with any of the comprise solutions or the current behavior at this point.

Answer 8 · 2022-03-20T14:27:06.000Z

@Kha What is the plan for this issue? You made a suggestion above, Lars raised a new concern, then you said "maybe the current behavior isn't too bad". I don't have a preference here, I am happy to close it with "wontfix", or try to fix it.

Answer 9 · 2022-03-20T14:36:36.000Z

I still don't really understand the original use case of selecting the identifier first, so I'm okay with closing.

Answer 10 · 2022-03-21T18:05:50.000Z

The user scenario is you use "find" to find something then you want to press F12, but it doesn't work because find selects the whole word, leaving the caret at the end of the word. Can the LSP fix this internally by just subtracting 1 from the position if it is at the end of a word and there is a following whitespace?

The other thing that is funky is the the tooltip shows it was looking for "append" and that's very confusing now, because we all know Array.append exists:

Answer 11 · 2022-03-22T08:53:54.000Z

Thanks for elaborating, that makes more sense. Is the use case still relevant now that we have workspace symbol search?

Answer 12 · 2022-03-22T20:24:02.000Z

Customers will complain because the Lean implementation of goto definition is non-standard. I vote we should fix it before that happens.

Answer 13 · 2022-03-24T00:48:06.000Z

I think Chris' user scenario is realistic, and I can reproduce it.
I am removing the "closing soon".