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

@@ -4674,61 +4674,69 @@ impl Editor {
             snippet = None;
             new_text = completion.new_text.clone();
         };
-        let selections = self.selections.all::<usize>(cx);
 
         let replace_range = choose_completion_range(&completion, intent, &buffer_handle, cx);
         let buffer = buffer_handle.read(cx);
-        let old_text = buffer
+        let snapshot = self.buffer.read(cx).snapshot(cx);
-            .text_for_range(replace_range.clone())
+        let replace_range_multibuffer = {
-            .collect::<String>();
+            let excerpt = snapshot
-
+                .excerpt_containing(self.selections.newest_anchor().range())
-        let newest_selection = self.selections.newest_anchor();
+                .unwrap();
-        if newest_selection.start.buffer_id != Some(buffer_handle.read(cx).remote_id()) {
+            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::<String>();
+        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));
+            .saturating_sub(newest_anchor.end.text_anchor.to_offset(buffer));
-        let mut common_prefix_len = 0;
+        let prefix = &old_text[..old_text.len() - lookahead];
-        for (a, b) in old_text.chars().zip(new_text.chars()) {
+        let suffix = &old_text[lookbehind..];
-            if a == b {
-                common_prefix_len += a.len_utf8();
-            } else {
-                break;
-            }
-        }
 
-        let snapshot = self.buffer.read(cx).snapshot(cx);
+        let selections = self.selections.all::<usize>(cx);
-        let mut range_to_replace: Option<Range<usize>> = None;
+        let mut edits = Vec::new();
-        let mut ranges = 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 edit = if selection.id == newest_anchor.id {
-                let start = selection.start.saturating_sub(lookbehind);
+                (replace_range_multibuffer.clone(), new_text.as_str())
-                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);
             } else {
-                common_prefix_len = 0;
+                let mut range = selection.range();
-                ranges.clear();
+                let mut text = new_text.as_str();
-                ranges.extend(selections.iter().map(|s| {
+
-                    if s.id == newest_selection.id {
+                // if prefix is present, don't duplicate it
-                        range_to_replace = Some(replace_range.clone());
+                if snapshot.contains_str_at(range.start.saturating_sub(lookbehind), prefix) {
-                        replace_range.clone()
+                    text = &new_text[lookbehind..];
-                    } else {
+
-                        s.start..s.end
+                    // 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(
+                        linked_edits
-                            edits
+                            .entry(buffer.clone())
-                                .into_iter()
+                            .or_default()
-                                .map(|range| (range, new_text[common_prefix_len..].to_owned())),
+                            .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::<OffsetUtf16>(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,
+            utf16_range_to_replace: None,
-            text: text.into(),
+            text: new_text.clone().into(),
         });
 
         self.transact(window, cx, |this, window, cx| {
             if let Some(mut snippet) = snippet {
-                snippet.text = text.to_string();
+                snippet.text = new_text.to_string();
-                for tabstop in snippet
+                let ranges = edits
-                    .tabstops
+                    .iter()
-                    .iter_mut()
+                    .map(|(range, _)| range.clone())
-                    .flat_map(|tabstop| tabstop.ranges.iter_mut())
+                    .collect::<Vec<_>>();
-                {
-                    tabstop.start -= common_prefix_len as isize;
-                    tabstop.end -= common_prefix_len as isize;
-                }
-
                 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

crates/editor/src/editor_tests.rs

@@ -9677,9 +9677,9 @@ async fn test_completion_mode(cx: &mut TestAppContext) {
             buffer_marked_text: "before <edi|tor> 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_mode: "before editorˇ after".into(),
-            expected_with_replace_subsequence_mode: "before ediˇtor after".into(),
+            expected_with_replace_subsequence_mode: "before editorˇ after".into(),
-            expected_with_replace_suffix_mode: "before ediˇtor 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: "[<el|element>]".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.<to_off|suffix>  // 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::<lsp::request::Completion, _, _>(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, |_| {});

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<Editor>,
 ) {
     let (unmarked_text, text_ranges) = marked_text_ranges(marked_text, true);
-    assert_eq!(editor.text(cx), unmarked_text);
+    assert_eq!(editor.text(cx), unmarked_text, "text doesn't match");
-    assert_eq!(editor.selections.ranges(cx), text_ranges);
+    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

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
     }
 }