Unnamed repository; edit this file 'description' to name the repository.
internal: address round 2 review for inline_call SyntaxEditor migration
- file-keyed editor map in inline_into_callers (one editor per file)
- inline() takes the file editor as a parameter
- SyntaxFactory gains async_move_block_expr and expr_reborrow
- Self type generic stripping uses replacen, no sub-editor
- factory used for record_expr_field, expr_paren, expr_ref, ident_pat, let_stmt, ty
- self-token rewriting hoisted into a shared closure
| -rw-r--r-- | crates/ide-assists/src/handlers/inline_call.rs | 121 | ||||
| -rw-r--r-- | crates/syntax/src/ast/syntax_factory/constructors.rs | 52 |
2 files changed, 110 insertions, 63 deletions
diff --git a/crates/ide-assists/src/handlers/inline_call.rs b/crates/ide-assists/src/handlers/inline_call.rs index 60ab364795..37a992f6bd 100644 --- a/crates/ide-assists/src/handlers/inline_call.rs +++ b/crates/ide-assists/src/handlers/inline_call.rs @@ -7,7 +7,7 @@ use hir::{ sym, }; use ide_db::{ - EditionedFileId, RootDatabase, + EditionedFileId, FileId, FxHashMap, RootDatabase, base_db::Crate, defs::Definition, imports::insert_use::remove_use_tree_if_simple, @@ -107,6 +107,7 @@ pub(crate) fn inline_into_callers(acc: &mut Assists, ctx: &AssistContext<'_>) -> let mut usages = usages.all(); let current_file_usage = usages.references.remove(&def_file); + let mut file_editors: FxHashMap<FileId, SyntaxEditor> = FxHashMap::default(); let mut remove_def = true; let mut inline_refs_for_file = |file_id: EditionedFileId, refs: Vec<FileReference>| { let file_id = file_id.file_id(ctx.db()); @@ -126,12 +127,14 @@ pub(crate) fn inline_into_callers(acc: &mut Assists, ctx: &AssistContext<'_>) -> .map(|ci| ci.node.syntax().clone()) .or_else(|| use_trees.first().map(|ut| ut.syntax().clone())); if let Some(anchor) = anchor { - let mut editor = builder.make_editor(&anchor); + let editor = + file_editors.entry(file_id).or_insert_with(|| builder.make_editor(&anchor)); let replaced = call_infos .into_iter() .map(|call_info| { let replacement = inline( &ctx.sema, def_file, function, &func_body, ¶ms, &call_info, + editor, ); editor.replace(call_info.node.syntax(), replacement.syntax()); }) @@ -139,12 +142,11 @@ pub(crate) fn inline_into_callers(acc: &mut Assists, ctx: &AssistContext<'_>) -> if replaced + use_trees.len() == count { // we replaced all usages in this file, so we can remove the imports for use_tree in &use_trees { - remove_use_tree_if_simple(use_tree, &mut editor); + remove_use_tree_if_simple(use_tree, editor); } } else { remove_def = false; } - builder.add_file_edits(file_id, editor); } else if use_trees.len() != count { remove_def = false; } @@ -157,9 +159,13 @@ pub(crate) fn inline_into_callers(acc: &mut Assists, ctx: &AssistContext<'_>) -> None => builder.edit_file(vfs_def_file), } if remove_def { - let mut editor = builder.make_editor(ast_func.syntax()); + let editor = file_editors + .entry(vfs_def_file) + .or_insert_with(|| builder.make_editor(ast_func.syntax())); editor.delete(ast_func.syntax()); - builder.add_file_edits(vfs_def_file, editor); + } + for (file_id, editor) in file_editors { + builder.add_file_edits(file_id, editor); } }, ) @@ -245,14 +251,11 @@ pub(crate) fn inline_call(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option< let syntax = call_info.node.syntax().clone(); acc.add(AssistId::refactor_inline("inline_call"), label, syntax.text_range(), |builder| { - let replacement = inline(&ctx.sema, file_id, function, &fn_body, ¶ms, &call_info); - builder.replace_ast( - match call_info.node { - ast::CallableExpr::Call(it) => ast::Expr::CallExpr(it), - ast::CallableExpr::MethodCall(it) => ast::Expr::MethodCallExpr(it), - }, - replacement, - ); + let mut editor = builder.make_editor(call_info.node.syntax()); + let replacement = + inline(&ctx.sema, file_id, function, &fn_body, ¶ms, &call_info, &mut editor); + editor.replace(call_info.node.syntax(), replacement.syntax()); + builder.add_file_edits(ctx.vfs_file_id(), editor); }) } @@ -328,7 +331,9 @@ fn inline( fn_body: &ast::BlockExpr, params: &[(ast::Pat, Option<ast::Type>, hir::Param<'_>)], CallInfo { node, arguments, generic_arg_list, krate }: &CallInfo, + file_editor: &mut SyntaxEditor, ) -> ast::Expr { + let factory = SyntaxFactory::with_mappings(); let file_id = sema.hir_file_for(fn_body.syntax()); let body_to_clone = if let Some(macro_file) = file_id.macro_file() { cov_mark::hit!(inline_call_defined_in_macro); @@ -340,15 +345,8 @@ fn inline( fn_body.clone() }; - // Capture indent level before SyntaxEditor re-roots via clone_subtree. - // For non-macro bodies, body_to_clone still has its parent, so from_node - // finds the whitespace before `{` and returns the correct source indent level. - // For macro/prettified bodies (already re-rooted at 0), from_node returns 0, - // which is also correct since prettify_macro_expansion rebuilds indent from 0. + // Capture before `with_ast_node` re-roots and loses the source-relative position. let original_body_indent = IndentLevel::from_node(body_to_clone.syntax()); - - // The original body's start offset — needed to adjust FileReference ranges - // since SyntaxEditor::with_ast_node re-roots the tree to offset 0 let body_offset = body_to_clone.syntax().text_range().start(); let (mut editor, body) = SyntaxEditor::with_ast_node(&body_to_clone); @@ -424,20 +422,22 @@ fn inline( .filter(|tok| tok.kind() == SyntaxKind::SELF_TYPE_KW) .collect(); for self_tok in self_tokens { - let mut replace_with = t.syntax().clone_subtree(); - if !is_in_type_path(&self_tok) - && let Some(ty) = ast::Type::cast(replace_with.clone()) - && let Some(generic_arg_list) = ty.generic_arg_list() + let replace_with = if !is_in_type_path(&self_tok) + && let Some(generic_arg_list) = t.generic_arg_list() { - // Build replacement without generics using a sub-editor - let (mut sub_editor, sub_root) = SyntaxEditor::new(replace_with.clone()); - let generic_node = sub_root - .descendants() - .find(|n| n.text_range() == generic_arg_list.syntax().text_range()) - .unwrap(); - sub_editor.delete(generic_node); - replace_with = sub_editor.finish().new_root().clone(); - } + // Strip the outer generic arg list and reparse, since turbofish-less + // generics aren't valid in expression position. The outermost + // `GenericArgList` text is unique within `t`'s text (any inner generics + // are nested inside it), so `replacen(.., 1)` is safe. + let stripped = t.syntax().text().to_string().replacen( + &generic_arg_list.syntax().text().to_string(), + "", + 1, + ); + factory.ty(&stripped).syntax().clone() + } else { + t.syntax().clone() + }; editor.replace(self_tok, replace_with); } } @@ -459,6 +459,14 @@ fn inline( let mut let_stmts: Vec<ast::Stmt> = Vec::new(); + let this_token = + factory.name_ref("this").syntax().first_token().expect("NameRef should have had a token."); + let rewrite_self_to_this = |editor: &mut SyntaxEditor| { + for usage in &self_token_usages { + editor.replace(usage.clone(), this_token.clone()); + } + }; + // Inline parameter expressions or generate `let` statements depending on whether inlining works or not. for ((pat, param_ty, param), usages, expr) in izip!(params, param_use_nodes, arguments) { // izip confuses RA due to our lack of hygiene info currently losing us type info causing incorrect errors @@ -487,7 +495,7 @@ fn inline( let is_self = param.name(sema.db).is_some_and(|name| name == sym::self_); if is_self { - let mut this_pat = make::ident_pat(false, false, make::name("this")); + let mut this_pat = factory.ident_pat(false, false, factory.name("this")); let mut expr = expr.clone(); if let Pat::IdentPat(pat) = pat { match (pat.ref_token(), pat.mut_token()) { @@ -495,11 +503,11 @@ fn inline( (None, None) => {} // mut self => let mut this = obj (None, Some(_)) => { - this_pat = make::ident_pat(false, true, make::name("this")); + this_pat = factory.ident_pat(false, true, factory.name("this")); } // &self => let this = &obj (Some(_), None) => { - expr = make::expr_ref(expr, false); + expr = factory.expr_ref(expr, false); } // let foo = &mut X; &mut self => let this = &mut obj // let mut foo = X; &mut self => let this = &mut *obj (reborrow) @@ -508,16 +516,16 @@ fn inline( .type_of_expr(&expr) .map(|ty| ty.original.is_mutable_reference()); expr = if let Some(true) = should_reborrow { - make::expr_reborrow(expr) + factory.expr_reborrow(expr) } else { - make::expr_ref(expr, true) + factory.expr_ref(expr, true) }; } } }; - let_stmts.push(make::let_stmt(this_pat.into(), ty, Some(expr)).into()) + let_stmts.push(factory.let_stmt(this_pat.into(), ty, Some(expr)).into()) } else { - let_stmts.push(make::let_stmt(pat.clone(), ty, Some(expr.clone())).into()); + let_stmts.push(factory.let_stmt(pat.clone(), ty, Some(expr.clone())).into()); } }; @@ -528,13 +536,7 @@ fn inline( // if it does then emit a let statement and continue if func_let_vars.contains(&expr.syntax().text().to_string()) { if is_self_param { - for usage in &self_token_usages { - let this_token = make::name_ref("this") - .syntax() - .first_token() - .expect("NameRef should have had a token."); - editor.replace(usage.clone(), this_token); - } + rewrite_self_to_this(&mut editor); } insert_let_stmt(); continue; @@ -545,7 +547,6 @@ fn inline( if let Some(field) = path_expr_as_record_field(usage) { cov_mark::hit!(inline_call_inline_direct_field); let field_name = field.field_name().unwrap(); - let factory = SyntaxFactory::without_mappings(); let new_field = factory.record_expr_field( factory.name_ref(&field_name.text()), Some(replacement.clone()), @@ -563,7 +564,7 @@ fn inline( && usage.syntax().parent().and_then(ast::Expr::cast).is_some() => { cov_mark::hit!(inline_call_inline_closure); - let expr = make::expr_paren(expr.clone()).into(); + let expr = factory.expr_paren(expr.clone()).into(); inline_direct(&mut editor, usage, &expr); } // inline single use literals @@ -580,14 +581,7 @@ fn inline( _ => { if is_self_param { // Rename all `self` tokens to `this` so the let binding matches. - for usage in &self_token_usages { - let this_token = make::name_ref("this") - .syntax() - .clone_subtree() - .first_token() - .expect("NameRef should have had a token."); - editor.replace(usage.clone(), this_token); - } + rewrite_self_to_this(&mut editor); } insert_let_stmt(); } @@ -610,11 +604,10 @@ fn inline( body = new_body; } - let factory = SyntaxFactory::without_mappings(); let is_async_fn = function.is_async(sema.db); if is_async_fn { cov_mark::hit!(inline_call_async_fn); - body = make::async_move_block_expr(body.statements(), body.tail_expr()); + body = factory.async_move_block_expr(body.statements(), body.tail_expr()); // Arguments should be evaluated outside the async block, and then moved into it. if !let_stmts.is_empty() { @@ -635,7 +628,7 @@ fn inline( body = body.dedent(original_body_indent).indent(original_indentation); let no_stmts = body.statements().next().is_none(); - match body.tail_expr() { + let result = match body.tail_expr() { Some(expr) if matches!(expr, ast::Expr::ClosureExpr(_)) && no_stmts => { factory.expr_paren(expr).into() } @@ -651,7 +644,9 @@ fn inline( } _ => ast::Expr::BlockExpr(body), }, - } + }; + file_editor.add_mappings(factory.finish_with_mappings()); + result } fn is_in_type_path(self_tok: &syntax::SyntaxToken) -> bool { diff --git a/crates/syntax/src/ast/syntax_factory/constructors.rs b/crates/syntax/src/ast/syntax_factory/constructors.rs index b9106408b3..5a01580c56 100644 --- a/crates/syntax/src/ast/syntax_factory/constructors.rs +++ b/crates/syntax/src/ast/syntax_factory/constructors.rs @@ -985,6 +985,37 @@ impl SyntaxFactory { ast } + pub fn async_move_block_expr( + &self, + statements: impl IntoIterator<Item = ast::Stmt>, + tail_expr: Option<ast::Expr>, + ) -> ast::BlockExpr { + let (statements, mut input) = iterator_input(statements); + + let ast = make::async_move_block_expr(statements, tail_expr.clone()).clone_for_update(); + + if let Some(mut mapping) = self.mappings() { + let stmt_list = ast.stmt_list().unwrap(); + let mut builder = SyntaxMappingBuilder::new(stmt_list.syntax().clone()); + + if let Some(input) = tail_expr { + builder.map_node( + input.syntax().clone(), + stmt_list.tail_expr().unwrap().syntax().clone(), + ); + } else if let Some(ast_tail) = stmt_list.tail_expr() { + let last_stmt = input.pop().unwrap(); + builder.map_node(last_stmt, ast_tail.syntax().clone()); + } + + builder.map_children(input, stmt_list.statements().map(|it| it.syntax().clone())); + + builder.finish(&mut mapping); + } + + ast + } + pub fn expr_empty_block(&self) -> ast::BlockExpr { make::expr_empty_block().clone_for_update() } @@ -1135,6 +1166,27 @@ impl SyntaxFactory { ast.into() } + pub fn expr_reborrow(&self, expr: ast::Expr) -> ast::Expr { + let ast::Expr::RefExpr(ast) = make::expr_reborrow(expr.clone()).clone_for_update() else { + unreachable!() + }; + + if let Some(mut mapping) = self.mappings() { + // Layout: RefExpr(&mut, PrefixExpr(*, expr)). Map `expr` to the + // inner expr inside the synthesized PrefixExpr. + let prefix = match ast.expr() { + Some(ast::Expr::PrefixExpr(p)) => p, + _ => unreachable!("expr_reborrow always produces `&mut *expr`"), + }; + let inner = prefix.expr().unwrap(); + let mut builder = SyntaxMappingBuilder::new(prefix.syntax().clone()); + builder.map_node(expr.syntax().clone(), inner.syntax().clone()); + builder.finish(&mut mapping); + } + + ast.into() + } + pub fn expr_raw_ref(&self, expr: ast::Expr, exclusive: bool) -> ast::Expr { let ast::Expr::RefExpr(ast) = make::expr_raw_ref(expr.clone(), exclusive).clone_for_update() |