From b63be3f1162ea645366c3259e40f7457efa7bd9c Mon Sep 17 00:00:00 2001 From: Hegui Dai Date: Fri, 21 Nov 2025 10:21:38 +0800 Subject: [PATCH] make add_missing_impl_members can handle naming conflicts --- .../src/handlers/add_missing_impl_members.rs | 359 ++++++++++++++++-- crates/ide-assists/src/utils.rs | 230 +++++++++-- 2 files changed, 523 insertions(+), 66 deletions(-) diff --git a/crates/ide-assists/src/handlers/add_missing_impl_members.rs b/crates/ide-assists/src/handlers/add_missing_impl_members.rs index e970bb7167d1..e7e29f867f88 100644 --- a/crates/ide-assists/src/handlers/add_missing_impl_members.rs +++ b/crates/ide-assists/src/handlers/add_missing_impl_members.rs @@ -1,6 +1,6 @@ use hir::HasSource; use syntax::{ - Edition, + Edition, ToSmolStr, ast::{self, AstNode, make}, syntax_editor::{Position, SyntaxEditor}, }; @@ -8,10 +8,7 @@ use syntax::{ use crate::{ AssistId, assist_context::{AssistContext, Assists}, - utils::{ - DefaultMethods, IgnoreAssocItems, add_trait_assoc_items_to_impl, filter_assoc_items, - gen_trait_fn_body, - }, + utils::{DefaultMethods, IgnoreAssocItems}, }; // Assist: add_impl_missing_members @@ -105,13 +102,12 @@ fn add_missing_impl_members_inner( acc: &mut Assists, ctx: &AssistContext<'_>, mode: DefaultMethods, - ignore_items: IgnoreAssocItems, + mut ignore_items: IgnoreAssocItems, assist_id: &'static str, label: &'static str, ) -> Option<()> { let _p = tracing::info_span!("add_missing_impl_members_inner").entered(); let impl_def = ctx.find_node_at_offset::()?; - let impl_ = ctx.sema.to_def(&impl_def)?; if ctx.token_at_offset().all(|t| { t.parent_ancestors() @@ -121,34 +117,30 @@ fn add_missing_impl_members_inner( return None; } - let target_scope = ctx.sema.scope(impl_def.syntax())?; + let impl_ = ctx.sema.to_def(&impl_def)?; let trait_ref = impl_.trait_ref(ctx.db())?; let trait_ = trait_ref.trait_(); + let db = ctx.db(); - let mut ign_item = ignore_items; - - if let IgnoreAssocItems::DocHiddenAttrPresent = ignore_items { - // Relax condition for local crates. - let db = ctx.db(); - if trait_.module(db).krate().origin(db).is_local() { - ign_item = IgnoreAssocItems::No; - } + if trait_.module(db).krate().origin(db).is_local() { + ignore_items = IgnoreAssocItems::No; } - let missing_items = filter_assoc_items( + let missing_items = crate::utils::filter_assoc_items( &ctx.sema, &ide_db::traits::get_missing_assoc_items(&ctx.sema, &impl_def), mode, - ign_item, + ignore_items, ); if missing_items.is_empty() { return None; } - let target = impl_def.syntax().text_range(); - acc.add(AssistId::quick_fix(assist_id), label, target, |edit| { - let new_item = add_trait_assoc_items_to_impl( + let target_scope = ctx.sema.scope(impl_def.syntax())?; + + acc.add(AssistId::quick_fix(assist_id), label, impl_def.syntax().text_range(), |edit| { + let new_items = crate::utils::add_trait_assoc_items_to_impl( &ctx.sema, ctx.config, &missing_items, @@ -157,7 +149,7 @@ fn add_missing_impl_members_inner( &target_scope, ); - let Some((first_new_item, other_items)) = new_item.split_first() else { + let Some((first_new_item, other_items)) = new_items.split_first() else { return; }; @@ -179,26 +171,25 @@ fn add_missing_impl_members_inner( Some(first_new_item.clone()) }; - let new_assoc_items = first_new_item - .clone() - .into_iter() - .chain(other_items.iter().cloned()) - .collect::>(); + let new_assoc_items: Vec = + first_new_item.clone().into_iter().chain(other_items.iter().cloned()).collect(); let mut editor = edit.make_editor(impl_def.syntax()); + if let Some(assoc_item_list) = impl_def.assoc_item_list() { assoc_item_list.add_items(&mut editor, new_assoc_items); } else { let assoc_item_list = make::assoc_item_list(Some(new_assoc_items)).clone_for_update(); editor.insert_all( Position::after(impl_def.syntax()), - vec![make::tokens::whitespace(" ").into(), assoc_item_list.syntax().clone().into()], + vec![make::tokens::single_space().into(), assoc_item_list.syntax().clone().into()], ); first_new_item = assoc_item_list.assoc_items().next(); } if let Some(cap) = ctx.config.snippet_cap { let mut placeholder = None; + if let DefaultMethods::No = mode && let Some(ast::AssocItem::Fn(func)) = &first_new_item && let Some(m) = func.syntax().descendants().find_map(ast::MacroCall::cast) @@ -215,6 +206,7 @@ fn add_missing_impl_members_inner( editor.add_annotation(first_new_item.syntax(), tabstop); }; }; + edit.add_file_edits(ctx.vfs_file_id(), editor); }) } @@ -226,12 +218,12 @@ fn try_gen_trait_body( impl_def: &ast::Impl, edition: Edition, ) -> Option { - let trait_path = make::ext::ident_path( - &trait_ref.trait_().name(ctx.db()).display(ctx.db(), edition).to_string(), - ); + let db = ctx.db(); + let trait_path = + make::ext::ident_path(&trait_ref.trait_().name(db).display(db, edition).to_smolstr()); let hir_ty = ctx.sema.resolve_type(&impl_def.self_ty()?)?; - let adt = hir_ty.as_adt()?.source(ctx.db())?; - gen_trait_fn_body(func, &trait_path, &adt.value, Some(trait_ref)) + let adt = hir_ty.as_adt()?.source(db)?; + crate::utils::gen_trait_fn_body(func, &trait_path, &adt.value, Some(trait_ref)) } #[cfg(test)] @@ -2534,6 +2526,307 @@ impl Test for () { ${0:todo!()} } } +"#, + ); + } + + #[test] + fn test_single_generic_name_collision() { + check_assist( + add_missing_impl_members, + r#" +trait Foo { fn bar>(iter: T) -> Self; } +impl Foo for () { + $0 +}"#, + r#" +trait Foo { fn bar>(iter: T) -> Self; } +impl Foo for () { + fn bar>(iter: T1) -> Self { + ${0:todo!()} + } +}"#, + ); + } + + #[test] + fn test_multiple_generic_name_collisions1() { + check_assist( + add_missing_impl_members, + r#" +trait Foo { + fn bar, U: IntoIterator>(iter: T, iter2: U) -> Self; +} +impl Foo for () { + $0 +} +"#, + r#" +trait Foo { + fn bar, U: IntoIterator>(iter: T, iter2: U) -> Self; +} +impl Foo for () { + fn bar, U1: IntoIterator>(iter: T1, iter2: U1) -> Self { + ${0:todo!()} + } +} +"#, + ); + } + + #[test] + fn test_multiple_generic_name_collisions2() { + check_assist( + add_missing_impl_members, + r#" +trait Foo { + fn bar, U: IntoIterator>(iter: T, iter2: U) -> Self; +} +impl Foo for () { + $0 +} +"#, + r#" +trait Foo { + fn bar, U: IntoIterator>(iter: T, iter2: U) -> Self; +} +impl Foo for () { + fn bar, U: IntoIterator>(iter: T1, iter2: U) -> Self { + ${0:todo!()} + } +} +"#, + ); + } + + #[test] + fn test_generic_name_collision_with_existing_renames() { + check_assist( + add_missing_impl_members, + r#" +trait Foo { + fn bar>(iter: T) -> Self; +} +impl Foo for () { + $0 +} +"#, + r#" +trait Foo { + fn bar>(iter: T) -> Self; +} +impl Foo for () { + fn bar>(iter: T2) -> Self { + ${0:todo!()} + } +} +"#, + ); + } + + #[test] + fn test_single_const_generic_name_collision() { + check_assist( + add_missing_default_members, + r#" +trait Foo { + fn bar() -> usize { + M + } +} +impl Foo for () { + $0 +} +"#, + r#" +trait Foo { + fn bar() -> usize { + M + } +} +impl Foo for () { + $0fn bar() -> usize { + N + } +} +"#, + ); + } + + #[test] + fn test_multiple_const_generic_name_collisions1() { + check_assist( + add_missing_impl_members, + r#" +trait Foo { + fn bar() -> usize; +} +impl Foo for () { + $0 +} +"#, + r#" +trait Foo { + fn bar() -> usize; +} +impl Foo for () { + fn bar() -> usize { + ${0:todo!()} + } +} +"#, + ); + } + + #[test] + fn test_mixed_generic_collisions() { + check_assist( + add_missing_impl_members, + r#" +trait Foo { + fn bar( + a: A, b: B, t: T, u: U, + ac: [A; C], bn: [B; N], mc: [T; M], ud: [U; D], + ); +} +impl Foo for () { + $0 +} +"#, + r#" +trait Foo { + fn bar( + a: A, b: B, t: T, u: U, + ac: [A; C], bn: [B; N], mc: [T; M], ud: [U; D], + ); +} +impl Foo for () { + fn bar( + a: A, b: U, t: T, u: U1, + ac: [A; C], bn: [U; N1], mc: [T; M], ud: [U1; N], + ) { + ${0:todo!()} + } +} +"#, + ); + } + + #[test] + fn test_single_lifetime_collision() { + check_assist( + add_missing_impl_members, + r#" +trait Foo<'b> { + fn bar<'a>(&'a self, other: &'b i32); +} +impl<'a> Foo<'a> for () { + $0 +} +"#, + r#" +trait Foo<'b> { + fn bar<'a>(&'a self, other: &'b i32); +} +impl<'a> Foo<'a> for () { + fn bar<'a1>(&'a1 self, other: &'a i32) { + ${0:todo!()} + } +} +"#, + ); + } + + #[test] + fn test_multiple_lifetime_collisions() { + check_assist( + add_missing_impl_members, + r#" +trait Foo<'m, 'n> { + fn bar<'a, 'b>(&'a self, other: &'b i32); +} +impl<'a, 'b> Foo<'a, 'b> for () { + $0 +} +"#, + r#" +trait Foo<'m, 'n> { + fn bar<'a, 'b>(&'a self, other: &'b i32); +} +impl<'a, 'b> Foo<'a, 'b> for () { + fn bar<'a1, 'b1>(&'a1 self, other: &'b1 i32) { + ${0:todo!()} + } +} +"#, + ); + } + + #[test] + fn test_all_kinds_of_collisions() { + check_assist( + add_missing_impl_members, + r#" +trait Foo<'g, 'h, A, B, const C: usize, const D: usize> { + fn bar<'j, 'k, T, U, const M: usize, const N: usize>( + a: A, b: B, t: T, u: U, + ac: [A; C], bn: [B; N], mc: [T; M], ud: [U; D], + life1: &'g str, life2: &'h str, life3: &'j str, life4: &'k str, + ); +} +impl<'g, 'k, A, U, const C: usize, const N: usize> Foo<'g, 'k, A, U, C, N> for () { + $0 +} +"#, + r#" +trait Foo<'g, 'h, A, B, const C: usize, const D: usize> { + fn bar<'j, 'k, T, U, const M: usize, const N: usize>( + a: A, b: B, t: T, u: U, + ac: [A; C], bn: [B; N], mc: [T; M], ud: [U; D], + life1: &'g str, life2: &'h str, life3: &'j str, life4: &'k str, + ); +} +impl<'g, 'k, A, U, const C: usize, const N: usize> Foo<'g, 'k, A, U, C, N> for () { + fn bar<'j, 'k1, T, U1, const M: usize, const N1: usize>( + a: A, b: U, t: T, u: U1, + ac: [A; C], bn: [U; N1], mc: [T; M], ud: [U1; N], + life1: &'g str, life2: &'k str, life3: &'j str, life4: &'k1 str, + ) { + ${0:todo!()} + } +} +"#, + ); + } + + #[test] + fn test_default_name_collisions() { + check_assist( + add_missing_default_members, + r#" +trait Foo<'g, 'h, A, B, const C: usize, const D: usize> { + fn complex<'j, 'k, T, U>(x: &'j T, y: &'k U) -> Box &'k U> + where + T: 'j, U: 'h, 'g: 'j, 'j: 'g, + { Box::new(move |z: &'g T| -> &'k U { unimplemented!() }) } +} +impl<'g, 'k, A, U, const C: usize, const N: usize> Foo<'g, 'k, A, U, C, N> for () { + $0 +} +"#, + r#" +trait Foo<'g, 'h, A, B, const C: usize, const D: usize> { + fn complex<'j, 'k, T, U>(x: &'j T, y: &'k U) -> Box &'k U> + where + T: 'j, U: 'h, 'g: 'j, 'j: 'g, + { Box::new(move |z: &'g T| -> &'k U { unimplemented!() }) } +} +impl<'g, 'k, A, U, const C: usize, const N: usize> Foo<'g, 'k, A, U, C, N> for () { + $0fn complex<'j, 'k1, T, U1>(x: &'j T, y: &'k1 U1) -> Box &'k1 U1> + where + T: 'j, U1: 'k, 'g: 'j, 'j: 'g, + { Box::new(move |z: &'g T| -> &'k1 U1 { unimplemented!() }) } +} "#, ); } diff --git a/crates/ide-assists/src/utils.rs b/crates/ide-assists/src/utils.rs index a00af9262668..14236fd66f22 100644 --- a/crates/ide-assists/src/utils.rs +++ b/crates/ide-assists/src/utils.rs @@ -9,7 +9,7 @@ use hir::{ db::{ExpandDatabase, HirDatabase}, }; use ide_db::{ - RootDatabase, + FxHashMap, FxHashSet, RootDatabase, assists::ExprFillDefaultMode, famous_defs::FamousDefs, path_transform::PathTransform, @@ -17,9 +17,9 @@ use ide_db::{ }; use stdx::format_to; use syntax::{ - AstNode, AstToken, Direction, NodeOrToken, SourceFile, + AstNode, AstToken, Direction, NodeOrToken, SmolStr, SourceFile, SyntaxKind::*, - SyntaxNode, SyntaxToken, T, TextRange, TextSize, WalkEvent, + SyntaxNode, SyntaxToken, T, TextRange, TextSize, ToSmolStr, WalkEvent, ast::{ self, HasArgList, HasAttrs, HasGenericParams, HasName, HasTypeBounds, Whitespace, edit::{AstNodeEdit, IndentLevel}, @@ -27,6 +27,7 @@ use syntax::{ make, syntax_factory::SyntaxFactory, }, + format_smolstr, syntax_editor::{Removable, SyntaxEditor}, }; @@ -195,40 +196,104 @@ pub fn add_trait_assoc_items_to_impl( impl_: &ast::Impl, target_scope: &hir::SemanticsScope<'_>, ) -> Vec { + use ast::GenericParam::*; + let new_indent_level = IndentLevel::from_node(impl_.syntax()) + 1; - original_items + return original_items .iter() .map(|InFile { file_id, value: original_item }| { - let mut cloned_item = { - if let Some(macro_file) = file_id.macro_file() { - let span_map = sema.db.expansion_span_map(macro_file); - let item_prettified = prettify_macro_expansion( - sema.db, - original_item.syntax().clone(), - &span_map, - target_scope.krate().into(), - ); - if let Some(formatted) = ast::AssocItem::cast(item_prettified) { - return formatted; - } else { - stdx::never!("formatted `AssocItem` could not be cast back to `AssocItem`"); + clone_and_update_assoc_item(sema, file_id, original_item, trait_, impl_, target_scope) + }) + .filter_map(|item| format_assoc_item(config, item)) + .map(|item| AstNodeEdit::indent(&item, new_indent_level)) + .collect(); + + fn clone_and_update_assoc_item( + sema: &Semantics<'_, RootDatabase>, + file_id: &hir::HirFileId, + original_item: &ast::AssocItem, + trait_: hir::Trait, + impl_: &ast::Impl, + target_scope: &hir::SemanticsScope<'_>, + ) -> ast::AssocItem { + let mut cloned_item = { + if let Some(macro_file) = file_id.macro_file() { + let span_map = sema.db.expansion_span_map(macro_file); + let item_prettified = prettify_macro_expansion( + sema.db, + original_item.syntax().clone(), + &span_map, + target_scope.krate().into(), + ); + if let Some(formatted) = ast::AssocItem::cast(item_prettified) { + return formatted; + } else { + stdx::never!("formatted `AssocItem` could not be cast back to `AssocItem`"); + } + } + original_item + } + .reset_indent(); + + if let Some(source_scope) = sema.scope(original_item.syntax()) { + let mut rename_map = FxHashMap::default(); + let impl_generic_param_names: FxHashSet = impl_ + .generic_param_list() + .into_iter() + .flat_map(|gen_param_list| gen_param_list.generic_params()) + .filter_map(|gen_param| extract_generic_param_name(&gen_param)) + .collect(); + + if let ast::AssocItem::Fn(fn_) = original_item + && let Some(assoc_fn_gen_param_list) = fn_.generic_param_list() + { + let item_params: FxHashSet = assoc_fn_gen_param_list + .generic_params() + .filter_map(|gen_param| extract_generic_param_name(&gen_param)) + .collect(); + + for gen_param in assoc_fn_gen_param_list.generic_params() { + if let Some(param_name) = extract_generic_param_name(&gen_param) + && impl_generic_param_names.contains(¶m_name) + { + let mut new_param_name = param_name.clone(); + let mut i = 1; + while impl_generic_param_names.contains(&new_param_name) + || item_params.contains(&new_param_name) + { + new_param_name = format_smolstr!("{param_name}{i}"); + i += 1; + } + + let origin_generic_param = match &gen_param { + TypeParam(it) => sema.to_def(it).map(hir::GenericParam::TypeParam), + ConstParam(it) => sema.to_def(it).map(hir::GenericParam::ConstParam), + LifetimeParam(it) => { + sema.to_def(it).map(hir::GenericParam::LifetimeParam) + } + }; + origin_generic_param.map(|it| rename_map.insert(it, new_param_name)); } } - original_item } - .reset_indent(); - - if let Some(source_scope) = sema.scope(original_item.syntax()) { - // FIXME: Paths in nested macros are not handled well. See - // `add_missing_impl_members::paths_in_nested_macro_should_get_transformed` test. - let transform = - PathTransform::trait_impl(target_scope, &source_scope, trait_, impl_.clone()); - cloned_item = ast::AssocItem::cast(transform.apply(cloned_item.syntax())).unwrap(); + + if !rename_map.is_empty() { + cloned_item = + apply_generic_param_rename(cloned_item, original_item, &rename_map, sema); } - cloned_item.remove_attrs_and_docs(); - cloned_item - }) - .filter_map(|item| match item { + + // FIXME: Paths in nested macros are not handled well. See + // `add_missing_impl_members::paths_in_nested_macro_should_get_transformed` test. + let transform = + PathTransform::trait_impl(target_scope, &source_scope, trait_, impl_.clone()); + cloned_item = ast::AssocItem::cast(transform.apply(cloned_item.syntax())).unwrap(); + } + cloned_item.remove_attrs_and_docs(); + cloned_item + } + + fn format_assoc_item(config: &AssistConfig, item: ast::AssocItem) -> Option { + match item { ast::AssocItem::Fn(fn_) if fn_.body().is_none() => { let fn_ = fn_.clone_subtree(); let new_body = &make::block_expr( @@ -257,9 +322,108 @@ pub fn add_trait_assoc_items_to_impl( } } item => Some(item), - }) - .map(|item| AstNodeEdit::indent(&item, new_indent_level)) - .collect() + } + } + + fn extract_generic_param_name(param: &ast::GenericParam) -> Option { + match param { + TypeParam(it) => it.name().map(|name| name.to_smolstr()), + ConstParam(it) => it.name().map(|name| name.to_smolstr()), + LifetimeParam(it) => it + .lifetime() + .and_then(|it| it.lifetime_ident_token()) + .map(|token| token.to_smolstr()), + } + } + + fn apply_generic_param_rename( + cloned_item: ast::AssocItem, + original_item: &ast::AssocItem, + rename_map: &FxHashMap, + sema: &Semantics<'_, RootDatabase>, + ) -> ast::AssocItem { + let match_root = cloned_item.syntax().clone_subtree(); + let mut editor = SyntaxEditor::new(match_root.clone()); + rename_recursive(original_item.syntax(), &match_root, &mut editor, rename_map, sema); + ast::AssocItem::cast(editor.finish().new_root().clone()).unwrap() + } + + fn rename_recursive( + original_node: &SyntaxNode, + cloned_node: &SyntaxNode, + editor: &mut SyntaxEditor, + rename_map: &FxHashMap, + sema: &Semantics<'_, RootDatabase>, + ) { + if let Some(gen_param) = ast::GenericParam::cast(original_node.clone()) + && let Some(origin_gen_param) = match &gen_param { + TypeParam(it) => sema.to_def(it).map(hir::GenericParam::TypeParam), + ConstParam(it) => sema.to_def(it).map(hir::GenericParam::ConstParam), + LifetimeParam(it) => sema.to_def(it).map(hir::GenericParam::LifetimeParam), + } + && let Some(new_name) = rename_map.get(&origin_gen_param) + && let Some(cloned_gen_param) = ast::GenericParam::cast(cloned_node.clone()) + { + return match cloned_gen_param { + TypeParam(it) => { + if let Some(name) = it.name() { + editor.replace( + name.syntax().clone(), + make::name(new_name).syntax().clone_for_update(), + ); + } + } + ConstParam(it) => { + if let Some(name) = it.name() { + editor.replace( + name.syntax().clone(), + make::name(new_name).syntax().clone_for_update(), + ); + } + } + LifetimeParam(it) => { + if let Some(lifetime) = it.lifetime() { + editor.replace( + lifetime.syntax().clone(), + make::lifetime(new_name).syntax().clone_for_update(), + ); + } + } + }; + } + + if let Some(path) = ast::Path::cast(original_node.clone()) + && let Some(path_resolution) = sema.resolve_path(&path) + && let Some(origin_gen_param) = match path_resolution { + PathResolution::TypeParam(it) => Some(hir::GenericParam::TypeParam(it)), + PathResolution::ConstParam(it) => Some(hir::GenericParam::ConstParam(it)), + _ => None, + } + && let Some(new_name) = rename_map.get(&origin_gen_param) + && let Some(cloned_path) = ast::Path::cast(cloned_node.clone()) + { + return editor.replace( + cloned_path.syntax().clone(), + make::ext::ident_path(new_name).syntax().clone_for_update(), + ); + } + + if let Some(lifetime) = ast::Lifetime::cast(original_node.clone()) + && let Some(lifetime_param) = sema.resolve_lifetime_param(&lifetime) + && let origin_gen_param = hir::GenericParam::LifetimeParam(lifetime_param) + && let Some(new_name) = rename_map.get(&origin_gen_param) + && let Some(cloned_lifetime) = ast::Lifetime::cast(cloned_node.clone()) + { + return editor.replace( + cloned_lifetime.syntax().clone(), + make::lifetime(new_name).syntax().clone_for_update(), + ); + } + + for (child_origin, child_cloned) in original_node.children().zip(cloned_node.children()) { + rename_recursive(&child_origin, &child_cloned, editor, rename_map, sema); + } + } } pub(crate) fn vis_offset(node: &SyntaxNode) -> TextSize {