From e8dc8ccc593e9972fc11c6d007083c0ab588a440 Mon Sep 17 00:00:00 2001 From: roife Date: Sat, 9 Dec 2023 17:01:30 +0800 Subject: [PATCH 1/4] fix: pick up new names when the name exists in 'introduce_named_generic' --- .../src/handlers/introduce_named_generic.rs | 20 +++++-- crates/ide-assists/src/utils/suggest_name.rs | 57 ++++++++++--------- 2 files changed, 45 insertions(+), 32 deletions(-) diff --git a/crates/ide-assists/src/handlers/introduce_named_generic.rs b/crates/ide-assists/src/handlers/introduce_named_generic.rs index e90a1ed79b4..abbdc5aee95 100644 --- a/crates/ide-assists/src/handlers/introduce_named_generic.rs +++ b/crates/ide-assists/src/handlers/introduce_named_generic.rs @@ -31,15 +31,16 @@ pub(crate) fn introduce_named_generic(acc: &mut Assists, ctx: &AssistContext<'_> |edit| { let impl_trait_type = edit.make_mut(impl_trait_type); let fn_ = edit.make_mut(fn_); - - let type_param_name = suggest_name::for_generic_parameter(&impl_trait_type); + let fn_generic_param_list = fn_.get_or_create_generic_param_list(); + let type_param_name = + suggest_name::for_generic_parameter(&impl_trait_type, &fn_generic_param_list); let type_param = make::type_param(make::name(&type_param_name), Some(type_bound_list)) .clone_for_update(); let new_ty = make::ty(&type_param_name).clone_for_update(); ted::replace(impl_trait_type.syntax(), new_ty.syntax()); - fn_.get_or_create_generic_param_list().add_generic_param(type_param.into()); + fn_generic_param_list.add_generic_param(type_param.into()); if let Some(cap) = ctx.config.snippet_cap { if let Some(generic_param) = @@ -111,12 +112,19 @@ fn foo<$0B: Bar #[test] fn replace_impl_trait_with_exist_generic_letter() { - // FIXME: This is wrong, we should pick a different name if the one we - // want is already bound. check_assist( introduce_named_generic, r#"fn foo(bar: $0impl Bar) {}"#, - r#"fn foo(bar: B) {}"#, + r#"fn foo(bar: B0) {}"#, + ); + } + + #[test] + fn replace_impl_trait_with_more_exist_generic_letter() { + check_assist( + introduce_named_generic, + r#"fn foo(bar: $0impl Bar) {}"#, + r#"fn foo(bar: B2) {}"#, ); } diff --git a/crates/ide-assists/src/utils/suggest_name.rs b/crates/ide-assists/src/utils/suggest_name.rs index 05d38117d43..ae3fd30b65c 100644 --- a/crates/ide-assists/src/utils/suggest_name.rs +++ b/crates/ide-assists/src/utils/suggest_name.rs @@ -58,38 +58,43 @@ const USELESS_METHODS: &[&str] = &[ "into_future", ]; -pub(crate) fn for_unique_generic_name( - name: &str, +pub(crate) fn for_generic_parameter( + ty: &ast::ImplTraitType, existing_params: &ast::GenericParamList, ) -> SmolStr { - let param_names = existing_params - .generic_params() - .map(|param| match param { - ast::GenericParam::TypeParam(t) => t.name().unwrap().to_string(), - p => p.to_string(), - }) - .collect_vec(); - let mut name = name.to_string(); - let base_len = name.len(); - // 4*len bytes for base, and 2 bytes for 2 digits - name.reserve(4 * base_len + 2); - - let mut count = 0; - while param_names.contains(&name) { - name.truncate(base_len); - name.push_str(&count.to_string()); - count += 1; - } - - name.into() -} - -pub(crate) fn for_generic_parameter(ty: &ast::ImplTraitType) -> SmolStr { let c = ty .type_bound_list() .and_then(|bounds| bounds.syntax().text().char_at(0.into())) .unwrap_or('T'); - c.encode_utf8(&mut [0; 4]).into() + + // let existing_params = existing_params.generic_params(); + let conflict = existing_params + .generic_params() + .filter(|param| { + param.syntax().text_range().len() == 1.into() + && param.syntax().text().char_at(0.into()).unwrap() == c + }) + .count() + > 0; + + let buffer = &mut [0; 4]; + if conflict { + let mut name = String::from(c.encode_utf8(buffer)); + name.reserve(6); // 4B for c, and 2B for 2 digits + let base_len = name.len(); + let mut count = 0; + loop { + name.truncate(base_len); + name.push_str(&count.to_string()); + if existing_params.generic_params().all(|param| param.to_string() != name) { + break; + } + count += 1; + } + SmolStr::from(name) + } else { + c.encode_utf8(buffer).into() + } } /// Suggest name of variable for given expression From 186553dab81c0b675cf41162c528c758e558d0ec Mon Sep 17 00:00:00 2001 From: roife Date: Sat, 9 Dec 2023 20:00:13 +0800 Subject: [PATCH 2/4] refactor: extracted the fn handling conflicts in generics and add docs * Extracted the function `for_unique_generic_name` that handling generics with identical names for reusability. * Renamed `for_generic_params` to `for_impl_trait_as_generic` for clarity * Added documentations for `for_impl_trait_as_generic` and `for_unique_generic_name` --- .../src/handlers/introduce_named_generic.rs | 2 +- crates/ide-assists/src/utils/suggest_name.rs | 72 +++++++++++-------- 2 files changed, 44 insertions(+), 30 deletions(-) diff --git a/crates/ide-assists/src/handlers/introduce_named_generic.rs b/crates/ide-assists/src/handlers/introduce_named_generic.rs index abbdc5aee95..b1daa7802ed 100644 --- a/crates/ide-assists/src/handlers/introduce_named_generic.rs +++ b/crates/ide-assists/src/handlers/introduce_named_generic.rs @@ -33,7 +33,7 @@ pub(crate) fn introduce_named_generic(acc: &mut Assists, ctx: &AssistContext<'_> let fn_ = edit.make_mut(fn_); let fn_generic_param_list = fn_.get_or_create_generic_param_list(); let type_param_name = - suggest_name::for_generic_parameter(&impl_trait_type, &fn_generic_param_list); + suggest_name::for_impl_trait_as_generic(&impl_trait_type, &fn_generic_param_list); let type_param = make::type_param(make::name(&type_param_name), Some(type_bound_list)) .clone_for_update(); diff --git a/crates/ide-assists/src/utils/suggest_name.rs b/crates/ide-assists/src/utils/suggest_name.rs index ae3fd30b65c..8a698e4068a 100644 --- a/crates/ide-assists/src/utils/suggest_name.rs +++ b/crates/ide-assists/src/utils/suggest_name.rs @@ -58,7 +58,48 @@ const USELESS_METHODS: &[&str] = &[ "into_future", ]; -pub(crate) fn for_generic_parameter( +/// Suggest a unique name for generic parameter. +/// +/// `existing_params` is used to check if the name conflicts with existing +/// generic parameters. +/// +/// The function checks if the name conflicts with existing generic parameters. +/// If so, it will try to resolve the conflict by adding a number suffix, e.g. +/// `T`, `T0`, `T1`, ... +pub(crate) fn for_unique_generic_name( + name: &str, + existing_params: &ast::GenericParamList, +) -> SmolStr { + let param_names = existing_params.generic_params().map(|param| param.to_string()).collect_vec(); + + let mut name = name.to_string(); + let base_len = name.len(); + // 4*len bytes for base, and 2 bytes for 2 digits + name.reserve(4 * base_len + 2); + + let mut count = 0; + while param_names.contains(&name) { + name.truncate(base_len); + name.push_str(&count.to_string()); + count += 1; + } + + name.into() +} + +/// Suggest name of impl trait type +/// +/// `existing_params` is used to check if the name conflicts with existing +/// generic parameters. +/// +/// # Current implementation +/// +/// In current implementation, the function tries to get the name from the first +/// character of the name for the first type bound. +/// +/// If the name conflicts with existing generic parameters, it will try to +/// resolve the conflict with `for_unique_generic_name`. +pub(crate) fn for_impl_trait_as_generic( ty: &ast::ImplTraitType, existing_params: &ast::GenericParamList, ) -> SmolStr { @@ -67,34 +108,7 @@ pub(crate) fn for_generic_parameter( .and_then(|bounds| bounds.syntax().text().char_at(0.into())) .unwrap_or('T'); - // let existing_params = existing_params.generic_params(); - let conflict = existing_params - .generic_params() - .filter(|param| { - param.syntax().text_range().len() == 1.into() - && param.syntax().text().char_at(0.into()).unwrap() == c - }) - .count() - > 0; - - let buffer = &mut [0; 4]; - if conflict { - let mut name = String::from(c.encode_utf8(buffer)); - name.reserve(6); // 4B for c, and 2B for 2 digits - let base_len = name.len(); - let mut count = 0; - loop { - name.truncate(base_len); - name.push_str(&count.to_string()); - if existing_params.generic_params().all(|param| param.to_string() != name) { - break; - } - count += 1; - } - SmolStr::from(name) - } else { - c.encode_utf8(buffer).into() - } + for_unique_generic_name(c.encode_utf8(&mut [0; 4]), existing_params) } /// Suggest name of variable for given expression From bc1a5774fd9fc1cbc2ffcb74d72e2894e2171d0b Mon Sep 17 00:00:00 2001 From: roife Date: Mon, 11 Dec 2023 00:53:44 +0800 Subject: [PATCH 3/4] fix: handle with type bounds in existing_params --- crates/ide-assists/src/utils/suggest_name.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/ide-assists/src/utils/suggest_name.rs b/crates/ide-assists/src/utils/suggest_name.rs index 8a698e4068a..455bbc0b681 100644 --- a/crates/ide-assists/src/utils/suggest_name.rs +++ b/crates/ide-assists/src/utils/suggest_name.rs @@ -70,8 +70,13 @@ pub(crate) fn for_unique_generic_name( name: &str, existing_params: &ast::GenericParamList, ) -> SmolStr { - let param_names = existing_params.generic_params().map(|param| param.to_string()).collect_vec(); - + let param_names = existing_params + .generic_params() + .map(|param| match param { + ast::GenericParam::TypeParam(t) => t.name().unwrap().to_string(), + p => p.to_string(), + }) + .collect_vec(); let mut name = name.to_string(); let base_len = name.len(); // 4*len bytes for base, and 2 bytes for 2 digits From 919ecc6c3232cad9dc8e3f6ea03082415c6609fb Mon Sep 17 00:00:00 2001 From: roife Date: Tue, 2 Jan 2024 21:33:06 +0800 Subject: [PATCH 4/4] Use HashSet to enhance performance in for_unique_generic_name in suggest_name --- crates/ide-assists/src/utils/suggest_name.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/ide-assists/src/utils/suggest_name.rs b/crates/ide-assists/src/utils/suggest_name.rs index 455bbc0b681..b4c6cbff2a4 100644 --- a/crates/ide-assists/src/utils/suggest_name.rs +++ b/crates/ide-assists/src/utils/suggest_name.rs @@ -1,5 +1,7 @@ //! This module contains functions to suggest names for expressions, functions and other items +use std::collections::HashSet; + use hir::Semantics; use ide_db::RootDatabase; use itertools::Itertools; @@ -76,12 +78,9 @@ pub(crate) fn for_unique_generic_name( ast::GenericParam::TypeParam(t) => t.name().unwrap().to_string(), p => p.to_string(), }) - .collect_vec(); + .collect::>(); let mut name = name.to_string(); let base_len = name.len(); - // 4*len bytes for base, and 2 bytes for 2 digits - name.reserve(4 * base_len + 2); - let mut count = 0; while param_names.contains(&name) { name.truncate(base_len);