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

crates/ide-assists/src/handlers/extract_module.rs

Please migrate remaining make::* usages to SyntaxFactory APIs

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

crates/ide-assists/src/handlers/extract_module.rs

+                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

crates/ide-assists/src/handlers/extract_module.rs

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


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

crates/ide-assists/src/handlers/extract_module.rs

@@ -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

crates/ide-assists/src/handlers/extract_module.rs

-        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

crates/ide-assists/src/handlers/extract_module.rs

+
        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

crates/ide-assists/src/handlers/extract_module.rs

+
+            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

crates/ide-assists/src/handlers/extract_module.rs

+                                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

…yntaxEditor

…ut_mappings()

rustbot · 2026-04-11T08:31:42Z

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.

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

crates/ide-assists/src/handlers/extract_module.rs

                            .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

crates/ide-assists/src/handlers/extract_module.rs

-        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

crates/ide-assists/src/handlers/extract_module.rs

+                    ],
+                );
+            }
+            *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

crates/ide-assists/src/handlers/extract_module.rs

+        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?

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

crates/ide-assists/src/handlers/extract_module.rs

Copy link
Copy Markdown

Member

ShoyuVanilla Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please migrate remaining make::* usages to SyntaxFactory APIs

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 added 12 commits April 11, 2026 01:29

internal: migrate extract_module assist to SyntaxEditor (partial)

0e62f1c

internal: migrate expand_and_group_usages_file_wise ted::replace to S…

9f45c31

…yntaxEditor

internal: migrate extract_module change_visibility to SyntaxEditor

787eb75

internal: clean up imports and comments in extract_module

4d3fad6

internal: rename factory to make in generate_module_def

9f3ec4b

remove unwrap on assoc_item_list in generate_module_def

2382c1c

migrate change_visibility to use SyntaxFactory::without_mappings()

ec15a53

migrate expand_and_group_usages_file_wise to use SyntaxFactory::witho…

4250c70

…ut_mappings()

remove dead is_clone_for_updated parameter

5a55154

add item_list and mod_ constructors to SyntaxFactory

b2486f5

migrate generate_module_def to use SyntaxFactory

0e0c4e5

Fix SyntaxEditor::new() tuple destructuring for upstream API

a9d2246

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

rename make_editor to make in generate_module_def

74c1a1e

Shourya742 reviewed Apr 11, 2026

View reviewed changes

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

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.

rustbot commented Apr 11, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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