Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Avoid modifying the LSP message before resolving it (#26347)

Browse source 
Closes https://github.com/zed-industries/zed/issues/21277

To the left is current Zed, right is the improved version.
3rd message, from Zed, to resolve the item, does not have `textEdit` on
the right side, and has one on the left.
Seems to not influence the end result though, but at least Zed behaves
more appropriate now.

<img width="1727" alt="image"
src="https://github.com/user-attachments/assets/ca1236fd-9ce2-41ba-88fe-1f3178cdcbde"
/>


Instead of modifying the original LSP completion item, store completion
list defaults and apply them when the item is requested (except `data`
defaults, needed for resolve).

Now, the only place that can modify the completion items is this method,
and Python impl seems to be the one doing it:


ca9c3af56f/crates/languages/src/python.rs (L182-L204)

Seems ok to leave untouched for now.

Release Notes:

- Fixed LSP completion items modified before resolve request
This commit is contained in:
Kirill Bulatov 2025-03-10 00:12:53 +02:00 • committed by GitHub
parent 6de3ac3e17
commit 8a7a78fafb
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 233 additions and 158 deletions
Download patch file Download diff file Expand all files Collapse all files

12
crates/editor/src/code_context_menus.rs
View file

@ -500,7 +500,7 @@ impl CompletionsMenu {
highlight.font_weight = None;
if completion
.source
.lsp_completion()
.lsp_completion(false)
.and_then(|lsp_completion| lsp_completion.deprecated)
.unwrap_or(false)
{
@ -711,10 +711,12 @@ impl CompletionsMenu {
let completion = &completions[mat.candidate_id];
let sort_key = completion.sort_key();
let sort_text = completion
.source
.lsp_completion()
.and_then(|lsp_completion| lsp_completion.sort_text.as_deref());
let sort_text =
if let CompletionSource::Lsp { lsp_completion, .. } = &completion.source {
lsp_completion.sort_text.as_deref()
} else {
None
};
let score = Reverse(OrderedFloat(mat.score));
if mat.score >= 0.2 {

1
crates/editor/src/editor.rs
View file

@ -17017,6 +17017,7 @@ fn snippet_completions(
sort_text: Some(char::MAX.to_string()),
..lsp::CompletionItem::default()
}),
lsp_defaults: None,
},
label: CodeLabel {
text: matching_prefix.clone(),

51
crates/editor/src/editor_tests.rs
View file

@ -12334,24 +12334,6 @@ async fn test_completions_default_resolve_data_handling(cx: &mut TestAppContext)
},
};
let item_0_out = lsp::CompletionItem {
commit_characters: Some(default_commit_characters.clone()),
insert_text_format: Some(default_insert_text_format),
..item_0
};
let items_out = iter::once(item_0_out)
.chain(items[1..].iter().map(|item| lsp::CompletionItem {
commit_characters: Some(default_commit_characters.clone()),
data: Some(default_data.clone()),
insert_text_mode: Some(default_insert_text_mode),
text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
range: default_edit_range,
new_text: item.label.clone(),
})),
..item.clone()
}))
.collect::<Vec<lsp::CompletionItem>>();
let mut cx = EditorLspTestContext::new_rust(
lsp::ServerCapabilities {
completion_provider: Some(lsp::CompletionOptions {
@ -12370,10 +12352,11 @@ async fn test_completions_default_resolve_data_handling(cx: &mut TestAppContext)
let completion_data = default_data.clone();
let completion_characters = default_commit_characters.clone();
let completion_items = items.clone();
cx.handle_request::<lsp::request::Completion, _, _>(move |_, _, _| {
let default_data = completion_data.clone();
let default_commit_characters = completion_characters.clone();
let items = items.clone();
let items = completion_items.clone();
async move {
Ok(Some(lsp::CompletionResponse::List(lsp::CompletionList {
items,
@ -12422,7 +12405,7 @@ async fn test_completions_default_resolve_data_handling(cx: &mut TestAppContext)
.iter()
.map(|mat| mat.string.clone())
.collect::<Vec<String>>(),
items_out
items
.iter()
.map(|completion| completion.label.clone())
.collect::<Vec<String>>()
@ -12435,14 +12418,18 @@ async fn test_completions_default_resolve_data_handling(cx: &mut TestAppContext)
// with 4 from the end.
assert_eq!(
*resolved_items.lock(),
[
&items_out[0..16],
&items_out[items_out.len() - 4..items_out.len()]
]
.concat()
.iter()
.cloned()
.collect::<Vec<lsp::CompletionItem>>()
[&items[0..16], &items[items.len() - 4..items.len()]]
.concat()
.iter()
.cloned()
.map(|mut item| {
if item.data.is_none() {
item.data = Some(default_data.clone());
}
item
})
.collect::<Vec<lsp::CompletionItem>>(),
"Items sent for resolve should be unchanged modulo resolve `data` filled with default if missing"
);
resolved_items.lock().clear();
@ -12453,9 +12440,15 @@ async fn test_completions_default_resolve_data_handling(cx: &mut TestAppContext)
// Completions that have already been resolved are skipped.
assert_eq!(
*resolved_items.lock(),
items_out[items_out.len() - 16..items_out.len() - 4]
items[items.len() - 16..items.len() - 4]
.iter()
.cloned()
.map(|mut item| {
if item.data.is_none() {
item.data = Some(default_data.clone());
}
item
})
.collect::<Vec<lsp::CompletionItem>>()
);
resolved_items.lock().clear();

124
crates/project/src/lsp_command.rs
View file

@ -1847,7 +1847,6 @@ impl LspCommand for GetCompletions {
let mut completions = if let Some(completions) = completions {
match completions {
lsp::CompletionResponse::Array(completions) => completions,
lsp::CompletionResponse::List(mut list) => {
let items = std::mem::take(&mut list.items);
response_list = Some(list);
@ -1855,74 +1854,19 @@ impl LspCommand for GetCompletions {
}
}
} else {
Default::default()
Vec::new()
};
let language_server_adapter = lsp_store
.update(&mut cx, |lsp_store, _| {
lsp_store.language_server_adapter_for_id(server_id)
})?
.ok_or_else(|| anyhow!("no such language server"))?;
.with_context(|| format!("no language server with id {server_id}"))?;
let item_defaults = response_list
let lsp_defaults = response_list
.as_ref()
.and_then(|list| list.item_defaults.as_ref());
if let Some(item_defaults) = item_defaults {
let default_data = item_defaults.data.as_ref();
let default_commit_characters = item_defaults.commit_characters.as_ref();
let default_edit_range = item_defaults.edit_range.as_ref();
let default_insert_text_format = item_defaults.insert_text_format.as_ref();
let default_insert_text_mode = item_defaults.insert_text_mode.as_ref();
if default_data.is_some()
|| default_commit_characters.is_some()
|| default_edit_range.is_some()
|| default_insert_text_format.is_some()
|| default_insert_text_mode.is_some()
{
for item in completions.iter_mut() {
if item.data.is_none() && default_data.is_some() {
item.data = default_data.cloned()
}
if item.commit_characters.is_none() && default_commit_characters.is_some() {
item.commit_characters = default_commit_characters.cloned()
}
if item.text_edit.is_none() {
if let Some(default_edit_range) = default_edit_range {
match default_edit_range {
CompletionListItemDefaultsEditRange::Range(range) => {
item.text_edit =
Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
range: *range,
new_text: item.label.clone(),
}))
}
CompletionListItemDefaultsEditRange::InsertAndReplace {
insert,
replace,
} => {
item.text_edit =
Some(lsp::CompletionTextEdit::InsertAndReplace(
lsp::InsertReplaceEdit {
new_text: item.label.clone(),
insert: *insert,
replace: *replace,
},
))
}
}
}
}
if item.insert_text_format.is_none() && default_insert_text_format.is_some() {
item.insert_text_format = default_insert_text_format.cloned()
}
if item.insert_text_mode.is_none() && default_insert_text_mode.is_some() {
item.insert_text_mode = default_insert_text_mode.cloned()
}
}
}
}
.and_then(|list| list.item_defaults.clone())
.map(Arc::new);
let mut completion_edits = Vec::new();
buffer.update(&mut cx, |buffer, _cx| {
@ -1930,12 +1874,34 @@ impl LspCommand for GetCompletions {
let clipped_position = buffer.clip_point_utf16(Unclipped(self.position), Bias::Left);
let mut range_for_token = None;
completions.retain_mut(|lsp_completion| {
let edit = match lsp_completion.text_edit.as_ref() {
completions.retain(|lsp_completion| {
let lsp_edit = lsp_completion.text_edit.clone().or_else(|| {
let default_text_edit = lsp_defaults.as_deref()?.edit_range.as_ref()?;
match default_text_edit {
CompletionListItemDefaultsEditRange::Range(range) => {
Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
range: *range,
new_text: lsp_completion.label.clone(),
}))
}
CompletionListItemDefaultsEditRange::InsertAndReplace {
insert,
replace,
} => Some(lsp::CompletionTextEdit::InsertAndReplace(
lsp::InsertReplaceEdit {
new_text: lsp_completion.label.clone(),
insert: *insert,
replace: *replace,
},
)),
}
});
let edit = match lsp_edit {
// If the language server provides a range to overwrite, then
// check that the range is valid.
Some(completion_text_edit) => {
match parse_completion_text_edit(completion_text_edit, &snapshot) {
match parse_completion_text_edit(&completion_text_edit, &snapshot) {
Some(edit) => edit,
None => return false,
}
@ -1949,14 +1915,15 @@ impl LspCommand for GetCompletions {
return false;
}
let default_edit_range = response_list
.as_ref()
.and_then(|list| list.item_defaults.as_ref())
.and_then(|defaults| defaults.edit_range.as_ref())
.and_then(|range| match range {
CompletionListItemDefaultsEditRange::Range(r) => Some(r),
_ => None,
});
let default_edit_range = lsp_defaults.as_ref().and_then(|lsp_defaults| {
lsp_defaults
.edit_range
.as_ref()
.and_then(|range| match range {
CompletionListItemDefaultsEditRange::Range(r) => Some(r),
_ => None,
})
});
let range = if let Some(range) = default_edit_range {
let range = range_from_lsp(*range);
@ -2006,14 +1973,25 @@ impl LspCommand for GetCompletions {
Ok(completions
.into_iter()
.zip(completion_edits)
.map(|(lsp_completion, (old_range, mut new_text))| {
.map(|(mut lsp_completion, (old_range, mut new_text))| {
LineEnding::normalize(&mut new_text);
if lsp_completion.data.is_none() {
if let Some(default_data) = lsp_defaults
.as_ref()
.and_then(|item_defaults| item_defaults.data.clone())
{
// Servers (e.g. JDTLS) prefer unchanged completions, when resolving the items later,
// so we do not insert the defaults here, but `data` is needed for resolving, so this is an exception.
lsp_completion.data = Some(default_data);
}
}
CoreCompletion {
old_range,
new_text,
source: CompletionSource::Lsp {
server_id,
lsp_completion: Box::new(lsp_completion),
lsp_defaults: lsp_defaults.clone(),
resolved: false,
},
}

110
crates/project/src/lsp_store.rs
View file

@ -49,10 +49,9 @@ use lsp::{
notification::DidRenameFiles, CodeActionKind, CompletionContext, DiagnosticSeverity,
DiagnosticTag, DidChangeWatchedFilesRegistrationOptions, Edit, FileOperationFilter,
FileOperationPatternKind, FileOperationRegistrationOptions, FileRename, FileSystemWatcher,
InsertTextFormat, LanguageServer, LanguageServerBinary, LanguageServerBinaryOptions,
LanguageServerId, LanguageServerName, LspRequestFuture, MessageActionItem, MessageType, OneOf,
RenameFilesParams, SymbolKind, TextEdit, WillRenameFiles, WorkDoneProgressCancelParams,
WorkspaceFolder,
LanguageServer, LanguageServerBinary, LanguageServerBinaryOptions, LanguageServerId,
LanguageServerName, LspRequestFuture, MessageActionItem, MessageType, OneOf, RenameFilesParams,
SymbolKind, TextEdit, WillRenameFiles, WorkDoneProgressCancelParams, WorkspaceFolder,
};
use node_runtime::read_package_installed_version;
use parking_lot::Mutex;
@ -70,6 +69,7 @@ use smol::channel::Sender;
use snippet::Snippet;
use std::{
any::Any,
borrow::Cow,
cell::RefCell,
cmp::Ordering,
convert::TryInto,
@ -4475,6 +4475,7 @@ impl LspStore {
completions: Rc<RefCell<Box<[Completion]>>>,
completion_index: usize,
) -> Result<()> {
let server_id = server.server_id();
let can_resolve = server
.capabilities()
.completion_provider
@ -4491,19 +4492,24 @@ impl LspStore {
CompletionSource::Lsp {
lsp_completion,
resolved,
server_id: completion_server_id,
..
} => {
if *resolved {
return Ok(());
}
anyhow::ensure!(
server_id == *completion_server_id,
"server_id mismatch, querying completion resolve for {server_id} but completion server id is {completion_server_id}"
);
server.request::<lsp::request::ResolveCompletionItem>(*lsp_completion.clone())
}
CompletionSource::Custom => return Ok(()),
}
};
let completion_item = request.await?;
let resolved_completion = request.await?;
if let Some(text_edit) = completion_item.text_edit.as_ref() {
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`.
@ -4520,24 +4526,26 @@ impl LspStore {
completion.old_range = old_range;
}
}
if completion_item.insert_text_format == Some(InsertTextFormat::SNIPPET) {
// vtsls might change the type of completion after resolution.
let mut completions = completions.borrow_mut();
let completion = &mut completions[completion_index];
if let Some(lsp_completion) = completion.source.lsp_completion_mut() {
if completion_item.insert_text_format != lsp_completion.insert_text_format {
lsp_completion.insert_text_format = completion_item.insert_text_format;
}
}
}
let mut completions = completions.borrow_mut();
let completion = &mut completions[completion_index];
completion.source = CompletionSource::Lsp {
lsp_completion: Box::new(completion_item),
resolved: true,
server_id: server.server_id(),
};
if let CompletionSource::Lsp {
lsp_completion,
resolved,
server_id: completion_server_id,
..
} = &mut completion.source
{
if *resolved {
return Ok(());
}
anyhow::ensure!(
server_id == *completion_server_id,
"server_id mismatch, applying completion resolve for {server_id} but completion server id is {completion_server_id}"
);
*lsp_completion = Box::new(resolved_completion);
*resolved = true;
}
Ok(())
}
@ -4549,8 +4557,8 @@ impl LspStore {
) -> Result<()> {
let completion_item = completions.borrow()[completion_index]
.source
.lsp_completion()
.cloned();
.lsp_completion(true)
.map(Cow::into_owned);
if let Some(lsp_documentation) = completion_item
.as_ref()
.and_then(|completion_item| completion_item.documentation.clone())
@ -4626,8 +4634,13 @@ impl LspStore {
CompletionSource::Lsp {
lsp_completion,
resolved,
server_id: completion_server_id,
..
} => {
anyhow::ensure!(
server_id == *completion_server_id,
"remote server_id mismatch, querying completion resolve for {server_id} but completion server id is {completion_server_id}"
);
if *resolved {
return Ok(());
}
@ -4647,7 +4660,7 @@ impl LspStore {
.request(request)
.await
.context("completion documentation resolve proto request")?;
let lsp_completion = serde_json::from_slice(&response.lsp_completion)?;
let resolved_lsp_completion = serde_json::from_slice(&response.lsp_completion)?;
let documentation = if response.documentation.is_empty() {
CompletionDocumentation::Undocumented
@ -4662,11 +4675,23 @@ impl LspStore {
let mut completions = completions.borrow_mut();
let completion = &mut completions[completion_index];
completion.documentation = Some(documentation);
completion.source = CompletionSource::Lsp {
server_id,
if let CompletionSource::Lsp {
lsp_completion,
resolved: true,
};
resolved,
server_id: completion_server_id,
lsp_defaults: _,
} = &mut completion.source
{
if *resolved {
return Ok(());
}
anyhow::ensure!(
server_id == *completion_server_id,
"remote server_id mismatch, applying completion resolve for {server_id} but completion server id is {completion_server_id}"
);
*lsp_completion = Box::new(resolved_lsp_completion);
*resolved = true;
}
let old_range = response
.old_start
@ -4750,7 +4775,7 @@ impl LspStore {
let completion = completions.borrow()[completion_index].clone();
let additional_text_edits = completion
.source
.lsp_completion()
.lsp_completion(true)
.as_ref()
.and_then(|lsp_completion| lsp_completion.additional_text_edits.clone());
if let Some(edits) = additional_text_edits {
@ -8153,21 +8178,26 @@ impl LspStore {
}
pub(crate) fn serialize_completion(completion: &CoreCompletion) -> proto::Completion {
let (source, server_id, lsp_completion, resolved) = match &completion.source {
let (source, server_id, lsp_completion, lsp_defaults, resolved) = match &completion.source {
CompletionSource::Lsp {
server_id,
lsp_completion,
lsp_defaults,
resolved,
} => (
proto::completion::Source::Lsp as i32,
server_id.0 as u64,
serde_json::to_vec(lsp_completion).unwrap(),
lsp_defaults
.as_deref()
.map(|lsp_defaults| serde_json::to_vec(lsp_defaults).unwrap()),
*resolved,
),
CompletionSource::Custom => (
proto::completion::Source::Custom as i32,
0,
Vec::new(),
None,
true,
),
};
@ -8178,6 +8208,7 @@ impl LspStore {
new_text: completion.new_text.clone(),
server_id,
lsp_completion,
lsp_defaults,
resolved,
source,
}
@ -8200,6 +8231,11 @@ impl LspStore {
Some(proto::completion::Source::Lsp) => CompletionSource::Lsp {
server_id: LanguageServerId::from_proto(completion.server_id),
lsp_completion: serde_json::from_slice(&completion.lsp_completion)?,
lsp_defaults: completion
.lsp_defaults
.as_deref()
.map(serde_json::from_slice)
.transpose()?,
resolved: completion.resolved,
},
_ => anyhow::bail!("Unexpected completion source {}", completion.source),
@ -8288,8 +8324,8 @@ async fn populate_labels_for_completions(
let lsp_completions = new_completions
.iter()
.filter_map(|new_completion| {
if let CompletionSource::Lsp { lsp_completion, .. } = &new_completion.source {
Some(*lsp_completion.clone())
if let Some(lsp_completion) = new_completion.source.lsp_completion(true) {
Some(lsp_completion.into_owned())
} else {
None
}
@ -8309,8 +8345,8 @@ async fn populate_labels_for_completions(
.fuse();
for completion in new_completions {
match &completion.source {
CompletionSource::Lsp { lsp_completion, .. } => {
match completion.source.lsp_completion(true) {
Some(lsp_completion) => {
let documentation = if let Some(docs) = lsp_completion.documentation.clone() {
Some(docs.into())
} else {
@ -8328,9 +8364,9 @@ async fn populate_labels_for_completions(
new_text: completion.new_text,
source: completion.source,
confirm: None,
})
});
}
CompletionSource::Custom => {
None => {
let mut label = CodeLabel::plain(completion.new_text.clone(), None);
ensure_uniform_list_compatible_label(&mut label);
completions.push(Completion {
@ -8340,7 +8376,7 @@ async fn populate_labels_for_completions(
new_text: completion.new_text,
source: completion.source,
confirm: None,
})
});
}
}
}

92
crates/project/src/project.rs
View file

@ -382,6 +382,8 @@ pub enum CompletionSource {
server_id: LanguageServerId,
/// The raw completion provided by the language server.
lsp_completion: Box<lsp::CompletionItem>,
/// A set of defaults for this completion item.
lsp_defaults: Option<Arc<lsp::CompletionListItemDefaults>>,
/// Whether this completion has been resolved, to ensure it happens once per completion.
resolved: bool,
},
@ -397,17 +399,76 @@ impl CompletionSource {
}
}
pub fn lsp_completion(&self) -> Option<&lsp::CompletionItem> {
if let Self::Lsp { lsp_completion, .. } = self {
Some(lsp_completion)
} else {
None
}
}
pub fn lsp_completion(&self, apply_defaults: bool) -> Option<Cow<lsp::CompletionItem>> {
if let Self::Lsp {
lsp_completion,
lsp_defaults,
..
} = self
{
if apply_defaults {
if let Some(lsp_defaults) = lsp_defaults {
let mut completion_with_defaults = *lsp_completion.clone();
let default_commit_characters = lsp_defaults.commit_characters.as_ref();
let default_edit_range = lsp_defaults.edit_range.as_ref();
let default_insert_text_format = lsp_defaults.insert_text_format.as_ref();
let default_insert_text_mode = lsp_defaults.insert_text_mode.as_ref();
fn lsp_completion_mut(&mut self) -> Option<&mut lsp::CompletionItem> {
if let Self::Lsp { lsp_completion, .. } = self {
Some(lsp_completion)
if default_commit_characters.is_some()
|| default_edit_range.is_some()
|| default_insert_text_format.is_some()
|| default_insert_text_mode.is_some()
{
if completion_with_defaults.commit_characters.is_none()
&& default_commit_characters.is_some()
{
completion_with_defaults.commit_characters =
default_commit_characters.cloned()
}
if completion_with_defaults.text_edit.is_none() {
match default_edit_range {
Some(lsp::CompletionListItemDefaultsEditRange::Range(range)) => {
completion_with_defaults.text_edit =
Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
range: *range,
new_text: completion_with_defaults.label.clone(),
}))
}
Some(
lsp::CompletionListItemDefaultsEditRange::InsertAndReplace {
insert,
replace,
},
) => {
completion_with_defaults.text_edit =
Some(lsp::CompletionTextEdit::InsertAndReplace(
lsp::InsertReplaceEdit {
new_text: completion_with_defaults.label.clone(),
insert: *insert,
replace: *replace,
},
))
}
None => {}
}
}
if completion_with_defaults.insert_text_format.is_none()
&& default_insert_text_format.is_some()
{
completion_with_defaults.insert_text_format =
default_insert_text_format.cloned()
}
if completion_with_defaults.insert_text_mode.is_none()
&& default_insert_text_mode.is_some()
{
completion_with_defaults.insert_text_mode =
default_insert_text_mode.cloned()
}
}
return Some(Cow::Owned(completion_with_defaults));
}
}
Some(Cow::Borrowed(lsp_completion))
} else {
None
}
@ -4640,7 +4701,8 @@ impl Completion {
const DEFAULT_KIND_KEY: usize = 2;
let kind_key = self
.source
.lsp_completion()
// `lsp::CompletionListItemDefaults` has no `kind` field
.lsp_completion(false)
.and_then(|lsp_completion| lsp_completion.kind)
.and_then(|lsp_completion_kind| match lsp_completion_kind {
lsp::CompletionItemKind::KEYWORD => Some(0),
@ -4654,7 +4716,8 @@ impl Completion {
/// Whether this completion is a snippet.
pub fn is_snippet(&self) -> bool {
self.source
.lsp_completion()
// `lsp::CompletionListItemDefaults` has `insert_text_format` field
.lsp_completion(true)
.map_or(false, |lsp_completion| {
lsp_completion.insert_text_format == Some(lsp::InsertTextFormat::SNIPPET)
})
@ -4664,9 +4727,10 @@ impl Completion {
///
/// Will return `None` if this completion's kind is not [`CompletionItemKind::COLOR`].
pub fn color(&self) -> Option<Hsla> {
let lsp_completion = self.source.lsp_completion()?;
// `lsp::CompletionListItemDefaults` has no `kind` field
let lsp_completion = self.source.lsp_completion(false)?;
if lsp_completion.kind? == CompletionItemKind::COLOR {
return color_extractor::extract_color(lsp_completion);
return color_extractor::extract_color(&lsp_completion);
}
None
}

1
crates/proto/proto/zed.proto
View file

@ -1000,6 +1000,7 @@ message Completion {
bytes lsp_completion = 5;
bool resolved = 6;
Source source = 7;
optional bytes lsp_defaults = 8;
enum Source {
Custom = 0;