From 59f8827a6fbd635d5314bec0f88284e8f3c603fd Mon Sep 17 00:00:00 2001 From: Sebastian Ziebell Date: Wed, 26 Apr 2023 16:13:58 +0200 Subject: [PATCH 1/4] Implement assist to replace named generic with impl This adds a new assist named "replace named generic with impl" to move the generic param type from the generic param list into the function signature. ```rust fn new(input: T) -> Self {} ``` becomes ```rust fn new(input: impl ToString) -> Self {} ``` The first step is to determine if the assist can be applied, there has to be a match between generic trait param & function paramter types. * replace function parameter type(s) with impl * add new `impl_trait_type` function to generate the new trait bounds with `impl` keyword for use in the function signature --- .../replace_named_generic_with_impl.rs | 143 ++++++++++++++++++ crates/ide-assists/src/lib.rs | 2 + crates/ide-assists/src/tests/generated.rs | 13 ++ crates/syntax/src/ast/make.rs | 4 + 4 files changed, 162 insertions(+) create mode 100644 crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs diff --git a/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs b/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs new file mode 100644 index 00000000000..bd73c7b9c9d --- /dev/null +++ b/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs @@ -0,0 +1,143 @@ +use syntax::{ + ast::{ + self, + make::{self, impl_trait_type}, + HasGenericParams, HasName, HasTypeBounds, + }, + ted, AstNode, +}; + +use crate::{AssistContext, AssistId, AssistKind, Assists}; + +// Assist: replace_named_generic_with_impl +// +// Replaces named generic with an `impl Trait` in function argument. +// +// ``` +// fn new>(location: P) -> Self {} +// ``` +// -> +// ``` +// fn new(location: impl AsRef) -> Self {} +// ``` +pub(crate) fn replace_named_generic_with_impl( + acc: &mut Assists, + ctx: &AssistContext<'_>, +) -> Option<()> { + // finds `>` + let type_param = ctx.find_node_at_offset::()?; + + // The list of type bounds / traits for generic name `P` + let type_bound_list = type_param.type_bound_list()?; + + // returns `P` + let type_param_name = type_param.name()?; + + let fn_ = type_param.syntax().ancestors().find_map(ast::Fn::cast)?; + let params = fn_ + .param_list()? + .params() + .filter_map(|param| { + // function parameter type needs to match generic type name + if let ast::Type::PathType(path_type) = param.ty()? { + let left = path_type.path()?.segment()?.name_ref()?.ident_token()?.to_string(); + let right = type_param_name.to_string(); + if left == right { + Some(param) + } else { + None + } + } else { + None + } + }) + .collect::>(); + + if params.is_empty() { + return None; + } + + let target = type_param.syntax().text_range(); + + acc.add( + AssistId("replace_named_generic_with_impl", AssistKind::RefactorRewrite), + "Replace named generic with impl", + target, + |edit| { + let type_param = edit.make_mut(type_param); + let fn_ = edit.make_mut(fn_); + + // Replace generic type in `>` to `

` + let new_ty = make::ty(&type_param_name.to_string()).clone_for_update(); + ted::replace(type_param.syntax(), new_ty.syntax()); + + if let Some(generic_params) = fn_.generic_param_list() { + if generic_params.generic_params().count() == 0 { + ted::remove(generic_params.syntax()); + } + } + + // Replace generic type parameter: `foo(p: P)` -> `foo(p: impl AsRef)` + let new_bounds = impl_trait_type(type_bound_list).clone_for_update(); + + for param in params { + if let Some(ast::Type::PathType(param_type)) = param.ty() { + let param_type = edit.make_mut(param_type).clone_for_update(); + ted::replace(param_type.syntax(), new_bounds.syntax()); + } + } + }, + ) +} + +#[cfg(test)] +mod tests { + use super::*; + + use crate::tests::check_assist; + + #[test] + fn replace_generic_moves_into_function() { + check_assist( + replace_named_generic_with_impl, + r#"fn new(input: T) -> Self {}"#, + r#"fn new(input: impl ToString) -> Self {}"#, + ); + } + + #[test] + fn replace_generic_with_inner_associated_type() { + check_assist( + replace_named_generic_with_impl, + r#"fn new>(input: P) -> Self {}"#, + r#"fn new(input: impl AsRef) -> Self {}"#, + ); + } + + #[test] + fn replace_generic_trait_applies_to_all_matching_params() { + check_assist( + replace_named_generic_with_impl, + r#"fn new(a: T, b: T) -> Self {}"#, + r#"fn new(a: impl ToString, b: impl ToString) -> Self {}"#, + ); + } + + #[test] + fn replace_generic_with_multiple_generic_names() { + check_assist( + replace_named_generic_with_impl, + r#"fn new, T$0: ToString>(t: T, p: P) -> Self {}"#, + r#"fn new>(t: impl ToString, p: P) -> Self {}"#, + ); + } + + #[test] + fn replace_generic_with_multiple_trait_bounds() { + check_assist( + replace_named_generic_with_impl, + r#"fn new(p: P) -> Self {}"#, + r#"fn new(p: impl Send + Sync) -> Self {}"#, + ); + } +} diff --git a/crates/ide-assists/src/lib.rs b/crates/ide-assists/src/lib.rs index fc03903e593..bd282e53434 100644 --- a/crates/ide-assists/src/lib.rs +++ b/crates/ide-assists/src/lib.rs @@ -193,6 +193,7 @@ mod handlers { mod replace_arith_op; mod introduce_named_generic; mod replace_let_with_if_let; + mod replace_named_generic_with_impl; mod replace_qualified_name_with_use; mod replace_string_with_char; mod replace_turbofish_with_explicit_type; @@ -299,6 +300,7 @@ mod handlers { replace_let_with_if_let::replace_let_with_if_let, replace_method_eager_lazy::replace_with_eager_method, replace_method_eager_lazy::replace_with_lazy_method, + replace_named_generic_with_impl::replace_named_generic_with_impl, replace_turbofish_with_explicit_type::replace_turbofish_with_explicit_type, replace_qualified_name_with_use::replace_qualified_name_with_use, replace_arith_op::replace_arith_with_wrapping, diff --git a/crates/ide-assists/src/tests/generated.rs b/crates/ide-assists/src/tests/generated.rs index 0096254ecb7..8a35fd290e6 100644 --- a/crates/ide-assists/src/tests/generated.rs +++ b/crates/ide-assists/src/tests/generated.rs @@ -2338,6 +2338,19 @@ fn handle(action: Action) { ) } +#[test] +fn doctest_replace_named_generic_with_impl() { + check_doc_test( + "replace_named_generic_with_impl", + r#####" +fn new>(location: P) -> Self {} +"#####, + r#####" +fn new(location: impl AsRef) -> Self {} +"#####, + ) +} + #[test] fn doctest_replace_qualified_name_with_use() { check_doc_test( diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index 8d2dc8709ba..3a61fb0a52c 100644 --- a/crates/syntax/src/ast/make.rs +++ b/crates/syntax/src/ast/make.rs @@ -232,6 +232,10 @@ pub fn impl_trait( ast_from_text(&format!("impl{ty_params_str} {trait_} for {ty}{ty_genargs_str} {{}}")) } +pub fn impl_trait_type(bounds: ast::TypeBoundList) -> ast::ImplTraitType { + ast_from_text(&format!("fn f(x: impl {bounds}) {{}}")) +} + pub fn path_segment(name_ref: ast::NameRef) -> ast::PathSegment { ast_from_text(&format!("type __ = {name_ref};")) } From 95f59668e65d53a985897af1c7ae4c2b449016ba Mon Sep 17 00:00:00 2001 From: Sebastian Ziebell Date: Mon, 15 May 2023 17:38:25 +0200 Subject: [PATCH 2/4] Fix removal of generic param from list This removes an existing generic param from the `GenericParamList`. It also considers to remove the extra colon & whitespace to the previous sibling. * change order to get all param types first and mark them as mutable before the first edit happens * add helper function to remove a generic parameter * fix test output --- .../replace_named_generic_with_impl.rs | 32 +++++++++---------- crates/syntax/src/ast/edit_in_place.rs | 15 +++++++++ 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs b/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs index bd73c7b9c9d..f658ba768d8 100644 --- a/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs +++ b/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs @@ -1,9 +1,5 @@ use syntax::{ - ast::{ - self, - make::{self, impl_trait_type}, - HasGenericParams, HasName, HasTypeBounds, - }, + ast::{self, make::impl_trait_type, HasGenericParams, HasName, HasTypeBounds}, ted, AstNode, }; @@ -27,7 +23,7 @@ pub(crate) fn replace_named_generic_with_impl( // finds `>` let type_param = ctx.find_node_at_offset::()?; - // The list of type bounds / traits for generic name `P` + // The list of type bounds / traits: `AsRef` let type_bound_list = type_param.type_bound_list()?; // returns `P` @@ -67,24 +63,26 @@ pub(crate) fn replace_named_generic_with_impl( let type_param = edit.make_mut(type_param); let fn_ = edit.make_mut(fn_); - // Replace generic type in `>` to `

` - let new_ty = make::ty(&type_param_name.to_string()).clone_for_update(); - ted::replace(type_param.syntax(), new_ty.syntax()); + // get all params + let param_types = params + .iter() + .filter_map(|param| match param.ty() { + Some(ast::Type::PathType(param_type)) => Some(edit.make_mut(param_type)), + _ => None, + }) + .collect::>(); if let Some(generic_params) = fn_.generic_param_list() { + generic_params.remove_generic_param(ast::GenericParam::TypeParam(type_param)); if generic_params.generic_params().count() == 0 { ted::remove(generic_params.syntax()); } } - // Replace generic type parameter: `foo(p: P)` -> `foo(p: impl AsRef)` - let new_bounds = impl_trait_type(type_bound_list).clone_for_update(); - - for param in params { - if let Some(ast::Type::PathType(param_type)) = param.ty() { - let param_type = edit.make_mut(param_type).clone_for_update(); - ted::replace(param_type.syntax(), new_bounds.syntax()); - } + // get type bounds in signature type: `P` -> `impl AsRef` + let new_bounds = impl_trait_type(type_bound_list); + for param_type in param_types.iter().rev() { + ted::replace(param_type.syntax(), new_bounds.clone_for_update().syntax()); } }, ) diff --git a/crates/syntax/src/ast/edit_in_place.rs b/crates/syntax/src/ast/edit_in_place.rs index 995e8d8d152..b3ea6ca8d46 100644 --- a/crates/syntax/src/ast/edit_in_place.rs +++ b/crates/syntax/src/ast/edit_in_place.rs @@ -236,6 +236,21 @@ impl ast::GenericParamList { } } + /// Removes the existing generic param + pub fn remove_generic_param(&self, generic_param: ast::GenericParam) { + if let Some(previous) = generic_param.syntax().prev_sibling() { + if let Some(next_token) = previous.next_sibling_or_token() { + ted::remove_all(next_token..=generic_param.syntax().clone().into()); + } + } else if let Some(next) = generic_param.syntax().next_sibling() { + if let Some(next_token) = next.prev_sibling_or_token() { + ted::remove_all(generic_param.syntax().clone().into()..=next_token); + } + } else { + ted::remove(generic_param.syntax()); + } + } + /// Constructs a matching [`ast::GenericArgList`] pub fn to_generic_args(&self) -> ast::GenericArgList { let args = self.generic_params().filter_map(|param| match param { From ce1c85317f80b96b5fdc65fa2e76d4150f26abfa Mon Sep 17 00:00:00 2001 From: Sebastian Ziebell Date: Fri, 26 May 2023 11:59:44 +0200 Subject: [PATCH 3/4] Check param is not referenced in function This checks the type param is referenced neither in the function body nor as a return type. * add tests --- .../replace_named_generic_with_impl.rs | 87 +++++++++++++++++-- 1 file changed, 82 insertions(+), 5 deletions(-) diff --git a/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs b/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs index f658ba768d8..4e7011b5ca3 100644 --- a/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs +++ b/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs @@ -1,3 +1,10 @@ +use hir::{Semantics, TypeParam}; +use ide_db::{ + base_db::{FileId, FileRange}, + defs::Definition, + search::SearchScope, + RootDatabase, +}; use syntax::{ ast::{self, make::impl_trait_type, HasGenericParams, HasName, HasTypeBounds}, ted, AstNode, @@ -22,13 +29,12 @@ pub(crate) fn replace_named_generic_with_impl( ) -> Option<()> { // finds `>` let type_param = ctx.find_node_at_offset::()?; + // returns `P` + let type_param_name = type_param.name()?; // The list of type bounds / traits: `AsRef` let type_bound_list = type_param.type_bound_list()?; - // returns `P` - let type_param_name = type_param.name()?; - let fn_ = type_param.syntax().ancestors().find_map(ast::Fn::cast)?; let params = fn_ .param_list()? @@ -53,6 +59,11 @@ pub(crate) fn replace_named_generic_with_impl( return None; } + let type_param_hir_def = ctx.sema.to_def(&type_param)?; + if is_referenced_outside(ctx.db(), type_param_hir_def, &fn_, ctx.file_id()) { + return None; + } + let target = type_param.syntax().text_range(); acc.add( @@ -88,11 +99,36 @@ pub(crate) fn replace_named_generic_with_impl( ) } +fn is_referenced_outside( + db: &RootDatabase, + type_param: TypeParam, + fn_: &ast::Fn, + file_id: FileId, +) -> bool { + let semantics = Semantics::new(db); + let type_param_def = Definition::GenericParam(hir::GenericParam::TypeParam(type_param)); + + // limit search scope to function body & return type + let search_ranges = vec![ + fn_.body().map(|body| body.syntax().text_range()), + fn_.ret_type().map(|ret_type| ret_type.syntax().text_range()), + ]; + + search_ranges.into_iter().filter_map(|search_range| search_range).any(|search_range| { + let file_range = FileRange { file_id, range: search_range }; + !type_param_def + .usages(&semantics) + .in_scope(SearchScope::file_range(file_range)) + .all() + .is_empty() + }) +} + #[cfg(test)] mod tests { use super::*; - use crate::tests::check_assist; + use crate::tests::{check_assist, check_assist_not_applicable}; #[test] fn replace_generic_moves_into_function() { @@ -122,12 +158,22 @@ mod tests { } #[test] - fn replace_generic_with_multiple_generic_names() { + fn replace_generic_with_multiple_generic_params() { check_assist( replace_named_generic_with_impl, r#"fn new, T$0: ToString>(t: T, p: P) -> Self {}"#, r#"fn new>(t: impl ToString, p: P) -> Self {}"#, ); + check_assist( + replace_named_generic_with_impl, + r#"fn new>(t: T, p: P) -> Self {}"#, + r#"fn new>(t: impl ToString, p: P) -> Self {}"#, + ); + check_assist( + replace_named_generic_with_impl, + r#"fn new(a: A, b: B, c: C) -> Self {}"#, + r#"fn new(a: A, b: impl ToString, c: C) -> Self {}"#, + ); } #[test] @@ -138,4 +184,35 @@ mod tests { r#"fn new(p: impl Send + Sync) -> Self {}"#, ); } + + #[test] + fn replace_generic_not_applicable_if_param_used_as_return_type() { + check_assist_not_applicable( + replace_named_generic_with_impl, + r#"fn new(p: P) -> P {}"#, + ); + } + + #[test] + fn replace_generic_not_applicable_if_param_used_in_fn_body() { + check_assist_not_applicable( + replace_named_generic_with_impl, + r#"fn new(p: P) { let x: &dyn P = &O; }"#, + ); + } + + #[test] + fn replace_generic_ignores_another_function_with_same_param_type() { + check_assist( + replace_named_generic_with_impl, + r#" + fn new(p: P) {} + fn hello(p: P) { println!("{:?}", p); } + "#, + r#" + fn new(p: impl Send + Sync) {} + fn hello(p: P) { println!("{:?}", p); } + "#, + ); + } } From e78df83e2fb89b475187f7d4b631a68619b590b5 Mon Sep 17 00:00:00 2001 From: Sebastian Ziebell Date: Fri, 26 May 2023 13:43:12 +0200 Subject: [PATCH 4/4] Integrate feedback * pass in existing `Semantics` object to function * pass in `Definition` for param type * refactor iterator to use `flatten` --- .../replace_named_generic_with_impl.rs | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs b/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs index 4e7011b5ca3..f32ceb42654 100644 --- a/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs +++ b/crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs @@ -1,4 +1,4 @@ -use hir::{Semantics, TypeParam}; +use hir::Semantics; use ide_db::{ base_db::{FileId, FileRange}, defs::Definition, @@ -60,7 +60,9 @@ pub(crate) fn replace_named_generic_with_impl( } let type_param_hir_def = ctx.sema.to_def(&type_param)?; - if is_referenced_outside(ctx.db(), type_param_hir_def, &fn_, ctx.file_id()) { + let type_param_def = Definition::GenericParam(hir::GenericParam::TypeParam(type_param_hir_def)); + + if is_referenced_outside(&ctx.sema, type_param_def, &fn_, ctx.file_id()) { return None; } @@ -100,27 +102,20 @@ pub(crate) fn replace_named_generic_with_impl( } fn is_referenced_outside( - db: &RootDatabase, - type_param: TypeParam, + sema: &Semantics<'_, RootDatabase>, + type_param_def: Definition, fn_: &ast::Fn, file_id: FileId, ) -> bool { - let semantics = Semantics::new(db); - let type_param_def = Definition::GenericParam(hir::GenericParam::TypeParam(type_param)); - // limit search scope to function body & return type let search_ranges = vec![ fn_.body().map(|body| body.syntax().text_range()), fn_.ret_type().map(|ret_type| ret_type.syntax().text_range()), ]; - search_ranges.into_iter().filter_map(|search_range| search_range).any(|search_range| { + search_ranges.into_iter().flatten().any(|search_range| { let file_range = FileRange { file_id, range: search_range }; - !type_param_def - .usages(&semantics) - .in_scope(SearchScope::file_range(file_range)) - .all() - .is_empty() + !type_param_def.usages(sema).in_scope(SearchScope::file_range(file_range)).all().is_empty() }) }