Fix completion race conditions

helix-term/src/commands.rs

@@ -5,6 +5,7 @@ pub(crate) mod typed;
 pub use dap::*;
 use helix_vcs::Hunk;
 pub use lsp::*;
+use tokio::sync::oneshot;
 use tui::widgets::Row;
 pub use typed::*;
 
@@ -52,7 +53,10 @@ use crate::{
     filter_picker_entry,
     job::Callback,
     keymap::ReverseKeymap,
-    ui::{self, overlay::overlayed, FilePicker, Picker, Popup, Prompt, PromptEvent},
+    ui::{
+        self, editor::InsertEvent, overlay::overlayed, FilePicker, Picker, Popup, Prompt,
+        PromptEvent,
+    },
 };
 
 use crate::job::{self, Jobs};
@@ -4166,6 +4170,24 @@ pub fn completion(cx: &mut Context) {
         None => return,
     };
 
+    // setup a chanel that allows the request to be canceled
+    let (tx, rx) = oneshot::channel();
+    // set completion_request so that this request can be canceled
+    // by setting completion_request, the old channel stored there is dropped
+    // and the associated request is automatically dropped
+    cx.editor.completion_request_handle = Some(tx);
+    let future = async move {
+        tokio::select! {
+            biased;
+            _ = rx => {
+                Ok(serde_json::Value::Null)
+            }
+            res = future => {
+                res
+            }
+        }
+    };
+
     let trigger_offset = cursor;
 
     // TODO: trigger_offset should be the cursor offset but we also need a starting offset from where we want to apply
@@ -4176,12 +4198,35 @@ pub fn completion(cx: &mut Context) {
     iter.reverse();
     let offset = iter.take_while(|ch| chars::char_is_word(*ch)).count();
     let start_offset = cursor.saturating_sub(offset);
+    let savepoint = doc.savepoint(view);
+
+    let trigger_doc = doc.id();
+    let trigger_view = view.id;
+
+    // FIXME: The commands Context can only have a single callback
+    // which means it gets overwritten when executing keybindings
+    // with multiple commands or macros. This would mean that completion
+    // might be incorrectly applied when repeating the insertmode action
+    //
+    // TODO: to solve this either make cx.callback a Vec of callbacks or
+    // alternatively move `last_insert` to `helix_view::Editor`
+    cx.callback = Some(Box::new(
+        move |compositor: &mut Compositor, _cx: &mut compositor::Context| {
+            let ui = compositor.find::<ui::EditorView>().unwrap();
+            ui.last_insert.1.push(InsertEvent::RequestCompletion);
+        },
+    ));
 
     cx.callback(
         future,
         move |editor, compositor, response: Option<lsp::CompletionResponse>| {
-            if editor.mode != Mode::Insert {
-                // we're not in insert mode anymore
+            let (view, doc) = current_ref!(editor);
+            // check if the completion request is stale.
+            //
+            // Completions are completed asynchrounsly and therefore the user could
+            //switch document/view or leave insert mode. In all of thoise cases the
+            // completion should be discarded
+            if editor.mode != Mode::Insert || view.id != trigger_view || doc.id() != trigger_doc {
                 return;
             }
 
@@ -4203,6 +4248,7 @@ pub fn completion(cx: &mut Context) {
             let ui = compositor.find::<ui::EditorView>().unwrap();
             ui.set_completion(
                 editor,
+                savepoint,
                 items,
                 offset_encoding,
                 start_offset,

helix-term/src/ui/completion.rs

@@ -1,12 +1,13 @@
 use crate::compositor::{Component, Context, Event, EventResult};
 use helix_view::{
+    document::SavePoint,
     editor::CompleteAction,
     theme::{Modifier, Style},
     ViewId,
 };
 use tui::{buffer::Buffer as Surface, text::Span};
 
-use std::borrow::Cow;
+use std::{borrow::Cow, sync::Arc};
 
 use helix_core::{Change, Transaction};
 use helix_view::{graphics::Rect, Document, Editor};
@@ -101,6 +102,7 @@ impl Completion {
 
     pub fn new(
         editor: &Editor,
+        savepoint: Arc<SavePoint>,
         mut items: Vec<CompletionItem>,
         offset_encoding: helix_lsp::OffsetEncoding,
         start_offset: usize,
@@ -172,11 +174,10 @@ impl Completion {
             let (view, doc) = current!(editor);
 
             // if more text was entered, remove it
-            doc.restore(view);
+            doc.restore(view, &savepoint);
 
             match event {
                 PromptEvent::Abort => {
-                    doc.restore(view);
                     editor.last_completion = None;
                 }
                 PromptEvent::Update => {
@@ -193,7 +194,6 @@ impl Completion {
                     );
 
                     // initialize a savepoint
-                    doc.savepoint();
                     doc.apply(&transaction, view.id);
 
                     editor.last_completion = Some(CompleteAction {

helix-term/src/ui/editor.rs

@@ -21,14 +21,14 @@ use helix_core::{
     visual_offset_from_block, Position, Range, Selection, Transaction,
 };
 use helix_view::{
-    document::{Mode, SCRATCH_BUFFER_NAME},
+    document::{Mode, SavePoint, SCRATCH_BUFFER_NAME},
     editor::{CompleteAction, CursorShapeConfig},
     graphics::{Color, CursorKind, Modifier, Rect, Style},
     input::{KeyEvent, MouseButton, MouseEvent, MouseEventKind},
     keyboard::{KeyCode, KeyModifiers},
     Document, Editor, Theme, View,
 };
-use std::{num::NonZeroUsize, path::PathBuf, rc::Rc};
+use std::{mem::take, num::NonZeroUsize, path::PathBuf, rc::Rc, sync::Arc};
 
 use tui::buffer::Buffer as Surface;
 
@@ -39,7 +39,7 @@ pub struct EditorView {
     pub keymaps: Keymaps,
     on_next_key: Option<OnKeyCallback>,
     pseudo_pending: Vec<KeyEvent>,
-    last_insert: (commands::MappableCommand, Vec<InsertEvent>),
+    pub(crate) last_insert: (commands::MappableCommand, Vec<InsertEvent>),
     pub(crate) completion: Option<Completion>,
     spinners: ProgressSpinners,
 }
@@ -49,6 +49,7 @@ pub enum InsertEvent {
     Key(KeyEvent),
     CompletionApply(CompleteAction),
     TriggerCompletion,
+    RequestCompletion,
 }
 
 impl Default for EditorView {
@@ -820,6 +821,7 @@ impl EditorView {
                 (Mode::Insert, Mode::Normal) => {
                     // if exiting insert mode, remove completion
                     self.completion = None;
+                    cxt.editor.completion_request_handle = None;
 
                     // TODO: Use an on_mode_change hook to remove signature help
                     cxt.jobs.callback(async {
@@ -890,14 +892,18 @@ impl EditorView {
                 for _ in 0..cxt.editor.count.map_or(1, NonZeroUsize::into) {
                     // first execute whatever put us into insert mode
                     self.last_insert.0.execute(cxt);
+                    let mut last_savepoint = None;
+                    let mut last_request_savepoint = None;
                     // then replay the inputs
                     for key in self.last_insert.1.clone() {
                         match key {
                             InsertEvent::Key(key) => self.insert_mode(cxt, key),
                             InsertEvent::CompletionApply(compl) => {
                                 let (view, doc) = current!(cxt.editor);
 
-                                doc.restore(view);
+                                if let Some(last_savepoint) = last_savepoint.as_deref() {
+                                    doc.restore(view, last_savepoint);
+                                }
 
                                 let text = doc.text().slice(..);
                                 let cursor = doc.selection(view.id).primary().cursor(text);
@@ -914,8 +920,11 @@ impl EditorView {
                                 doc.apply(&tx, view.id);
                             }
                             InsertEvent::TriggerCompletion => {
-                                let (_, doc) = current!(cxt.editor);
-                                doc.savepoint();
+                                last_savepoint = take(&mut last_request_savepoint);
+                            }
+                            InsertEvent::RequestCompletion => {
+                                let (view, doc) = current!(cxt.editor);
+                                last_request_savepoint = Some(doc.savepoint(view));
                             }
                         }
                     }
@@ -940,26 +949,31 @@ impl EditorView {
         }
     }
 
+    #[allow(clippy::too_many_arguments)]
     pub fn set_completion(
         &mut self,
         editor: &mut Editor,
+        savepoint: Arc<SavePoint>,
         items: Vec<helix_lsp::lsp::CompletionItem>,
         offset_encoding: helix_lsp::OffsetEncoding,
         start_offset: usize,
         trigger_offset: usize,
         size: Rect,
     ) {
-        let mut completion =
-            Completion::new(editor, items, offset_encoding, start_offset, trigger_offset);
+        let mut completion = Completion::new(
+            editor,
+            savepoint,
+            items,
+            offset_encoding,
+            start_offset,
+            trigger_offset,
+        );
 
         if completion.is_empty() {
             // skip if we got no completion results
             return;
         }
 
-        // Immediately initialize a savepoint
-        doc_mut!(editor).savepoint();
-
         editor.last_completion = None;
         self.last_insert.1.push(InsertEvent::TriggerCompletion);
 
@@ -972,8 +986,6 @@ impl EditorView {
         self.completion = None;
 
         // Clear any savepoints
-        let doc = doc_mut!(editor);
-        doc.savepoint = None;
         editor.clear_idle_timer(); // don't retrigger
     }
 

helix-view/Cargo.toml

@@ -43,6 +43,7 @@ toml = "0.7"
 log = "~0.4"
 
 which = "4.4"
+parking_lot = "0.12.1"
 
 
 [target.'cfg(windows)'.dependencies]

helix-view/src/document.rs

@@ -9,6 +9,7 @@ use helix_core::text_annotations::TextAnnotations;
 use helix_core::Range;
 use helix_vcs::{DiffHandle, DiffProviderRegistry};
 
+use ::parking_lot::Mutex;
 use serde::de::{self, Deserialize, Deserializer};
 use serde::Serialize;
 use std::borrow::Cow;
@@ -18,7 +19,7 @@ use std::fmt::Display;
 use std::future::Future;
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
-use std::sync::Arc;
+use std::sync::{Arc, Weak};
 use std::time::SystemTime;
 
 use helix_core::{
@@ -105,6 +106,13 @@ pub struct DocumentSavedEvent {
 pub type DocumentSavedEventResult = Result<DocumentSavedEvent, anyhow::Error>;
 pub type DocumentSavedEventFuture = BoxFuture<'static, DocumentSavedEventResult>;
 
+#[derive(Debug)]
+pub struct SavePoint {
+    /// The view this savepoint is associated with
+    pub view: ViewId,
+    revert: Mutex<Transaction>,
+}
+
 pub struct Document {
     pub(crate) id: DocumentId,
     text: Rope,
@@ -136,7 +144,7 @@ pub struct Document {
     pub history: Cell<History>,
     pub config: Arc<dyn DynAccess<Config>>,
 
-    pub savepoint: Option<Transaction>,
+    savepoints: Vec<Weak<SavePoint>>,
 
     // Last time we wrote to the file. This will carry the time the file was last opened if there
     // were no saves.
@@ -389,7 +397,7 @@ impl Document {
             diagnostics: Vec::new(),
             version: 0,
             history: Cell::new(History::default()),
-            savepoint: None,
+            savepoints: Vec::new(),
             last_saved_time: SystemTime::now(),
             last_saved_revision: 0,
             modified_since_accessed: false,
@@ -846,11 +854,18 @@ impl Document {
             }
 
             // generate revert to savepoint
-            if self.savepoint.is_some() {
-                take_with(&mut self.savepoint, |prev_revert| {
-                    let revert = transaction.invert(&old_doc);
-                    Some(revert.compose(prev_revert.unwrap()))
-                });
+            if !self.savepoints.is_empty() {
+                let revert = transaction.invert(&old_doc);
+                self.savepoints
+                    .retain_mut(|save_point| match save_point.upgrade() {
+                        Some(savepoint) => {
+                            let mut revert_to_savepoint = savepoint.revert.lock();
+                            *revert_to_savepoint =
+                                revert.clone().compose(mem::take(&mut revert_to_savepoint));
+                            true
+                        }
+                        None => false,
+                    })
             }
 
             // update tree-sitter syntax tree
@@ -940,14 +955,39 @@ impl Document {
         self.undo_redo_impl(view, false)
     }
 
-    pub fn savepoint(&mut self) {
-        self.savepoint = Some(Transaction::new(self.text()));
+    /// Creates a reference counted snapshot (called savpepoint) of the document.
+    ///
+    /// The snapshot will remain valid (and updated) idenfinitly as long as ereferences to it exist.
+    /// Restoring the snapshot will restore the selection and the contents of the document to
+    /// the state it had when this function was called.
+    pub fn savepoint(&mut self, view: &View) -> Arc<SavePoint> {
+        let revert = Transaction::new(self.text()).with_selection(self.selection(view.id).clone());
+        let savepoint = Arc::new(SavePoint {
+            view: view.id,
+            revert: Mutex::new(revert),
+        });
+        self.savepoints.push(Arc::downgrade(&savepoint));
+        savepoint
     }
 
-    pub fn restore(&mut self, view: &mut View) {
-        if let Some(revert) = self.savepoint.take() {
-            self.apply(&revert, view.id);
-        }
+    pub fn restore(&mut self, view: &mut View, savepoint: &SavePoint) {
+        assert_eq!(
+            savepoint.view, view.id,
+            "Savepoint must not be used with a different view!"
+        );
+        // search and remove savepoint using a ptr comparison
+        // this avoids a deadlock as we need to lock the mutex
+        let savepoint_idx = self
+            .savepoints
+            .iter()
+            .position(|savepoint_ref| savepoint_ref.as_ptr() == savepoint as *const _)
+            .expect("Savepoint must belong to this document");
+
+        let savepoint_ref = self.savepoints.remove(savepoint_idx);
+        let mut revert = savepoint.revert.lock();
+        self.apply(&revert, view.id);
+        *revert = Transaction::new(self.text()).with_selection(self.selection(view.id).clone());
+        self.savepoints.push(savepoint_ref)
     }
 
     fn earlier_later_impl(&mut self, view: &mut View, uk: UndoKind, earlier: bool) -> bool {