Skip to content

feat: Assist to replace generic with impl trait #14816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
213 changes: 213 additions & 0 deletions crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
use hir::Semantics;
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,
};

use crate::{AssistContext, AssistId, AssistKind, Assists};

// Assist: replace_named_generic_with_impl
//
// Replaces named generic with an `impl Trait` in function argument.
//
// ```
// fn new<P$0: AsRef<Path>>(location: P) -> Self {}
// ```
// ->
// ```
// fn new(location: impl AsRef<Path>) -> Self {}
// ```
pub(crate) fn replace_named_generic_with_impl(
acc: &mut Assists,
ctx: &AssistContext<'_>,
) -> Option<()> {
// finds `<P: AsRef<Path>>`
let type_param = ctx.find_node_at_offset::<ast::TypeParam>()?;
// returns `P`
let type_param_name = type_param.name()?;

// The list of type bounds / traits: `AsRef<Path>`
let type_bound_list = type_param.type_bound_list()?;

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::<Vec<_>>();
Comment on lines +39 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken this fails to consider params whose type contains P in their generic arguments, like:

fn foo<P: Trait>(
    _: P, // this will be replaced
    _: Option<P>, // this and following `P` would dangle after this assist
    _: impl Iterator<Item = P>,
    _: &dyn Iterator<Item = P>,
) {}

Either also collect them and replace the generic params in them, or we can just skip doing this (by not providing this assist if there are such params) for this initial implementation.

If I've missed things and they are actually supported, could you add some tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good catch, will add tests & try to replace them as well. If that takes too much time & effort I may skip the assist in these cases, because I'm still learning how to navigate all the constructs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Take your time :)

I'll add one more thing just so that I don't forget, but don't take it too hard; it's rather hard to implement these perfectly correctly and we don't require assists to always produce compiling code.

  • Ideally we shouldn't provide the assist when P is used in the following positions because they'd produce invalid code:

    fn foo<P: Trait>(
        _: <P as Trait>::Assoc,          // within path type qualifier
        _: <() as OtherTrait<P>>::Assoc, // same as above
        _: P::Assoc,                     // associated type shorthand
        _: impl OtherTrait<P>            // generic arg in impl trait (note that associated type bindings are fine)
        _: &dyn Fn(P)                    // param type and/or return type for Fn* traits
    ) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the list, this is really helpful. I hope to find a way to evaluate them so they are covered too.


if params.is_empty() {
return None;
}

let type_param_hir_def = ctx.sema.to_def(&type_param)?;
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;
}

let target = type_param.syntax().text_range();

acc.add(
AssistId("replace_named_generic_with_impl", AssistKind::RefactorRewrite),
"Replace named generic with impl",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Replace named generic with impl",
"Replace named generic with impl trait",

target,
|edit| {
let type_param = edit.make_mut(type_param);
let fn_ = edit.make_mut(fn_);

// 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::<Vec<_>>();

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());
}
}

// get type bounds in signature type: `P` -> `impl AsRef<Path>`
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());
}
},
)
}

fn is_referenced_outside(
sema: &Semantics<'_, RootDatabase>,
type_param_def: Definition,
fn_: &ast::Fn,
file_id: FileId,
) -> bool {
// 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()),
];
Comment on lines +111 to +114
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid allocation since this is always fixed length. Something like

Suggested change
let search_ranges = vec![
fn_.body().map(|body| body.syntax().text_range()),
fn_.ret_type().map(|ret_type| ret_type.syntax().text_range()),
];
let search_ranges = [
fn_.body().map(|body| body.syntax().text_range()),
fn_.ret_type().map(|ret_type| ret_type.syntax().text_range()),
];


search_ranges.into_iter().flatten().any(|search_range| {
let file_range = FileRange { file_id, range: search_range };
!type_param_def.usages(sema).in_scope(SearchScope::file_range(file_range)).all().is_empty()
})
}

#[cfg(test)]
mod tests {
use super::*;

use crate::tests::{check_assist, check_assist_not_applicable};

#[test]
fn replace_generic_moves_into_function() {
check_assist(
replace_named_generic_with_impl,
r#"fn new<T$0: ToString>(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<P$0: AsRef<Path>>(input: P) -> Self {}"#,
r#"fn new(input: impl AsRef<Path>) -> Self {}"#,
);
}

#[test]
fn replace_generic_trait_applies_to_all_matching_params() {
check_assist(
replace_named_generic_with_impl,
r#"fn new<T$0: ToString>(a: T, b: T) -> Self {}"#,
r#"fn new(a: impl ToString, b: impl ToString) -> Self {}"#,
);
}

#[test]
fn replace_generic_with_multiple_generic_params() {
check_assist(
replace_named_generic_with_impl,
r#"fn new<P: AsRef<Path>, T$0: ToString>(t: T, p: P) -> Self {}"#,
r#"fn new<P: AsRef<Path>>(t: impl ToString, p: P) -> Self {}"#,
);
check_assist(
replace_named_generic_with_impl,
r#"fn new<T$0: ToString, P: AsRef<Path>>(t: T, p: P) -> Self {}"#,
r#"fn new<P: AsRef<Path>>(t: impl ToString, p: P) -> Self {}"#,
);
check_assist(
replace_named_generic_with_impl,
r#"fn new<A: Send, B$0: ToString, C: Debug>(a: A, b: B, c: C) -> Self {}"#,
r#"fn new<A: Send, C: Debug>(a: A, b: impl ToString, c: C) -> Self {}"#,
);
}

#[test]
fn replace_generic_with_multiple_trait_bounds() {
check_assist(
replace_named_generic_with_impl,
r#"fn new<P$0: Send + Sync>(p: P) -> Self {}"#,
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$0: Send + Sync>(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$0: ToString>(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$0: Send + Sync>(p: P) {}
fn hello<P: Debug>(p: P) { println!("{:?}", p); }
"#,
r#"
fn new(p: impl Send + Sync) {}
fn hello<P: Debug>(p: P) { println!("{:?}", p); }
"#,
);
}
}
2 changes: 2 additions & 0 deletions crates/ide-assists/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
13 changes: 13 additions & 0 deletions crates/ide-assists/src/tests/generated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<P$0: AsRef<Path>>(location: P) -> Self {}
"#####,
r#####"
fn new(location: impl AsRef<Path>) -> Self {}
"#####,
)
}

#[test]
fn doctest_replace_qualified_name_with_use() {
check_doc_test(
Expand Down
15 changes: 15 additions & 0 deletions crates/syntax/src/ast/edit_in_place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions crates/syntax/src/ast/make.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};"))
}
Expand Down