diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index c59a15a7..a9ac1606 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -3954,7 +3954,7 @@ fn normal_mode(cx: &mut Context) { fn push_jump(view: &mut View, doc: &mut Document) { doc.append_changes_to_history(view); let jump = (doc.id(), doc.selection(view.id).clone()); - view.jumps.push(jump); + view.push_jump(doc, jump); } fn goto_line(cx: &mut Context) { diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs index a1f8898f..98079917 100644 --- a/helix-term/src/commands/typed.rs +++ b/helix-term/src/commands/typed.rs @@ -2141,7 +2141,7 @@ pub(super) fn goto_line_number( .expect("update_goto_line_number_preview should always set last_selection"); let (view, doc) = current!(cx.editor); - view.jumps.push((doc.id(), last_selection)); + view.push_jump(doc, (doc.id(), last_selection)); } // When a user hits backspace and there are no numbers left, diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index 1adc2a3e..12d2a2e2 100644 --- a/helix-term/src/ui/mod.rs +++ b/helix-term/src/ui/mod.rs @@ -147,7 +147,7 @@ pub fn raw_regex_prompt( if event == PromptEvent::Validate { // Equivalent to push_jump to store selection just before jump - view.jumps.push((doc_id, snapshot.clone())); + view.push_jump(doc, (doc_id, snapshot.clone())); } fun(cx, regex, input, event); diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 104adb39..3e795680 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -1898,7 +1898,7 @@ impl Editor { } } else { let jump = (view.doc, doc.selection(view.id).clone()); - view.jumps.push(jump); + view.push_jump(doc, jump); // Set last accessed doc if it is a different document if doc.id != id { view.add_to_history(view.doc); @@ -2511,11 +2511,13 @@ impl Editor { pub fn jump_backward(&mut self, view_id: ViewId, count: usize) { let view = view_mut!(self, view_id); - if let Some((doc_id, selection)) = view - .jumps - .backward(view_id, doc_mut!(self, &view.doc), count) - .cloned() - { + let doc = doc_mut!(self, &view.doc); + // `backward` may push the current selection (valid at the document's + // current revision) onto the jumplist. Sync first so the view's + // `doc_revisions` matches, otherwise that entry would be left ahead of + // it and a later sync would map it out of bounds. + view.sync_changes(doc); + if let Some((doc_id, selection)) = view.jumps.backward(view_id, doc, count).cloned() { self.jump_to(view_id, doc_id, selection); } } diff --git a/helix-view/src/view.rs b/helix-view/src/view.rs index 69db47d2..d211d8e6 100644 --- a/helix-view/src/view.rs +++ b/helix-view/src/view.rs @@ -56,7 +56,7 @@ impl JumpList { num_removed_from_front } - pub fn push(&mut self, jump: Jump) { + pub(crate) fn push(&mut self, jump: Jump) { self.push_impl(jump); } @@ -698,6 +698,16 @@ impl View { doc.history.get_mut().changes_since(current_revision) } + + pub fn push_jump(&mut self, doc: &mut Document, jump: (DocumentId, Selection)) { + // The pushed selection is valid at the document's *current* revision, so the + // view must be synced to that revision first. Otherwise the new entry would + // be left ahead of `doc_revisions[doc]`, and the next `sync_changes` would + // map it through a changeset whose pre-image predates it, panicking in + // `ChangeSet::update_positions` when the document has since grown. + self.sync_changes(doc); + self.jumps.push(jump); + } } #[cfg(test)] @@ -1173,4 +1183,73 @@ mod tests { assert_eq!(jumps.current, 0); assert_eq!(entries(&jumps), vec![doc_b, doc_b]); } + + /// `JumpList::push` records a jump using the document's *current* selection + /// (i.e. positions valid at the document's current history revision), but it + /// never advances the view's `doc_revisions` entry for that document. As a + /// result, the next `View::sync_changes` computes `changes_since(old_revision)` + /// and maps *every* jump - including the freshly pushed one - through a + /// changeset whose pre-image is the *older*, shorter document. A jump that + /// points past the end of that older document then runs off the end of the + /// changeset and panics in `ChangeSet::update_positions` + /// ("Positions ... are out of range for changeset len ..."). + /// + /// To trigger the stale `doc_revisions` window without reaching into private + /// fields we use two views over the same document: committing an edit through + /// `view2` advances the shared document's history and updates *view2*'s + /// `doc_revisions`, while `view1`'s `doc_revisions` is left pointing at the + /// pre-edit revision. Pushing a jump into `view1` afterwards reproduces the + /// exact situation `push` fails to guard against. + #[test] + fn jumplist_push_keeps_doc_revisions_in_sync() { + let config = Arc::new(ArcSwap::new(Arc::new(Config::default()))); + let loader = Arc::new(ArcSwap::from_pointee(syntax::Loader::default())); + + // Revision 0: a short document. + let mut doc = Document::from(Rope::from_str("ab"), None, config, loader); + + let mut view1 = View::new(doc.id(), GutterConfig::default()); + let mut view2 = View::new(doc.id(), GutterConfig::default()); + doc.ensure_view_init(view1.id); + doc.ensure_view_init(view2.id); + + // Sync view1 at revision 0 so its `doc_revisions` records the short doc. + view1.sync_changes(&mut doc); + assert_eq!(doc.get_current_revision(), 0); + + // Commit an insertion *through view2*, growing the document and advancing + // its history to revision 1. Only view2's `doc_revisions` is updated here; + // view1 still believes the document is at revision 0. + let insert = Transaction::change( + doc.text(), + std::iter::once((2, 2, Some("XXXXXXXXXX".into()))), + ); + assert!(doc.apply(&insert, view2.id)); + doc.append_changes_to_history(&mut view2); + assert_eq!(doc.get_current_revision(), 1); + let len = doc.text().len_chars(); + assert_eq!(len, 12); + + // Push a jump into view1 at the end of the *current* (revision 1) document. + // This selection is perfectly valid for the document as it stands now. + // + // `View::push_jump` syncs view1 to the document's current revision before + // appending, so the new entry and `doc_revisions` agree. The raw + // `view1.jumps.push(...)` would instead leave the entry ahead of + // `doc_revisions` (still revision 0), and the `sync_changes` below would + // map position 12 through the revision 0 -> 1 changeset (pre-image only + // 2 chars) and panic in `ChangeSet::update_positions`. + let jump = (doc.id(), Selection::point(len)); + view1.push_jump(&mut doc, jump); + + // With the fix in place this is a no-op for the freshly pushed entry and + // the jump remains a valid, in-bounds selection. + view1.sync_changes(&mut doc); + + let (_, selection) = view1.jumps.iter().next_back().unwrap(); + assert!( + selection.primary().head <= doc.text().len_chars(), + "jumplist selection must stay within document bounds after sync", + ); + } }