From 9c8a57ed08a4341f881302c81633bf33ac99a6ed Mon Sep 17 00:00:00 2001 From: roife Date: Thu, 14 Mar 2024 02:00:39 +0800 Subject: [PATCH 1/9] fix: simplify extract_module --- .../src/handlers/extract_module.rs | 490 +++++++----------- 1 file changed, 182 insertions(+), 308 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index af834c8a53..95bb4e174b 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -16,7 +16,7 @@ use syntax::{ ast::{ self, edit::{AstNodeEdit, IndentLevel}, - make, HasName, HasVisibility, + make, HasVisibility, }, match_ast, ted, AstNode, SourceFile, SyntaxKind::{self, WHITESPACE}, @@ -134,16 +134,13 @@ pub(crate) fn extract_module(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti let mut body = body_items.join("\n\n"); if let Some(impl_) = &impl_parent { - let mut impl_body_def = String::new(); - if let Some(self_ty) = impl_.self_ty() { - { - let impl_indent = old_item_indent + 1; - format_to!( - impl_body_def, - "{impl_indent}impl {self_ty} {{\n{body}\n{impl_indent}}}", - ); - } + let impl_indent = old_item_indent + 1; + let mut impl_body_def = String::new(); + format_to!( + impl_body_def, + "{impl_indent}impl {self_ty} {{\n{body}\n{impl_indent}}}", + ); body = impl_body_def; // Add the import for enum/struct corresponding to given impl block @@ -156,7 +153,6 @@ pub(crate) fn extract_module(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti } let mut module_def = String::new(); - let module_name = module.name; format_to!(module_def, "mod {module_name} {{\n{body}\n{old_item_indent}}}"); @@ -177,10 +173,6 @@ pub(crate) fn extract_module(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti builder.replace(usage_to_be_processed.0, usage_to_be_processed.1) } - for import_path_text_range in import_paths_to_be_removed { - builder.delete(import_path_text_range); - } - if let Some(impl_) = impl_parent { // Remove complete impl block if it has only one child (as such it will be empty // after deleting that child) @@ -199,6 +191,14 @@ pub(crate) fn extract_module(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti builder.insert(impl_.syntax().text_range().end(), format!("\n\n{module_def}")); } else { + for import_path_text_range in import_paths_to_be_removed { + if module.text_range.intersect(import_path_text_range).is_some() { + module.text_range = module.text_range.cover(import_path_text_range); + } else { + builder.delete(import_path_text_range); + } + } + builder.replace(module.text_range, module_def) } }, @@ -394,78 +394,43 @@ impl Module { fn resolve_imports( &mut self, - curr_parent_module: Option, + module: Option, ctx: &AssistContext<'_>, ) -> Vec { - let mut import_paths_to_be_removed: Vec = vec![]; - let mut node_set: FxHashSet = FxHashSet::default(); + let mut imports_to_remove = vec![]; + let mut node_set = FxHashSet::default(); for item in self.body_items.clone() { - for x in item.syntax().descendants() { - if let Some(name) = ast::Name::cast(x.clone()) { - if let Some(name_classify) = NameClass::classify(&ctx.sema, &name) { - //Necessary to avoid two same names going through - if !node_set.contains(&name.syntax().to_string()) { - node_set.insert(name.syntax().to_string()); - let def_opt: Option = match name_classify { - NameClass::Definition(def) => Some(def), - _ => None, - }; - - if let Some(def) = def_opt { - if let Some(import_path) = self - .process_names_and_namerefs_for_import_resolve( - def, - name.syntax(), - &curr_parent_module, - ctx, - ) - { - check_intersection_and_push( - &mut import_paths_to_be_removed, - import_path, - ); - } - } + item.syntax() + .descendants() + .filter_map(|x| { + if let Some(name) = ast::Name::cast(x.clone()) { + NameClass::classify(&ctx.sema, &name).and_then(|nc| match nc { + NameClass::Definition(def) => Some((name.syntax().clone(), def)), + _ => None, + }) + } else if let Some(name_ref) = ast::NameRef::cast(x) { + NameRefClass::classify(&ctx.sema, &name_ref).and_then(|nc| match nc { + NameRefClass::Definition(def) => Some((name_ref.syntax().clone(), def)), + _ => None, + }) + } else { + None + } + }) + .for_each(|(node, def)| { + if node_set.insert(node.to_string()) { + if let Some(import) = self.process_def_in_sel(def, &node, &module, ctx) { + check_intersection_and_push(&mut imports_to_remove, import); } } - } - - if let Some(name_ref) = ast::NameRef::cast(x) { - if let Some(name_classify) = NameRefClass::classify(&ctx.sema, &name_ref) { - //Necessary to avoid two same names going through - if !node_set.contains(&name_ref.syntax().to_string()) { - node_set.insert(name_ref.syntax().to_string()); - let def_opt: Option = match name_classify { - NameRefClass::Definition(def) => Some(def), - _ => None, - }; - - if let Some(def) = def_opt { - if let Some(import_path) = self - .process_names_and_namerefs_for_import_resolve( - def, - name_ref.syntax(), - &curr_parent_module, - ctx, - ) - { - check_intersection_and_push( - &mut import_paths_to_be_removed, - import_path, - ); - } - } - } - } - } - } + }) } - import_paths_to_be_removed + imports_to_remove } - fn process_names_and_namerefs_for_import_resolve( + fn process_def_in_sel( &mut self, def: Definition, node_syntax: &SyntaxNode, @@ -474,51 +439,38 @@ impl Module { ) -> Option { //We only need to find in the current file let selection_range = ctx.selection_trimmed(); - let curr_file_id = ctx.file_id(); - let search_scope = SearchScope::single_file(curr_file_id); - let usage_res = def.usages(&ctx.sema).in_scope(&search_scope).all(); - let file = ctx.sema.parse(curr_file_id); + let file_id = ctx.file_id(); + let usage_res = def.usages(&ctx.sema).in_scope(&SearchScope::single_file(file_id)).all(); + let file = ctx.sema.parse(file_id); + // track uses which does not exists in `Use` let mut exists_inside_sel = false; let mut exists_outside_sel = false; - for (_, refs) in usage_res.iter() { - let mut non_use_nodes_itr = refs.iter().filter_map(|x| { - if find_node_at_range::(file.syntax(), x.range).is_none() { - let path_opt = find_node_at_range::(file.syntax(), x.range); - return path_opt; - } - - None - }); - - if non_use_nodes_itr - .clone() - .any(|x| !selection_range.contains_range(x.syntax().text_range())) + 'outside: for (_, refs) in usage_res.iter() { + for x in refs + .iter() + .filter(|x| find_node_at_range::(file.syntax(), x.range).is_none()) + .filter_map(|x| find_node_at_range::(file.syntax(), x.range)) { - exists_outside_sel = true; - } - if non_use_nodes_itr.any(|x| selection_range.contains_range(x.syntax().text_range())) { - exists_inside_sel = true; + let in_selectin = selection_range.contains_range(x.syntax().text_range()); + exists_inside_sel |= in_selectin; + exists_outside_sel |= !in_selectin; + + if exists_inside_sel && exists_outside_sel { + break 'outside; + } } } - let source_exists_outside_sel_in_same_mod = does_source_exists_outside_sel_in_same_mod( - def, - ctx, - curr_parent_module, - selection_range, - curr_file_id, - ); + let def_in_mod_and_out_sel = + check_def_in_mod_and_out_sel(def, ctx, curr_parent_module, selection_range, file_id); - let use_stmt_opt: Option = usage_res.into_iter().find_map(|(file_id, refs)| { - if file_id == curr_file_id { - refs.into_iter() - .rev() - .find_map(|fref| find_node_at_range(file.syntax(), fref.range)) - } else { - None - } - }); + // Find use stmt that use def in current file + let use_stmt: Option = usage_res + .into_iter() + .filter(|(use_file_id, _)| *use_file_id == file_id) + .flat_map(|(_, refs)| refs.into_iter().rev()) + .find_map(|fref| find_node_at_range(file.syntax(), fref.range)); let mut use_tree_str_opt: Option> = None; //Exists inside and outside selection @@ -539,32 +491,32 @@ impl Module { //If use_stmt exists, find the use_tree_str, reconstruct it inside new module //If not, insert a use stmt with super and the given nameref - if let Some((use_tree_str, _)) = - self.process_use_stmt_for_import_resolve(use_stmt_opt, node_syntax) - { - use_tree_str_opt = Some(use_tree_str); - } else if source_exists_outside_sel_in_same_mod { - //Considered only after use_stmt is not present - //source_exists_outside_sel_in_same_mod | exists_outside_sel(exists_inside_sel = - //true for all cases) - // false | false -> Do nothing - // false | true -> If source is in selection -> nothing to do, If source is outside - // mod -> ust_stmt transversal - // true | false -> super import insertion - // true | true -> super import insertion - self.make_use_stmt_of_node_with_super(node_syntax); + match self.process_use_stmt_for_import_resolve(use_stmt, node_syntax) { + Some((use_tree_str, _)) => use_tree_str_opt = Some(use_tree_str), + None if def_in_mod_and_out_sel => { + //Considered only after use_stmt is not present + //def_in_mod_and_out_sel | exists_outside_sel(exists_inside_sel = + //true for all cases) + // false | false -> Do nothing + // false | true -> If source is in selection -> nothing to do, If source is outside + // mod -> ust_stmt transversal + // true | false -> super import insertion + // true | true -> super import insertion + self.make_use_stmt_of_node_with_super(node_syntax); + } + None => {} } } else if exists_inside_sel && !exists_outside_sel { //Changes to be made inside new module, and remove import from outside if let Some((mut use_tree_str, text_range_opt)) = - self.process_use_stmt_for_import_resolve(use_stmt_opt, node_syntax) + self.process_use_stmt_for_import_resolve(use_stmt, node_syntax) { if let Some(text_range) = text_range_opt { import_path_to_be_removed = Some(text_range); } - if source_exists_outside_sel_in_same_mod { + if def_in_mod_and_out_sel { if let Some(first_path_in_use_tree) = use_tree_str.last() { let first_path_in_use_tree_str = first_path_in_use_tree.to_string(); if !first_path_in_use_tree_str.contains("super") @@ -577,7 +529,7 @@ impl Module { } use_tree_str_opt = Some(use_tree_str); - } else if source_exists_outside_sel_in_same_mod { + } else if def_in_mod_and_out_sel { self.make_use_stmt_of_node_with_super(node_syntax); } } @@ -586,13 +538,10 @@ impl Module { let mut use_tree_str = use_tree_str; use_tree_str.reverse(); - if !(!exists_outside_sel && exists_inside_sel && source_exists_outside_sel_in_same_mod) - { + if exists_outside_sel || !exists_inside_sel || !def_in_mod_and_out_sel { if let Some(first_path_in_use_tree) = use_tree_str.first() { - let first_path_in_use_tree_str = first_path_in_use_tree.to_string(); - if first_path_in_use_tree_str.contains("super") { - let super_path = make::ext::ident_path("super"); - use_tree_str.insert(0, super_path) + if first_path_in_use_tree.to_string().contains("super") { + use_tree_str.insert(0, make::ext::ident_path("super")); } } } @@ -621,33 +570,26 @@ impl Module { fn process_use_stmt_for_import_resolve( &self, - use_stmt_opt: Option, + use_stmt: Option, node_syntax: &SyntaxNode, ) -> Option<(Vec, Option)> { - if let Some(use_stmt) = use_stmt_opt { - for desc in use_stmt.syntax().descendants() { - if let Some(path_seg) = ast::PathSegment::cast(desc) { - if path_seg.syntax().to_string() == node_syntax.to_string() { - let mut use_tree_str = vec![path_seg.parent_path()]; - get_use_tree_paths_from_path(path_seg.parent_path(), &mut use_tree_str); - for ancs in path_seg.syntax().ancestors() { - //Here we are looking for use_tree with same string value as node - //passed above as the range_to_remove function looks for a comma and - //then includes it in the text range to remove it. But the comma only - //appears at the use_tree level - if let Some(use_tree) = ast::UseTree::cast(ancs) { - if use_tree.syntax().to_string() == node_syntax.to_string() { - return Some(( - use_tree_str, - Some(range_to_remove(use_tree.syntax())), - )); - } - } - } + let use_stmt = use_stmt?; + for path_seg in use_stmt.syntax().descendants().filter_map(ast::PathSegment::cast) { + if path_seg.syntax().to_string() == node_syntax.to_string() { + let mut use_tree_str = vec![path_seg.parent_path()]; + get_use_tree_paths_from_path(path_seg.parent_path(), &mut use_tree_str); - return Some((use_tree_str, None)); + //Here we are looking for use_tree with same string value as node + //passed above as the range_to_remove function looks for a comma and + //then includes it in the text range to remove it. But the comma only + //appears at the use_tree level + for use_tree in path_seg.syntax().ancestors().filter_map(ast::UseTree::cast) { + if use_tree.syntax().to_string() == node_syntax.to_string() { + return Some((use_tree_str, Some(range_to_remove(use_tree.syntax())))); } } + + return Some((use_tree_str, None)); } } @@ -676,145 +618,56 @@ fn check_intersection_and_push( import_paths_to_be_removed.push(import_path); } -fn does_source_exists_outside_sel_in_same_mod( +fn check_def_in_mod_and_out_sel( def: Definition, ctx: &AssistContext<'_>, curr_parent_module: &Option, selection_range: TextRange, curr_file_id: FileId, ) -> bool { - let mut source_exists_outside_sel_in_same_mod = false; + macro_rules! check_item { + ($x:ident) => { + if let Some(source) = $x.source(ctx.db()) { + let have_same_parent = if let Some(ast_module) = &curr_parent_module { + ctx.sema.to_module_def(ast_module).is_some_and(|it| it == $x.module(ctx.db())) + } else { + source.file_id.original_file(ctx.db()) == curr_file_id + }; + + if have_same_parent { + return !selection_range.contains_range(source.value.syntax().text_range()); + } + } + }; + } + match def { Definition::Module(x) => { let source = x.definition_source(ctx.db()); - let have_same_parent = if let Some(ast_module) = &curr_parent_module { - if let Some(hir_module) = x.parent(ctx.db()) { - compare_hir_and_ast_module(ast_module, hir_module, ctx).is_some() - } else { - let source_file_id = source.file_id.original_file(ctx.db()); - source_file_id == curr_file_id + let have_same_parent = match (&curr_parent_module, x.parent(ctx.db())) { + (Some(ast_module), Some(hir_module)) => { + ctx.sema.to_module_def(ast_module).is_some_and(|it| it == hir_module) } - } else { - let source_file_id = source.file_id.original_file(ctx.db()); - source_file_id == curr_file_id + _ => source.file_id.original_file(ctx.db()) == curr_file_id, }; if have_same_parent { if let ModuleSource::Module(module_) = source.value { - source_exists_outside_sel_in_same_mod = - !selection_range.contains_range(module_.syntax().text_range()); - } - } - } - Definition::Function(x) => { - if let Some(source) = x.source(ctx.db()) { - let have_same_parent = if let Some(ast_module) = &curr_parent_module { - compare_hir_and_ast_module(ast_module, x.module(ctx.db()), ctx).is_some() - } else { - let source_file_id = source.file_id.original_file(ctx.db()); - source_file_id == curr_file_id - }; - - if have_same_parent { - source_exists_outside_sel_in_same_mod = - !selection_range.contains_range(source.value.syntax().text_range()); - } - } - } - Definition::Adt(x) => { - if let Some(source) = x.source(ctx.db()) { - let have_same_parent = if let Some(ast_module) = &curr_parent_module { - compare_hir_and_ast_module(ast_module, x.module(ctx.db()), ctx).is_some() - } else { - let source_file_id = source.file_id.original_file(ctx.db()); - source_file_id == curr_file_id - }; - - if have_same_parent { - source_exists_outside_sel_in_same_mod = - !selection_range.contains_range(source.value.syntax().text_range()); - } - } - } - Definition::Variant(x) => { - if let Some(source) = x.source(ctx.db()) { - let have_same_parent = if let Some(ast_module) = &curr_parent_module { - compare_hir_and_ast_module(ast_module, x.module(ctx.db()), ctx).is_some() - } else { - let source_file_id = source.file_id.original_file(ctx.db()); - source_file_id == curr_file_id - }; - - if have_same_parent { - source_exists_outside_sel_in_same_mod = - !selection_range.contains_range(source.value.syntax().text_range()); - } - } - } - Definition::Const(x) => { - if let Some(source) = x.source(ctx.db()) { - let have_same_parent = if let Some(ast_module) = &curr_parent_module { - compare_hir_and_ast_module(ast_module, x.module(ctx.db()), ctx).is_some() - } else { - let source_file_id = source.file_id.original_file(ctx.db()); - source_file_id == curr_file_id - }; - - if have_same_parent { - source_exists_outside_sel_in_same_mod = - !selection_range.contains_range(source.value.syntax().text_range()); - } - } - } - Definition::Static(x) => { - if let Some(source) = x.source(ctx.db()) { - let have_same_parent = if let Some(ast_module) = &curr_parent_module { - compare_hir_and_ast_module(ast_module, x.module(ctx.db()), ctx).is_some() - } else { - let source_file_id = source.file_id.original_file(ctx.db()); - source_file_id == curr_file_id - }; - - if have_same_parent { - source_exists_outside_sel_in_same_mod = - !selection_range.contains_range(source.value.syntax().text_range()); - } - } - } - Definition::Trait(x) => { - if let Some(source) = x.source(ctx.db()) { - let have_same_parent = if let Some(ast_module) = &curr_parent_module { - compare_hir_and_ast_module(ast_module, x.module(ctx.db()), ctx).is_some() - } else { - let source_file_id = source.file_id.original_file(ctx.db()); - source_file_id == curr_file_id - }; - - if have_same_parent { - source_exists_outside_sel_in_same_mod = - !selection_range.contains_range(source.value.syntax().text_range()); - } - } - } - Definition::TypeAlias(x) => { - if let Some(source) = x.source(ctx.db()) { - let have_same_parent = if let Some(ast_module) = &curr_parent_module { - compare_hir_and_ast_module(ast_module, x.module(ctx.db()), ctx).is_some() - } else { - let source_file_id = source.file_id.original_file(ctx.db()); - source_file_id == curr_file_id - }; - - if have_same_parent { - source_exists_outside_sel_in_same_mod = - !selection_range.contains_range(source.value.syntax().text_range()); + return !selection_range.contains_range(module_.syntax().text_range()); } } } + Definition::Function(x) => check_item!(x), + Definition::Adt(x) => check_item!(x), + Definition::Variant(x) => check_item!(x), + Definition::Const(x) => check_item!(x), + Definition::Static(x) => check_item!(x), + Definition::Trait(x) => check_item!(x), + Definition::TypeAlias(x) => check_item!(x), _ => {} } - source_exists_outside_sel_in_same_mod + false } fn get_replacements_for_visibility_change( @@ -834,24 +687,30 @@ fn get_replacements_for_visibility_change( *item = item.clone_for_update(); } //Use stmts are ignored + macro_rules! push_to_replacement { + ($it:ident) => { + replacements.push(($it.visibility(), $it.syntax().clone())) + }; + } + match item { - ast::Item::Const(it) => replacements.push((it.visibility(), it.syntax().clone())), - ast::Item::Enum(it) => replacements.push((it.visibility(), it.syntax().clone())), - ast::Item::ExternCrate(it) => replacements.push((it.visibility(), it.syntax().clone())), - ast::Item::Fn(it) => replacements.push((it.visibility(), it.syntax().clone())), + ast::Item::Const(it) => push_to_replacement!(it), + ast::Item::Enum(it) => push_to_replacement!(it), + ast::Item::ExternCrate(it) => push_to_replacement!(it), + ast::Item::Fn(it) => push_to_replacement!(it), //Associated item's visibility should not be changed ast::Item::Impl(it) if it.for_token().is_none() => impls.push(it.clone()), - ast::Item::MacroDef(it) => replacements.push((it.visibility(), it.syntax().clone())), - ast::Item::Module(it) => replacements.push((it.visibility(), it.syntax().clone())), - ast::Item::Static(it) => replacements.push((it.visibility(), it.syntax().clone())), + ast::Item::MacroDef(it) => push_to_replacement!(it), + ast::Item::Module(it) => push_to_replacement!(it), + ast::Item::Static(it) => push_to_replacement!(it), ast::Item::Struct(it) => { - replacements.push((it.visibility(), it.syntax().clone())); + push_to_replacement!(it); record_field_parents.push((it.visibility(), it.syntax().clone())); } - ast::Item::Trait(it) => replacements.push((it.visibility(), it.syntax().clone())), - ast::Item::TypeAlias(it) => replacements.push((it.visibility(), it.syntax().clone())), + ast::Item::Trait(it) => push_to_replacement!(it), + ast::Item::TypeAlias(it) => push_to_replacement!(it), ast::Item::Union(it) => { - replacements.push((it.visibility(), it.syntax().clone())); + push_to_replacement!(it); record_field_parents.push((it.visibility(), it.syntax().clone())); } _ => (), @@ -865,8 +724,11 @@ fn get_use_tree_paths_from_path( path: ast::Path, use_tree_str: &mut Vec, ) -> Option<&mut Vec> { - path.syntax().ancestors().filter(|x| x.to_string() != path.to_string()).find_map(|x| { - if let Some(use_tree) = ast::UseTree::cast(x) { + path.syntax() + .ancestors() + .filter(|x| x.to_string() != path.to_string()) + .filter_map(ast::UseTree::cast) + .find_map(|use_tree| { if let Some(upper_tree_path) = use_tree.path() { if upper_tree_path.to_string() != path.to_string() { use_tree_str.push(upper_tree_path.clone()); @@ -874,9 +736,8 @@ fn get_use_tree_paths_from_path( return Some(use_tree); } } - } - None - })?; + None + })?; Some(use_tree_str) } @@ -890,20 +751,6 @@ fn add_change_vis(vis: Option, node_or_token_opt: Option, -) -> Option<()> { - let hir_mod_name = hir_module.name(ctx.db())?; - let ast_mod_name = ast_module.name()?; - if hir_mod_name.display(ctx.db()).to_string() != ast_mod_name.to_string() { - return None; - } - - Some(()) -} - fn indent_range_before_given_node(node: &SyntaxNode) -> Option { node.siblings_with_tokens(syntax::Direction::Prev) .find(|x| x.kind() == WHITESPACE) @@ -1799,6 +1646,33 @@ mod modname { pub(crate) condvar: B, } } +"#, + ); + } + + #[test] + fn test_remove_import_path_inside_selection() { + check_assist( + extract_module, + r#" +$0struct Point; +impl Point { + pub const fn direction(self, other: Self) -> Option { + Some(Vertical) + } +} + +pub enum Direction { + Horizontal, + Vertical, +} +use Direction::{Horizontal, Vertical};$0 + +fn main() { + let x = Vertical; +} +"#, + r#" "#, ); } From 418056597b6a5c55fa8652b26728e7cb9589ebc8 Mon Sep 17 00:00:00 2001 From: roife Date: Thu, 14 Mar 2024 15:18:31 +0800 Subject: [PATCH 2/9] fix: donot generate redundant use stmt for items in selection in extract_module --- .../src/handlers/extract_module.rs | 31 +++++++++++-------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index 95bb4e174b..7c486ae6f7 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -462,7 +462,7 @@ impl Module { } } - let def_in_mod_and_out_sel = + let (def_in_mod, def_out_sel) = check_def_in_mod_and_out_sel(def, ctx, curr_parent_module, selection_range, file_id); // Find use stmt that use def in current file @@ -493,9 +493,9 @@ impl Module { //If not, insert a use stmt with super and the given nameref match self.process_use_stmt_for_import_resolve(use_stmt, node_syntax) { Some((use_tree_str, _)) => use_tree_str_opt = Some(use_tree_str), - None if def_in_mod_and_out_sel => { + None if def_in_mod && def_out_sel => { //Considered only after use_stmt is not present - //def_in_mod_and_out_sel | exists_outside_sel(exists_inside_sel = + //def_in_mod && def_out_sel | exists_outside_sel(exists_inside_sel = //true for all cases) // false | false -> Do nothing // false | true -> If source is in selection -> nothing to do, If source is outside @@ -516,7 +516,7 @@ impl Module { import_path_to_be_removed = Some(text_range); } - if def_in_mod_and_out_sel { + if def_in_mod && def_out_sel { if let Some(first_path_in_use_tree) = use_tree_str.last() { let first_path_in_use_tree_str = first_path_in_use_tree.to_string(); if !first_path_in_use_tree_str.contains("super") @@ -529,7 +529,7 @@ impl Module { } use_tree_str_opt = Some(use_tree_str); - } else if def_in_mod_and_out_sel { + } else if def_in_mod && def_out_sel { self.make_use_stmt_of_node_with_super(node_syntax); } } @@ -538,7 +538,7 @@ impl Module { let mut use_tree_str = use_tree_str; use_tree_str.reverse(); - if exists_outside_sel || !exists_inside_sel || !def_in_mod_and_out_sel { + if exists_outside_sel || !exists_inside_sel || !def_in_mod || !def_out_sel { if let Some(first_path_in_use_tree) = use_tree_str.first() { if first_path_in_use_tree.to_string().contains("super") { use_tree_str.insert(0, make::ext::ident_path("super")); @@ -549,7 +549,10 @@ impl Module { let use_ = make::use_(None, make::use_tree(make::join_paths(use_tree_str), None, None, false)); let item = ast::Item::from(use_); - self.use_items.insert(0, item); + + if def_out_sel { + self.use_items.insert(0, item.clone()); + } } import_path_to_be_removed @@ -624,7 +627,7 @@ fn check_def_in_mod_and_out_sel( curr_parent_module: &Option, selection_range: TextRange, curr_file_id: FileId, -) -> bool { +) -> (bool, bool) { macro_rules! check_item { ($x:ident) => { if let Some(source) = $x.source(ctx.db()) { @@ -634,9 +637,8 @@ fn check_def_in_mod_and_out_sel( source.file_id.original_file(ctx.db()) == curr_file_id }; - if have_same_parent { - return !selection_range.contains_range(source.value.syntax().text_range()); - } + let in_sel = !selection_range.contains_range(source.value.syntax().text_range()); + return (have_same_parent, in_sel); } }; } @@ -653,9 +655,12 @@ fn check_def_in_mod_and_out_sel( if have_same_parent { if let ModuleSource::Module(module_) = source.value { - return !selection_range.contains_range(module_.syntax().text_range()); + let in_sel = !selection_range.contains_range(module_.syntax().text_range()); + return (have_same_parent, in_sel); } } + + return (have_same_parent, false); } Definition::Function(x) => check_item!(x), Definition::Adt(x) => check_item!(x), @@ -667,7 +672,7 @@ fn check_def_in_mod_and_out_sel( _ => {} } - false + (false, false) } fn get_replacements_for_visibility_change( From 02214a6d12843826b3ceb54792a3d4dccc5f1228 Mon Sep 17 00:00:00 2001 From: roife Date: Thu, 14 Mar 2024 16:54:45 +0800 Subject: [PATCH 3/9] fix: remove redundant use node insertion --- .../src/handlers/extract_module.rs | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index 7c486ae6f7..95bdd341ec 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -1,10 +1,12 @@ use std::iter; -use hir::{HasSource, HirFileIdExt, ModuleSource}; +use hir::{HirFileIdExt, ModuleSource}; use ide_db::{ assists::{AssistId, AssistKind}, base_db::FileId, defs::{Definition, NameClass, NameRefClass}, + helpers::item_name, + items_locator::items_with_name, search::{FileReference, SearchScope}, FxHashMap, FxHashSet, }; @@ -433,7 +435,7 @@ impl Module { fn process_def_in_sel( &mut self, def: Definition, - node_syntax: &SyntaxNode, + use_node: &SyntaxNode, curr_parent_module: &Option, ctx: &AssistContext<'_>, ) -> Option { @@ -491,7 +493,7 @@ impl Module { //If use_stmt exists, find the use_tree_str, reconstruct it inside new module //If not, insert a use stmt with super and the given nameref - match self.process_use_stmt_for_import_resolve(use_stmt, node_syntax) { + match self.process_use_stmt_for_import_resolve(use_stmt, use_node) { Some((use_tree_str, _)) => use_tree_str_opt = Some(use_tree_str), None if def_in_mod && def_out_sel => { //Considered only after use_stmt is not present @@ -502,7 +504,7 @@ impl Module { // mod -> ust_stmt transversal // true | false -> super import insertion // true | true -> super import insertion - self.make_use_stmt_of_node_with_super(node_syntax); + self.make_use_stmt_of_node_with_super(use_node); } None => {} } @@ -510,7 +512,7 @@ impl Module { //Changes to be made inside new module, and remove import from outside if let Some((mut use_tree_str, text_range_opt)) = - self.process_use_stmt_for_import_resolve(use_stmt, node_syntax) + self.process_use_stmt_for_import_resolve(use_stmt, use_node) { if let Some(text_range) = text_range_opt { import_path_to_be_removed = Some(text_range); @@ -530,7 +532,7 @@ impl Module { use_tree_str_opt = Some(use_tree_str); } else if def_in_mod && def_out_sel { - self.make_use_stmt_of_node_with_super(node_syntax); + self.make_use_stmt_of_node_with_super(use_node); } } @@ -550,7 +552,20 @@ impl Module { make::use_(None, make::use_tree(make::join_paths(use_tree_str), None, None, false)); let item = ast::Item::from(use_); - if def_out_sel { + let is_item = match def { + Definition::Macro(_) => true, + Definition::Module(_) => true, + Definition::Function(_) => true, + Definition::Adt(_) => true, + Definition::Const(_) => true, + Definition::Static(_) => true, + Definition::Trait(_) => true, + Definition::TraitAlias(_) => true, + Definition::TypeAlias(_) => true, + _ => false, + }; + + if def_out_sel || !is_item { self.use_items.insert(0, item.clone()); } } From 6248b45340e9d9ec1afc7fe6068a37018ffb4d36 Mon Sep 17 00:00:00 2001 From: roife Date: Thu, 14 Mar 2024 19:50:36 +0800 Subject: [PATCH 4/9] fix: do not add use stmt when use stmt is selected in extract_module --- crates/ide-assists/src/handlers/extract_module.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index 95bdd341ec..4c04e1d2fd 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -1,12 +1,10 @@ use std::iter; -use hir::{HirFileIdExt, ModuleSource}; +use hir::{HasSource, HirFileIdExt, ModuleSource}; use ide_db::{ assists::{AssistId, AssistKind}, base_db::FileId, defs::{Definition, NameClass, NameRefClass}, - helpers::item_name, - items_locator::items_with_name, search::{FileReference, SearchScope}, FxHashMap, FxHashSet, }; @@ -473,6 +471,9 @@ impl Module { .filter(|(use_file_id, _)| *use_file_id == file_id) .flat_map(|(_, refs)| refs.into_iter().rev()) .find_map(|fref| find_node_at_range(file.syntax(), fref.range)); + let use_stmt_not_in_sel = use_stmt.as_ref().is_some_and(|use_stmt| { + !selection_range.contains_range(use_stmt.syntax().text_range()) + }); let mut use_tree_str_opt: Option> = None; //Exists inside and outside selection @@ -565,7 +566,7 @@ impl Module { _ => false, }; - if def_out_sel || !is_item { + if (def_out_sel || !is_item) && use_stmt_not_in_sel { self.use_items.insert(0, item.clone()); } } From 5b2809f329a1dd7cdce6dcadff773f628ea0b96a Mon Sep 17 00:00:00 2001 From: roife Date: Fri, 15 Mar 2024 02:47:41 +0800 Subject: [PATCH 5/9] fix: simplification on extract_module --- .../src/handlers/extract_module.rs | 209 ++++++++---------- 1 file changed, 95 insertions(+), 114 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index 4c04e1d2fd..f161bbd4aa 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -1,5 +1,6 @@ use std::iter; +use either::Either; use hir::{HasSource, HirFileIdExt, ModuleSource}; use ide_db::{ assists::{AssistId, AssistKind}, @@ -10,7 +11,6 @@ use ide_db::{ }; use itertools::Itertools; use smallvec::SmallVec; -use stdx::format_to; use syntax::{ algo::find_node_at_range, ast::{ @@ -18,7 +18,7 @@ use syntax::{ edit::{AstNodeEdit, IndentLevel}, make, HasVisibility, }, - match_ast, ted, AstNode, SourceFile, + match_ast, ted, AstNode, SyntaxKind::{self, WHITESPACE}, SyntaxNode, TextRange, }; @@ -114,63 +114,23 @@ pub(crate) fn extract_module(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti let import_paths_to_be_removed = module.resolve_imports(curr_parent_module, ctx); module.change_visibility(record_fields); - let mut body_items: Vec = Vec::new(); - let mut items_to_be_processed: Vec = module.body_items.clone(); + let module_def = generate_module_def(&impl_parent, &mut module, old_item_indent); - let new_item_indent = if impl_parent.is_some() { - old_item_indent + 2 - } else { - items_to_be_processed = [module.use_items.clone(), items_to_be_processed].concat(); - old_item_indent + 1 - }; - - for item in items_to_be_processed { - let item = item.indent(IndentLevel(1)); - let mut indented_item = String::new(); - format_to!(indented_item, "{new_item_indent}{item}"); - body_items.push(indented_item); - } - - let mut body = body_items.join("\n\n"); - - if let Some(impl_) = &impl_parent { - if let Some(self_ty) = impl_.self_ty() { - let impl_indent = old_item_indent + 1; - let mut impl_body_def = String::new(); - format_to!( - impl_body_def, - "{impl_indent}impl {self_ty} {{\n{body}\n{impl_indent}}}", - ); - body = impl_body_def; - - // Add the import for enum/struct corresponding to given impl block - module.make_use_stmt_of_node_with_super(self_ty.syntax()); - for item in module.use_items { - let item_indent = old_item_indent + 1; - body = format!("{item_indent}{item}\n\n{body}"); - } - } - } - - let mut module_def = String::new(); - let module_name = module.name; - format_to!(module_def, "mod {module_name} {{\n{body}\n{old_item_indent}}}"); - - let mut usages_to_be_updated_for_curr_file = vec![]; - for usages_to_be_updated_for_file in usages_to_be_processed { - if usages_to_be_updated_for_file.0 == ctx.file_id() { - usages_to_be_updated_for_curr_file = usages_to_be_updated_for_file.1; + let mut usages_to_be_processed_for_cur_file = vec![]; + for (file_id, usages) in usages_to_be_processed { + if file_id == ctx.file_id() { + usages_to_be_processed_for_cur_file = usages; continue; } - builder.edit_file(usages_to_be_updated_for_file.0); - for usage_to_be_processed in usages_to_be_updated_for_file.1 { - builder.replace(usage_to_be_processed.0, usage_to_be_processed.1) + builder.edit_file(file_id); + for (text_range, usage) in usages { + builder.replace(text_range, usage) } } builder.edit_file(ctx.file_id()); - for usage_to_be_processed in usages_to_be_updated_for_curr_file { - builder.replace(usage_to_be_processed.0, usage_to_be_processed.1) + for (text_range, usage) in usages_to_be_processed_for_cur_file { + builder.replace(text_range, usage); } if let Some(impl_) = impl_parent { @@ -205,6 +165,37 @@ pub(crate) fn extract_module(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti ) } +fn generate_module_def( + parent_impl: &Option, + module: &mut Module, + old_indent: IndentLevel, +) -> String { + let (items_to_be_processed, new_item_indent) = if parent_impl.is_some() { + (Either::Left(module.body_items.iter()), old_indent + 2) + } else { + (Either::Right(module.use_items.iter().chain(module.body_items.iter())), old_indent + 1) + }; + + let mut body = items_to_be_processed + .map(|item| item.indent(IndentLevel(1))) + .map(|item| format!("{new_item_indent}{item}")) + .join("\n\n"); + + if let Some(self_ty) = parent_impl.as_ref().and_then(|imp| imp.self_ty()) { + let impl_indent = old_indent + 1; + body = format!("{impl_indent}impl {self_ty} {{\n{body}\n{impl_indent}}}"); + + // Add the import for enum/struct corresponding to given impl block + module.make_use_stmt_of_node_with_super(self_ty.syntax()); + for item in module.use_items.iter() { + body = format!("{impl_indent}{item}\n\n{body}"); + } + } + + let module_name = module.name; + format!("mod {module_name} {{\n{body}\n{old_indent}}}") +} + #[derive(Debug)] struct Module { text_range: TextRange, @@ -240,6 +231,7 @@ impl Module { //Here impl is not included as each item inside impl will be tied to the parent of //implementing block(a struct, enum, etc), if the parent is in selected module, it will //get updated by ADT section given below or if it is not, then we dont need to do any operation + for item in &self.body_items { match_ast! { match (item.syntax()) { @@ -320,48 +312,38 @@ impl Module { node_def: Definition, refs_in_files: &mut FxHashMap>, ) { - for (file_id, references) in node_def.usages(&ctx.sema).all() { + let mod_name = self.name; + let out_of_sel = |node: &SyntaxNode| !self.text_range.contains_range(node.text_range()); + for (file_id, refs) in node_def.usages(&ctx.sema).all() { let source_file = ctx.sema.parse(file_id); - let usages_in_file = references - .into_iter() - .filter_map(|usage| self.get_usage_to_be_processed(&source_file, usage)); - refs_in_files.entry(file_id).or_default().extend(usages_in_file); - } - } + let usages = refs.into_iter().filter_map(|FileReference { range, name, .. }| { + let path: ast::Path = find_node_at_range(source_file.syntax(), range)?; + let name = name.syntax().to_string(); - fn get_usage_to_be_processed( - &self, - source_file: &SourceFile, - FileReference { range, name, .. }: FileReference, - ) -> Option<(TextRange, String)> { - let path: ast::Path = find_node_at_range(source_file.syntax(), range)?; - - for desc in path.syntax().descendants() { - if desc.to_string() == name.syntax().to_string() - && !self.text_range.contains_range(desc.text_range()) - { - if let Some(name_ref) = ast::NameRef::cast(desc) { - let mod_name = self.name; - return Some(( - name_ref.syntax().text_range(), - format!("{mod_name}::{name_ref}"), - )); + for desc in path.syntax().descendants() { + if desc.to_string() == name && out_of_sel(&desc) { + if let Some(name_ref) = ast::NameRef::cast(desc) { + let new_ref = format!("{mod_name}::{name_ref}"); + return Some((name_ref.syntax().text_range(), new_ref)); + } + } } - } - } - None + None + }); + refs_in_files.entry(file_id).or_default().extend(usages); + } } fn change_visibility(&mut self, record_fields: Vec) { let (mut replacements, record_field_parents, impls) = get_replacements_for_visibility_change(&mut self.body_items, false); - let mut impl_items: Vec = impls + let mut impl_items = impls .into_iter() .flat_map(|impl_| impl_.syntax().descendants()) .filter_map(ast::Item::cast) - .collect(); + .collect_vec(); let (mut impl_item_replacements, _, _) = get_replacements_for_visibility_change(&mut impl_items, true); @@ -444,8 +426,8 @@ impl Module { let file = ctx.sema.parse(file_id); // track uses which does not exists in `Use` - let mut exists_inside_sel = false; - let mut exists_outside_sel = false; + let mut uses_exist_in_sel = false; + let mut uses_exist_out_sel = false; 'outside: for (_, refs) in usage_res.iter() { for x in refs .iter() @@ -453,10 +435,10 @@ impl Module { .filter_map(|x| find_node_at_range::(file.syntax(), x.range)) { let in_selectin = selection_range.contains_range(x.syntax().text_range()); - exists_inside_sel |= in_selectin; - exists_outside_sel |= !in_selectin; + uses_exist_in_sel |= in_selectin; + uses_exist_out_sel |= !in_selectin; - if exists_inside_sel && exists_outside_sel { + if uses_exist_in_sel && uses_exist_out_sel { break 'outside; } } @@ -475,7 +457,7 @@ impl Module { !selection_range.contains_range(use_stmt.syntax().text_range()) }); - let mut use_tree_str_opt: Option> = None; + let mut use_tree_paths: Option> = None; //Exists inside and outside selection // - Use stmt for item is present -> get the use_tree_str and reconstruct the path in new // module @@ -489,13 +471,13 @@ impl Module { //get the use_tree_str, reconstruct the use stmt in new module let mut import_path_to_be_removed: Option = None; - if exists_inside_sel && exists_outside_sel { + if uses_exist_in_sel && uses_exist_out_sel { //Changes to be made only inside new module //If use_stmt exists, find the use_tree_str, reconstruct it inside new module //If not, insert a use stmt with super and the given nameref match self.process_use_stmt_for_import_resolve(use_stmt, use_node) { - Some((use_tree_str, _)) => use_tree_str_opt = Some(use_tree_str), + Some((use_tree_str, _)) => use_tree_paths = Some(use_tree_str), None if def_in_mod && def_out_sel => { //Considered only after use_stmt is not present //def_in_mod && def_out_sel | exists_outside_sel(exists_inside_sel = @@ -509,7 +491,7 @@ impl Module { } None => {} } - } else if exists_inside_sel && !exists_outside_sel { + } else if uses_exist_in_sel && !uses_exist_out_sel { //Changes to be made inside new module, and remove import from outside if let Some((mut use_tree_str, text_range_opt)) = @@ -531,43 +513,42 @@ impl Module { } } - use_tree_str_opt = Some(use_tree_str); + use_tree_paths = Some(use_tree_str); } else if def_in_mod && def_out_sel { self.make_use_stmt_of_node_with_super(use_node); } } - if let Some(use_tree_str) = use_tree_str_opt { - let mut use_tree_str = use_tree_str; - use_tree_str.reverse(); + if let Some(mut use_tree_paths) = use_tree_paths { + use_tree_paths.reverse(); - if exists_outside_sel || !exists_inside_sel || !def_in_mod || !def_out_sel { - if let Some(first_path_in_use_tree) = use_tree_str.first() { + if uses_exist_out_sel || !uses_exist_in_sel || !def_in_mod || !def_out_sel { + if let Some(first_path_in_use_tree) = use_tree_paths.first() { if first_path_in_use_tree.to_string().contains("super") { - use_tree_str.insert(0, make::ext::ident_path("super")); + use_tree_paths.insert(0, make::ext::ident_path("super")); } } } - let use_ = - make::use_(None, make::use_tree(make::join_paths(use_tree_str), None, None, false)); - let item = ast::Item::from(use_); - - let is_item = match def { - Definition::Macro(_) => true, - Definition::Module(_) => true, - Definition::Function(_) => true, - Definition::Adt(_) => true, - Definition::Const(_) => true, - Definition::Static(_) => true, - Definition::Trait(_) => true, - Definition::TraitAlias(_) => true, - Definition::TypeAlias(_) => true, - _ => false, - }; + let is_item = matches!( + def, + Definition::Macro(_) + | Definition::Module(_) + | Definition::Function(_) + | Definition::Adt(_) + | Definition::Const(_) + | Definition::Static(_) + | Definition::Trait(_) + | Definition::TraitAlias(_) + | Definition::TypeAlias(_) + ); if (def_out_sel || !is_item) && use_stmt_not_in_sel { - self.use_items.insert(0, item.clone()); + let use_ = make::use_( + None, + make::use_tree(make::join_paths(use_tree_paths), None, None, false), + ); + self.use_items.insert(0, ast::Item::from(use_)); } } From de716058c9900d47ae3f23c112316f0657702c83 Mon Sep 17 00:00:00 2001 From: roife Date: Fri, 15 Mar 2024 19:54:58 +0800 Subject: [PATCH 6/9] fix: remove useless loop --- crates/ide-assists/src/handlers/extract_module.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index f161bbd4aa..b3ca7f9c35 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -314,19 +314,17 @@ impl Module { ) { let mod_name = self.name; let out_of_sel = |node: &SyntaxNode| !self.text_range.contains_range(node.text_range()); + for (file_id, refs) in node_def.usages(&ctx.sema).all() { let source_file = ctx.sema.parse(file_id); let usages = refs.into_iter().filter_map(|FileReference { range, name, .. }| { - let path: ast::Path = find_node_at_range(source_file.syntax(), range)?; + // handle normal usages + let name_ref = find_node_at_range::(source_file.syntax(), range)?; let name = name.syntax().to_string(); - for desc in path.syntax().descendants() { - if desc.to_string() == name && out_of_sel(&desc) { - if let Some(name_ref) = ast::NameRef::cast(desc) { - let new_ref = format!("{mod_name}::{name_ref}"); - return Some((name_ref.syntax().text_range(), new_ref)); - } - } + if out_of_sel(name_ref.syntax()) { + let new_ref = format!("{mod_name}::{name_ref}"); + return Some((name_ref.syntax().text_range(), new_ref)); } None From 513c6d35ed04a5731c5aca18397a0c1b2454680f Mon Sep 17 00:00:00 2001 From: roife Date: Fri, 15 Mar 2024 21:04:51 +0800 Subject: [PATCH 7/9] fix: re-insert use stmts that is extracted --- .../src/handlers/extract_module.rs | 61 +++++++++++++++---- 1 file changed, 48 insertions(+), 13 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index b3ca7f9c35..b1e43ceb52 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -20,7 +20,7 @@ use syntax::{ }, match_ast, ted, AstNode, SyntaxKind::{self, WHITESPACE}, - SyntaxNode, TextRange, + SyntaxNode, TextRange, TextSize, }; use crate::{AssistContext, Assists}; @@ -109,7 +109,14 @@ pub(crate) fn extract_module(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti //We are getting item usages and record_fields together, record_fields //for change_visibility and usages for first point mentioned above in the process - let (usages_to_be_processed, record_fields) = module.get_usages_and_record_fields(ctx); + + let (usages_to_be_processed, record_fields, use_stmts_to_be_inserted) = + module.get_usages_and_record_fields(ctx); + + builder.edit_file(ctx.file_id()); + use_stmts_to_be_inserted.into_iter().for_each(|(_, use_stmt)| { + builder.insert(ctx.selection_trimmed().end(), format!("\n{use_stmt}")); + }); let import_paths_to_be_removed = module.resolve_imports(curr_parent_module, ctx); module.change_visibility(record_fields); @@ -224,9 +231,12 @@ impl Module { fn get_usages_and_record_fields( &self, ctx: &AssistContext<'_>, - ) -> (FxHashMap>, Vec) { + ) -> (FxHashMap>, Vec, FxHashMap) + { let mut adt_fields = Vec::new(); let mut refs: FxHashMap> = FxHashMap::default(); + // use `TextSize` as key to avoid repeated use stmts + let mut use_stmts_to_be_inserted = FxHashMap::default(); //Here impl is not included as each item inside impl will be tied to the parent of //implementing block(a struct, enum, etc), if the parent is in selected module, it will @@ -238,7 +248,7 @@ impl Module { ast::Adt(it) => { if let Some( nod ) = ctx.sema.to_def(&it) { let node_def = Definition::Adt(nod); - self.expand_and_group_usages_file_wise(ctx, node_def, &mut refs); + self.expand_and_group_usages_file_wise(ctx, node_def, &mut refs, &mut use_stmts_to_be_inserted); //Enum Fields are not allowed to explicitly specify pub, it is implied match it { @@ -272,30 +282,30 @@ impl Module { ast::TypeAlias(it) => { if let Some( nod ) = ctx.sema.to_def(&it) { let node_def = Definition::TypeAlias(nod); - self.expand_and_group_usages_file_wise(ctx, node_def, &mut refs); + self.expand_and_group_usages_file_wise(ctx, node_def, &mut refs, &mut use_stmts_to_be_inserted); } }, ast::Const(it) => { if let Some( nod ) = ctx.sema.to_def(&it) { let node_def = Definition::Const(nod); - self.expand_and_group_usages_file_wise(ctx, node_def, &mut refs); + self.expand_and_group_usages_file_wise(ctx, node_def, &mut refs, &mut use_stmts_to_be_inserted); } }, ast::Static(it) => { if let Some( nod ) = ctx.sema.to_def(&it) { let node_def = Definition::Static(nod); - self.expand_and_group_usages_file_wise(ctx, node_def, &mut refs); + self.expand_and_group_usages_file_wise(ctx, node_def, &mut refs, &mut use_stmts_to_be_inserted); } }, ast::Fn(it) => { if let Some( nod ) = ctx.sema.to_def(&it) { let node_def = Definition::Function(nod); - self.expand_and_group_usages_file_wise(ctx, node_def, &mut refs); + self.expand_and_group_usages_file_wise(ctx, node_def, &mut refs, &mut use_stmts_to_be_inserted); } }, ast::Macro(it) => { if let Some(nod) = ctx.sema.to_def(&it) { - self.expand_and_group_usages_file_wise(ctx, Definition::Macro(nod), &mut refs); + self.expand_and_group_usages_file_wise(ctx, Definition::Macro(nod), &mut refs, &mut use_stmts_to_be_inserted); } }, _ => (), @@ -303,7 +313,7 @@ impl Module { } } - (refs, adt_fields) + (refs, adt_fields, use_stmts_to_be_inserted) } fn expand_and_group_usages_file_wise( @@ -311,20 +321,45 @@ impl Module { ctx: &AssistContext<'_>, node_def: Definition, refs_in_files: &mut FxHashMap>, + use_stmts_to_be_inserted: &mut FxHashMap, ) { let mod_name = self.name; + let covering_node = match ctx.covering_element() { + syntax::NodeOrToken::Node(node) => node, + syntax::NodeOrToken::Token(tok) => tok.parent().unwrap(), // won't panic + }; let out_of_sel = |node: &SyntaxNode| !self.text_range.contains_range(node.text_range()); + let mut use_stmts_set = FxHashSet::default(); for (file_id, refs) in node_def.usages(&ctx.sema).all() { let source_file = ctx.sema.parse(file_id); - let usages = refs.into_iter().filter_map(|FileReference { range, name, .. }| { + let usages = refs.into_iter().filter_map(|FileReference { range, .. }| { // handle normal usages let name_ref = find_node_at_range::(source_file.syntax(), range)?; - let name = name.syntax().to_string(); if out_of_sel(name_ref.syntax()) { let new_ref = format!("{mod_name}::{name_ref}"); - return Some((name_ref.syntax().text_range(), new_ref)); + return Some((range, new_ref)); + } else if let Some(use_) = name_ref.syntax().ancestors().find_map(ast::Use::cast) { + // handle usages in use_stmts which is in_sel + // check if `use` is top stmt in selection + if use_.syntax().parent().is_some_and(|parent| parent == covering_node) + && use_stmts_set.insert(use_.syntax().text_range().start()) + { + let use_ = use_stmts_to_be_inserted + .entry(use_.syntax().text_range().start()) + .or_insert_with(|| use_.clone_subtree().clone_for_update()); + for seg in use_ + .syntax() + .descendants() + .filter_map(ast::NameRef::cast) + .filter(|seg| seg.syntax().to_string() == name_ref.to_string()) + { + let new_ref = make::path_from_text(&format!("{mod_name}::{seg}")) + .clone_for_update(); + ted::replace(seg.syntax().parent()?, new_ref.syntax()); + } + } } None From d40c0fe48b1d0947db5b2ea18df92a52db068319 Mon Sep 17 00:00:00 2001 From: roife Date: Fri, 15 Mar 2024 21:05:04 +0800 Subject: [PATCH 8/9] test: add test for extract_module --- .../src/handlers/extract_module.rs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index b1e43ceb52..c6a1bd0838 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -1708,6 +1708,27 @@ fn main() { } "#, r#" +mod modname { + use Direction::{Horizontal, Vertical}; + + pub(crate) struct Point; + + impl Point { + pub const fn direction(self, other: Self) -> Option { + Some(Vertical) + } + } + + pub enum Direction { + Horizontal, + Vertical, + } +} +use modname::Direction::{Horizontal, Vertical}; + +fn main() { + let x = Vertical; +} "#, ); } From 10aa999c743038f26c14939749c72e73e22f7e49 Mon Sep 17 00:00:00 2001 From: roife Date: Fri, 15 Mar 2024 21:14:17 +0800 Subject: [PATCH 9/9] fix: typo --- crates/ide-assists/src/handlers/extract_module.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index c6a1bd0838..42f935651c 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -467,9 +467,9 @@ impl Module { .filter(|x| find_node_at_range::(file.syntax(), x.range).is_none()) .filter_map(|x| find_node_at_range::(file.syntax(), x.range)) { - let in_selectin = selection_range.contains_range(x.syntax().text_range()); - uses_exist_in_sel |= in_selectin; - uses_exist_out_sel |= !in_selectin; + let in_selection = selection_range.contains_range(x.syntax().text_range()); + uses_exist_in_sel |= in_selection; + uses_exist_out_sel |= !in_selection; if uses_exist_in_sel && uses_exist_out_sel { break 'outside;