From 2ba82fdd2401d5dc32b90af703394c663525fc42 Mon Sep 17 00:00:00 2001 From: Luuk Wester Date: Sun, 2 Mar 2025 19:30:33 +0100 Subject: [PATCH 1/5] wip --- crates/ide-assists/src/handlers/remove_dbg.rs | 61 ++++++++++--------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/crates/ide-assists/src/handlers/remove_dbg.rs b/crates/ide-assists/src/handlers/remove_dbg.rs index 1f57f7d3d376..510fabdb72ad 100644 --- a/crates/ide-assists/src/handlers/remove_dbg.rs +++ b/crates/ide-assists/src/handlers/remove_dbg.rs @@ -1,7 +1,9 @@ use itertools::Itertools; use syntax::{ ast::{self, make, AstNode, AstToken}, - match_ast, ted, Edition, NodeOrToken, SyntaxElement, TextRange, TextSize, T, + match_ast, + syntax_editor::SyntaxEditor, + Edition, NodeOrToken, SyntaxElement, TextRange, TextSize, T, }; use crate::{AssistContext, AssistId, AssistKind, Assists}; @@ -37,22 +39,18 @@ pub(crate) fn remove_dbg(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<( .collect() }; - let replacements = - macro_calls.into_iter().filter_map(compute_dbg_replacement).collect::>(); + let root = match ctx.covering_element() { + NodeOrToken::Node(node) => node.ancestors().last().unwrap(), + NodeOrToken::Token(token) => token.parent_ancestors().last().unwrap(), + }; + let mut editor = SyntaxEditor::new(root); + let replacements = macro_calls.iter().filter_map(|mc| run_dbg_replacement(mc, &mut editor)); acc.add( AssistId("remove_dbg", AssistKind::QuickFix), "Remove dbg!()", - replacements.iter().map(|&(range, _)| range).reduce(|acc, range| acc.cover(range))?, - |builder| { - for (range, expr) in replacements { - if let Some(expr) = expr { - builder.replace(range, expr.to_string()); - } else { - builder.delete(range); - } - } - }, + replacements.map(|(range, _)| range).reduce(|acc, range| acc.cover(range))?, + |builder| builder.add_file_edits(ctx.file_id(), editor), ) } @@ -62,7 +60,10 @@ pub(crate) fn remove_dbg(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<( /// - (`macro_expr` has no parent - is that possible?) /// /// Returns `Some(_, None)` when the macro call should just be removed. -fn compute_dbg_replacement(macro_expr: ast::MacroExpr) -> Option<(TextRange, Option)> { +fn run_dbg_replacement( + macro_expr: &ast::MacroExpr, + editor: &mut SyntaxEditor, +) -> Option<(TextRange, Option)> { let macro_call = macro_expr.macro_call()?; let tt = macro_call.token_tree()?; let r_delim = NodeOrToken::Token(tt.right_delimiter_token()?); @@ -92,6 +93,7 @@ fn compute_dbg_replacement(macro_expr: ast::MacroExpr) -> Option<(TextRange, Opt Some(start) => range.cover_offset(start), None => range, }; + editor.delete(macro_expr.syntax()); (range, None) }, ast::ExprStmt(it) => { @@ -100,15 +102,18 @@ fn compute_dbg_replacement(macro_expr: ast::MacroExpr) -> Option<(TextRange, Opt Some(start) => range.cover_offset(start), None => range, }; + editor.delete(macro_expr.syntax()); (range, None) }, - _ => (macro_call.syntax().text_range(), Some(make::ext::expr_unit())), + _ => { + editor.replace(macro_call.syntax(), make::ext::expr_unit().syntax().clone_for_update()); + (macro_call.syntax().text_range(), Some(make::ext::expr_unit())) + }, } } } // dbg!(expr0) [expr] => { - // dbg!(expr, &parent); let wrap = match ast::Expr::cast(parent) { Some(parent) => match (expr, parent) { (ast::Expr::CastExpr(_), ast::Expr::CastExpr(_)) => false, @@ -144,25 +149,27 @@ fn compute_dbg_replacement(macro_expr: ast::MacroExpr) -> Option<(TextRange, Opt }, None => false, }; - let expr = replace_nested_dbgs(expr.clone()); - let expr = if wrap { make::expr_paren(expr) } else { expr.clone_subtree() }; + let expr = replace_nested_dbgs(expr.clone(), editor); + let expr = if wrap { make::expr_paren(expr.clone()) } else { expr.clone_subtree() }; + editor.replace(macro_call.syntax(), expr.syntax().clone_for_update()); (macro_call.syntax().text_range(), Some(expr)) } // dbg!(expr0, expr1, ...) exprs => { - let exprs = exprs.iter().cloned().map(replace_nested_dbgs); + let exprs = exprs.iter().map(|expr| replace_nested_dbgs(expr.clone(), editor)); let expr = make::expr_tuple(exprs); + editor.replace(macro_call.syntax(), expr.syntax().clone_for_update()); (macro_call.syntax().text_range(), Some(expr.into())) } }) } -fn replace_nested_dbgs(expanded: ast::Expr) -> ast::Expr { +fn replace_nested_dbgs(expanded: ast::Expr, editor: &mut SyntaxEditor) -> ast::Expr { if let ast::Expr::MacroExpr(mac) = &expanded { // Special-case when `expanded` itself is `dbg!()` since we cannot replace the whole tree // with `ted`. It should be fairly rare as it means the user wrote `dbg!(dbg!(..))` but you // never know how code ends up being! - let replaced = if let Some((_, expr_opt)) = compute_dbg_replacement(mac.clone()) { + let replaced = if let Some((_, expr_opt)) = run_dbg_replacement(mac, editor) { match expr_opt { Some(expr) => expr, None => { @@ -177,22 +184,18 @@ fn replace_nested_dbgs(expanded: ast::Expr) -> ast::Expr { return replaced; } - let expanded = expanded.clone_for_update(); - - // We need to collect to avoid mutation during traversal. - let macro_exprs: Vec<_> = - expanded.syntax().descendants().filter_map(ast::MacroExpr::cast).collect(); + let macro_exprs = expanded.syntax().descendants().filter_map(ast::MacroExpr::cast); for mac in macro_exprs { - let expr_opt = match compute_dbg_replacement(mac.clone()) { + let expr_opt = match run_dbg_replacement(&mac, editor) { Some((_, expr)) => expr, None => continue, }; if let Some(expr) = expr_opt { - ted::replace(mac.syntax(), expr.syntax().clone_for_update()); + editor.replace(mac.syntax(), expr.syntax().clone_for_update()); } else { - ted::remove(mac.syntax()); + editor.delete(mac.syntax()); } } From 60bdb2d66df28695a68552c5f6d043a738cc21d1 Mon Sep 17 00:00:00 2001 From: Luuk Wester Date: Sun, 13 Apr 2025 23:13:14 +0200 Subject: [PATCH 2/5] delete the whole ast::ExprStmt instead of just the dbg macro if the parent is an ExprStmt --- crates/ide-assists/src/handlers/remove_dbg.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ide-assists/src/handlers/remove_dbg.rs b/crates/ide-assists/src/handlers/remove_dbg.rs index 510fabdb72ad..9cd58397bf60 100644 --- a/crates/ide-assists/src/handlers/remove_dbg.rs +++ b/crates/ide-assists/src/handlers/remove_dbg.rs @@ -96,9 +96,9 @@ fn run_dbg_replacement( editor.delete(macro_expr.syntax()); (range, None) }, - ast::ExprStmt(it) => { - let range = it.syntax().text_range(); - let range = match whitespace_start(it.syntax().prev_sibling_or_token()) { + ast::ExprStmt(_) => { + let range = parent.text_range(); + let range = match whitespace_start(parent.prev_sibling_or_token()) { Some(start) => range.cover_offset(start), None => range, }; From df0606bf8d71e875fc52dcfc22608eec648f2992 Mon Sep 17 00:00:00 2001 From: Luuk Wester Date: Mon, 14 Apr 2025 00:00:10 +0200 Subject: [PATCH 3/5] actually properly delete the whole ExprStmt --- crates/ide-assists/src/handlers/remove_dbg.rs | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/crates/ide-assists/src/handlers/remove_dbg.rs b/crates/ide-assists/src/handlers/remove_dbg.rs index b742a7de6927..9c20532985a8 100644 --- a/crates/ide-assists/src/handlers/remove_dbg.rs +++ b/crates/ide-assists/src/handlers/remove_dbg.rs @@ -89,20 +89,28 @@ fn run_dbg_replacement( match parent { ast::StmtList(_) => { let range = macro_expr.syntax().text_range(); - let range = match whitespace_start(macro_expr.syntax().prev_sibling_or_token()) { - Some(start) => range.cover_offset(start), - None => range, + let prev = macro_expr.syntax().prev_sibling_or_token(); + let range = match (whitespace_start(prev.clone()), prev) { + (Some(start), Some(prev)) => { + editor.delete(prev); + range.cover_offset(start) + }, + _ => range, }; editor.delete(macro_expr.syntax()); (range, None) }, ast::ExprStmt(_) => { let range = parent.text_range(); - let range = match whitespace_start(parent.prev_sibling_or_token()) { - Some(start) => range.cover_offset(start), - None => range, + let prev = parent.prev_sibling_or_token(); + let range = match (whitespace_start(prev.clone()), prev) { + (Some(start), Some(prev)) => { + editor.delete(prev); + range.cover_offset(start) + }, + _ => range, }; - editor.delete(macro_expr.syntax()); + editor.delete(parent); (range, None) }, _ => { From 212d4bfce734d419c85ab55674f1502cf49f6fcc Mon Sep 17 00:00:00 2001 From: Luuk Wester Date: Mon, 14 Apr 2025 01:25:55 +0200 Subject: [PATCH 4/5] wip: switch to (multiple??) sub editors --- crates/ide-assists/src/handlers/remove_dbg.rs | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/crates/ide-assists/src/handlers/remove_dbg.rs b/crates/ide-assists/src/handlers/remove_dbg.rs index 9c20532985a8..559f88519587 100644 --- a/crates/ide-assists/src/handlers/remove_dbg.rs +++ b/crates/ide-assists/src/handlers/remove_dbg.rs @@ -157,14 +157,14 @@ fn run_dbg_replacement( }, None => false, }; - let expr = replace_nested_dbgs(expr.clone(), editor); + let expr = replace_nested_dbgs(expr.clone()); let expr = if wrap { make::expr_paren(expr.clone()) } else { expr.clone_subtree() }; editor.replace(macro_call.syntax(), expr.syntax().clone_for_update()); (macro_call.syntax().text_range(), Some(expr)) } // dbg!(expr0, expr1, ...) exprs => { - let exprs = exprs.iter().map(|expr| replace_nested_dbgs(expr.clone(), editor)); + let exprs = exprs.iter().map(|expr| replace_nested_dbgs(expr.clone())); let expr = make::expr_tuple(exprs); editor.replace(macro_call.syntax(), expr.syntax().clone_for_update()); (macro_call.syntax().text_range(), Some(expr.into())) @@ -172,42 +172,44 @@ fn run_dbg_replacement( }) } -fn replace_nested_dbgs(expanded: ast::Expr, editor: &mut SyntaxEditor) -> ast::Expr { - if let ast::Expr::MacroExpr(mac) = &expanded { - // Special-case when `expanded` itself is `dbg!()` since we cannot replace the whole tree - // with `ted`. It should be fairly rare as it means the user wrote `dbg!(dbg!(..))` but you - // never know how code ends up being! - let replaced = if let Some((_, expr_opt)) = run_dbg_replacement(mac, editor) { - match expr_opt { - Some(expr) => expr, - None => { - stdx::never!("dbg! inside dbg! should not be just removed"); - expanded - } - } - } else { - expanded - }; +fn replace_nested_dbgs(expanded: ast::Expr) -> ast::Expr { + let mut sub_editor = SyntaxEditor::new(expanded.syntax().clone()); + // if let ast::Expr::MacroExpr(mac) = &expanded { + // // Special-case when `expanded` itself is `dbg!()` since we cannot replace the whole tree + // // with `ted`. It should be fairly rare as it means the user wrote `dbg!(dbg!(..))` but you + // // never know how code ends up being! + // let replaced = if let Some((_, expr_opt)) = run_dbg_replacement(mac, &mut sub_editor) { + // match expr_opt { + // Some(expr) => expr, + // None => { + // stdx::never!("dbg! inside dbg! should not be just removed"); + // expanded + // } + // } + // } else { + // expanded + // }; - return replaced; - } + // return replaced; + // } let macro_exprs = expanded.syntax().descendants().filter_map(ast::MacroExpr::cast); for mac in macro_exprs { - let expr_opt = match run_dbg_replacement(&mac, editor) { + // let mut fake_editor = SyntaxEditor::new(expanded.syntax().clone()); + let expr_opt = match run_dbg_replacement(&mac, &mut sub_editor) { Some((_, expr)) => expr, None => continue, }; - if let Some(expr) = expr_opt { - editor.replace(mac.syntax(), expr.syntax().clone_for_update()); - } else { - editor.delete(mac.syntax()); - } + // if let Some(expr) = expr_opt { + // sub_editor.replace(mac.syntax(), expr.syntax().clone()); + // } else { + // sub_editor.delete(mac.syntax()); + // } } - expanded + ast::Expr::cast(sub_editor.finish().new_root().clone()).unwrap() } fn whitespace_start(it: Option) -> Option { From b7cff26c7c45165772bcee488a6c2ad48e84a0ed Mon Sep 17 00:00:00 2001 From: Luuk Wester Date: Sun, 27 Apr 2025 21:44:08 +0200 Subject: [PATCH 5/5] back to a single sub editor --- crates/ide-assists/src/handlers/remove_dbg.rs | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/crates/ide-assists/src/handlers/remove_dbg.rs b/crates/ide-assists/src/handlers/remove_dbg.rs index ac9ac815ee14..c212f24ee159 100644 --- a/crates/ide-assists/src/handlers/remove_dbg.rs +++ b/crates/ide-assists/src/handlers/remove_dbg.rs @@ -174,24 +174,24 @@ fn run_dbg_replacement( fn replace_nested_dbgs(expanded: ast::Expr) -> ast::Expr { let mut sub_editor = SyntaxEditor::new(expanded.syntax().clone()); - // if let ast::Expr::MacroExpr(mac) = &expanded { - // // Special-case when `expanded` itself is `dbg!()` since we cannot replace the whole tree - // // with `ted`. It should be fairly rare as it means the user wrote `dbg!(dbg!(..))` but you - // // never know how code ends up being! - // let replaced = if let Some((_, expr_opt)) = run_dbg_replacement(mac, &mut sub_editor) { - // match expr_opt { - // Some(expr) => expr, - // None => { - // stdx::never!("dbg! inside dbg! should not be just removed"); - // expanded - // } - // } - // } else { - // expanded - // }; + if let ast::Expr::MacroExpr(mac) = &expanded { + // Special-case when `expanded` itself is `dbg!()` since we cannot replace the whole tree + // with `ted`. It should be fairly rare as it means the user wrote `dbg!(dbg!(..))` but you + // never know how code ends up being! + let replaced = if let Some((_, expr_opt)) = run_dbg_replacement(mac, &mut sub_editor) { + match expr_opt { + Some(expr) => expr, + None => { + stdx::never!("dbg! inside dbg! should not be just removed"); + expanded + } + } + } else { + expanded + }; - // return replaced; - // } + return replaced; + } let macro_exprs = expanded.syntax().descendants().filter_map(ast::MacroExpr::cast); @@ -202,11 +202,11 @@ fn replace_nested_dbgs(expanded: ast::Expr) -> ast::Expr { None => continue, }; - // if let Some(expr) = expr_opt { - // sub_editor.replace(mac.syntax(), expr.syntax().clone()); - // } else { - // sub_editor.delete(mac.syntax()); - // } + if let Some(expr) = expr_opt { + sub_editor.replace(mac.syntax(), expr.syntax().clone()); + } else { + sub_editor.delete(mac.syntax()); + } } ast::Expr::cast(sub_editor.finish().new_root().clone()).unwrap()