From ff41be30dc4b838d7e81996544d645a742ef1cb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Marcos?= Date: Mon, 14 Apr 2025 15:09:28 -0300 Subject: [PATCH] Fix bugs with multicursor completions (#28586) Release Notes: - Fixed completions with multiple cursors leaving duplicated prefixes. - Fixed crash when accepting a completion in a multibuffer with multiple cursors. - Vim: improved `single-repeat` after accepting a completion, now pressing `.` to replay the completion will re-insert the completion text at the cursor position. --- crates/editor/src/editor.rs | 121 +++++------ crates/editor/src/editor_tests.rs | 274 +++++++++++++++++++++++- crates/editor/src/test.rs | 15 +- crates/multi_buffer/src/multi_buffer.rs | 7 +- 4 files changed, 341 insertions(+), 76 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index d00d8e9b39..c80670bd45 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -4674,61 +4674,69 @@ impl Editor { snippet = None; new_text = completion.new_text.clone(); }; - let selections = self.selections.all::(cx); let replace_range = choose_completion_range(&completion, intent, &buffer_handle, cx); let buffer = buffer_handle.read(cx); - let old_text = buffer - .text_for_range(replace_range.clone()) - .collect::(); - - let newest_selection = self.selections.newest_anchor(); - if newest_selection.start.buffer_id != Some(buffer_handle.read(cx).remote_id()) { + let snapshot = self.buffer.read(cx).snapshot(cx); + let replace_range_multibuffer = { + let excerpt = snapshot + .excerpt_containing(self.selections.newest_anchor().range()) + .unwrap(); + let multibuffer_anchor = snapshot + .anchor_in_excerpt(excerpt.id(), buffer.anchor_before(replace_range.start)) + .unwrap() + ..snapshot + .anchor_in_excerpt(excerpt.id(), buffer.anchor_before(replace_range.end)) + .unwrap(); + multibuffer_anchor.start.to_offset(&snapshot) + ..multibuffer_anchor.end.to_offset(&snapshot) + }; + let newest_anchor = self.selections.newest_anchor(); + if newest_anchor.head().buffer_id != Some(buffer.remote_id()) { return None; } - let lookbehind = newest_selection + let old_text = buffer + .text_for_range(replace_range.clone()) + .collect::(); + let lookbehind = newest_anchor .start .text_anchor .to_offset(buffer) .saturating_sub(replace_range.start); let lookahead = replace_range .end - .saturating_sub(newest_selection.end.text_anchor.to_offset(buffer)); - let mut common_prefix_len = 0; - for (a, b) in old_text.chars().zip(new_text.chars()) { - if a == b { - common_prefix_len += a.len_utf8(); - } else { - break; - } - } + .saturating_sub(newest_anchor.end.text_anchor.to_offset(buffer)); + let prefix = &old_text[..old_text.len() - lookahead]; + let suffix = &old_text[lookbehind..]; - let snapshot = self.buffer.read(cx).snapshot(cx); - let mut range_to_replace: Option> = None; - let mut ranges = Vec::new(); + let selections = self.selections.all::(cx); + let mut edits = Vec::new(); let mut linked_edits = HashMap::<_, Vec<_>>::default(); + for selection in &selections { - if snapshot.contains_str_at(selection.start.saturating_sub(lookbehind), &old_text) { - let start = selection.start.saturating_sub(lookbehind); - let end = selection.end + lookahead; - if selection.id == newest_selection.id { - range_to_replace = Some(start + common_prefix_len..end); - } - ranges.push(start + common_prefix_len..end); + let edit = if selection.id == newest_anchor.id { + (replace_range_multibuffer.clone(), new_text.as_str()) } else { - common_prefix_len = 0; - ranges.clear(); - ranges.extend(selections.iter().map(|s| { - if s.id == newest_selection.id { - range_to_replace = Some(replace_range.clone()); - replace_range.clone() - } else { - s.start..s.end + let mut range = selection.range(); + let mut text = new_text.as_str(); + + // if prefix is present, don't duplicate it + if snapshot.contains_str_at(range.start.saturating_sub(lookbehind), prefix) { + text = &new_text[lookbehind..]; + + // if suffix is also present, mimic the newest cursor and replace it + if selection.id != newest_anchor.id + && snapshot.contains_str_at(range.end, suffix) + { + range.end += lookahead; } - })); - break; - } + } + (range, text) + }; + + edits.push(edit); + if !self.linked_edit_ranges.is_empty() { let start_anchor = snapshot.anchor_before(selection.head()); let end_anchor = snapshot.anchor_after(selection.tail()); @@ -4736,45 +4744,30 @@ impl Editor { .linked_editing_ranges_for(start_anchor.text_anchor..end_anchor.text_anchor, cx) { for (buffer, edits) in ranges { - linked_edits.entry(buffer.clone()).or_default().extend( - edits - .into_iter() - .map(|range| (range, new_text[common_prefix_len..].to_owned())), - ); + linked_edits + .entry(buffer.clone()) + .or_default() + .extend(edits.into_iter().map(|range| (range, new_text.to_owned()))); } } } } - let text = &new_text[common_prefix_len..]; - let utf16_range_to_replace = range_to_replace.map(|range| { - let newest_selection = self.selections.newest::(cx).range(); - let selection_start_utf16 = newest_selection.start.0 as isize; - - range.start.to_offset_utf16(&snapshot).0 as isize - selection_start_utf16 - ..range.end.to_offset_utf16(&snapshot).0 as isize - selection_start_utf16 - }); cx.emit(EditorEvent::InputHandled { - utf16_range_to_replace, - text: text.into(), + utf16_range_to_replace: None, + text: new_text.clone().into(), }); self.transact(window, cx, |this, window, cx| { if let Some(mut snippet) = snippet { - snippet.text = text.to_string(); - for tabstop in snippet - .tabstops - .iter_mut() - .flat_map(|tabstop| tabstop.ranges.iter_mut()) - { - tabstop.start -= common_prefix_len as isize; - tabstop.end -= common_prefix_len as isize; - } - + snippet.text = new_text.to_string(); + let ranges = edits + .iter() + .map(|(range, _)| range.clone()) + .collect::>(); this.insert_snippet(&ranges, snippet, window, cx).log_err(); } else { this.buffer.update(cx, |buffer, cx| { - let edits = ranges.iter().map(|range| (range.clone(), text)); let auto_indent = if completion.insert_text_mode == Some(InsertTextMode::AS_IS) { None diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 78f9e67df6..51647b0226 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -9677,9 +9677,9 @@ async fn test_completion_mode(cx: &mut TestAppContext) { buffer_marked_text: "before after".into(), completion_text: "editor", expected_with_insert_mode: "before editorˇtor after".into(), - expected_with_replace_mode: "before ediˇtor after".into(), - expected_with_replace_subsequence_mode: "before ediˇtor after".into(), - expected_with_replace_suffix_mode: "before ediˇtor after".into(), + expected_with_replace_mode: "before editorˇ after".into(), + expected_with_replace_subsequence_mode: "before editorˇ after".into(), + expected_with_replace_suffix_mode: "before editorˇ after".into(), }, Run { run_description: "End of word matches completion text -- cursor at end", @@ -9727,9 +9727,9 @@ async fn test_completion_mode(cx: &mut TestAppContext) { buffer_marked_text: "[]".into(), completion_text: "element", expected_with_insert_mode: "[elementˇelement]".into(), - expected_with_replace_mode: "[elˇement]".into(), + expected_with_replace_mode: "[elementˇ]".into(), expected_with_replace_subsequence_mode: "[elementˇelement]".into(), - expected_with_replace_suffix_mode: "[elˇement]".into(), + expected_with_replace_suffix_mode: "[elementˇ]".into(), }, Run { run_description: "Ends with matching suffix", @@ -9923,6 +9923,270 @@ async fn test_completion_with_mode_specified_by_action(cx: &mut TestAppContext) apply_additional_edits.await.unwrap(); } +#[gpui::test] +async fn test_completion_replacing_suffix_in_multicursors(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + let mut cx = EditorLspTestContext::new_rust( + lsp::ServerCapabilities { + completion_provider: Some(lsp::CompletionOptions { + resolve_provider: Some(true), + ..Default::default() + }), + ..Default::default() + }, + cx, + ) + .await; + + let initial_state = indoc! {" + 1. buf.to_offˇsuffix + 2. buf.to_offˇsuf + 3. buf.to_offˇfix + 4. buf.to_offˇ + 5. into_offˇensive + 6. ˇsuffix + 7. let ˇ // + 8. aaˇzz + 9. buf.to_off«zzzzzˇ»suffix + 10. buf.«ˇzzzzz»suffix + 11. to_off«ˇzzzzz» + + buf.to_offˇsuffix // newest cursor + "}; + let completion_marked_buffer = indoc! {" + 1. buf.to_offsuffix + 2. buf.to_offsuf + 3. buf.to_offfix + 4. buf.to_off + 5. into_offensive + 6. suffix + 7. let // + 8. aazz + 9. buf.to_offzzzzzsuffix + 10. buf.zzzzzsuffix + 11. to_offzzzzz + + buf. // newest cursor + "}; + let completion_text = "to_offset"; + let expected = indoc! {" + 1. buf.to_offsetˇ + 2. buf.to_offsetˇsuf + 3. buf.to_offsetˇfix + 4. buf.to_offsetˇ + 5. into_offsetˇensive + 6. to_offsetˇsuffix + 7. let to_offsetˇ // + 8. aato_offsetˇzz + 9. buf.to_offsetˇ + 10. buf.to_offsetˇsuffix + 11. to_offsetˇ + + buf.to_offsetˇ // newest cursor + "}; + + cx.set_state(initial_state); + cx.update_editor(|editor, window, cx| { + editor.show_completions(&ShowCompletions { trigger: None }, window, cx); + }); + + let counter = Arc::new(AtomicUsize::new(0)); + handle_completion_request_with_insert_and_replace( + &mut cx, + completion_marked_buffer, + vec![completion_text], + counter.clone(), + ) + .await; + cx.condition(|editor, _| editor.context_menu_visible()) + .await; + assert_eq!(counter.load(atomic::Ordering::Acquire), 1); + + let apply_additional_edits = cx.update_editor(|editor, window, cx| { + editor + .confirm_completion_replace(&ConfirmCompletionReplace, window, cx) + .unwrap() + }); + cx.assert_editor_state(expected); + handle_resolve_completion_request(&mut cx, None).await; + apply_additional_edits.await.unwrap(); +} + +// This used to crash +#[gpui::test] +async fn test_completion_in_multibuffer_with_replace_range(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + + let buffer_text = indoc! {" + fn main() { + 10.satu; + + // + // separate cursors so they open in different excerpts (manually reproducible) + // + + 10.satu20; + } + "}; + let multibuffer_text_with_selections = indoc! {" + fn main() { + 10.satuˇ; + + // + + // + + 10.satuˇ20; + } + "}; + let expected_multibuffer = indoc! {" + fn main() { + 10.saturating_sub()ˇ; + + // + + // + + 10.saturating_sub()ˇ; + } + "}; + + let first_excerpt_end = buffer_text.find("//").unwrap() + 3; + let second_excerpt_end = buffer_text.rfind("//").unwrap() - 4; + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/a"), + json!({ + "main.rs": buffer_text, + }), + ) + .await; + + let project = Project::test(fs, [path!("/a").as_ref()], cx).await; + let language_registry = project.read_with(cx, |project, _| project.languages().clone()); + language_registry.add(rust_lang()); + let mut fake_servers = language_registry.register_fake_lsp( + "Rust", + FakeLspAdapter { + capabilities: lsp::ServerCapabilities { + completion_provider: Some(lsp::CompletionOptions { + resolve_provider: None, + ..lsp::CompletionOptions::default() + }), + ..lsp::ServerCapabilities::default() + }, + ..FakeLspAdapter::default() + }, + ); + let workspace = cx.add_window(|window, cx| Workspace::test_new(project.clone(), window, cx)); + let cx = &mut VisualTestContext::from_window(*workspace, cx); + let buffer = project + .update(cx, |project, cx| { + project.open_local_buffer(path!("/a/main.rs"), cx) + }) + .await + .unwrap(); + + let multi_buffer = cx.new(|cx| { + let mut multi_buffer = MultiBuffer::new(Capability::ReadWrite); + multi_buffer.push_excerpts( + buffer.clone(), + [ExcerptRange::new(0..first_excerpt_end)], + cx, + ); + multi_buffer.push_excerpts( + buffer.clone(), + [ExcerptRange::new(second_excerpt_end..buffer_text.len())], + cx, + ); + multi_buffer + }); + + let editor = workspace + .update(cx, |_, window, cx| { + cx.new(|cx| { + Editor::new( + EditorMode::Full { + scale_ui_elements_with_buffer_font_size: false, + show_active_line_background: false, + }, + multi_buffer.clone(), + Some(project.clone()), + window, + cx, + ) + }) + }) + .unwrap(); + + let pane = workspace + .update(cx, |workspace, _, _| workspace.active_pane().clone()) + .unwrap(); + pane.update_in(cx, |pane, window, cx| { + pane.add_item(Box::new(editor.clone()), true, true, None, window, cx); + }); + + let fake_server = fake_servers.next().await.unwrap(); + + editor.update_in(cx, |editor, window, cx| { + editor.change_selections(None, window, cx, |s| { + s.select_ranges([ + Point::new(1, 11)..Point::new(1, 11), + Point::new(7, 11)..Point::new(7, 11), + ]) + }); + + assert_text_with_selections(editor, multibuffer_text_with_selections, cx); + }); + + editor.update_in(cx, |editor, window, cx| { + editor.show_completions(&ShowCompletions { trigger: None }, window, cx); + }); + + fake_server + .set_request_handler::(move |_, _| async move { + let completion_item = lsp::CompletionItem { + label: "saturating_sub()".into(), + text_edit: Some(lsp::CompletionTextEdit::InsertAndReplace( + lsp::InsertReplaceEdit { + new_text: "saturating_sub()".to_owned(), + insert: lsp::Range::new( + lsp::Position::new(7, 7), + lsp::Position::new(7, 11), + ), + replace: lsp::Range::new( + lsp::Position::new(7, 7), + lsp::Position::new(7, 13), + ), + }, + )), + ..lsp::CompletionItem::default() + }; + + Ok(Some(lsp::CompletionResponse::Array(vec![completion_item]))) + }) + .next() + .await + .unwrap(); + + cx.condition(&editor, |editor, _| editor.context_menu_visible()) + .await; + + editor + .update_in(cx, |editor, window, cx| { + editor + .confirm_completion_replace(&ConfirmCompletionReplace, window, cx) + .unwrap() + }) + .await + .unwrap(); + + editor.update(cx, |editor, cx| { + assert_text_with_selections(editor, expected_multibuffer, cx); + }) +} + #[gpui::test] async fn test_completion(cx: &mut TestAppContext) { init_test(cx, |_| {}); diff --git a/crates/editor/src/test.rs b/crates/editor/src/test.rs index e207b9988e..b197c56bbc 100644 --- a/crates/editor/src/test.rs +++ b/crates/editor/src/test.rs @@ -1,8 +1,7 @@ pub mod editor_lsp_test_context; pub mod editor_test_context; -use std::sync::LazyLock; - +pub use crate::rust_analyzer_ext::expand_macro_recursively; use crate::{ DisplayPoint, Editor, EditorMode, FoldPlaceholder, MultiBuffer, display_map::{DisplayMap, DisplaySnapshot, ToDisplayPoint}, @@ -11,11 +10,11 @@ use gpui::{ AppContext as _, Context, Entity, Font, FontFeatures, FontStyle, FontWeight, Pixels, Window, font, }; +use pretty_assertions::assert_eq; use project::Project; +use std::sync::LazyLock; use util::test::{marked_text_offsets, marked_text_ranges}; -pub use crate::rust_analyzer_ext::expand_macro_recursively; - #[cfg(test)] #[ctor::ctor] fn init_logger() { @@ -96,8 +95,12 @@ pub fn assert_text_with_selections( cx: &mut Context, ) { let (unmarked_text, text_ranges) = marked_text_ranges(marked_text, true); - assert_eq!(editor.text(cx), unmarked_text); - assert_eq!(editor.selections.ranges(cx), text_ranges); + assert_eq!(editor.text(cx), unmarked_text, "text doesn't match"); + assert_eq!( + editor.selections.ranges(cx), + text_ranges, + "selections don't match", + ); } // RA thinks this is dead code even though it is used in a whole lot of tests diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 18c17b3b02..994815910c 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -7694,7 +7694,12 @@ impl ToOffset for Point { impl ToOffset for usize { #[track_caller] fn to_offset<'a>(&self, snapshot: &MultiBufferSnapshot) -> usize { - assert!(*self <= snapshot.len(), "offset is out of range"); + assert!( + *self <= snapshot.len(), + "offset {} is greater than the snapshot.len() {}", + *self, + snapshot.len(), + ); *self } }