feat: Assist to replace generic with impl trait #14816

justahero · 2023-05-16T09:00:57Z

This adds a new assist named "Replace named generic with impl". It is the inverse operation to the existing "Replace impl trait with generic" assist.

It allows to refactor the following statement:

//      👇 cursor
fn new<T$0: ToString>(input: T) -> Self {}

to be transformed into:

fn new(input: impl ToString) -> Self {}

adds new helper function impl_trait_type to create AST node
add method to remove an existing generic param type from param list

Closes #14626

Veykril

One thing we need to make sure is that the param is not used inside the function body or return type, as replacing it would make those uses invalid. You can do so by resolving the type_param to its hir definition via sema.to_def and then searching for usages on that via hir_def.usages()/ (You can also use that to collect the parameters then I believe)

crates/syntax/src/ast/edit_in_place.rs

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<T: ToString>(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

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

crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs

This checks the type param is referenced neither in the function body nor as a return type. * add tests

* pass in existing `Semantics` object to function * pass in `Definition` for param type * refactor iterator to use `flatten`

Veykril · 2023-05-26T12:03:38Z

@bors r+

bors · 2023-05-26T12:03:40Z

📌 Commit e78df83 has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-05-26T12:03:47Z

⌛ Testing commit e78df83 with merge e963846...

lowr

Sorry for the abrupt r-, current implementation may create dangling params unless I'm missing something.

@bors r-

lowr · 2023-05-26T11:59:43Z

crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs

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


lowr · 2023-05-26T12:11:52Z

crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs

+    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<_>>();


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, // 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?

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.

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

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

lowr · 2023-05-26T12:13:31Z

crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs

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


Suggested change

"Replace named generic with impl",

"Replace named generic with impl trait",

bors · 2023-05-26T12:22:11Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing e963846 to master...

bors · 2023-05-26T12:22:12Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing e963846 to master...

bors · 2023-05-26T12:22:15Z

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

lowr · 2023-05-26T12:27:34Z

Whoa bors doesn't see r- in review comments? Well, let's do this in a follow-up PR then!

@justahero Would you be willing to address my comments? I can follow up in case you're busy.

Veykril · 2023-05-26T12:30:04Z

Right, I think bors does not scan review comments unfortunately (the old one we used did I think)

lowr · 2023-05-26T12:38:14Z

Heh, I see. At least I learned a new thing so I'm one step closer to bors master 🤦‍♂️

lnicola · 2023-05-29T05:53:56Z

replace-generic.mp4

Fix Assist "replace named generic type with impl trait" This is a follow-up PR to fix the assist "replace named generic type with impl trait" described in #14626 to filter invalid param types. It integrates the feedback given in PR #14816 . The change updates the logic to determine when a function parameter is safe to replace a type param with its trait implementation. Some parameter definitions are invalid & should not be replaced by their traits, therefore skipping the assist completely. First, all usages of the generic type under the cursor are determined. These usage references are checked to see if they occur outside the function parameter list. If an outside reference is found, e.g. in body, return type or where clause, the assist is skipped. All remaining usages need to appear only in the function param list. For each usage the param type is further inspected to see if it's valid. The logic to determine if a function parameter is valid, follows a heuristic and may not cover all possible parameter definitions. With this change the following param types (as given in [this comment](#14816 (comment))) are not replaced & therefore skip the assist. ```rust fn foo<P: Trait>( _: ::Assoc, // within path type qualifier _: <() as OtherTrait>::Assoc, // same as above _: P::Assoc, // associated type shorthand _: impl OtherTrait // generic arg in impl trait (note that associated type bindings are fine) _: &dyn Fn(P) // param type and/or return type for Fn* traits ) {} ```

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2023

Veykril reviewed May 24, 2023

View reviewed changes

crates/syntax/src/ast/edit_in_place.rs Show resolved Hide resolved

justahero added 2 commits May 26, 2023 13:13

Veykril reviewed May 26, 2023

View reviewed changes

crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs Outdated Show resolved Hide resolved

crates/ide-assists/src/handlers/replace_named_generic_with_impl.rs Outdated Show resolved Hide resolved

justahero added 2 commits May 26, 2023 13:24

Check param is not referenced in function

ce1c853

This checks the type param is referenced neither in the function body nor as a return type. * add tests

Integrate feedback

e78df83

* pass in existing `Semantics` object to function * pass in `Definition` for param type * refactor iterator to use `flatten`

Veykril changed the title ~~Assist to replace generic with impl trait~~ feat: Assist to replace generic with impl trait May 26, 2023

lowr suggested changes May 26, 2023

View reviewed changes

bors merged commit e963846 into rust-lang:master May 26, 2023

justahero mentioned this pull request Jun 2, 2023

Fix Assist "replace named generic type with impl trait" #14945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Assist to replace generic with impl trait #14816

feat: Assist to replace generic with impl trait #14816

justahero commented May 16, 2023

Veykril left a comment

Veykril commented May 26, 2023

bors commented May 26, 2023

bors commented May 26, 2023

lowr left a comment

lowr May 26, 2023

lowr May 26, 2023

justahero May 26, 2023

lowr May 26, 2023

justahero May 26, 2023

lowr May 26, 2023

bors commented May 26, 2023

bors commented May 26, 2023

bors commented May 26, 2023

lowr commented May 26, 2023

Veykril commented May 26, 2023

lowr commented May 26, 2023

lnicola commented May 29, 2023 •

edited

Loading

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

feat: Assist to replace generic with impl trait #14816

feat: Assist to replace generic with impl trait #14816

Conversation

justahero commented May 16, 2023

Veykril left a comment

Choose a reason for hiding this comment

Veykril commented May 26, 2023

bors commented May 26, 2023

bors commented May 26, 2023

lowr left a comment

Choose a reason for hiding this comment

lowr May 26, 2023

Choose a reason for hiding this comment

lowr May 26, 2023

Choose a reason for hiding this comment

justahero May 26, 2023

Choose a reason for hiding this comment

lowr May 26, 2023

Choose a reason for hiding this comment

justahero May 26, 2023

Choose a reason for hiding this comment

lowr May 26, 2023

Choose a reason for hiding this comment

bors commented May 26, 2023

bors commented May 26, 2023

bors commented May 26, 2023

lowr commented May 26, 2023

Veykril commented May 26, 2023

lowr commented May 26, 2023

lnicola commented May 29, 2023 • edited Loading

lnicola commented May 29, 2023 •

edited

Loading