Fix panic in min_width_1/grapheme_aligned when Range extends past bounds

[TylerAldrich]

This change addresses a bug when using jump_forward / jump_backwards, when the saved selection in the jumplist contains a range that extends past the end of the document.

The easiest way to reproduce this bug is to write "hi", save the space after "hi" to the jumplist via ctrl+s, delete everything, use ctrl+o to jump and you receive

thread 'main' panicked at 'assertion failed: char_idx <= slice.len_chars()', helix-core/src/graphemes.rs:37:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

because the range extends past the slice length.

I am very unfamiliar with this codebase, and I did add some tests and can confirm that after my changes, the use case I described now no longer panics, but if this is not the correct place to implement this bounds checking, please let me know.

[cessen]

This seems like a reasonable place to fix this. But I am a little concerned that clamping like this might end up masking out-of-bounds bugs that should otherwise be fixed.

For example, although I haven't looked closely at all, there's a chance that this change is actually just masking the bug you're running into, which may also manifest in other ways than out-of-bounds.

[cessen]

If we're going to add the check here, let's add it to the list of checks in the comment as well.

[cessen]

Thumbs up here. This is a good method to have, regardless of whether we incorporate it into ensure_invariants() or not.

[TylerAldrich]

This seems like a reasonable place to fix this. But I am a little concerned that clamping like this might end up masking out-of-bounds bugs that should otherwise be fixed.

For example, although I haven't looked closely at all, there's a chance that this change is actually just masking the bug you're running into, which may also manifest in other ways than out-of-bounds.

This is a great point - being unfamiliar with the codebase I'm not exactly sure how many places use Selection/Ranges that may have subtle bugs hidden by this.

I did a little extra digging and from what I can tell, the cause of the panic can manifest in a few ways, but it's due to the view's JumpList containing a jump that is now out-of-bounds. After further experimentation it can happen in quite a few ways - e.g. from the last line of a file, use :goto <n>, saving your previous location in the JumpList. Then, removing that line will cause your next jump to panic due to this out-of-bounds.

It seems like this could feasibly be addressed when returning the Option<&Jump> from JumpList::forward/JumpList::backward (before the call to doc.set_selection, which uses ensure_invariants to fix the out-of-bounds range).

I did however push an update to the comment of ensure_invariants - definitely agree the range clamping should be explicitly mentioned there if it is to remain!

[cessen]

Adding @archseer since I think he had some thoughts about this.

[archseer]

Yeah so the issue here is that the jumplists aren't updated as documents change, so a selection on the jumplist can become invalid if you delete enough of the document.

The solution would be to also map any external references when calling doc.apply but this will require some thought to properly fix so I think your solution is fine to prevent panics.

Could you also log a warning if clamping happened? I'd also like us to panic in debug mode (potentially with extra checks) so these cases can be caught in tests. kakoune for example has a few methods that do checking in debug:

https://github.com/mawww/kakoune/blob/d758bbf09b949f0fef88ef3c6762cc55098cdb95/src/selection.cc#L267-L297=

https://github.com/mawww/kakoune/blob/d758bbf09b949f0fef88ef3c6762cc55098cdb95/src/buffer.cc#L383-L393=

[the-mikedavis]

Fixed in #4186