vim: Update change surrounds to match vim's behavior (#38721)

Dino and Conrad Irwin created

These changes refactor the whitespace handling logic for Vim's change
surrounds command (`cs`), making its behavior closely match
[tpope/vim-surround](https://github.com/tpope/vim-surround), following
[this
discussion](https://github.com/zed-industries/zed/issues/38169#issuecomment-3304129461).

Zed's current implementation has two main differences when compared to
[tpope/vim-surround](https://github.com/tpope/vim-surround):

- It only considers whether a single space should be added or removed,
instead of all the space that is between the surrounding character and
the content
- It only takes into consideration the new surrounding characters in
order to determine whether to add or remove that space

A review of
[tpope/vim-surround](https://github.com/tpope/vim-surround)'s behavior
reveals these rules for whitespace:

* Quote to Quote
    * Whitespace is never changed
* Quote to Bracket
    * If opening bracket, add one space
    * If closing bracket, do not add space
* Bracket to Bracket
    * If opening to opening, keep only one space
    * If opening to closing, remove all space
    * If closing to opening, add one space
    * If closing to closing, do not change space
* Bracket to Quote
    * If opening, remove all space
    * If closing, preserve all space

Below is a table with examples for each scenario. A new test has also
been added to specifically check the scenarios outlined above,
`vim::surrounds::test::test_change_surrounds_vim`.

| Type              | Before      | Command | After         |
|-------------------|-------------|---------|---------------|
| Quote β†’ Quote     | `'   a   '` | `cs'"`  | `"   a   "`   |
| Quote β†’ Quote     | `"   a   "` | `cs"'`  | `'   a   '`   |
| Quote β†’ Bracket   | `'   a   '` | `cs'{`  | `{    a    }` |
| Quote β†’ Bracket   | `'   a   '` | `cs'}`  | `{   a   }`   |
| Bracket β†’ Bracket | `[   a   ]` | `cs[{`  | `{ a }`       |
| Bracket β†’ Bracket | `[   a   ]` | `cs[}`  | `{a}`         |
| Bracket β†’ Bracket | `[   a   ]` | `cs]{`  | `{    a    }` |
| Bracket β†’ Bracket | `[   a   ]` | `cs]}`  | `{   a   }`   |
| Bracket β†’ Quote   | `[   a   ]` | `cs['`  | `'a'`         |
| Bracket β†’ Quote   | `[   a   ]` | `cs]'`  | `'   a   '`   |

These changes diverge from
[tpope/vim-surround](https://github.com/tpope/vim-surround) when
handling newlines. For example, with the following snippet:

```rust
fn test_surround() {
    if 2 > 1 {
        println!("place cursor here");
    }
};
```

Placing the cursor inside the string and running any combination of
β€Ž`cs{[`, β€Ž`cs{]`, β€Ž`cs}[`, or β€Ž`cs}]` would previously remove newline
characters. With these changes, using commands like β€Ž`cs}]` will now
preserve newlines.

Related to #38169
Closes #39334

Release Notes:

- Improved Vim’s change surround command to closely match
[tpope/vim-surround](https://github.com/tpope/vim-surround)Β behavior.

---------

Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>

Change summary

assets/keymaps/vim.json     |   8 
crates/vim/src/normal.rs    |   4 
crates/vim/src/object.rs    |  73 +++++++++++++----
crates/vim/src/state.rs     |  11 +
crates/vim/src/surrounds.rs | 161 ++++++++++++++++++++++++++++++++------
crates/vim/src/vim.rs       |   6 
6 files changed, 210 insertions(+), 53 deletions(-)

Detailed changes

assets/keymaps/vim.json πŸ”—

@@ -580,18 +580,18 @@
       // "q": "vim::AnyQuotes",
       "q": "vim::MiniQuotes",
       "|": "vim::VerticalBars",
-      "(": "vim::Parentheses",
+      "(": ["vim::Parentheses", { "opening": true }],
       ")": "vim::Parentheses",
       "b": "vim::Parentheses",
       // "b": "vim::AnyBrackets",
       // "b": "vim::MiniBrackets",
-      "[": "vim::SquareBrackets",
+      "[": ["vim::SquareBrackets", { "opening": true }],
       "]": "vim::SquareBrackets",
       "r": "vim::SquareBrackets",
-      "{": "vim::CurlyBrackets",
+      "{": ["vim::CurlyBrackets", { "opening": true }],
       "}": "vim::CurlyBrackets",
       "shift-b": "vim::CurlyBrackets",
-      "<": "vim::AngleBrackets",
+      "<": ["vim::AngleBrackets", { "opening": true }],
       ">": "vim::AngleBrackets",
       "a": "vim::Argument",
       "i": "vim::IndentObj",

crates/vim/src/normal.rs πŸ”—

@@ -450,6 +450,7 @@ impl Vim {
         &mut self,
         object: Object,
         times: Option<usize>,
+        opening: bool,
         window: &mut Window,
         cx: &mut Context<Self>,
     ) {
@@ -520,10 +521,11 @@ impl Vim {
             Some(Operator::DeleteSurrounds) => {
                 waiting_operator = Some(Operator::DeleteSurrounds);
             }
-            Some(Operator::ChangeSurrounds { target: None }) => {
+            Some(Operator::ChangeSurrounds { target: None, .. }) => {
                 if self.check_and_move_to_valid_bracket_pair(object, window, cx) {
                     waiting_operator = Some(Operator::ChangeSurrounds {
                         target: Some(object),
+                        opening,
                     });
                 }
             }

crates/vim/src/object.rs πŸ”—

@@ -85,6 +85,41 @@ pub struct CandidateWithRanges {
     close_range: Range<usize>,
 }
 
+/// Selects text at the same indentation level.
+#[derive(Clone, Deserialize, JsonSchema, PartialEq, Action)]
+#[action(namespace = vim)]
+#[serde(deny_unknown_fields)]
+struct Parentheses {
+    #[serde(default)]
+    opening: bool,
+}
+
+/// Selects text at the same indentation level.
+#[derive(Clone, Deserialize, JsonSchema, PartialEq, Action)]
+#[action(namespace = vim)]
+#[serde(deny_unknown_fields)]
+struct SquareBrackets {
+    #[serde(default)]
+    opening: bool,
+}
+
+/// Selects text at the same indentation level.
+#[derive(Clone, Deserialize, JsonSchema, PartialEq, Action)]
+#[action(namespace = vim)]
+#[serde(deny_unknown_fields)]
+struct AngleBrackets {
+    #[serde(default)]
+    opening: bool,
+}
+/// Selects text at the same indentation level.
+#[derive(Clone, Deserialize, JsonSchema, PartialEq, Action)]
+#[action(namespace = vim)]
+#[serde(deny_unknown_fields)]
+struct CurlyBrackets {
+    #[serde(default)]
+    opening: bool,
+}
+
 fn cover_or_next<I: Iterator<Item = (Range<usize>, Range<usize>)>>(
     candidates: Option<I>,
     caret: DisplayPoint,
@@ -275,18 +310,10 @@ actions!(
         DoubleQuotes,
         /// Selects text within vertical bars (pipes).
         VerticalBars,
-        /// Selects text within parentheses.
-        Parentheses,
         /// Selects text within the nearest brackets.
         MiniBrackets,
         /// Selects text within any type of brackets.
         AnyBrackets,
-        /// Selects text within square brackets.
-        SquareBrackets,
-        /// Selects text within curly brackets.
-        CurlyBrackets,
-        /// Selects text within angle brackets.
-        AngleBrackets,
         /// Selects a function argument.
         Argument,
         /// Selects an HTML/XML tag.
@@ -350,17 +377,17 @@ pub fn register(editor: &mut Editor, cx: &mut Context<Vim>) {
     Vim::action(editor, cx, |vim, _: &DoubleQuotes, window, cx| {
         vim.object(Object::DoubleQuotes, window, cx)
     });
-    Vim::action(editor, cx, |vim, _: &Parentheses, window, cx| {
-        vim.object(Object::Parentheses, window, cx)
+    Vim::action(editor, cx, |vim, action: &Parentheses, window, cx| {
+        vim.object_impl(Object::Parentheses, action.opening, window, cx)
     });
-    Vim::action(editor, cx, |vim, _: &SquareBrackets, window, cx| {
-        vim.object(Object::SquareBrackets, window, cx)
+    Vim::action(editor, cx, |vim, action: &SquareBrackets, window, cx| {
+        vim.object_impl(Object::SquareBrackets, action.opening, window, cx)
     });
-    Vim::action(editor, cx, |vim, _: &CurlyBrackets, window, cx| {
-        vim.object(Object::CurlyBrackets, window, cx)
+    Vim::action(editor, cx, |vim, action: &CurlyBrackets, window, cx| {
+        vim.object_impl(Object::CurlyBrackets, action.opening, window, cx)
     });
-    Vim::action(editor, cx, |vim, _: &AngleBrackets, window, cx| {
-        vim.object(Object::AngleBrackets, window, cx)
+    Vim::action(editor, cx, |vim, action: &AngleBrackets, window, cx| {
+        vim.object_impl(Object::AngleBrackets, action.opening, window, cx)
     });
     Vim::action(editor, cx, |vim, _: &VerticalBars, window, cx| {
         vim.object(Object::VerticalBars, window, cx)
@@ -394,10 +421,22 @@ pub fn register(editor: &mut Editor, cx: &mut Context<Vim>) {
 
 impl Vim {
     fn object(&mut self, object: Object, window: &mut Window, cx: &mut Context<Self>) {
+        self.object_impl(object, false, window, cx);
+    }
+
+    fn object_impl(
+        &mut self,
+        object: Object,
+        opening: bool,
+        window: &mut Window,
+        cx: &mut Context<Self>,
+    ) {
         let count = Self::take_count(cx);
 
         match self.mode {
-            Mode::Normal | Mode::HelixNormal => self.normal_object(object, count, window, cx),
+            Mode::Normal | Mode::HelixNormal => {
+                self.normal_object(object, count, opening, window, cx)
+            }
             Mode::Visual | Mode::VisualLine | Mode::VisualBlock | Mode::HelixSelect => {
                 self.visual_object(object, count, window, cx)
             }

crates/vim/src/state.rs πŸ”—

@@ -109,6 +109,9 @@ pub enum Operator {
     },
     ChangeSurrounds {
         target: Option<Object>,
+        /// Represents whether the opening bracket was used for the target
+        /// object.
+        opening: bool,
     },
     DeleteSurrounds,
     Mark,
@@ -1077,7 +1080,9 @@ impl Operator {
             | Operator::Replace
             | Operator::Digraph { .. }
             | Operator::Literal { .. }
-            | Operator::ChangeSurrounds { target: Some(_) }
+            | Operator::ChangeSurrounds {
+                target: Some(_), ..
+            }
             | Operator::DeleteSurrounds => true,
             Operator::Change
             | Operator::Delete
@@ -1094,7 +1099,7 @@ impl Operator {
             | Operator::ReplaceWithRegister
             | Operator::Exchange
             | Operator::Object { .. }
-            | Operator::ChangeSurrounds { target: None }
+            | Operator::ChangeSurrounds { target: None, .. }
             | Operator::OppositeCase
             | Operator::ToggleComments
             | Operator::HelixMatch
@@ -1121,7 +1126,7 @@ impl Operator {
             | Operator::Rewrap
             | Operator::ShellCommand
             | Operator::AddSurrounds { target: None }
-            | Operator::ChangeSurrounds { target: None }
+            | Operator::ChangeSurrounds { target: None, .. }
             | Operator::DeleteSurrounds
             | Operator::Exchange
             | Operator::HelixNext { .. }

crates/vim/src/surrounds.rs πŸ”—

@@ -221,6 +221,7 @@ impl Vim {
         &mut self,
         text: Arc<str>,
         target: Object,
+        opening: bool,
         window: &mut Window,
         cx: &mut Context<Self>,
     ) {
@@ -241,16 +242,19 @@ impl Vim {
                         },
                     };
 
-                    // Determines whether space should be added after
-                    // and before the surround pairs.
-                    // Space is only added in the following cases:
-                    // - new surround is not quote and is opening bracket (({[<)
-                    // - new surround is quote and original was also quote
-                    let surround = if pair.start != pair.end {
-                        pair.end != surround_alias((*text).as_ref())
-                    } else {
-                        will_replace_pair.start == will_replace_pair.end
-                    };
+                    // A single space should be added if the new surround is a
+                    // bracket and not a quote (pair.start != pair.end) and if
+                    // the bracket used is the opening bracket.
+                    let add_space =
+                        !(pair.start == pair.end) && (pair.end != surround_alias((*text).as_ref()));
+
+                    // Space should be preserved if either the surrounding
+                    // characters being updated are quotes
+                    // (will_replace_pair.start == will_replace_pair.end) or if
+                    // the bracket used in the command is not an opening
+                    // bracket.
+                    let preserve_space =
+                        will_replace_pair.start == will_replace_pair.end || !opening;
 
                     let (display_map, selections) = editor.selections.all_adjusted_display(cx);
                     let mut edits = Vec::new();
@@ -269,23 +273,36 @@ impl Vim {
                                     continue;
                                 }
                             }
+
+                            // Keeps track of the length of the string that is
+                            // going to be edited on the start so we can ensure
+                            // that the end replacement string does not exceed
+                            // this value. Helpful when dealing with newlines.
+                            let mut edit_len = 0;
                             let mut chars_and_offset = display_map
                                 .buffer_chars_at(range.start.to_offset(&display_map, Bias::Left))
                                 .peekable();
+
                             while let Some((ch, offset)) = chars_and_offset.next() {
                                 if ch.to_string() == will_replace_pair.start {
                                     let mut open_str = pair.start.clone();
                                     let start = offset;
                                     let mut end = start + 1;
-                                    if let Some((next_ch, _)) = chars_and_offset.peek() {
-                                        // If the next position is already a space or line break,
-                                        // we don't need to splice another space even under around
-                                        if surround && !next_ch.is_whitespace() {
-                                            open_str.push(' ');
-                                        } else if !surround && next_ch.to_string() == " " {
-                                            end += 1;
+                                    while let Some((next_ch, _)) = chars_and_offset.next()
+                                        && next_ch.to_string() == " "
+                                    {
+                                        end += 1;
+
+                                        if preserve_space {
+                                            open_str.push(next_ch);
                                         }
                                     }
+
+                                    if add_space {
+                                        open_str.push(' ');
+                                    };
+
+                                    edit_len = end - start;
                                     edits.push((start..end, open_str));
                                     anchors.push(start..start);
                                     break;
@@ -299,16 +316,25 @@ impl Vim {
                                 .peekable();
                             while let Some((ch, offset)) = reverse_chars_and_offsets.next() {
                                 if ch.to_string() == will_replace_pair.end {
-                                    let mut close_str = pair.end.clone();
+                                    let mut close_str = String::new();
                                     let mut start = offset;
                                     let end = start + 1;
-                                    if let Some((next_ch, _)) = reverse_chars_and_offsets.peek() {
-                                        if surround && !next_ch.is_whitespace() {
-                                            close_str.insert(0, ' ')
-                                        } else if !surround && next_ch.to_string() == " " {
-                                            start -= 1;
+                                    while let Some((next_ch, _)) = reverse_chars_and_offsets.next()
+                                        && next_ch.to_string() == " "
+                                        && close_str.len() < edit_len - 1
+                                    {
+                                        start -= 1;
+
+                                        if preserve_space {
+                                            close_str.push(next_ch);
                                         }
                                     }
+
+                                    if add_space {
+                                        close_str.push(' ');
+                                    };
+
+                                    close_str.push_str(&pair.end);
                                     edits.push((start..end, close_str));
                                     break;
                                 }
@@ -448,7 +474,7 @@ impl Vim {
                 surround: true,
                 newline: false,
             }),
-            Object::CurlyBrackets => Some(BracketPair {
+            Object::CurlyBrackets { .. } => Some(BracketPair {
                 start: "{".to_string(),
                 end: "}".to_string(),
                 close: true,
@@ -1194,7 +1220,30 @@ mod test {
             };"},
             Mode::Normal,
         );
-        cx.simulate_keystrokes("c s { [");
+        cx.simulate_keystrokes("c s } ]");
+        cx.assert_state(
+            indoc! {"
+            fn test_surround() Λ‡[
+                if 2 > 1 Λ‡[
+                    println!(\"it is fine\");
+                ]
+            ];"},
+            Mode::Normal,
+        );
+
+        // Currently, the same test case but using the closing bracket `]`
+        // actually removes a whitespace before the closing bracket, something
+        // that might need to be fixed?
+        cx.set_state(
+            indoc! {"
+            fn test_surround() {
+                ifˇ 2 > 1 {
+                    Λ‡println!(\"it is fine\");
+                }
+            };"},
+            Mode::Normal,
+        );
+        cx.simulate_keystrokes("c s { ]");
         cx.assert_state(
             indoc! {"
             fn test_surround() Λ‡[
@@ -1270,7 +1319,7 @@ mod test {
         cx.assert_state(indoc! {"Λ‡[ bracketed ]"}, Mode::Normal);
 
         cx.set_state(indoc! {"(< name: Λ‡'Zed' >)"}, Mode::Normal);
-        cx.simulate_keystrokes("c s b {");
+        cx.simulate_keystrokes("c s b }");
         cx.assert_state(indoc! {"(Λ‡{ name: 'Zed' })"}, Mode::Normal);
 
         cx.set_state(
@@ -1290,6 +1339,66 @@ mod test {
         );
     }
 
+    // The following test cases all follow tpope/vim-surround's behaviour
+    // and are more focused on how whitespace is handled.
+    #[gpui::test]
+    async fn test_change_surrounds_vim(cx: &mut gpui::TestAppContext) {
+        let mut cx = VimTestContext::new(cx, true).await;
+
+        // Changing quote to quote should never change the surrounding
+        // whitespace.
+        cx.set_state(indoc! {"'  Λ‡a  '"}, Mode::Normal);
+        cx.simulate_keystrokes("c s ' \"");
+        cx.assert_state(indoc! {"Λ‡\"  a  \""}, Mode::Normal);
+
+        cx.set_state(indoc! {"\"  Λ‡a  \""}, Mode::Normal);
+        cx.simulate_keystrokes("c s \" '");
+        cx.assert_state(indoc! {"Λ‡'  a  '"}, Mode::Normal);
+
+        // Changing quote to bracket adds one more space when the opening
+        // bracket is used, does not affect whitespace when the closing bracket
+        // is used.
+        cx.set_state(indoc! {"'  Λ‡a  '"}, Mode::Normal);
+        cx.simulate_keystrokes("c s ' {");
+        cx.assert_state(indoc! {"Λ‡{   a   }"}, Mode::Normal);
+
+        cx.set_state(indoc! {"'  Λ‡a  '"}, Mode::Normal);
+        cx.simulate_keystrokes("c s ' }");
+        cx.assert_state(indoc! {"Λ‡{  a  }"}, Mode::Normal);
+
+        // Changing bracket to quote should remove all space when the
+        // opening bracket is used and preserve all space when the
+        // closing one is used.
+        cx.set_state(indoc! {"{  Λ‡a  }"}, Mode::Normal);
+        cx.simulate_keystrokes("c s { '");
+        cx.assert_state(indoc! {"Λ‡'a'"}, Mode::Normal);
+
+        cx.set_state(indoc! {"{  Λ‡a  }"}, Mode::Normal);
+        cx.simulate_keystrokes("c s } '");
+        cx.assert_state(indoc! {"Λ‡'  a  '"}, Mode::Normal);
+
+        // Changing bracket to bracket follows these rules:
+        // * opening β†’ opening – keeps only one space.
+        // * opening β†’ closing – removes all space.
+        // * closing β†’ opening – adds one space.
+        // * closing β†’ closing – does not change space.
+        cx.set_state(indoc! {"{   Λ‡a   }"}, Mode::Normal);
+        cx.simulate_keystrokes("c s { [");
+        cx.assert_state(indoc! {"Λ‡[ a ]"}, Mode::Normal);
+
+        cx.set_state(indoc! {"{   Λ‡a   }"}, Mode::Normal);
+        cx.simulate_keystrokes("c s { ]");
+        cx.assert_state(indoc! {"Λ‡[a]"}, Mode::Normal);
+
+        cx.set_state(indoc! {"{  Λ‡a  }"}, Mode::Normal);
+        cx.simulate_keystrokes("c s } [");
+        cx.assert_state(indoc! {"Λ‡[   a   ]"}, Mode::Normal);
+
+        cx.set_state(indoc! {"{  Λ‡a  }"}, Mode::Normal);
+        cx.simulate_keystrokes("c s } ]");
+        cx.assert_state(indoc! {"Λ‡[  a  ]"}, Mode::Normal);
+    }
+
     #[gpui::test]
     async fn test_surrounds(cx: &mut gpui::TestAppContext) {
         let mut cx = VimTestContext::new(cx, true).await;

crates/vim/src/vim.rs πŸ”—

@@ -678,6 +678,7 @@ impl Vim {
                     vim.push_operator(
                         Operator::ChangeSurrounds {
                             target: action.target,
+                            opening: false,
                         },
                         window,
                         cx,
@@ -945,6 +946,7 @@ impl Vim {
                 self.update_editor(cx, |_, editor, cx| {
                     editor.hide_mouse_cursor(HideMouseCursorOrigin::MovementAction, cx)
                 });
+
                 return;
             }
         } else if window.has_pending_keystrokes() || keystroke_event.keystroke.is_ime_in_progress()
@@ -1780,10 +1782,10 @@ impl Vim {
                 }
                 _ => self.clear_operator(window, cx),
             },
-            Some(Operator::ChangeSurrounds { target }) => match self.mode {
+            Some(Operator::ChangeSurrounds { target, opening }) => match self.mode {
                 Mode::Normal => {
                     if let Some(target) = target {
-                        self.change_surrounds(text, target, window, cx);
+                        self.change_surrounds(text, target, opening, window, cx);
                         self.clear_operator(window, cx);
                     }
                 }