From b4f2d62952f768639812b160841cb52000946b6d Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Fri, 10 Jan 2025 14:33:21 -0600 Subject: [PATCH 1/5] internal: Improve reporting of intersecting changes --- crates/syntax/src/syntax_editor.rs | 59 +++++++++++++ crates/syntax/src/syntax_editor/edit_algo.rs | 87 ++++++++++++++++++-- crates/test-utils/src/lib.rs | 4 +- 3 files changed, 142 insertions(+), 8 deletions(-) diff --git a/crates/syntax/src/syntax_editor.rs b/crates/syntax/src/syntax_editor.rs index b82181ae13ad..48c160b9a9ac 100644 --- a/crates/syntax/src/syntax_editor.rs +++ b/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 { diff --git a/crates/syntax/src/syntax_editor/edit_algo.rs b/crates/syntax/src/syntax_editor/edit_algo.rs index 57ecbe57019b..d6d903715d10 100644 --- a/crates/syntax/src/syntax_editor/edit_algo.rs +++ b/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, @@ -293,6 +296,78 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit { } } +fn report_intersecting_changes( + changes: &[Change], + mut get_node_depth: impl FnMut(rowan::SyntaxNode) -> usize, + root: &rowan::SyntaxNode, +) { + 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 = 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 = 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 = 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 = 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(), diff --git a/crates/test-utils/src/lib.rs b/crates/test-utils/src/lib.rs index e7279fa1f661..d3afac85017f 100644 --- a/crates/test-utils/src/lib.rs +++ b/crates/test-utils/src/lib.rs @@ -421,8 +421,8 @@ pub fn format_diff(chunks: Vec>) -> 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); } From daca6b88c4ed76b26c420cf0099da62f3b080a9f Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Fri, 29 Nov 2024 18:34:37 -0600 Subject: [PATCH 2/5] internal: Migrate `remove_mut` assist to `SyntaxEditor` --- crates/ide-assists/src/handlers/remove_mut.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/crates/ide-assists/src/handlers/remove_mut.rs b/crates/ide-assists/src/handlers/remove_mut.rs index 0b299e8349ac..43740a5a6d5c 100644 --- a/crates/ide-assists/src/handlers/remove_mut.rs +++ b/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); }, ) } From 6dda2e81c6dcc243afdba87c02265e2fede5eaad Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Wed, 4 Dec 2024 19:51:15 -0500 Subject: [PATCH 3/5] internal: Migrate `remove_parentheses` assist to `SyntaxEditor` --- .../src/handlers/remove_parentheses.rs | 17 +++++++++++++---- docs/book/src/assists_generated.md | 2 +- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/crates/ide-assists/src/handlers/remove_parentheses.rs b/crates/ide-assists/src/handlers/remove_parentheses.rs index f74fc2611282..143d5e542428 100644 --- a/crates/ide-assists/src/handlers/remove_parentheses.rs +++ b/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); }, ) } diff --git a/docs/book/src/assists_generated.md b/docs/book/src/assists_generated.md index 52da98dbd21d..a05536e2d2d5 100644 --- a/docs/book/src/assists_generated.md +++ b/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. From 93c9f068704197791ee0e92c8863ba03673021f0 Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Wed, 4 Dec 2024 19:53:22 -0500 Subject: [PATCH 4/5] fix: Properly handle removals in `SyntaxEditor` --- crates/syntax/src/syntax_editor/edit_algo.rs | 24 +++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/crates/syntax/src/syntax_editor/edit_algo.rs b/crates/syntax/src/syntax_editor/edit_algo.rs index d6d903715d10..fa51fb6eef42 100644 --- a/crates/syntax/src/syntax_editor/edit_algo.rs +++ b/crates/syntax/src/syntax_editor/edit_algo.rs @@ -102,6 +102,7 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit { let mut changed_ancestors: VecDeque = 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) @@ -116,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 @@ -195,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") } @@ -234,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; From f155aef64d10d9ce36135588b1c1ee92b8111480 Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Wed, 4 Dec 2024 19:55:16 -0500 Subject: [PATCH 5/5] internal: Migrate `remove_unused_param` assist to `SyntaxEditor` --- .../src/handlers/remove_unused_param.rs | 62 ++++++++++++++----- crates/syntax/src/algo.rs | 24 ++++++- docs/book/src/assists_generated.md | 2 +- 3 files changed, 71 insertions(+), 17 deletions(-) diff --git a/crates/ide-assists/src/handlers/remove_unused_param.rs b/crates/ide-assists/src/handlers/remove_unused_param.rs index 75120768da0f..5ddb17b20729 100644 --- a/crates/ide-assists/src/handlers/remove_unused_param.rs +++ b/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 = 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 { +) -> Option> { let call_expr_opt: Option = 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 = @@ -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 { + 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}; diff --git a/crates/syntax/src/algo.rs b/crates/syntax/src/algo.rs index 2acb2158318a..3b85b137aa9b 100644 --- a/crates/syntax/src/algo.rs +++ b/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 Option { + 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(me: &T, direction: Direction) -> Option { me.syntax().siblings(direction).skip(1).find_map(T::cast) } diff --git a/docs/book/src/assists_generated.md b/docs/book/src/assists_generated.md index a05536e2d2d5..2d233ca62ad6 100644 --- a/docs/book/src/assists_generated.md +++ b/docs/book/src/assists_generated.md @@ -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.