Move import text edit calculation into a completion resolve request #6706
Conversation
|
While we at it, we can discuss the autocompletion flag name: #6631 (comment) |
SomeoneToIgnore
force-pushed
the
resolve-completions
branch
from
d789eaa
to
b07ddef
Compare
SomeoneToIgnore
force-pushed
the
resolve-completions
branch
from
d87ac79
to
a9d90ac
Compare
crates/completion/src/item.rs
Outdated
| if let Some(import_edit) = | ||
| self.import_to_add.as_ref().and_then(|import_edit| import_edit.to_text_edit()) | ||
| { | ||
| text_edit.union(import_edit).expect("Failed to unite import and completion edits"); |
There was a problem hiding this comment.
I get nervous about panicing since I've seen it take down the server during assists. Is there any way we can avoid it?
There was a problem hiding this comment.
The question here is "what to do if not to panic?".
IMO we should not return a half-baked CompletionItem in such case at all, but if I change the return type to Option or Result, the change will touch a lot of other code, so I did not dare to do this in this PR.
But sure, I've adjusted the code to log an error instead and to return some completion nontheless.
I did not remove the panic on serialisation in handlers.rs , since the whole file is abundant with them anyway.
|
I think this looks pretty good but would like another set of eyes on it. Can we add some tests that do not go through the LSP layer? |
They kind of function this way already by doing that eager import application, but I've added one more to check the completion when we have the corresponding resolve capability in the completion settings. |
SomeoneToIgnore
force-pushed
the
resolve-completions
branch
from
2447695
to
4942564
Compare
SomeoneToIgnore
force-pushed
the
resolve-completions
branch
from
c34ac0d
to
3183ff3
Compare
|
A follow-up of #6594 (comment) had been added before it's too late to change the things. |
| }) | ||
| .collect_vec(); | ||
|
|
||
| if !all_edits_are_disjoint(&original_completion, &additional_edits) { |
There was a problem hiding this comment.
Does this case signify a bug in our code? Or is it sanity-check for misbehaving client which, eg, asks to resolve old completion after some changes to the fiels where done?
There was a problem hiding this comment.
Both, yeah: I'm finding it hard to trust the incoming object, since there are no guarantees on its contents.
|
bors r+ |
Part of #6612 (presumably fixing it)
Part of #6366 (does not cover all possible resolve capabilities we can do)
Closes #6594
Further improves imports on completion performance by deferring the computations for import inserts.
To use the new mode, you have to have the experimental completions enabled and use the LSP 3.16-compliant client that reports
additionalTextEditsin itsCompletionItemCapabilityResolveSupportfield in the client capabilities.rust-analyzer VSCode extension does this already hence picks up the changes completely.
Performance implications are descrbed in: #6633 (comment)