project: Fix extra } at the end of import on completion accept (#35494)
Closes #34094 Bug in https://github.com/zed-industries/zed/pull/11157 **Context:** In https://github.com/zed-industries/zed/pull/31872, we added logic to avoid re-querying language server completions (`textDocument/completion`) when possible. This means the list of `lsp::CompletionItem` objects we have might be stale and not contain accurate data like `text_edit`, which is only valid for the buffer at the initial position when these completions were requested. We don't really care about this because we already extract all the useful data we need (like insert/replace ranges) into `Completion`, which converts `text_edit` to anchors. This means further user edits simply push/move those anchors, and our insert/replace ranges persist for completion accept. ```jsonc // on initial textDocument/completion "textEdit":{"insert":{"start":{"line":2,"character":0},"end":{"line":2,"character":11}},"replace":{"start":{"line":2,"character":0},"end":{"line":2,"character":11}} ``` However, for showing documentation of visible `Completion` items, we need to call resolve (`completionItem/resolve`) with the existing `lsp::CompletionItem`, which returns the same `text_edit` and other existing data along with additional new data that was previously optional, like `documentation` and `detail`. **Problem:** This new data like `documentation` and `detail` doesn't really change on buffer edits for a given completion item, so we can use it. But `text_edit` from this resolved `lsp::CompletionItem` was valid when the the initial (`textDocument/completion`) was queried but now the underlying buffer is different. Hence, creating anchors from this ends up creating them in wrong places. ```jsonc // calling completionItem/resolve on cached lsp::CompletionItem results into same textEdit, despite buffer edits "textEdit":{"insert":{"start":{"line":2,"character":0},"end":{"line":2,"character":11}},"replace":{"start":{"line":2,"character":0},"end":{"line":2,"character":11}} ``` It looks like the only reason to override the new text and these ranges was to handle an edge case with `typescript-language-server`, as mentioned in the code comment. However, according to the LSP specification for [Completion Request](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion): > All other properties (usually sortText, filterText, insertText and textEdit) must be provided in the textDocument/completion response and **must not be changed during resolve.** If any language server responds with different `textEdit`, `insertText`, etc. in `completionItem/resolve` than in `textDocument/completion`, they should fix that. Bug in this case in `typescript-language-server`: https://github.com/typescript-language-server/typescript-language-server/pull/303#discussion_r869102064 We don't really need to override these at all. Keeping the existing Anchors results in correct replacement. Release Notes: - Fixed issue where in some cases there would be an extra `}` at the end of imports when accepting completions.
This commit is contained in:
parent
edac6e4246
commit
4d79edc753
2 changed files with 2 additions and 31 deletions
|
@ -21129,13 +21129,6 @@ fn process_completion_for_edit(
|
|||
.is_le(),
|
||||
"replace_range should start before or at cursor position"
|
||||
);
|
||||
debug_assert!(
|
||||
insert_range
|
||||
.end
|
||||
.cmp(&cursor_position, &buffer_snapshot)
|
||||
.is_le(),
|
||||
"insert_range should end before or at cursor position"
|
||||
);
|
||||
|
||||
let should_replace = match intent {
|
||||
CompletionIntent::CompleteWithInsert => false,
|
||||
|
|
|
@ -6044,7 +6044,6 @@ impl LspStore {
|
|||
|
||||
let resolved = Self::resolve_completion_local(
|
||||
server,
|
||||
&buffer_snapshot,
|
||||
completions.clone(),
|
||||
completion_index,
|
||||
)
|
||||
|
@ -6077,7 +6076,6 @@ impl LspStore {
|
|||
|
||||
async fn resolve_completion_local(
|
||||
server: Arc<lsp::LanguageServer>,
|
||||
snapshot: &BufferSnapshot,
|
||||
completions: Rc<RefCell<Box<[Completion]>>>,
|
||||
completion_index: usize,
|
||||
) -> Result<()> {
|
||||
|
@ -6122,26 +6120,8 @@ impl LspStore {
|
|||
.into_response()
|
||||
.context("resolve completion")?;
|
||||
|
||||
if let Some(text_edit) = resolved_completion.text_edit.as_ref() {
|
||||
// Technically we don't have to parse the whole `text_edit`, since the only
|
||||
// language server we currently use that does update `text_edit` in `completionItem/resolve`
|
||||
// is `typescript-language-server` and they only update `text_edit.new_text`.
|
||||
// But we should not rely on that.
|
||||
let edit = parse_completion_text_edit(text_edit, snapshot);
|
||||
|
||||
if let Some(mut parsed_edit) = edit {
|
||||
LineEnding::normalize(&mut parsed_edit.new_text);
|
||||
|
||||
let mut completions = completions.borrow_mut();
|
||||
let completion = &mut completions[completion_index];
|
||||
|
||||
completion.new_text = parsed_edit.new_text;
|
||||
completion.replace_range = parsed_edit.replace_range;
|
||||
if let CompletionSource::Lsp { insert_range, .. } = &mut completion.source {
|
||||
*insert_range = parsed_edit.insert_range;
|
||||
}
|
||||
}
|
||||
}
|
||||
// We must not use any data such as sortText, filterText, insertText and textEdit to edit `Completion` since they are not suppose change during resolve.
|
||||
// Refer: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion
|
||||
|
||||
let mut completions = completions.borrow_mut();
|
||||
let completion = &mut completions[completion_index];
|
||||
|
@ -6391,12 +6371,10 @@ impl LspStore {
|
|||
}) else {
|
||||
return Task::ready(Ok(None));
|
||||
};
|
||||
let snapshot = buffer_handle.read(&cx).snapshot();
|
||||
|
||||
cx.spawn(async move |this, cx| {
|
||||
Self::resolve_completion_local(
|
||||
server.clone(),
|
||||
&snapshot,
|
||||
completions.clone(),
|
||||
completion_index,
|
||||
)
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue