From ff1124918e9035d401fb707f6307e99ace052f66 Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Sat, 16 Nov 2024 20:01:05 -0500 Subject: [PATCH 1/8] fix: Properly determine `SyntaxEditor` replacement intersection Bordering replacements should not be considered intersecting --- crates/syntax/src/syntax_editor/edit_algo.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/syntax/src/syntax_editor/edit_algo.rs b/crates/syntax/src/syntax_editor/edit_algo.rs index b769c941105b..44a1fae23beb 100644 --- a/crates/syntax/src/syntax_editor/edit_algo.rs +++ b/crates/syntax/src/syntax_editor/edit_algo.rs @@ -73,7 +73,7 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit { }) .all(|(l, r)| { get_node_depth(l.target_parent()) != get_node_depth(r.target_parent()) - || l.target_range().intersect(r.target_range()).is_none() + || (l.target_range().end() <= r.target_range().start()) }); if stdx::never!( From 5eb8affdc5a5f86ce60c9e112a3af257070c1798 Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Sun, 17 Nov 2024 15:36:57 -0500 Subject: [PATCH 2/8] fix: Don't produce `ChangedAncestor` for `SyntaxToken`s --- crates/syntax/src/syntax_editor.rs | 48 +++++++++++++++++++- crates/syntax/src/syntax_editor/edit_algo.rs | 14 ++---- 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/crates/syntax/src/syntax_editor.rs b/crates/syntax/src/syntax_editor.rs index 7e5d0f27e0a0..992a847663af 100644 --- a/crates/syntax/src/syntax_editor.rs +++ b/crates/syntax/src/syntax_editor.rs @@ -327,7 +327,7 @@ mod tests { use crate::{ ast::{self, make, syntax_factory::SyntaxFactory}, - AstNode, + AstNode, SyntaxKind, }; use super::*; @@ -540,4 +540,50 @@ mod tests { }"#]]; expect.assert_eq(&edit.new_root.to_string()); } + + #[test] + fn test_replace_token_in_parent() { + let parent_fn = make::fn_( + None, + make::name("it"), + None, + None, + make::param_list(None, []), + make::block_expr([], Some(make::expr_unit())), + Some(make::ret_type(make::ty_unit())), + false, + false, + false, + false, + ); + + let mut editor = SyntaxEditor::new(parent_fn.syntax().clone()); + + if let Some(ret_ty) = parent_fn.ret_type() { + editor.delete(ret_ty.syntax().clone()); + + if let Some(SyntaxElement::Token(token)) = ret_ty.syntax().next_sibling_or_token() { + if token.kind().is_trivia() { + editor.delete(token); + } + } + } + + if let Some(tail) = parent_fn.body().unwrap().tail_expr() { + // FIXME: We do this because `xtask tidy` will not allow us to have trailing whitespace in the expect string. + if let Some(SyntaxElement::Token(token)) = tail.syntax().prev_sibling_or_token() { + if let SyntaxKind::WHITESPACE = token.kind() { + editor.delete(token); + } + } + editor.delete(tail.syntax().clone()); + } + + let edit = editor.finish(); + + let expect = expect![[r#" +fn it() { +}"#]]; + expect.assert_eq(&edit.new_root.to_string()); + } } diff --git a/crates/syntax/src/syntax_editor/edit_algo.rs b/crates/syntax/src/syntax_editor/edit_algo.rs index 44a1fae23beb..71b69dbec1d6 100644 --- a/crates/syntax/src/syntax_editor/edit_algo.rs +++ b/crates/syntax/src/syntax_editor/edit_algo.rs @@ -128,13 +128,14 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit { // Add to changed ancestors, if applicable match change { - Change::Insert(_, _) | Change::InsertAll(_, _) => {} - Change::Replace(target, _) | Change::ReplaceWithMany(target, _) => { + Change::Replace(SyntaxElement::Node(target), _) + | Change::ReplaceWithMany(SyntaxElement::Node(target), _) => { changed_ancestors.push_back(ChangedAncestor::single(target, change_index)) } Change::ReplaceAll(range, _) => { changed_ancestors.push_back(ChangedAncestor::multiple(range, change_index)) } + _ => (), } } @@ -304,13 +305,8 @@ enum ChangedAncestorKind { } impl ChangedAncestor { - fn single(element: &SyntaxElement, change_index: usize) -> Self { - let kind = match element { - SyntaxElement::Node(node) => ChangedAncestorKind::Single { node: node.clone() }, - SyntaxElement::Token(token) => { - ChangedAncestorKind::Single { node: token.parent().unwrap() } - } - }; + fn single(node: &SyntaxNode, change_index: usize) -> Self { + let kind = ChangedAncestorKind::Single { node: node.clone() }; Self { kind, change_index } } From 64060486269b6ff5e54606796fe66cb17189186b Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Thu, 21 Nov 2024 13:34:12 -0500 Subject: [PATCH 3/8] minor: Add `token` constructor to `SyntaxFactory` --- crates/syntax/src/ast/syntax_factory/constructors.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/syntax/src/ast/syntax_factory/constructors.rs b/crates/syntax/src/ast/syntax_factory/constructors.rs index 8b0610940730..0d8fe4cd23ad 100644 --- a/crates/syntax/src/ast/syntax_factory/constructors.rs +++ b/crates/syntax/src/ast/syntax_factory/constructors.rs @@ -4,7 +4,7 @@ use itertools::Itertools; use crate::{ ast::{self, make, HasName, HasTypeBounds}, syntax_editor::SyntaxMappingBuilder, - AstNode, + AstNode, SyntaxKind, SyntaxToken, }; use super::SyntaxFactory; @@ -151,4 +151,8 @@ impl SyntaxFactory { ast } + + pub fn token(&self, kind: SyntaxKind) -> SyntaxToken { + make::token(kind) + } } From fbb392062ab202073eb19144ebae407be21c127b Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Thu, 21 Nov 2024 15:16:06 -0500 Subject: [PATCH 4/8] minor: Add `expr_bin` constructor to `SyntaxFactory` --- .../src/ast/syntax_factory/constructors.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/crates/syntax/src/ast/syntax_factory/constructors.rs b/crates/syntax/src/ast/syntax_factory/constructors.rs index 0d8fe4cd23ad..944724ff05c1 100644 --- a/crates/syntax/src/ast/syntax_factory/constructors.rs +++ b/crates/syntax/src/ast/syntax_factory/constructors.rs @@ -80,6 +80,23 @@ impl SyntaxFactory { ast } + pub fn expr_bin(&self, lhs: ast::Expr, op: ast::BinaryOp, rhs: ast::Expr) -> ast::BinExpr { + let ast::Expr::BinExpr(ast) = + make::expr_bin_op(lhs.clone(), op, rhs.clone()).clone_for_update() + else { + unreachable!() + }; + + if let Some(mut mapping) = self.mappings() { + let mut builder = SyntaxMappingBuilder::new(ast.syntax().clone()); + builder.map_node(lhs.syntax().clone(), ast.lhs().unwrap().syntax().clone()); + builder.map_node(rhs.syntax().clone(), ast.rhs().unwrap().syntax().clone()); + builder.finish(&mut mapping); + } + + ast + } + pub fn expr_path(&self, path: ast::Path) -> ast::Expr { let ast::Expr::PathExpr(ast) = make::expr_path(path.clone()).clone_for_update() else { unreachable!() From 09dee81412a6f5e152eae7ce78d96e1e06f45930 Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Thu, 21 Nov 2024 16:23:27 -0500 Subject: [PATCH 5/8] feat: Migrate `flip_binexpr` assist to `SyntaxEditor` --- .../ide-assists/src/handlers/flip_binexpr.rs | 55 ++++++++++--------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/crates/ide-assists/src/handlers/flip_binexpr.rs b/crates/ide-assists/src/handlers/flip_binexpr.rs index 8b46a23f9a64..601fd29f8e7c 100644 --- a/crates/ide-assists/src/handlers/flip_binexpr.rs +++ b/crates/ide-assists/src/handlers/flip_binexpr.rs @@ -1,4 +1,7 @@ -use syntax::ast::{self, AstNode, BinExpr}; +use syntax::{ + ast::{self, syntax_factory::SyntaxFactory, AstNode, BinExpr}, + SyntaxKind, T, +}; use crate::{AssistContext, AssistId, AssistKind, Assists}; @@ -19,22 +22,17 @@ use crate::{AssistContext, AssistId, AssistKind, Assists}; // ``` pub(crate) fn flip_binexpr(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { let expr = ctx.find_node_at_offset::()?; - let rhs = expr.rhs()?.syntax().clone(); - let lhs = expr.lhs()?.syntax().clone(); - - let lhs = if let Some(bin_expr) = BinExpr::cast(lhs.clone()) { - if bin_expr.op_kind() == expr.op_kind() { - bin_expr.rhs()?.syntax().clone() - } else { - lhs - } - } else { - lhs + let lhs = expr.lhs()?; + let rhs = expr.rhs()?; + + let lhs = match &lhs { + ast::Expr::BinExpr(bin_expr) if bin_expr.op_kind() == expr.op_kind() => bin_expr.rhs()?, + _ => lhs, }; - let op_range = expr.op_token()?.text_range(); + let op_token = expr.op_token()?; // The assist should be applied only if the cursor is on the operator - let cursor_in_range = op_range.contains_range(ctx.selection_trimmed()); + let cursor_in_range = op_token.text_range().contains_range(ctx.selection_trimmed()); if !cursor_in_range { return None; } @@ -47,13 +45,18 @@ pub(crate) fn flip_binexpr(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option acc.add( AssistId("flip_binexpr", AssistKind::RefactorRewrite), "Flip binary expression", - op_range, - |edit| { - if let FlipAction::FlipAndReplaceOp(new_op) = action { - edit.replace(op_range, new_op); - } - edit.replace(lhs.text_range(), rhs.text()); - edit.replace(rhs.text_range(), lhs.text()); + op_token.text_range(), + |builder| { + let mut editor = builder.make_editor(&expr.syntax().parent().unwrap()); + let make = SyntaxFactory::new(); + if let FlipAction::FlipAndReplaceOp(binary_op) = action { + editor.replace(op_token, make.token(binary_op)) + }; + // FIXME: remove `clone_for_update` when `SyntaxEditor` handles it for us + editor.replace(lhs.syntax(), rhs.syntax().clone_for_update()); + editor.replace(rhs.syntax(), lhs.syntax().clone_for_update()); + editor.add_mappings(make.finish_with_mappings()); + builder.add_file_edits(ctx.file_id(), editor); }, ) } @@ -62,7 +65,7 @@ enum FlipAction { // Flip the expression Flip, // Flip the expression and replace the operator with this string - FlipAndReplaceOp(&'static str), + FlipAndReplaceOp(SyntaxKind), // Do not flip the expression DontFlip, } @@ -73,10 +76,10 @@ impl From for FlipAction { ast::BinaryOp::Assignment { .. } => FlipAction::DontFlip, ast::BinaryOp::CmpOp(ast::CmpOp::Ord { ordering, strict }) => { let rev_op = match (ordering, strict) { - (ast::Ordering::Less, true) => ">", - (ast::Ordering::Less, false) => ">=", - (ast::Ordering::Greater, true) => "<", - (ast::Ordering::Greater, false) => "<=", + (ast::Ordering::Less, true) => T![>], + (ast::Ordering::Less, false) => T![>=], + (ast::Ordering::Greater, true) => T![<], + (ast::Ordering::Greater, false) => T![<=], }; FlipAction::FlipAndReplaceOp(rev_op) } From d55879d1cbf38f5b7f8b36a41164412e7b78617e Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Fri, 22 Nov 2024 09:49:19 -0500 Subject: [PATCH 6/8] feat: Migrate `flip_trait_bound` assist to `SyntaxEditor` --- .../ide-assists/src/handlers/flip_trait_bound.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/ide-assists/src/handlers/flip_trait_bound.rs b/crates/ide-assists/src/handlers/flip_trait_bound.rs index 70b5efcb645a..03366bd8616c 100644 --- a/crates/ide-assists/src/handlers/flip_trait_bound.rs +++ b/crates/ide-assists/src/handlers/flip_trait_bound.rs @@ -23,11 +23,11 @@ pub(crate) fn flip_trait_bound(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op let plus = ctx.find_token_syntax_at_offset(T![+])?; // Make sure we're in a `TypeBoundList` - ast::TypeBoundList::cast(plus.parent()?)?; + let parent = ast::TypeBoundList::cast(plus.parent()?)?; let (before, after) = ( - non_trivia_sibling(plus.clone().into(), Direction::Prev)?, - non_trivia_sibling(plus.clone().into(), Direction::Next)?, + non_trivia_sibling(plus.clone().into(), Direction::Prev)?.into_node()?, + non_trivia_sibling(plus.clone().into(), Direction::Next)?.into_node()?, ); let target = plus.text_range(); @@ -35,9 +35,11 @@ pub(crate) fn flip_trait_bound(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op AssistId("flip_trait_bound", AssistKind::RefactorRewrite), "Flip trait bounds", target, - |edit| { - edit.replace(before.text_range(), after.to_string()); - edit.replace(after.text_range(), before.to_string()); + |builder| { + let mut editor = builder.make_editor(parent.syntax()); + editor.replace(before.clone(), after.clone_for_update()); + editor.replace(after.clone(), before.clone_for_update()); + builder.add_file_edits(ctx.file_id(), editor); }, ) } From 8fd7790eb51b546b18cbe209f910c396917bd88e Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Sat, 23 Nov 2024 12:41:48 -0500 Subject: [PATCH 7/8] minor: Add `token_tree` constructor to `SyntaxFactory` --- .../src/ast/syntax_factory/constructors.rs | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/crates/syntax/src/ast/syntax_factory/constructors.rs b/crates/syntax/src/ast/syntax_factory/constructors.rs index 944724ff05c1..54f17bd721d5 100644 --- a/crates/syntax/src/ast/syntax_factory/constructors.rs +++ b/crates/syntax/src/ast/syntax_factory/constructors.rs @@ -4,7 +4,7 @@ use itertools::Itertools; use crate::{ ast::{self, make, HasName, HasTypeBounds}, syntax_editor::SyntaxMappingBuilder, - AstNode, SyntaxKind, SyntaxToken, + AstNode, NodeOrToken, SyntaxKind, SyntaxNode, SyntaxToken, }; use super::SyntaxFactory; @@ -169,6 +169,32 @@ impl SyntaxFactory { ast } + pub fn token_tree( + &self, + delimiter: SyntaxKind, + tt: Vec>, + ) -> ast::TokenTree { + let tt: Vec<_> = tt.into_iter().collect(); + let input: Vec<_> = tt.iter().cloned().filter_map(only_nodes).collect(); + + let ast = make::token_tree(delimiter, tt).clone_for_update(); + + if let Some(mut mapping) = self.mappings() { + let mut builder = SyntaxMappingBuilder::new(ast.syntax().clone()); + builder.map_children( + input.into_iter(), + ast.token_trees_and_tokens().filter_map(only_nodes), + ); + builder.finish(&mut mapping); + } + + return ast; + + fn only_nodes(element: NodeOrToken) -> Option { + element.as_node().map(|it| it.syntax().clone()) + } + } + pub fn token(&self, kind: SyntaxKind) -> SyntaxToken { make::token(kind) } From d329329b76dc4a15d9803a5e5adec45e48f940fd Mon Sep 17 00:00:00 2001 From: Giga Bowser <45986823+Giga-Bowser@users.noreply.github.com> Date: Sat, 23 Nov 2024 12:42:49 -0500 Subject: [PATCH 8/8] feat: Migrate `flip_comma` assist to `SyntaxEditor` --- crates/ide-assists/src/handlers/flip_comma.rs | 122 ++++++++++++------ 1 file changed, 79 insertions(+), 43 deletions(-) diff --git a/crates/ide-assists/src/handlers/flip_comma.rs b/crates/ide-assists/src/handlers/flip_comma.rs index af2c2c759ec7..490a9ee3c04d 100644 --- a/crates/ide-assists/src/handlers/flip_comma.rs +++ b/crates/ide-assists/src/handlers/flip_comma.rs @@ -1,7 +1,8 @@ -use ide_db::base_db::SourceDatabase; -use syntax::TextSize; use syntax::{ - algo::non_trivia_sibling, ast, AstNode, Direction, SyntaxKind, SyntaxToken, TextRange, T, + algo::non_trivia_sibling, + ast::{self, syntax_factory::SyntaxFactory}, + syntax_editor::{Element, SyntaxMapping}, + AstNode, Direction, NodeOrToken, SyntaxElement, SyntaxKind, SyntaxToken, T, }; use crate::{AssistContext, AssistId, AssistKind, Assists}; @@ -25,8 +26,6 @@ pub(crate) fn flip_comma(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<( let comma = ctx.find_token_syntax_at_offset(T![,])?; let prev = non_trivia_sibling(comma.clone().into(), Direction::Prev)?; let next = non_trivia_sibling(comma.clone().into(), Direction::Next)?; - let (mut prev_text, mut next_text) = (prev.to_string(), next.to_string()); - let (mut prev_range, mut next_range) = (prev.text_range(), next.text_range()); // Don't apply a "flip" in case of a last comma // that typically comes before punctuation @@ -40,53 +39,85 @@ pub(crate) fn flip_comma(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<( return None; } - if let Some(parent) = comma.parent().and_then(ast::TokenTree::cast) { - // An attribute. It often contains a path followed by a token tree (e.g. `align(2)`), so we have - // to be smarter. - let prev_start = - match comma.siblings_with_tokens(Direction::Prev).skip(1).find(|it| it.kind() == T![,]) - { - Some(it) => position_after_token(it.as_token().unwrap()), - None => position_after_token(&parent.left_delimiter_token()?), - }; - let prev_end = prev.text_range().end(); - let next_start = next.text_range().start(); - let next_end = - match comma.siblings_with_tokens(Direction::Next).skip(1).find(|it| it.kind() == T![,]) - { - Some(it) => position_before_token(it.as_token().unwrap()), - None => position_before_token(&parent.right_delimiter_token()?), - }; - prev_range = TextRange::new(prev_start, prev_end); - next_range = TextRange::new(next_start, next_end); - let file_text = ctx.db().file_text(ctx.file_id().file_id()); - prev_text = file_text[prev_range].to_owned(); - next_text = file_text[next_range].to_owned(); - } + // FIXME: remove `clone_for_update` when `SyntaxEditor` handles it for us + let prev = match prev { + SyntaxElement::Node(node) => node.clone_for_update().syntax_element(), + _ => prev, + }; + let next = match next { + SyntaxElement::Node(node) => node.clone_for_update().syntax_element(), + _ => next, + }; acc.add( AssistId("flip_comma", AssistKind::RefactorRewrite), "Flip comma", comma.text_range(), - |edit| { - edit.replace(prev_range, next_text); - edit.replace(next_range, prev_text); + |builder| { + let parent = comma.parent().unwrap(); + let mut editor = builder.make_editor(&parent); + + if let Some(parent) = ast::TokenTree::cast(parent) { + // An attribute. It often contains a path followed by a + // token tree (e.g. `align(2)`), so we have to be smarter. + let (new_tree, mapping) = flip_tree(parent.clone(), comma); + editor.replace(parent.syntax(), new_tree.syntax()); + editor.add_mappings(mapping); + } else { + editor.replace(prev.clone(), next.clone()); + editor.replace(next.clone(), prev.clone()); + } + + builder.add_file_edits(ctx.file_id(), editor); }, ) } -fn position_before_token(token: &SyntaxToken) -> TextSize { - match non_trivia_sibling(token.clone().into(), Direction::Prev) { - Some(prev_token) => prev_token.text_range().end(), - None => token.text_range().start(), - } -} - -fn position_after_token(token: &SyntaxToken) -> TextSize { - match non_trivia_sibling(token.clone().into(), Direction::Next) { - Some(prev_token) => prev_token.text_range().start(), - None => token.text_range().end(), - } +fn flip_tree(tree: ast::TokenTree, comma: SyntaxToken) -> (ast::TokenTree, SyntaxMapping) { + let mut tree_iter = tree.token_trees_and_tokens(); + let before: Vec<_> = + tree_iter.by_ref().take_while(|it| it.as_token() != Some(&comma)).collect(); + let after: Vec<_> = tree_iter.collect(); + + let not_ws = |element: &NodeOrToken<_, SyntaxToken>| match element { + NodeOrToken::Token(token) => token.kind() != SyntaxKind::WHITESPACE, + NodeOrToken::Node(_) => true, + }; + + let is_comma = |element: &NodeOrToken<_, SyntaxToken>| match element { + NodeOrToken::Token(token) => token.kind() == T![,], + NodeOrToken::Node(_) => false, + }; + + let prev_start_untrimmed = match before.iter().rposition(is_comma) { + Some(pos) => pos + 1, + None => 1, + }; + let prev_end = 1 + before.iter().rposition(not_ws).unwrap(); + let prev_start = prev_start_untrimmed + + before[prev_start_untrimmed..prev_end].iter().position(not_ws).unwrap(); + + let next_start = after.iter().position(not_ws).unwrap(); + let next_end_untrimmed = match after.iter().position(is_comma) { + Some(pos) => pos, + None => after.len() - 1, + }; + let next_end = 1 + after[..next_end_untrimmed].iter().rposition(not_ws).unwrap(); + + let result = [ + &before[1..prev_start], + &after[next_start..next_end], + &before[prev_end..], + &[NodeOrToken::Token(comma)], + &after[..next_start], + &before[prev_start..prev_end], + &after[next_end..after.len() - 1], + ] + .concat(); + + let make = SyntaxFactory::new(); + let new_token_tree = make.token_tree(tree.left_delimiter_token().unwrap().kind(), result); + (new_token_tree, make.finish_with_mappings()) } #[cfg(test)] @@ -147,4 +178,9 @@ mod tests { r#"#[foo(bar, qux, baz(1 + 1), other)] struct Foo;"#, ); } + + #[test] + fn flip_comma_attribute_incomplete() { + check_assist_not_applicable(flip_comma, r#"#[repr(align(2),$0)] struct Foo;"#); + } }