internal: migrate `inline_call` assist to `SyntaxEditor` by Albab-Hasan · Pull Request #22020 · rust-lang/rust-analyzer

Albab-Hasan · 2026-04-12T03:50:15Z

replace clone_for_update + ted mutations with SyntaxEditor. inline() uses SyntaxEditor::with_ast_node on the body clone for all parameter replacements, self-token renaming and Self type substitution. inline_into_callers uses make_editor + add_file_edits. import removal uses the Removable trait instead of split_refs_and_uses + make_syntax_mut.

ChayimFriedman2

Round 1 review. @Shourya742 would appreciate a review from you too.

View changes since this review

ChayimFriedman2 · 2026-04-13T18:45:49Z


            let mut remove_def = true;
            let mut inline_refs_for_file = |file_id: EditionedFileId, refs: Vec<FileReference>| {
+                use syntax::syntax_editor::Removable;


This should go at the top.

ChayimFriedman2 · 2026-04-13T18:47:08Z

-                // The collects are required as we are otherwise iterating while mutating 🙅‍♀️🙅‍♂️
-                let (name_refs, name_refs_use) = split_refs_and_uses(builder, refs, Some);
+                // Split refs into call-site name refs and use-tree paths
+                let (name_refs, use_trees): (Vec<ast::NameRef>, Vec<ast::UseTree>) = refs


Did you inline the call to split_refs_and_uses() because of the make_mut()? You can still use it but remove the make_mut(), and call it in the other place it's called.

ChayimFriedman2 · 2026-04-13T18:53:45Z

-                    // we replaced all usages in this file, so we can remove the imports
-                    name_refs_use.iter().for_each(remove_path_if_in_use_stmt);
-                } else {
+                if let Some(first) = call_infos.first() {


Why this if?

ChayimFriedman2 · 2026-04-13T18:56:10Z

+                            let replacement = inline(
+                                &ctx.sema, def_file, function, &func_body, &params, &call_info,
+                            );
+                            editor.replace(call_info.node.syntax(), replacement.syntax());


You cannot count on all calls to be in the same editor, and if you make different editors you'll risk them to overlap. I think you need to have an editor per file with the root node of the file, and it may help to have an abstraction for managing multiple editors per file.

ChayimFriedman2 · 2026-04-13T18:57:32Z

+                    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 {
+                            if use_tree.use_tree_list().is_some() || use_tree.star_token().is_some()


We can duplicate remove_path_if_in_use_stmt() using SyntaxEditor, near it (then remove the original eventually).

ChayimFriedman2 · 2026-04-13T18:58:48Z

            }
            if remove_def {
-                builder.delete(ast_func.syntax().text_range());
+                let mut editor = builder.make_editor(ast_func.syntax());


We can leave this as-is, no need for an editor here.

Shourya742 · 2026-04-14T07:21:37Z

    EditionedFileId, RootDatabase,
    base_db::Crate,
    defs::Definition,
-    imports::insert_use::remove_path_if_in_use_stmt,


View changes since the review

We can remove this method from ide-db. I don't think we use it anywhere else...

split_refs_and_uses lives in inline_call.rs (as pub(super)) not in ide-db. i think its still needed there by inline_type_alias.rs which imports it using super::inline_call::split_refs_and_uses. ive refactored it in this pr to drop the builder: &mut SourceChangeBuilder parameter and return Vec<ast::UseTree> instead of Vec<ast::Path> removing the make_mut call as suggested by chayim.

Shourya742 · 2026-04-14T07:28:22Z

+                    );
+                    editor.replace(field.syntax(), new_field.syntax());
+                } else {
+                    editor.replace(usage.syntax(), replacement.syntax().clone_subtree());


View changes since the review

Suggested change

editor.replace(usage.syntax(), replacement.syntax().clone_subtree());

editor.replace(usage.syntax(), replacement.syntax());

Shourya742 · 2026-04-14T07:29:04Z

+                    let new_field = make::record_expr_field(
+                        make::name_ref(&field_name.text()),
+                        Some(replacement.clone()),


View changes since the review

lets construct this via call from syntax_factory

Shourya742 · 2026-04-14T07:29:43Z

    match body.tail_expr() {
        Some(expr) if matches!(expr, ast::Expr::ClosureExpr(_)) && no_stmts => {
-            make::expr_paren(expr).clone_for_update().into()
+            make::expr_paren(expr).into()


View changes since the review

lets call this via SyntaxFactory

Shourya742 · 2026-04-14T07:29:57Z

        {
            Some(lhs) if lhs.syntax() == node.syntax() => {
-                make::expr_paren(ast::Expr::BlockExpr(body)).clone_for_update().into()
+                make::expr_paren(ast::Expr::BlockExpr(body)).into()


View changes since the review

lets call this via SyntaxFactory

Shourya742 · 2026-04-14T07:30:18Z

-            body.indent(IndentLevel(1));
-            body = make::block_expr(let_stmts, Some(body.into())).clone_for_update();
+            body = body.indent(IndentLevel(1));
+            body = make::block_expr(let_stmts, Some(body.into()));


View changes since the review

lets call via SyntaxFactory

Shourya742 · 2026-04-14T07:30:29Z

+    } else if !let_stmts.is_empty() {
+        // Prepend let statements to the body's existing statements
+        let stmts: Vec<ast::Stmt> = let_stmts.into_iter().chain(body.statements()).collect();
+        body = make::block_expr(stmts, body.tail_expr());


View changes since the review

lets call via SyntaxFactory

Shourya742 · 2026-04-14T07:30:48Z

    if is_async_fn {
        cov_mark::hit!(inline_call_async_fn);
-        body = make::async_move_block_expr(body.statements(), body.tail_expr()).clone_for_update();
+        body = make::async_move_block_expr(body.statements(), body.tail_expr());


View changes since the review

lets call via SyntaxFactory

SyntaxFactory doesnt currently have an async_move_block_expr constructor so ive kept make::async_move_block_expr here. i think it would be good if this was done separately. happy to do it as a follow up pr if wanted.

I agree with @Shourya742, you should change that in this PR.

Shourya742 · 2026-04-14T07:31:38Z

+                for usage in &self_token_usages {
+                    let this_token = make::name_ref("this")
+                        .syntax()
+                        .clone_subtree()


View changes since the review

We don't need clone_subtree here, considering we are creating a new node here, with no parent

Shourya742 · 2026-04-14T07:33:07Z

+            .filter(|tok| tok.kind() == SyntaxKind::SELF_TYPE_KW)
+            .collect();
+        for self_tok in self_tokens {
+            let mut replace_with = t.clone_subtree().syntax().clone_subtree();


View changes since the review

Editor gonna call clone_subtree internally and detach the subtree if the node is part of pre-existing tree

the editor handling clone_subtree internally only applies when passing to editor.replace() directly. here i need to derive generic_arg_list from replace_with before constructing the sub editor. if replace_with still has a parent SyntaxEditor::new will call clone_subtree internally making sub_root start at offset 0. but generic_arg_list was derived from the original so its text range still points into the file. the find returns None and the unwrap panics. confirmed by removing the clone_subtree which caused inline_trait_method_call_with_lifetimes to fail. reduced the double t.clone_subtree().syntax().clone_subtree() to a single t.syntax().clone_subtree().

ChayimFriedman2

Few more comments.

View changes since this review

ChayimFriedman2 · 2026-04-22T02:11:50Z

-                builder.delete(ast_func.syntax().text_range());
+                let mut editor = builder.make_editor(ast_func.syntax());
+                editor.delete(ast_func.syntax());
+                builder.add_file_edits(vfs_def_file, editor);


This is again problematic if the function is in the same file as one of its callers. I think you need to have a HashMap<FileId, SyntaxEditor> and use it.

ChayimFriedman2 · 2026-04-22T02:14:31Z

+    // 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);


This function should take the editor as a parameter, because in inline_into_callers() we already have it.

ChayimFriedman2 · 2026-04-22T02:17:28Z

            {
-                ted::remove(generic_arg_list.syntax());
+                // Build replacement without generics using a sub-editor
+                let (mut sub_editor, sub_root) = SyntaxEditor::new(replace_with.clone());


We generally shouldn't use sub-editors. Why can't you use the original here?

body_to_clone is always a clone and may come from a macro in a different file. SyntaxEditor::with_ast_node just does clone_subtree internally. not a sub-editor in the merge sense. file_editor can't operate on it directly. the transformed body comes out of editor.finish() and gets handed to file_editor.replace()

ChayimFriedman2 · 2026-04-22T02:18:21Z

-                let_stmts.push(
-                    make::let_stmt(pat.clone(), ty, Some(expr.clone())).clone_for_update().into(),
-                );
+                let_stmts.push(make::let_stmt(pat.clone(), ty, Some(expr.clone())).into());


Replace these make calls with the factory here and above.

ChayimFriedman2 · 2026-04-22T02:19:02Z

        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")


Here too use the factory.

Can also extract this out of the loop.

ChayimFriedman2 · 2026-04-22T02:20:34Z

+                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();


Why can't you use the editor's factory here?

ChayimFriedman2 · 2026-04-22T02:20:54Z

            _ => {
+                if is_self_param {
+                    // Rename all `self` tokens to `this` so the let binding matches.
+                    for usage in &self_token_usages {


This has potential for code sharing with above.

ChayimFriedman2 · 2026-04-22T02:21:22Z

    if is_async_fn {
        cov_mark::hit!(inline_call_async_fn);
-        body = make::async_move_block_expr(body.statements(), body.tail_expr()).clone_for_update();
+        body = make::async_move_block_expr(body.statements(), body.tail_expr());


I agree with @Shourya742, you should change that in this PR.

ChayimFriedman2 · 2026-04-22T02:22:51Z

+    let mut body = ast::BlockExpr::cast(edit.new_root().clone())
+        .expect("editor root should still be a BlockExpr");
+
+    // Apply generic substitution (needs immutable tree)


Why does it needs an immutable tree? I'd prefer to use the same editor.

PathTransform::apply returns a transformed clone so the editor has to finish() to feed it. moving PathTransform before the editor breaks range - body_offset lookup once substitution shifts text. editor aware apply_to would work but means rewriting Ctx::apply. follow up or in this pr?

Replace clone_for_update + ted mutations with SyntaxEditor throughout. inline() uses SyntaxEditor::with_ast_node on the body clone for all parameter replacements, self-token renaming, and Self type substitution. inline_into_callers uses make_editor + add_file_edits; import removal uses the Removable trait instead of split_refs_and_uses + make_syntax_mut.

- 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

rustbot · 2026-04-26T18:01:48Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Albab-Hasan · 2026-04-27T03:21:58Z

@ChayimFriedman2 @Shourya742 done.

Shourya742 · 2026-04-28T05:54:59Z

    }
 }

+pub fn remove_use_tree_if_simple(use_tree: &ast::UseTree, editor: &mut SyntaxEditor) {


Suggested change

pub fn remove_use_tree_if_simple(use_tree: &ast::UseTree, editor: &mut SyntaxEditor) {

pub fn remove_use_tree_if_simple(use_tree: &ast::UseTree, editor: &SyntaxEditor) {

View changes since the review

Shourya742 · 2026-04-28T05:55:33Z

            let mut inline_refs_for_file = |file_id, refs: Vec<FileReference>| {
                let source = ctx.sema.parse(file_id);
-                let editor = builder.make_editor(source.syntax());
+                let mut editor = builder.make_editor(source.syntax());


Suggested change

let mut editor = builder.make_editor(source.syntax());

let editor = builder.make_editor(source.syntax());

View changes since the review

Shourya742 · 2026-04-28T05:55:45Z

                    .iter()
-                    .flat_map(ast_to_remove_for_path_in_use_stmt)
-                    .for_each(|x| editor.delete(x.syntax()));
+                    .for_each(|use_tree| remove_use_tree_if_simple(use_tree, &mut editor));


Suggested change

.for_each(|use_tree| remove_use_tree_if_simple(use_tree, &mut editor));

.for_each(|use_tree| remove_use_tree_if_simple(use_tree, &editor));

View changes since the review

Shourya742 · 2026-04-28T05:57:18Z

    CallInfo { node, arguments, generic_arg_list, krate }: &CallInfo,
+    file_editor: &SyntaxEditor,
 ) -> ast::Expr {
+    let factory = file_editor.make();


Lets stick to make naming

Suggested change

let factory = file_editor.make();

let make = file_editor.make();

View changes since the review

Shourya742 · 2026-04-28T05:58:17Z

+                    "",
+                    1,
+                );
+                factory.ty(&stripped).syntax().clone()


This factory doesn't associate to this local editor

View changes since the review

Shourya742 · 2026-04-28T05:59:18Z

+    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.");


Same for this, this factory is associated to file_editor above and not the local editor

View changes since the review

Shourya742 · 2026-04-28T05:59:54Z

-                field.replace_expr(replacement.clone_for_update());
+                let field_name = field.field_name().unwrap();
+                let new_field = factory.record_expr_field(
+                    factory.name_ref(&field_name.text()),


Same for this

View changes since the review

Shourya742 · 2026-04-28T06:02:04Z

                        FileReferenceNode::NameRef(_) => body
                            .syntax()
-                            .covering_element(range)
+                            .covering_element(range - body_offset)


TBH, I don’t like range-based edits, they’re quite rigid. If possible, it would be better to use the editor APIs. Not a blocker for this PR.

View changes since the review

Shourya742 · 2026-04-28T06:16:39Z

            cov_mark::hit!(inline_call_async_fn_with_let_stmts);
-            body.indent(IndentLevel(1));
-            body = make::block_expr(let_stmts, Some(body.into())).clone_for_update();
+            body = body.indent(IndentLevel(1));


Can you add this test in inline_call, and check, this implementation kinda panics on nested calls

#[test] fn inline_callers_nested_calls() { check_assist( inline_into_callers, r#" fn id$0(x: u32) -> u32 { x } fn main() { let x = id(id(1)); } "#, r#" fn main() { let x = { let x = id(1); x }; } "#, ); }

View changes since the review

done. added the test and fixed the panic by filtering out nested calls before processing — calls whose text range is contained within another call being inlined are skipped and counted as implicitly handled when checking whether the function definition should be deleted.

Shourya742 · 2026-04-28T06:17:31Z

@@ -347,6 +347,17 @@ pub fn remove_path_if_in_use_stmt(path: &ast::Path) {
    }
 }


This can be removed now

View changes since the review

- use `editor.make()` for nodes passed to body editor (Self type replacement, `this` token, record_expr_field, closure paren wrap) so they are tracked by the correct editor's factory; rename `factory` → `make` for file-level nodes (let_stmts, post-finish block construction) per reviewer naming - drop `remove_path_if_in_use_stmt` and `ast_to_remove_for_path_in_use_stmt` from `ide-db` (unused after SyntaxEditor migration) - change `remove_use_tree_if_simple` signature to `&SyntaxEditor` (interior mutability via RefCell makes `&mut` unnecessary) - fix `inline_type_alias` to use `let editor` (not `mut`) and pass `&editor` to `remove_use_tree_if_simple` - filter calls nested inside other calls in `inline_into_callers` to prevent overlapping SyntaxEditor edits; nested calls are implicitly replaced when the outer call is inlined, so they count toward the "all handled" check - add `inline_callers_nested_calls` test covering the above

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2026

ChayimFriedman2 requested changes Apr 13, 2026

View reviewed changes

Shourya742 reviewed Apr 14, 2026

View reviewed changes

ChayimFriedman2 requested changes Apr 22, 2026

View reviewed changes

Albab-Hasan added 5 commits April 27, 2026 00:01

internal: fix collapsible_if clippy lints in inline_call

87f3e22

internal: address review feedback for inline_call SyntaxEditor migration

6425a07

internal: fix rustfmt in inline_type_alias

3bf08bc

Albab-Hasan force-pushed the syntax-editor-inline-call branch from afc57c7 to 34d7c45 Compare April 26, 2026 18:01

internal: adapt inline_call to new SyntaxFactory-inside-SyntaxEditor API

be8646b

Shourya742 reviewed Apr 28, 2026

View reviewed changes

	editor.replace(usage.syntax(), replacement.syntax().clone_subtree());
	editor.replace(usage.syntax(), replacement.syntax());

	pub fn remove_use_tree_if_simple(use_tree: &ast::UseTree, editor: &mut SyntaxEditor) {
	pub fn remove_use_tree_if_simple(use_tree: &ast::UseTree, editor: &SyntaxEditor) {

	let mut editor = builder.make_editor(source.syntax());
	let editor = builder.make_editor(source.syntax());

	.for_each(\|use_tree\| remove_use_tree_if_simple(use_tree, &mut editor));
	.for_each(\|use_tree\| remove_use_tree_if_simple(use_tree, &editor));

	let factory = file_editor.make();
	let make = file_editor.make();

		@@ -347,6 +347,17 @@ pub fn remove_path_if_in_use_stmt(path: &ast::Path) {
		}
		}

Conversation

Albab-Hasan commented Apr 12, 2026

Uh oh!

ChayimFriedman2 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shourya742 Apr 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shourya742 Apr 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shourya742 Apr 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shourya742 Apr 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shourya742 Apr 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shourya742 Apr 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shourya742 Apr 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shourya742 Apr 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shourya742 Apr 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shourya742 Apr 14, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChayimFriedman2 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ChayimFriedman2 left a comment •

edited by rustbot

Loading

Shourya742 Apr 14, 2026 •

edited by rustbot

Loading

Shourya742 Apr 14, 2026 •

edited by rustbot

Loading

Shourya742 Apr 14, 2026 •

edited by rustbot

Loading

Shourya742 Apr 14, 2026 •

edited by rustbot

Loading

Shourya742 Apr 14, 2026 •

edited by rustbot

Loading

Shourya742 Apr 14, 2026 •

edited by rustbot

Loading

Shourya742 Apr 14, 2026 •

edited by rustbot

Loading

Shourya742 Apr 14, 2026 •

edited by rustbot

Loading

Shourya742 Apr 14, 2026 •

edited by rustbot

Loading

Shourya742 Apr 14, 2026 •

edited by rustbot

Loading

ChayimFriedman2 left a comment •

edited by rustbot

Loading

Shourya742 Apr 28, 2026 •

edited by rustbot

Loading

Shourya742 Apr 28, 2026 •

edited by rustbot

Loading

Shourya742 Apr 28, 2026 •

edited by rustbot

Loading

Shourya742 Apr 28, 2026 •

edited by rustbot

Loading

Shourya742 Apr 28, 2026 •

edited by rustbot

Loading

Shourya742 Apr 28, 2026 •

edited by rustbot

Loading