From 90b3f1c09f08bda60a2c6f2274b0beb7cbc24ff6 Mon Sep 17 00:00:00 2001
From: Michael Davis <mcarsondavis@gmail.com>
Date: Wed, 24 Jul 2024 12:04:42 -0400
Subject: [PATCH] Reorganize Document::apply_impl

These changes are ported from
<https://redirect.github.com/helix-editor/helix/pull/9801>. It's a
cleanup of `Document::apply_impl` that uses some early returns to
reduce nesting and some reordering of the steps. The early returns
bail out of `apply_impl` early if the transaction fails to apply or
if the changes are empty (in which case we emit the SelectionDidChange
event). It's a somewhat cosmetic refactor that makes the function easier
to reason about but it also makes it harder to introduce bugs by mapping
positions through empty changesets for example.

Co-authored-by: Pascal Kuthe <pascalkuthe@pm.me>
---
 helix-view/src/document.rs | 275 +++++++++++++++++++------------------
 1 file changed, 142 insertions(+), 133 deletions(-)

diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs
index 532f4c344c63..4895551129c8 100644
--- a/helix-view/src/document.rs
+++ b/helix-view/src/document.rs
@@ -1222,34 +1222,12 @@ impl Document {
         use helix_core::Assoc;
 
         let old_doc = self.text().clone();
+        let changes = transaction.changes();
+        if !changes.apply(&mut self.text) {
+            return false;
+        }
 
-        let success = transaction.changes().apply(&mut self.text);
-
-        if success {
-            if emit_lsp_notification {
-                helix_event::dispatch(DocumentDidChange {
-                    doc: self,
-                    view: view_id,
-                    old_text: &old_doc,
-                });
-            }
-
-            for selection in self.selections.values_mut() {
-                *selection = selection
-                    .clone()
-                    // Map through changes
-                    .map(transaction.changes())
-                    // Ensure all selections across all views still adhere to invariants.
-                    .ensure_invariants(self.text.slice(..));
-            }
-
-            for view_data in self.view_data.values_mut() {
-                view_data.view_position.anchor = transaction
-                    .changes()
-                    .map_pos(view_data.view_position.anchor, Assoc::Before);
-            }
-
-            // if specified, the current selection should instead be replaced by transaction.selection
+        if changes.is_empty() {
             if let Some(selection) = transaction.selection() {
                 self.selections.insert(
                     view_id,
@@ -1260,129 +1238,160 @@ impl Document {
                     view: view_id,
                 });
             }
+            return true;
+        }
 
-            self.modified_since_accessed = true;
+        self.modified_since_accessed = true;
+        self.version += 1;
+
+        for selection in self.selections.values_mut() {
+            *selection = selection
+                .clone()
+                // Map through changes
+                .map(transaction.changes())
+                // Ensure all selections across all views still adhere to invariants.
+                .ensure_invariants(self.text.slice(..));
         }
 
-        if !transaction.changes().is_empty() {
-            self.version += 1;
-            // start computing the diff in parallel
-            if let Some(diff_handle) = &self.diff_handle {
-                diff_handle.update_document(self.text.clone(), false);
-            }
+        for view_data in self.view_data.values_mut() {
+            view_data.view_position.anchor = transaction
+                .changes()
+                .map_pos(view_data.view_position.anchor, Assoc::Before);
+        }
 
-            // generate revert to savepoint
-            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,
-                    })
+        // generate revert to savepoint
+        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
+        if let Some(syntax) = &mut self.syntax {
+            // TODO: no unwrap
+            let res = syntax.update(
+                old_doc.slice(..),
+                self.text.slice(..),
+                transaction.changes(),
+            );
+            if res.is_err() {
+                log::error!("TS parser failed, disabling TS for the current buffer: {res:?}");
+                self.syntax = None;
             }
+        }
 
-            // update tree-sitter syntax tree
-            if let Some(syntax) = &mut self.syntax {
-                // TODO: no unwrap
-                let res = syntax.update(
-                    old_doc.slice(..),
-                    self.text.slice(..),
-                    transaction.changes(),
-                );
-                if res.is_err() {
-                    log::error!("TS parser failed, disabling TS for the current buffer: {res:?}");
-                    self.syntax = None;
-                }
+        // TODO: all of that should likely just be hooks
+        // start computing the diff in parallel
+        if let Some(diff_handle) = &self.diff_handle {
+            diff_handle.update_document(self.text.clone(), false);
+        }
+
+        // map diagnostics over changes too
+        changes.update_positions(self.diagnostics.iter_mut().map(|diagnostic| {
+            let assoc = if diagnostic.starts_at_word {
+                Assoc::BeforeWord
+            } else {
+                Assoc::After
+            };
+            (&mut diagnostic.range.start, assoc)
+        }));
+        changes.update_positions(self.diagnostics.iter_mut().filter_map(|diagnostic| {
+            if diagnostic.zero_width {
+                // for zero width diagnostics treat the diagnostic as a point
+                // rather than a range
+                return None;
+            }
+            let assoc = if diagnostic.ends_at_word {
+                Assoc::AfterWord
+            } else {
+                Assoc::Before
+            };
+            Some((&mut diagnostic.range.end, assoc))
+        }));
+        self.diagnostics.retain_mut(|diagnostic| {
+            if diagnostic.zero_width {
+                diagnostic.range.end = diagnostic.range.start
+            } else if diagnostic.range.start >= diagnostic.range.end {
+                return false;
             }
+            diagnostic.line = self.text.char_to_line(diagnostic.range.start);
+            true
+        });
 
-            let changes = transaction.changes();
+        self.diagnostics
+            .sort_by_key(|diagnostic| (diagnostic.range, diagnostic.severity, diagnostic.provider));
 
-            // map diagnostics over changes too
-            changes.update_positions(self.diagnostics.iter_mut().map(|diagnostic| {
-                let assoc = if diagnostic.starts_at_word {
-                    Assoc::BeforeWord
-                } else {
-                    Assoc::After
-                };
-                (&mut diagnostic.range.start, assoc)
-            }));
-            changes.update_positions(self.diagnostics.iter_mut().filter_map(|diagnostic| {
-                if diagnostic.zero_width {
-                    // for zero width diagnostics treat the diagnostic as a point
-                    // rather than a range
-                    return None;
-                }
-                let assoc = if diagnostic.ends_at_word {
-                    Assoc::AfterWord
-                } else {
-                    Assoc::Before
-                };
-                Some((&mut diagnostic.range.end, assoc))
-            }));
-            self.diagnostics.retain_mut(|diagnostic| {
-                if diagnostic.zero_width {
-                    diagnostic.range.end = diagnostic.range.start
-                } else if diagnostic.range.start >= diagnostic.range.end {
-                    return false;
-                }
-                diagnostic.line = self.text.char_to_line(diagnostic.range.start);
-                true
-            });
+        // Update the inlay hint annotations' positions, helping ensure they are displayed in the proper place
+        let apply_inlay_hint_changes = |annotations: &mut Vec<InlineAnnotation>| {
+            changes.update_positions(
+                annotations
+                    .iter_mut()
+                    .map(|annotation| (&mut annotation.char_idx, Assoc::After)),
+            );
+        };
 
-            self.diagnostics.sort_by_key(|diagnostic| {
-                (diagnostic.range, diagnostic.severity, diagnostic.provider)
-            });
+        self.inlay_hints_oudated = true;
+        for text_annotation in self.inlay_hints.values_mut() {
+            let DocumentInlayHints {
+                id: _,
+                type_inlay_hints,
+                parameter_inlay_hints,
+                other_inlay_hints,
+                padding_before_inlay_hints,
+                padding_after_inlay_hints,
+            } = text_annotation;
+
+            apply_inlay_hint_changes(padding_before_inlay_hints);
+            apply_inlay_hint_changes(type_inlay_hints);
+            apply_inlay_hint_changes(parameter_inlay_hints);
+            apply_inlay_hint_changes(other_inlay_hints);
+            apply_inlay_hint_changes(padding_after_inlay_hints);
+        }
 
-            // Update the inlay hint annotations' positions, helping ensure they are displayed in the proper place
-            let apply_inlay_hint_changes = |annotations: &mut Vec<InlineAnnotation>| {
-                changes.update_positions(
-                    annotations
-                        .iter_mut()
-                        .map(|annotation| (&mut annotation.char_idx, Assoc::After)),
-                );
-            };
+        helix_event::dispatch(DocumentDidChange {
+            doc: self,
+            view: view_id,
+            old_text: &old_doc,
+        });
 
-            self.inlay_hints_oudated = true;
-            for text_annotation in self.inlay_hints.values_mut() {
-                let DocumentInlayHints {
-                    id: _,
-                    type_inlay_hints,
-                    parameter_inlay_hints,
-                    other_inlay_hints,
-                    padding_before_inlay_hints,
-                    padding_after_inlay_hints,
-                } = text_annotation;
-
-                apply_inlay_hint_changes(padding_before_inlay_hints);
-                apply_inlay_hint_changes(type_inlay_hints);
-                apply_inlay_hint_changes(parameter_inlay_hints);
-                apply_inlay_hint_changes(other_inlay_hints);
-                apply_inlay_hint_changes(padding_after_inlay_hints);
-            }
+        // if specified, the current selection should instead be replaced by transaction.selection
+        if let Some(selection) = transaction.selection() {
+            self.selections.insert(
+                view_id,
+                selection.clone().ensure_invariants(self.text.slice(..)),
+            );
+            helix_event::dispatch(SelectionDidChange {
+                doc: self,
+                view: view_id,
+            });
+        }
 
-            if emit_lsp_notification {
-                // TODO: move to hook
-                // emit lsp notification
-                for language_server in self.language_servers() {
-                    let notify = language_server.text_document_did_change(
-                        self.versioned_identifier(),
-                        &old_doc,
-                        self.text(),
-                        changes,
-                    );
+        if emit_lsp_notification {
+            // TODO: move to hook
+            // emit lsp notification
+            for language_server in self.language_servers() {
+                let notify = language_server.text_document_did_change(
+                    self.versioned_identifier(),
+                    &old_doc,
+                    self.text(),
+                    changes,
+                );
 
-                    if let Some(notify) = notify {
-                        tokio::spawn(notify);
-                    }
+                if let Some(notify) = notify {
+                    tokio::spawn(notify);
                 }
             }
         }
-        success
+
+        true
     }
 
     fn apply_inner(