From 9267343830228490d379c90537ff1a6e4bba1260 Mon Sep 17 00:00:00 2001
From: Mike Trinkala <trink@acm.org>
Date: Sun, 3 Mar 2024 09:55:09 -0800
Subject: [PATCH] Fix panic when using surround_replace/delete (#9796)

1. Create a document containing `{A}`
1. C-w v # vsplit
1. gl    # goto_line_end
1. b     # move_prev_word_start
1. `     # switch_to_lowercase
1. mrm(  # surround replace
1. C-w v # vsplit

In the debug build surround_replace/delete will immedately assert with
`assertion failed: last <= from', transaction.rs:597:13`. The splits and
lowercase conversion are not needed to trigger the bug.

In the release build the surround becomes `)a(` and the last vsplit
causes the transaction to panic.
`internal error: entered unreachable code:
(Some(Retain(18446744073709551573)))', transaction.rs:185:46`

Since the selection direction is backwards get_surround_pos returns the
pairs reversed but the downstream code assumes they are in the forward
direction.
---
 helix-core/src/surround.rs        |  3 +-
 helix-term/tests/test/movement.rs | 54 +++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/helix-core/src/surround.rs b/helix-core/src/surround.rs
index b96cce5a0..513f87493 100644
--- a/helix-core/src/surround.rs
+++ b/helix-core/src/surround.rs
@@ -260,7 +260,8 @@ pub fn get_surround_pos(
         if change_pos.contains(&open_pos) || change_pos.contains(&close_pos) {
             return Err(Error::CursorOverlap);
         }
-        change_pos.extend_from_slice(&[open_pos, close_pos]);
+        // ensure the positions are always paired in the forward direction
+        change_pos.extend_from_slice(&[open_pos.min(close_pos), close_pos.max(open_pos)]);
     }
     Ok(change_pos)
 }
diff --git a/helix-term/tests/test/movement.rs b/helix-term/tests/test/movement.rs
index e3c2668da..0873edbe5 100644
--- a/helix-term/tests/test/movement.rs
+++ b/helix-term/tests/test/movement.rs
@@ -552,3 +552,57 @@ async fn find_char_line_ending() -> anyhow::Result<()> {
 
     Ok(())
 }
+
+#[tokio::test(flavor = "multi_thread")]
+async fn test_surround_replace() -> anyhow::Result<()> {
+    test((
+        platform_line(indoc! {"\
+            (#[|a]#)
+            "}),
+        "mrm{",
+        platform_line(indoc! {"\
+            {#[|a]#}
+            "}),
+    ))
+    .await?;
+
+    test((
+        platform_line(indoc! {"\
+            (#[a|]#)
+            "}),
+        "mrm{",
+        platform_line(indoc! {"\
+            {#[a|]#}
+            "}),
+    ))
+    .await?;
+
+    Ok(())
+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn test_surround_delete() -> anyhow::Result<()> {
+    test((
+        platform_line(indoc! {"\
+            (#[|a]#)
+            "}),
+        "mdm",
+        platform_line(indoc! {"\
+            #[|a]#
+            "}),
+    ))
+    .await?;
+
+    test((
+        platform_line(indoc! {"\
+            (#[a|]#)
+            "}),
+        "mdm",
+        platform_line(indoc! {"\
+            #[a|]#
+            "}),
+    ))
+    .await?;
+
+    Ok(())
+}