kanielrkirby
commented
4 months ago I poked around in the code a bit, mostly to learn the codebase a little better. I'll start with the editor design side of things.
In editors like VSCode, transactions are oriented around spaces, but this doesn't follow the conventions of modal editors like (Neo)Vim and Kakoune, which only make transactions after exiting insert mode (e.g., enter insert mode, type stuff, exit insert mode, undo, everything done in insert mode is undone).
Here's the relevant code at helix-term/src/ui/editor.rs#1432:
...
Event::Key(mut key) => {
...
// Store a history state if not in insert mode. This also takes care of
// committing changes when leaving insert mode.
if mode != Mode::Insert {
doc.append_changes_to_history(view);
}
...
}
...There's a couple solutions, most likely of which (in my opinion) is that this won't be supported. Another option is to start committing transactions every space like VSCode does, but this will change how a very common functionality in modal editors works, so I'm not personally keen on that. A middle ground could be that we create a temporary Vec of Transaction/Revisions when entering insert mode that works like normal editors, and that gets cleared when exiting insert mode.
Now, onto the actual bug, the panic.
thread 'main' panicked at helix-core/src/transaction.rs:293:9:
assertion failed: original_doc.len_chars() == self.len
...
12: 0x56051c5dcaa7 - helix_core::transaction::ChangeSet::invert::ha8b1f55c37e72755The bug is that (with a non-empty History), if you go into insert mode, make some changes, and try to undo them whilst in insert mode, the underlying ChangeSet hasn't been updated with the document's length, and so it panics from an assert that was added into invert (as well as other methods like this):
/// Returns a new changeset that reverts this one. Useful for `undo` implementation.
/// The document parameter expects the original document before this change was applied.
pub fn invert(&self, original_doc: &Rope) -> Self {
assert!(original_doc.len_chars() == self.len); // panics here#[derive(Debug, Default, Clone, PartialEq, Eq)]
pub struct ChangeSet {
pub(crate) changes: Vec<Operation>,
/// The required document length. Will refuse to apply changes unless it matches.
len: usize,
len_after: usize,
}I'm still learning the codebase, so I'm not positive what the best way to address this is. I think it partly depends on what the preferred approach to "Undo in insert mode" is, whether that is that it's not supported, a temporary insert-specific Vec, or simply making more Transactions (though even that won't work without addressing the original_doc.len == ChangeSet.len constraint).
Another note, this also happens with :later, and there's probably some other ways to trigger this bug with other commands in insert mode that eventually trigger compose or invert.
TLDR is that the invert function is being a little too safe here, and doesn't expect earlier/later/undo/redo/etc. to be called without Revisions and the doc being in sync.
the-mikedavis
adminy
Summary
here is what I have in the config:
Reproduction Steps
Now I set the editor in insert mode, I type a bunch of characters then I press
Ctrl+z, says there is nothing to undo, so I keep typing more characters, thenCtrl+zthen crash:Helix log
~/.cache/helix/helix.log
``` empty?! ```Platform
Linux (NixOS)
Terminal Emulator
alacritty 0.13.2
Installation Method
nixpkgs master
Helix Version
helix 24.3 (2cadec0b)