Skip to content

Commit

Permalink
Fix panic in surround_replace/delete nested multi-cursor (#9815)
Browse files Browse the repository at this point in the history
Test Document
-------------
```
{{

}
}

```

Steps To Reproduce
------------------
1. 2j  # move_visual_line_down
1. C   # copy_selection_on_next_line
1. mdm # surround_delete

Debug
-----
`assertion failed: last <= from', transaction.rs:597:13`

Release
-------
`called `Result::unwrap()` on an `Err` value: Char range out of bounds:
char range 18446744073709551614..18446744073709551615,
Rope/RopeSlice char length 7', ropey-1.6.1/src/rope.rs:546:37`

Description
-----------

Processing the surrounding pairs in order violates the assertion the
ranges are ordered. To handle nested surrounds all positions have to
be sorted. Also surround_replace has to track the proper replacement
character for each position.
  • Loading branch information
trink committed Mar 7, 2024
1 parent b93fae9 commit cb01e52
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 5 deletions.
19 changes: 14 additions & 5 deletions helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5282,12 +5282,21 @@ fn surround_replace(cx: &mut Context) {
None => return doc.set_selection(view.id, selection),
};
let (open, close) = surround::get_pair(to);

// the changeset has to be sorted to allow nested surrounds
let mut sorted_pos: Vec<(usize, char)> = Vec::new();
for p in change_pos.chunks(2) {
sorted_pos.push((p[0], open));
sorted_pos.push((p[1], close));
}
sorted_pos.sort_unstable();

let transaction = Transaction::change(
doc.text(),
change_pos.iter().enumerate().map(|(i, &pos)| {
sorted_pos.iter().map(|&pos| {
let mut t = Tendril::new();
t.push(if i % 2 == 0 { open } else { close });
(pos, pos + 1, Some(t))
t.push(pos.1);
(pos.0, pos.0 + 1, Some(t))
}),
);
doc.set_selection(view.id, selection);
Expand All @@ -5309,14 +5318,14 @@ fn surround_delete(cx: &mut Context) {
let text = doc.text().slice(..);
let selection = doc.selection(view.id);

let change_pos = match surround::get_surround_pos(text, selection, surround_ch, count) {
let mut change_pos = match surround::get_surround_pos(text, selection, surround_ch, count) {
Ok(c) => c,
Err(err) => {
cx.editor.set_error(err.to_string());
return;
}
};

change_pos.sort_unstable(); // the changeset has to be sorted to allow nested surrounds
let transaction =
Transaction::change(doc.text(), change_pos.into_iter().map(|p| (p, p + 1, None)));
doc.apply(&transaction, view.id);
Expand Down
29 changes: 29 additions & 0 deletions helix-term/tests/test/movement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,23 @@ async fn test_surround_replace() -> anyhow::Result<()> {
))
.await?;

test((
platform_line(indoc! {"\
{{
#(}|)#
#[}|]#
"}),
"mrm)",
platform_line(indoc! {"\
((
#()|)#
#[)|]#
"}),
))
.await?;

Ok(())
}

Expand Down Expand Up @@ -604,5 +621,17 @@ async fn test_surround_delete() -> anyhow::Result<()> {
))
.await?;

test((
platform_line(indoc! {"\
{{
#(}|)#
#[}|]#
"}),
"mdm",
platform_line("\n\n#(\n|)##[\n|]#"),
))
.await?;

Ok(())
}

0 comments on commit cb01e52

Please sign in to comment.