Skip to content

internal: migrate extract_module to SyntaxEditor#21868

Open
itang06 wants to merge 5 commits intorust-lang:masterfrom
itang06:feature/US-18285-extract-module-syntax-editor
Open

internal: migrate extract_module to SyntaxEditor#21868
itang06 wants to merge 5 commits intorust-lang:masterfrom
itang06:feature/US-18285-extract-module-syntax-editor

Conversation

@itang06
Copy link

@itang06 itang06 commented Mar 25, 2026

Part of #18285

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

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

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

continue;
}

let mut editor = SyntaxEditor::new(body_item.syntax().clone());
Copy link
Member

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

Here, too

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

Choose a reason for hiding this comment

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

Kindly replace use of make with SyntaxFactory

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();
Copy link
Member

Choose a reason for hiding this comment

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

Remove unwraps


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);
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?


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();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be removed?

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

Choose a reason for hiding this comment

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

Why this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants