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.

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

Postfix snippets no longer work in vscode #17036

Closed
jtraub opened this issue Apr 8, 2024 · 12 comments · Fixed by #17054
Closed

Postfix snippets no longer work in vscode #17036

jtraub opened this issue Apr 8, 2024 · 12 comments · Fixed by #17054
Assignees
Labels
A-completion autocompletion Broken Window Bugs / technical debt to be addressed immediately C-bug Category: bug

Comments

@jtraub
Copy link

jtraub commented Apr 8, 2024

rust-analyzer version: 0.4.1909-standalone

editor or extension: VSCode with extension v0.4.1909 (pre-release)

Postfix snippets stopped working after I've updated vscode extension.

After downgrading a few times I found that the last version that has postfix snippets working is v0.4.1908. Upgrading it to v0.4.1909 breaks them.

v0.4.1908
image

v0.4.1909
image

@jtraub jtraub added the C-bug Category: bug label Apr 8, 2024
@ealinye
Copy link

ealinye commented Apr 9, 2024

me too

@roife
Copy link
Member

roife commented Apr 9, 2024

Possibly related to #17000.

That's strange since it works fine on emacs (eglot) 🤔

@facefaceless
Copy link

Magic completion dosen't show up in the completion list too after upgrading to a newer version.
I use vscode on windows, and RA 0.3.1906 (release version) is the newest version that works fine for me.
Maybe they are the same problem.

@Veykril Veykril added A-completion autocompletion Broken Window Bugs / technical debt to be addressed immediately labels Apr 9, 2024
@roife
Copy link
Member

roife commented Apr 11, 2024

@rustbot claim

I've identified the problem. For illustration, let's use the following example:

1 . o k
^ ^ ^ ^ ^
0 1 2 3 4

Before #17000, r-a would send TextEdit delete 2..4, insert Ok(1) to the editor within CompletionItem. After the change, r-a would send delete 0..4, insert Ok(1).

But in VS Code, the starting position for TextEdit replace cannot precede the completion position (i.e., cannot precede 2), and VS Code automatically filters out items that violate it.

Currently, I've thought of several solutions:

  1. Delete 0..2 in additionalTextEdit, then TextEdit still sends delete 2..4, insert Ok(1). I've tried it and this fixes the problem in VS Code, but it seems that helix does not yet support additionalTextEdit.
  2. Revert c5686c8.
  3. Apply special handling for VS Code (a bit dirty). 🤔

I would prefer the second solution.

@kericsson-dotnet
Copy link

Postfix snippets seems to be broken in Neovim as well. Working fine in 2024-04-01, not showing up in 2024-04-08

@pascalkuthe
Copy link
Contributor

pascalkuthe commented Apr 15, 2024

. I've tried it and this fixes the problem in VS Code, but it seems that helix does not yet support additionalTextEdit

Helix fully support additionalTextEdits. We just dont preview them since we expect them to not be associated with the cursor. Quote from the LSP spec:

Additional text edits should be used to change text unrelated to the current cursor positition

So we only apply additionalTextEditd when accepting a completion. Qhile the lossy in preview is a bit annoying it's not that big of a deal.

The real problem is that these kinds of edits do not work with multicursor completions. You can map the main completion edit to each cursor (by computing relative positions to the curosr). But you can't do that with additonalTextEdit Since they are meant for edits not associated with the cursor (like imports) that should not be repeated for each cursor.

These edits are perfectly valid according to the LSP spec. The spec says the completion position must be contained within the edit but not that it must start exactly at the cursor:

The range of the edit must be a single line range and it must contain the position at which completion has been requested

The reason this gets filtered is not that vacode filters out all edits that precede the cursor position. The reason is that vscode uses the replace range for filtering so to take the example from above if the edit replaces 1.ok with Ok then vscode filters that with 1.ok and sincr the filter text doesnt contain 1. it gets filtered out (which is spec compliant and what I am working on moving helix towards).

The right way to fix this issue in a spec compliant manner is to adjust the filter text of these completions to include the prefix they are replacing

@Veykril
Copy link
Member

Veykril commented Apr 15, 2024

So, we should be able to change the replace range here if we also fix up the filter text used for postfix completions. Then VSCode should be happy as well?

@pascalkuthe
Copy link
Contributor

pascalkuthe commented Apr 15, 2024

So, we should be able to change the replace range here if we also fix up the filter text used for postfix completions. Then VSCode should be happy as well?

Exactly, sorry my comment wasn't perfectly clear. I didn't have my first coffee yet.

In fact RA already does exactly that for other edits I think (there are some edits which replace the word the cursor is in. These also replace text before the curosr and they work in VSCode because they set the filtertext correctly.

I was planning to do a PR that implements what I describe once I have some freetime. That should fix the problems we are seeing in helix while also making sure that the completions don't get filtered out in VSCode

@Veykril
Copy link
Member

Veykril commented Apr 15, 2024

Great, thanks for that :) (fwiw in r-a the filter field is called lookup in our CompletionItem)

@roife
Copy link
Member

roife commented Apr 15, 2024

@pascalkuthe, thanks for your correction! I followed your description to add both prefix and postfix to the lookup field in CompletionItem, and indeed it fixed all the issues (helix & vscode). If you don't mind, may I submit the PR to fix it?

@pascalkuthe
Copy link
Contributor

@pascalkuthe, thanks for your correction! I followed your description to add both prefix and postfix to the lookup field in CompletionItem, and indeed it fixed all the issues (helix & vscode). If you don't mind, may I submit the PR to fix it?

sure go ahead I just want to see this fixed :)

bors added a commit that referenced this issue Apr 15, 2024
Better inline preview for postfix completion

Better inline preview for postfix completion, a proper implementation of c5686c8.

Here editors may filter completion item with the text within `delete_range`, so we need to include the `receiver text` in the `lookup` (aka `FilterText` in LSP spec) for editors to find the completion item. (See #17036 (comment), Thanks to [pascalkuthe](https://github.com/pascalkuthe))
lnicola pushed a commit to lnicola/rust that referenced this issue Apr 20, 2024
Revert "fix: set the right postfix snippets competion source range"

This reverts commit c5686c8.

Fix rust-lang#17036. See rust-lang/rust-analyzer#17036 (comment)
lnicola pushed a commit to lnicola/rust that referenced this issue Apr 20, 2024
Better inline preview for postfix completion

Better inline preview for postfix completion, a proper implementation of c5686c8.

Here editors may filter completion item with the text within `delete_range`, so we need to include the `receiver text` in the `lookup` (aka `FilterText` in LSP spec) for editors to find the completion item. (See rust-lang/rust-analyzer#17036 (comment), Thanks to [pascalkuthe](https://github.com/pascalkuthe))
@mrnossiom
Copy link
Contributor

Sorry there, I've not received notifications about this. Glad you found another way around.

@mrnossiom
Copy link
Contributor

I believe that both fixes for this got reverted because of issues with VSCode and other editors. Still, it would be great if we had proper visualization on Helix.

If this was the spec compliant way, wouldn't it be possible to take a look at VS Code src? I can try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-completion autocompletion Broken Window Bugs / technical debt to be addressed immediately C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants