internal: migrate extract_module to SyntaxEditor by itang06 · Pull Request #21868 · rust-lang/rust-analyzer

itang06 · 2026-03-25T08:45:02Z

Migrates all ted:: calls in crates/ide-assists/src/handlers/extract_module.rs to the new SyntaxEditor/SyntaxFactory abstraction

Changes:

generate_module_def: replaced ted:: with SyntaxEditor/SyntaxFactory
expand_and_group_usages_file_wise: replaced ted::replace with SyntaxEditor
change_visibility: replaced ted::insert (via add_change_vis helper) with SyntaxEditor::insert_all; removes the add_change_vis function entirely

AI assistance was used for this contribution

ShoyuVanilla · 2026-03-25T14:22:45Z

Please migrate remaining make::* usages to SyntaxFactory APIs

ShoyuVanilla · 2026-03-25T14:25:14Z

+                continue;
+            }
+
+            let mut editor = SyntaxEditor::new(body_item.syntax().clone());


Is this new SyntaxEditor instance and .finish() call on it necessary? Couldn't it be done by passing the mutable reference of existing one from the assist entrance function as a parameter of this function instead?

the items here are detached clone_subtree() nodes, so the editor from the entry point can't reach them. I can use SyntaxFactory::without_mappings() locally to get rid of the .clone_for_update() calls instead, does that work?

Hmm, yeah, I guess it could be still uplifted to the root SyntaxEditor but that would require more involved refactoring, so I think this is fine as is for now

ShoyuVanilla · 2026-03-25T14:25:38Z

+                            })
+                            .collect();
+                        if !replacements.is_empty() {
+                            let mut editor = SyntaxEditor::new(entry.syntax().clone());


Shourya742 · 2026-03-25T12:33:27Z

@@ -18,8 +18,10 @@ use syntax::{
        self, HasVisibility,
        edit::{AstNodeEdit, IndentLevel},
        make,


Kindly replace use of make with SyntaxFactory

Shourya742 · 2026-03-25T12:34:12Z

-        let assoc_item_list = make::assoc_item_list(Some(assoc_items)).clone_for_update();
-        let impl_ = impl_.reset_indent();
-        ted::replace(impl_.get_or_create_assoc_item_list().syntax(), assoc_item_list.syntax());
+        let impl_detached = ast::Impl::cast(impl_.syntax().clone_subtree()).unwrap();


Remove unwraps

Shourya742 · 2026-03-25T12:36:46Z

+
        let (mut replacements, record_field_parents, impls) =
-            get_replacements_for_visibility_change(&mut self.body_items, false);
+            get_replacements_for_visibility_change(&mut self.body_items, true);


Why this change?

this was a workaround because of the local SyntaxEditor approach, was an incorrect change

Shourya742 · 2026-03-25T12:37:35Z

+
+            let mut editor = SyntaxEditor::new(body_item.syntax().clone());
+            for target in insert_targets {
+                let pub_crate_vis = make::visibility_pub_crate().clone_for_update();


Shouldn't this be removed?

will fix, good catch

Shourya742 · 2026-03-25T14:39:09Z

+                                Some((
+                                    seg.syntax().parent()?,
+                                    make::path_from_text(&format!("{mod_name}::{seg}"))
+                                        .clone_for_update(),


i missed that this should use SyntaxFactory, will replace it

ChayimFriedman2 · 2026-04-06T03:16:18Z

@itang06 What's the status of this?

itang06 · 2026-04-09T07:54:51Z

@itang06 What's the status of this?

i addressed the unwrap issue. waiting for ShoyuVanilla's response to my question about using SyntaxFactory::without_mappings() locally in the helpers instead of passing the file-level editor since they operate on detached nodes

Shourya742 · 2026-04-11T11:23:25Z

                            .entry(use_.syntax().text_range().start())
-                            .or_insert_with(|| use_.clone_subtree().clone_for_update());
-                        for seg in use_
+                            .or_insert_with(|| use_.clone_subtree());


View changes since the review

remove clone_subtree

Shourya742 · 2026-04-11T11:24:00Z

-        let assoc_item_list = make::assoc_item_list(Some(assoc_items)).clone_for_update();
-        let impl_ = impl_.reset_indent();
-        ted::replace(impl_.get_or_create_assoc_item_list().syntax(), assoc_item_list.syntax());
+        let impl_detached = ast::Impl::cast(impl_.syntax().clone_subtree()).unwrap();


Shourya742 · 2026-04-11T11:25:02Z

+                    ],
+                );
+            }
+            *body_item = ast::Item::cast(editor.finish().new_root().clone_subtree()).unwrap();


View changes since the review

Suggested change

*body_item = ast::Item::cast(editor.finish().new_root().clone_subtree()).unwrap();

*body_item = ast::Item::cast(editor.finish().new_root()).unwrap();

Shourya742 · 2026-04-11T11:26:48Z

+        for item in &mut self.body_items {
+            *item = ast::Item::cast(item.reset_indent().syntax().clone_subtree()).unwrap();
+        }


View changes since the review

No need to call clone_subtree here anymore. SyntaxEditor manages this internally

i tried removing the clone_subtree() calls but found that 3 of the 4 calls require using the detached root returned by SyntaxEditor::new() instead of just dropping clone_subtree(). i think the 4th at line 436 has to stay because replacements is collected from body_items immediately after. is there something i'm missing or is there a different approach you had in mind?

ChayimFriedman2 · 2026-04-24T14:25:27Z

@itang06 What's the status of this?

itang06 · 2026-04-26T08:22:32Z

@itang06 What's the status of this?

waiting on feedback from Shourya742 regarding the approach for removing clone_subtree() calls (see my comment above). all tests pass but i just need confirmation on the direction before committing

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

Hey @itang06, can you rebase the changes onto the latest master? A lot of the semantics around syntaxEditor have changed. The clone_subtree issue can be resolved using a local editor.

Shourya742 · 2026-04-28T06:29:15Z

Can you also please squash the commits, seems like most are changing the same file.

Shourya742 · 2026-05-09T06:15:11Z

Hey @itang06 are you still working on this?

itang06 · 2026-05-10T06:44:16Z

Yes, still working on it. i'll have an update soon.

Shourya742 · 2026-05-12T10:27:00Z

Make sure to remove/update ted variant of get_or_create_assoc_item_list from edit-in-place .

itang06 · 2026-05-13T08:53:53Z

@Shourya742 i found that after rebasing. removing the clone_subtree() calls causes is_ancestor_or_self_of_element to fail, since when SyntaxEditor::new() clones internally, the node references collected before the editor was created end up pointing to the wrong tree. would the fix be to collect targets from the editor's detached root instead?

Shourya742 · 2026-05-13T09:09:23Z

@Shourya742 i found that after rebasing. removing the clone_subtree() calls causes is_ancestor_or_self_of_element to fail, since when SyntaxEditor::new() clones internally, the node references collected before the editor was created end up pointing to the wrong tree. would the fix be to collect targets from the editor's detached root instead?

Yes

Shourya742 · 2026-05-18T09:34:44Z

-                    _ => true,
-                }
-            });
+        let make = SyntaxFactory::without_mappings();


You don't need to initialize the without_mappining here, you can get it from the editor and have mapped changes stored in the factory.

View changes since the review

done, using editor.make() in a scoped block so it's dropped before editor.finish()

Shourya742 · 2026-05-18T09:38:44Z

+                            let (_, root) = SyntaxEditor::with_ast_node(&use_);
+                            root
+                        });
+                        let make = SyntaxFactory::without_mappings();


We don't need this

View changes since the review

we can use the make via the editor

done, simplified to use_.clone()

Shourya742 · 2026-05-18T12:02:39Z

@@ -741,7 +788,6 @@

 fn get_replacements_for_visibility_change(
    items: &mut [ast::Item],


Since we are not longer mutating it.

Suggested change

items: &mut [ast::Item],

items: &[ast::Item],

View changes since the review

Shourya742 · 2026-05-18T12:56:06Z

+
        let (mut replacements, record_field_parents, impls) =
-            get_replacements_for_visibility_change(&mut self.body_items, false);
+            get_replacements_for_visibility_change(&mut self.body_items);


We dont need mut here now.

View changes since the review

Shourya742 · 2026-05-18T12:56:17Z


        let (mut impl_item_replacements, _, _) =
-            get_replacements_for_visibility_change(&mut impl_items, true);
+            get_replacements_for_visibility_change(&mut impl_items);


We don't need mut here now.

View changes since the review

Shourya742 · 2026-05-18T13:24:06Z

@@ -186,6 +187,7 @@
    parent_impl: &Option<ast::Impl>,
    Module { name, body_items, use_items }: &Module,
 ) -> ast::Module {
+    let make = SyntaxFactory::without_mappings();


We can pass factory to generate_module_def method from extract_module

View changes since the review

done. also did the same for make_use_stmt_of_node_with_super and change_visibility which had the same issue

Shourya742 · 2026-05-18T13:25:54Z

        imports_to_remove
    }

    fn process_def_in_sel(


We can pass factory to process_def_in_sel

View changes since the review

done, threaded it through resolve_imports

Shourya742 · 2026-05-22T09:24:32Z

Pinging @ChayimFriedman2, for a final look.

Shourya742 · 2026-06-03T03:37:54Z

@itang06 Can you rebase the PR to current master?

Part of rust-lang#18285

rustbot · 2026-06-03T06:14:18Z

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.

rustbot · 2026-06-03T06:14:19Z

⚠️ Warning ⚠️

There are uncanonicalized issue links (such as #123) in the commit messages of the following commits.
Please add the organization and repository before the issue number (like so rust-lang/rust#123) to avoid issues with subtree.
- e583b10

itang06 · 2026-06-03T06:24:16Z

@itang06 Can you rebase the PR to current master?

Rebased to current master

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

ShoyuVanilla reviewed Mar 25, 2026

View reviewed changes

Shourya742 reviewed Mar 25, 2026

View reviewed changes

itang06 force-pushed the feature/US-18285-extract-module-syntax-editor branch from 11ff3da to fbf2dfa Compare April 11, 2026 08:14

This comment has been minimized.

Sign in to view

itang06 force-pushed the feature/US-18285-extract-module-syntax-editor branch from fbf2dfa to a9d2246 Compare April 11, 2026 08:31

This comment has been minimized.

Sign in to view

Shourya742 reviewed Apr 11, 2026

View reviewed changes

itang06 force-pushed the feature/US-18285-extract-module-syntax-editor branch from 74c1a1e to de6c326 Compare May 13, 2026 08:44

This comment has been minimized.

Sign in to view

itang06 force-pushed the feature/US-18285-extract-module-syntax-editor branch from de6c326 to 7b0b9c7 Compare May 13, 2026 08:56

itang06 force-pushed the feature/US-18285-extract-module-syntax-editor branch 2 times, most recently from f828748 to 5b44119 Compare May 17, 2026 09:34

Shourya742 reviewed May 18, 2026

View reviewed changes

itang06 force-pushed the feature/US-18285-extract-module-syntax-editor branch from 5b44119 to 563cdc6 Compare May 22, 2026 08:40

Shourya742 mentioned this pull request Jun 3, 2026

Bye bye ted #22516

Draft

itang06 added 2 commits June 2, 2026 23:09

internal: add item_list and mod_ constructors to SyntaxFactory

e583b10

Part of rust-lang#18285

internal: migrate extract_module assist to SyntaxEditor

774aeb2

itang06 force-pushed the feature/US-18285-extract-module-syntax-editor branch from 563cdc6 to 774aeb2 Compare June 3, 2026 06:14

Veykril approved these changes Jun 12, 2026

View reviewed changes

Veykril added this pull request to the merge queue Jun 12, 2026

Merged via the queue into rust-lang:master with commit 1edcb12 Jun 12, 2026
18 checks passed

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

This was referenced Jun 12, 2026

Migrate merge imports to editor #22351

Open

Add a ton more database lifetimes #22548

Open

	*body_item = ast::Item::cast(editor.finish().new_root().clone_subtree()).unwrap();
	*body_item = ast::Item::cast(editor.finish().new_root()).unwrap();

		@@ -741,7 +788,6 @@

		fn get_replacements_for_visibility_change(
		items: &mut [ast::Item],

Uh oh!

Conversation

itang06 commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itang06 Mar 29, 2026 • edited 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!

itang06 Mar 29, 2026 • edited 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!

ChayimFriedman2 commented Apr 6, 2026

Uh oh!

itang06 commented Apr 9, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Shourya742 Apr 11, 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 11, 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 11, 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 commented Apr 24, 2026

Uh oh!

itang06 commented Apr 26, 2026

Uh oh!

Shourya742 commented Apr 28, 2026

Uh oh!

Shourya742 commented Apr 28, 2026

Uh oh!

Shourya742 commented May 9, 2026

Uh oh!

itang06 commented May 10, 2026

Uh oh!

Shourya742 commented May 12, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

itang06 commented May 13, 2026

Uh oh!

Shourya742 commented May 13, 2026

Uh oh!

itang06 commented Mar 25, 2026 •

edited

Loading

itang06 Mar 29, 2026 •

edited

Loading

itang06 Mar 29, 2026 •

edited

Loading

Shourya742 Apr 11, 2026 •

edited by rustbot

Loading

Shourya742 Apr 11, 2026 •

edited by rustbot

Loading

Shourya742 Apr 11, 2026 •

edited by rustbot

Loading

Shourya742 May 18, 2026 •

edited by rustbot

Loading

itang06 May 22, 2026 •

edited

Loading

Shourya742 May 18, 2026 •

edited by rustbot

Loading

Shourya742 May 18, 2026 •

edited by rustbot

Loading

Shourya742 May 18, 2026 •

edited by rustbot

Loading

Shourya742 May 18, 2026 •

edited by rustbot

Loading

Shourya742 May 18, 2026 •

edited by rustbot

Loading

Shourya742 May 18, 2026 •

edited by rustbot

Loading