Merge pull request #19225 from Giga-Bowser/remove-assists

internal: Migrate some low-hanging `remove_*` assists to `SyntaxEditor`

crates/ide-assists/src/handlers/remove_mut.rs

@@ -1,4 +1,4 @@
-use syntax::{SyntaxKind, TextRange, T};
+use syntax::{SyntaxKind, T};
 
 use crate::{AssistContext, AssistId, AssistKind, Assists};
 
@@ -19,11 +19,6 @@ use crate::{AssistContext, AssistId, AssistKind, Assists};
 // ```
 pub(crate) fn remove_mut(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> {
     let mut_token = ctx.find_token_syntax_at_offset(T![mut])?;
-    let delete_from = mut_token.text_range().start();
-    let delete_to = match mut_token.next_token() {
-        Some(it) if it.kind() == SyntaxKind::WHITESPACE => it.text_range().end(),
-        _ => mut_token.text_range().end(),
-    };
 
     let target = mut_token.text_range();
     acc.add(
@@ -31,7 +26,13 @@ pub(crate) fn remove_mut(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<(
         "Remove `mut` keyword",
         target,
         |builder| {
-            builder.delete(TextRange::new(delete_from, delete_to));
+            let mut editor = builder.make_editor(&mut_token.parent().unwrap());
+            match mut_token.next_token() {
+                Some(it) if it.kind() == SyntaxKind::WHITESPACE => editor.delete(it),
+                _ => (),
+            }
+            editor.delete(mut_token);
+            builder.add_file_edits(ctx.file_id(), editor);
         },
     )
 }

crates/ide-assists/src/handlers/remove_parentheses.rs

@@ -1,4 +1,8 @@
-use syntax::{ast, AstNode, SyntaxKind, T};
+use syntax::{
+    ast::{self, syntax_factory::SyntaxFactory},
+    syntax_editor::Position,
+    AstNode, SyntaxKind, T,
+};
 
 use crate::{AssistContext, AssistId, AssistKind, Assists};
 
@@ -40,6 +44,7 @@ pub(crate) fn remove_parentheses(acc: &mut Assists, ctx: &AssistContext<'_>) ->
         "Remove redundant parentheses",
         target,
         |builder| {
+            let mut editor = builder.make_editor(parens.syntax());
             let prev_token = parens.syntax().first_token().and_then(|it| it.prev_token());
             let need_to_add_ws = match prev_token {
                 Some(it) => {
@@ -48,9 +53,13 @@ pub(crate) fn remove_parentheses(acc: &mut Assists, ctx: &AssistContext<'_>) ->
                 }
                 None => false,
             };
-            let expr = if need_to_add_ws { format!(" {expr}") } else { expr.to_string() };
-
-            builder.replace(parens.syntax().text_range(), expr)
+            if need_to_add_ws {
+                let make = SyntaxFactory::new();
+                editor.insert(Position::before(parens.syntax()), make.whitespace(" "));
+                editor.add_mappings(make.finish_with_mappings());
+            }
+            editor.replace(parens.syntax(), expr.syntax());
+            builder.add_file_edits(ctx.file_id(), editor);
         },
     )
 }

crates/ide-assists/src/handlers/remove_unused_param.rs

@@ -1,8 +1,9 @@
 use ide_db::{defs::Definition, search::FileReference, EditionedFileId};
 use syntax::{
-    algo::find_node_at_range,
+    algo::{find_node_at_range, least_common_ancestor_element},
     ast::{self, HasArgList},
-    AstNode, SourceFile, SyntaxKind, SyntaxNode, TextRange, T,
+    syntax_editor::Element,
+    AstNode, SourceFile, SyntaxElement, SyntaxKind, SyntaxNode, TextRange, T,
 };
 
 use SyntaxKind::WHITESPACE;
@@ -74,15 +75,21 @@ pub(crate) fn remove_unused_param(acc: &mut Assists, ctx: &AssistContext<'_>) ->
         cov_mark::hit!(keep_used);
         return None;
     }
+    let parent = param.syntax().parent()?;
     acc.add(
         AssistId("remove_unused_param", AssistKind::Refactor),
         "Remove unused parameter",
         param.syntax().text_range(),
         |builder| {
-            builder.delete(range_to_remove(param.syntax()));
+            let mut editor = builder.make_editor(&parent);
+            let elements = elements_to_remove(param.syntax());
+            for element in elements {
+                editor.delete(element);
+            }
             for (file_id, references) in fn_def.usages(&ctx.sema).all() {
                 process_usages(ctx, builder, file_id, references, param_position, is_self_present);
             }
+            builder.add_file_edits(ctx.file_id(), editor);
         },
     )
 }
@@ -96,20 +103,24 @@ fn process_usages(
     is_self_present: bool,
 ) {
     let source_file = ctx.sema.parse(file_id);
-    builder.edit_file(file_id);
     let possible_ranges = references
         .into_iter()
         .filter_map(|usage| process_usage(&source_file, usage, arg_to_remove, is_self_present));
 
-    let mut ranges_to_delete: Vec<TextRange> = vec![];
-    for range in possible_ranges {
-        if !ranges_to_delete.iter().any(|it| it.contains_range(range)) {
-            ranges_to_delete.push(range)
+    for element_range in possible_ranges {
+        let Some(SyntaxElement::Node(parent)) = element_range
+            .iter()
+            .cloned()
+            .reduce(|a, b| least_common_ancestor_element(&a, &b).unwrap().syntax_element())
+        else {
+            continue;
+        };
+        let mut editor = builder.make_editor(&parent);
+        for element in element_range {
+            editor.delete(element);
         }
-    }
 
-    for range in ranges_to_delete {
-        builder.delete(range)
+        builder.add_file_edits(file_id, editor);
     }
 }
 
@@ -118,7 +129,7 @@ fn process_usage(
     FileReference { range, .. }: FileReference,
     mut arg_to_remove: usize,
     is_self_present: bool,
-) -> Option<TextRange> {
+) -> Option<Vec<SyntaxElement>> {
     let call_expr_opt: Option<ast::CallExpr> = find_node_at_range(source_file.syntax(), range);
     if let Some(call_expr) = call_expr_opt {
         let call_expr_range = call_expr.expr()?.syntax().text_range();
@@ -127,7 +138,7 @@ fn process_usage(
         }
 
         let arg = call_expr.arg_list()?.args().nth(arg_to_remove)?;
-        return Some(range_to_remove(arg.syntax()));
+        return Some(elements_to_remove(arg.syntax()));
     }
 
     let method_call_expr_opt: Option<ast::MethodCallExpr> =
@@ -143,7 +154,7 @@ fn process_usage(
         }
 
         let arg = method_call_expr.arg_list()?.args().nth(arg_to_remove)?;
-        return Some(range_to_remove(arg.syntax()));
+        return Some(elements_to_remove(arg.syntax()));
     }
 
     None
@@ -174,6 +185,29 @@ pub(crate) fn range_to_remove(node: &SyntaxNode) -> TextRange {
     }
 }
 
+pub(crate) fn elements_to_remove(node: &SyntaxNode) -> Vec<SyntaxElement> {
+    let up_to_comma = next_prev().find_map(|dir| {
+        node.siblings_with_tokens(dir)
+            .filter_map(|it| it.into_token())
+            .find(|it| it.kind() == T![,])
+            .map(|it| (dir, it))
+    });
+    if let Some((dir, token)) = up_to_comma {
+        let after = token.siblings_with_tokens(dir).nth(1).unwrap();
+        let mut result: Vec<_> =
+            node.siblings_with_tokens(dir).take_while(|it| it != &after).collect();
+        if node.next_sibling().is_some() {
+            result.extend(
+                token.siblings_with_tokens(dir).skip(1).take_while(|it| it.kind() == WHITESPACE),
+            );
+        }
+
+        result
+    } else {
+        vec![node.syntax_element()]
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use crate::tests::{check_assist, check_assist_not_applicable};

crates/syntax/src/algo.rs

@@ -3,8 +3,8 @@
 use itertools::Itertools;
 
 use crate::{
-    AstNode, Direction, NodeOrToken, SyntaxElement, SyntaxKind, SyntaxNode, SyntaxToken, TextRange,
-    TextSize,
+    syntax_editor::Element, AstNode, Direction, NodeOrToken, SyntaxElement, SyntaxKind, SyntaxNode,
+    SyntaxToken, TextRange, TextSize,
 };
 
 /// Returns ancestors of the node at the offset, sorted by length. This should
@@ -89,6 +89,26 @@ pub fn least_common_ancestor(u: &SyntaxNode, v: &SyntaxNode) -> Option<SyntaxNod
     Some(res)
 }
 
+pub fn least_common_ancestor_element(u: impl Element, v: impl Element) -> Option<SyntaxNode> {
+    let u = u.syntax_element();
+    let v = v.syntax_element();
+    if u == v {
+        return match u {
+            NodeOrToken::Node(node) => Some(node),
+            NodeOrToken::Token(token) => token.parent(),
+        };
+    }
+
+    let u_depth = u.ancestors().count();
+    let v_depth = v.ancestors().count();
+    let keep = u_depth.min(v_depth);
+
+    let u_candidates = u.ancestors().skip(u_depth - keep);
+    let v_candidates = v.ancestors().skip(v_depth - keep);
+    let (res, _) = u_candidates.zip(v_candidates).find(|(x, y)| x == y)?;
+    Some(res)
+}
+
 pub fn neighbor<T: AstNode>(me: &T, direction: Direction) -> Option<T> {
     me.syntax().siblings(direction).skip(1).find_map(T::cast)
 }

crates/syntax/src/syntax_editor.rs

@@ -5,6 +5,7 @@
 //! [`SyntaxEditor`]: https://github.com/dotnet/roslyn/blob/43b0b05cc4f492fd5de00f6f6717409091df8daa/src/Workspaces/Core/Portable/Editing/SyntaxEditor.cs
 
 use std::{
+    fmt,
     num::NonZeroU32,
     ops::RangeInclusive,
     sync::atomic::{AtomicU32, Ordering},
@@ -282,6 +283,64 @@ enum ChangeKind {
     Replace,
 }
 
+impl fmt::Display for Change {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Change::Insert(position, node_or_token) => {
+                let parent = position.parent();
+                let mut parent_str = parent.to_string();
+                let target_range = self.target_range().start() - parent.text_range().start();
+
+                parent_str.insert_str(
+                    target_range.into(),
+                    &format!("\x1b[42m{node_or_token}\x1b[0m\x1b[K"),
+                );
+                f.write_str(&parent_str)
+            }
+            Change::InsertAll(position, vec) => {
+                let parent = position.parent();
+                let mut parent_str = parent.to_string();
+                let target_range = self.target_range().start() - parent.text_range().start();
+                let insertion: String = vec.iter().map(|it| it.to_string()).collect();
+
+                parent_str
+                    .insert_str(target_range.into(), &format!("\x1b[42m{insertion}\x1b[0m\x1b[K"));
+                f.write_str(&parent_str)
+            }
+            Change::Replace(old, new) => {
+                if let Some(new) = new {
+                    write!(f, "\x1b[41m{old}\x1b[42m{new}\x1b[0m\x1b[K")
+                } else {
+                    write!(f, "\x1b[41m{old}\x1b[0m\x1b[K")
+                }
+            }
+            Change::ReplaceWithMany(old, vec) => {
+                let new: String = vec.iter().map(|it| it.to_string()).collect();
+                write!(f, "\x1b[41m{old}\x1b[42m{new}\x1b[0m\x1b[K")
+            }
+            Change::ReplaceAll(range, vec) => {
+                let parent = range.start().parent().unwrap();
+                let parent_str = parent.to_string();
+                let pre_range =
+                    TextRange::new(parent.text_range().start(), range.start().text_range().start());
+                let old_range = TextRange::new(
+                    range.start().text_range().start(),
+                    range.end().text_range().end(),
+                );
+                let post_range =
+                    TextRange::new(range.end().text_range().end(), parent.text_range().end());
+
+                let pre_str = &parent_str[pre_range - parent.text_range().start()];
+                let old_str = &parent_str[old_range - parent.text_range().start()];
+                let post_str = &parent_str[post_range - parent.text_range().start()];
+                let new: String = vec.iter().map(|it| it.to_string()).collect();
+
+                write!(f, "{pre_str}\x1b[41m{old_str}\x1b[42m{new}\x1b[0m\x1b[K{post_str}")
+            }
+        }
+    }
+}
+
 /// Utility trait to allow calling syntax editor functions with references or owned
 /// nodes. Do not use outside of this module.
 pub trait Element {

crates/syntax/src/syntax_editor/edit_algo.rs

@@ -1,9 +1,14 @@
 //! Implementation of applying changes to a syntax tree.
 
-use std::{cmp::Ordering, collections::VecDeque, ops::RangeInclusive};
+use std::{
+    cmp::Ordering,
+    collections::VecDeque,
+    ops::{Range, RangeInclusive},
+};
 
 use rowan::TextRange;
 use rustc_hash::FxHashMap;
+use stdx::format_to;
 
 use crate::{
     syntax_editor::{mapping::MissingMapping, Change, ChangeKind, PositionRepr},
@@ -76,11 +81,9 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit {
                 || (l.target_range().end() <= r.target_range().start())
         });
 
-    if stdx::never!(
-        !disjoint_replaces_ranges,
-        "some replace change ranges intersect: {:?}",
-        changes
-    ) {
+    if !disjoint_replaces_ranges {
+        report_intersecting_changes(&changes, get_node_depth, &root);
+
         return SyntaxEdit {
             old_root: root.clone(),
             new_root: root,
@@ -99,6 +102,7 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit {
     let mut changed_ancestors: VecDeque<ChangedAncestor> = VecDeque::new();
     let mut dependent_changes = vec![];
     let mut independent_changes = vec![];
+    let mut outdated_changes = vec![];
 
     for (change_index, change) in changes.iter().enumerate() {
         // Check if this change is dependent on another change (i.e. it's contained within another range)
@@ -113,10 +117,14 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit {
             // FIXME: Resolve changes that depend on a range of elements
             let ancestor = &changed_ancestors[index];
 
-            dependent_changes.push(DependentChange {
-                parent: ancestor.change_index as u32,
-                child: change_index as u32,
-            });
+            if let Change::Replace(_, None) = changes[ancestor.change_index] {
+                outdated_changes.push(change_index as u32);
+            } else {
+                dependent_changes.push(DependentChange {
+                    parent: ancestor.change_index as u32,
+                    child: change_index as u32,
+                });
+            }
         } else {
             // This change is independent of any other change
 
@@ -192,8 +200,9 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit {
             Change::Replace(target, Some(new_target)) => {
                 (to_owning_node(target), to_owning_node(new_target))
             }
-            // Silently drop outdated change
-            Change::Replace(_, None) => continue,
+            Change::Replace(_, None) => {
+                unreachable!("deletions should not generate dependent changes")
+            }
             Change::ReplaceAll(_, _) | Change::ReplaceWithMany(_, _) => {
                 unimplemented!("cannot resolve changes that depend on replacing many elements")
             }
@@ -231,6 +240,12 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit {
         }
     }
 
+    // We reverse here since we pushed to this in ascending order,
+    // and we want to remove elements in descending order
+    for idx in outdated_changes.into_iter().rev() {
+        changes.remove(idx as usize);
+    }
+
     // Apply changes
     let mut root = tree_mutator.mutable_clone;
 
@@ -293,6 +308,78 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit {
     }
 }
 
+fn report_intersecting_changes(
+    changes: &[Change],
+    mut get_node_depth: impl FnMut(rowan::SyntaxNode<crate::RustLanguage>) -> usize,
+    root: &rowan::SyntaxNode<crate::RustLanguage>,
+) {
+    let intersecting_changes = changes
+        .iter()
+        .zip(changes.iter().skip(1))
+        .filter(|(l, r)| {
+            // We only care about checking for disjoint replace ranges.
+            matches!(
+                (l.change_kind(), r.change_kind()),
+                (
+                    ChangeKind::Replace | ChangeKind::ReplaceRange,
+                    ChangeKind::Replace | ChangeKind::ReplaceRange
+                )
+            )
+        })
+        .filter(|(l, r)| {
+            get_node_depth(l.target_parent()) == get_node_depth(r.target_parent())
+                && (l.target_range().end() > r.target_range().start())
+        });
+
+    let mut error_msg = String::from("some replace change ranges intersect!\n");
+
+    let parent_str = root.to_string();
+
+    for (l, r) in intersecting_changes {
+        let mut highlighted_str = parent_str.clone();
+        let l_range = l.target_range();
+        let r_range = r.target_range();
+
+        let i_range = l_range.intersect(r_range).unwrap();
+        let i_str = format!("\x1b[46m{}", &parent_str[i_range]);
+
+        let pre_range: Range<usize> = l_range.start().into()..i_range.start().into();
+        let pre_str = format!("\x1b[44m{}", &parent_str[pre_range]);
+
+        let (highlight_range, highlight_str) = if l_range == r_range {
+            format_to!(error_msg, "\x1b[46mleft change:\x1b[0m  {l:?} {l}\n");
+            format_to!(error_msg, "\x1b[46mequals\x1b[0m\n");
+            format_to!(error_msg, "\x1b[46mright change:\x1b[0m {r:?} {r}\n");
+            let i_highlighted = format!("{i_str}\x1b[0m\x1b[K");
+            let total_range: Range<usize> = i_range.into();
+            (total_range, i_highlighted)
+        } else {
+            format_to!(error_msg, "\x1b[44mleft change:\x1b[0m  {l:?} {l}\n");
+            let range_end = if l_range.contains_range(r_range) {
+                format_to!(error_msg, "\x1b[46mcovers\x1b[0m\n");
+                format_to!(error_msg, "\x1b[46mright change:\x1b[0m {r:?} {r}\n");
+                l_range.end()
+            } else {
+                format_to!(error_msg, "\x1b[46mintersects\x1b[0m\n");
+                format_to!(error_msg, "\x1b[42mright change:\x1b[0m {r:?} {r}\n");
+                r_range.end()
+            };
+
+            let post_range: Range<usize> = i_range.end().into()..range_end.into();
+
+            let post_str = format!("\x1b[42m{}", &parent_str[post_range]);
+            let result = format!("{pre_str}{i_str}{post_str}\x1b[0m\x1b[K");
+            let total_range: Range<usize> = l_range.start().into()..range_end.into();
+            (total_range, result)
+        };
+        highlighted_str.replace_range(highlight_range, &highlight_str);
+
+        format_to!(error_msg, "{highlighted_str}\n");
+    }
+
+    stdx::always!(false, "{}", error_msg);
+}
+
 fn to_owning_node(element: &SyntaxElement) -> SyntaxNode {
     match element {
         SyntaxElement::Node(node) => node.clone(),

crates/test-utils/src/lib.rs

@@ -421,8 +421,8 @@ pub fn format_diff(chunks: Vec<dissimilar::Chunk<'_>>) -> String {
     for chunk in chunks {
         let formatted = match chunk {
             dissimilar::Chunk::Equal(text) => text.into(),
-            dissimilar::Chunk::Delete(text) => format!("\x1b[41m{text}\x1b[0m"),
-            dissimilar::Chunk::Insert(text) => format!("\x1b[42m{text}\x1b[0m"),
+            dissimilar::Chunk::Delete(text) => format!("\x1b[41m{text}\x1b[0m\x1b[K"),
+            dissimilar::Chunk::Insert(text) => format!("\x1b[42m{text}\x1b[0m\x1b[K"),
         };
         buf.push_str(&formatted);
     }

docs/book/src/assists_generated.md

@@ -2974,7 +2974,7 @@ impl Walrus {
 
 
 ### `remove_parentheses`
-**Source:**  [remove_parentheses.rs](https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-assists/src/handlers/remove_parentheses.rs#L5) 
+**Source:**  [remove_parentheses.rs](https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-assists/src/handlers/remove_parentheses.rs#L9) 
 
 Removes redundant parentheses.
 
@@ -3015,7 +3015,7 @@ mod foo {
 
 
 ### `remove_unused_param`
-**Source:**  [remove_unused_param.rs](https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-assists/src/handlers/remove_unused_param.rs#L15) 
+**Source:**  [remove_unused_param.rs](https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-assists/src/handlers/remove_unused_param.rs#L16) 
 
 Removes unused function parameter.