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

In VS Code, rust-analyzer prefers postfix completions over methods/traits #17077

Closed
davidbarsky opened this issue Apr 15, 2024 · 11 comments
Closed

Comments

@davidbarsky
Copy link
Contributor

davidbarsky commented Apr 15, 2024

Just as a heads up, as of #17073, postfix completions/helpers are showing up first in VS Code. With a rust-analyzer built at c037130, these are the completions I'm getting:

Screenshot 2024-04-15 at 11 23 34 AM

With a rust-analyzer built at af72874, I'm getting the postfix completions first:

Screenshot 2024-04-15 at 11 25 57 AM

I'm not sure how intentional that is, but it certainly feels a bit like a bug to me.

Originally posted by @davidbarsky in #17073 (comment)

@davidbarsky
Copy link
Contributor Author

Note that adding "editor.snippetSuggestions": "bottom" to the settings brought back the prior behavior, but I'm not sure it's a good idea to users to make that change.

@davidbarsky
Copy link
Contributor Author

Is this because VS Code considers 1.ok to be a better match for 1. than oxxf (some fns)? 🤔

I think so.

We might need to keep the order with sort_text.

In rst-analyzer itself or the extension?

@roife
Copy link
Member

roife commented Apr 15, 2024

r-a would calculate a "score" based on various factors and converts it into sort_text.

But it is quite strange because I found that the score for borrow() is 15 while the score for box is 13, yet box ranks higher.

@pascalkuthe
Copy link
Contributor

pascalkuthe commented Apr 15, 2024

VSCode does not follow the spec and both filters and sorts incomplete completion lists based on text score (and most servers including ra rely on that to some degree so every other client is forced to follow this non-standard behavior if we dont want broken completions). The real fix would for vscode to comply with the LSP spec.

I am not sure you can do much about this server side if sorttext is already set correctly.

@pascalkuthe
Copy link
Contributor

pascalkuthe commented Apr 15, 2024

Thus will probably not be a huge problem once you typed one or two characters since then the real completions will have a better textual score. So you could just hide postfix completions when there is a trigger char (after . and ::)

@davidbarsky
Copy link
Contributor Author

VSCode does not follow the spec and both filters and sorts incomplete completion lists based on text score (and most servers including ra rely on that to some degree so every other client is forced to follow this non-standard behavior if we dont want broken completions). The real fix would for vscode to comply with the LSP spec.

That's unfortunately a common situation, but that's not really something we can do in the next week.

Thus will probably not be a huge problem once you typed one or two characters since then the real completions will have a better textual score. So you could just hide postfix completions when there is a trigger char (after . and ::)

I think you underestimate how much of an issue this might be, especially given that people would write a single dot completion to determine what methods are available to them. Given that VS Code is the primary supported editor of rust-analyzer, we'll need to find a workaround.

@pascalkuthe
Copy link
Contributor

pascalkuthe commented Apr 15, 2024

I meant to hide postfix completions like ok after trigger chars. That would allow scrolling trough available methods without seeing the postfix snippets.

Once you start actually typing text you can show the postfix snuppets again because then the score of whatever you are looking for will be higher than postfix snippets (except when you are actually looking for postfix snippets)

@davidbarsky
Copy link
Contributor Author

davidbarsky commented Apr 15, 2024

I meant to hide postfix completions like ok after trigger chars. That would allow scrolling trough available methods without seeing the postfix snippets.

That could work, but I'm not sure how you're proposing to distinguish a trigger character for postfix completions versus a method completions (I'm thinking of some postfix snippets like .match).

The alternative I'm considering is to scoring behavior by editor (e.g., special-casing VS Code).

@pascalkuthe
Copy link
Contributor

That could work, but I'm not sure how you're proposing to distinguish a trigger character for postfix completions versus a method completions (I'm thinking of some postfix snippets like .match).

Afaim postfix completions (like .ok, .match,...) only show up after periods right now. The PR that regressed this made RA include the text before the period (and the period itself) in the filter text. So I thought you could simply check if:

  • filterText contains a period
  • the completion trigger position is immieditly after a period

If both are true the completion is not send to the client.

If this solution works without side-effects that would be ideal since it would be editor agnostic.

The alternative I'm considering is to change behavior in scoring by editor—special case VS Code.

As far as I can tell the problem here is that vscode does uncofigurable and incorrect client side scoring so I don't think you would be able to do editor specific scoring on the server side.

This could also be just an issue with the ra side scoring/sort text but if I understand the comment above correctly (which matches with what I found recently looking trough the vscode codebase). Vscode always does client side scoring even when you disabled it by marking the completion as incomplete.

@davidbarsky
Copy link
Contributor Author

Afaim postfix completions (like .ok, .match,...) only show up after periods right now. The PR that regressed this made RA include the text before the period (and the period itself) in the filter text. So I thought you could simply check if:

  • filterText contains a period
  • the completion trigger position is immieditly after a period

If both are true the completion is not send to the client.

From my testing, this seems to effectively disable postfix/magic completions entirely, especially since there isn't any other way to activate them beyond dot completions.

In reference to your prior comment about the LSP specification, I'm not sure I agree with your interpretation for the following reasons:

  • The phrasing of "[t]he range of the edit must be a single line range and it must contain the position at which completion has been requested" appears to only be in reference to the primary textEdit on CompletionItem, not additionalTextEdits.
  • Similarly, the constraint of "the range of the edit" does not appear on TextEdit, which makes me think the aforementioned range/position constraint is only applicable to CompletionItem::textEdit, not TextEdits in general.
  • The documentation for additionalTextEdits says "[a]dditional text edits should be used to change text unrelated to the current cursor position", with imports being a cited as an example. However, adding a &mut before a . also fits the definition of "unrelated", albeit more tenuously. Therefore, I think rust-analyzer has the latitude to be more liberal with its usage of additionalTextEdits because of the distinction between "should" and "must", despite the difficulties with multi-cursor edits.

As far as I can tell the problem here is that vscode does uncofigurable and incorrect client side scoring so I don't think you would be able to do editor specific scoring on the server side.

The approach I had in mind was to gate the behavior introduced in #17073 by the editor name, but that feels like a nightmare to test/maintain. I have no love lost for VS Code, but the behavior is technically configurable by the end-user, just not language servers. Regardless, I think it's problematic to require end-users to make a change to their editor in order to return to behavior that they're used to, which is why I'm personally inclined to revert #17073.

@davidbarsky
Copy link
Contributor Author

Anyways, I was talking @Wilfred about VS Code's autocomplete/intellisense, and he pointed out that it is very good about finding applicable completions even if you mistype a portion of your what you expect to be autocompleted, and I think a portion of that magic-when-it-works experience is them factoring in the starting point of a line range. Moving the range to start prior to the cursor is probably getting VS Code to prioritize those completions more because they might be using that as a proxy for "the user is in the middle of writing a long method, so we should suggest that".

bors added a commit that referenced this issue Apr 16, 2024
Revert #17073: Better inline preview for postfix completion

See discussion on #17077, but I strongly suspect that the changes to the `TextEdit` ranges caused VS Code's autocomplete to prefer the snippets over method completions. I explain why I think that [here](#17077 (comment)).
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 17, 2024
…kril

Revert rust-lang#17073: Better inline preview for postfix completion

See discussion on rust-lang#17077, but I strongly suspect that the changes to the `TextEdit` ranges caused VS Code's autocomplete to prefer the snippets over method completions. I explain why I think that [here](rust-lang#17077 (comment)).
lnicola pushed a commit to lnicola/rust that referenced this issue Apr 20, 2024
…kril

Revert rust-lang#17073: Better inline preview for postfix completion

See discussion on rust-lang/rust-analyzer#17077, but I strongly suspect that the changes to the `TextEdit` ranges caused VS Code's autocomplete to prefer the snippets over method completions. I explain why I think that [here](rust-lang/rust-analyzer#17077 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants