fix: Support generics in extract_function assist #12556

DorianListens · 2022-06-16T17:27:16Z

This change attempts to resolve issue #7636: Extract into Function does not
create a generic function with constraints when extracting generic code.

In FunctionBody::analyze_container, we now traverse the ancestors in search
of AnyHasGenericParams, and attach any GenericParamLists and WhereClauses
we find to the ContainerInfo.

Later, in format_function, we collect all the GenericParams and
WherePreds from the container, and filter them to keep only types matching
TypeParams used within the newly extracted function body or param list. We
can then include the new GenericParamList and WhereClause in the new
function definition.

This change only impacts TypeParams. LifetimeParams and ConstParams are
out of scope for this change.

I've never contributed to this project before, but I did try to follow the style guide. I believe that this change represents an improvement over the status quo, but I think it's also fair to argue that it doesn't fully "fix" the linked issue. I'm totally open to merging this as is, or going further to try to make a more complete solution. Also: if there are other unit or integration tests I should add, please let me know where to look!

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

Veykril

I'd like to argue that this simple change is not actually an improvement over the status quo. If we are in a generic function and we extract something that does not rely on the generics at all we now copy all the generics and bounds needlessly, requiring the user to remove things after invoking the assist which is usually a pretty bad user experience (I'd rather have to manually add more things, than having to remove unnecessary things, as the IDE might have more tools that make the addition of whats missing still simpler).

Fixing this is probably non trivial, we'd basically have to check several things, are the types of the parameters actually using the generics, and is the extracted snippet using them. Part of that can be done syntactically probably, though I am unsure whether a pure syntax apparoch would work here (it should get us far enough at least).

DorianListens · 2022-06-22T18:34:33Z

Yeah, after reflecting on this, I'm inclined to agree with you that as-is this change doesn't represent an unambiguous improvement.

I've started trying to filter the generic param list by usages, first by inspecting the types of the params.
I know "directly in the parameters" is only a subset of the possible uses of a generic parameter within a function, but I figured it was a reasonable starting point.

I haven't started trying to filter the where clauses, as that will probably be even more challenging.

Definitely possible I'm out of my depth here, but this is a fun learning project for me. No worries if we don't get to a point where this is shippable, but I'm also very open to advice/direction as to how to move further in that direction.

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

DorianListens · 2022-06-23T17:20:33Z

Update: I've made some further progress here.

Both the generic param list and the where clause are now being filtered to only include items that match type params that appear in the new function's params list or body.

Caveat: For the moment, I'm only matching type params, not constants or lifetimes.

DorianListens · 2022-06-23T20:40:28Z

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

+    used_type_params: &[TypeParam],
+) -> bool {
+    match param {
+        ast::GenericParam::ConstParam(_) | ast::GenericParam::LifetimeParam(_) => true,


@Veykril Interested in your thoughts here. I could imagine three potential approaches:

Always copy ConstParam and LifetimeParam (as implemented here)

Never copy ConstParam or LifetimeParam (essentially a more constrained version of the status quo before this change)

"Do the right thing" ie filter out unused ConstParam and LifetimeParam, only include the ones referenced within the function

I feel like "Always Copy" is probably the wrong approach, for basically the same reasons you gave on my initial change.

Never copying these params would be trivial to implement, but potentially frustrating for users.

In order to "do the right thing", I'll need to do a similar kind of filtration as I've already done for TypeParams, for both consts and lifetimes. I think lifetimes should be relatively straightforward, but I'm not exactly sure how I'd go about this for const params. For instance, unclear to me when/if they should be passed down as params, or via the turbo fish at the call site.

I think I'm currently leaning towards the "never copy" approach, ie changing the true above to a false, and then handling lifetime and const params in separate future changes.

Especially since generic lifetime param support would be limited in usefulness until #9945 is fixed

DorianListens · 2022-07-06T22:41:01Z

I've updated this change to account for nested generics.

Instead of only looking for generics and where clauses on the immediate parent,
we can traverse all the ancestors and collect all the generics and where
clauses that may be in scope.

bors · 2022-07-08T14:11:52Z

☔ The latest upstream changes (presumably #12676) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2022-07-08T15:12:00Z

☔ The latest upstream changes (presumably #12706) made this pull request unmergeable. Please resolve the merge conflicts.

DorianListens · 2022-07-08T16:07:49Z

@Veykril I think this is ready for review again. Is there anything else you're hoping to see here?

Veykril · 2022-07-13T12:27:36Z

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

+fn parent_where_clauses(parent: &SyntaxNode) -> Vec<ast::WhereClause> {
+    let mut where_clause: Vec<ast::WhereClause> = parent
+        .ancestors()
+        .filter_map(ast::AnyHasGenericParams::cast)
+        .filter_map(|it| it.where_clause())
+        .collect();
+    where_clause.reverse();
+    where_clause
+}
+
+fn parent_generic_param_lists(parent: &SyntaxNode) -> Vec<ast::GenericParamList> {
+    let mut generic_param_list: Vec<ast::GenericParamList> = parent
+        .ancestors()
+        .filter_map(ast::AnyHasGenericParams::cast)
+        .filter_map(|it| it.generic_param_list())
+        .collect();
+    generic_param_list.reverse();
+    generic_param_list
+}


These are a bit too "weakly typed". There are only really up to two things that can offer use generics, the function this is an expression of (or const/static but those don't currently have generics) and then a trait/impl that contains that. So we should check for those two only.

So parent.ancestors().find_map(ast::Item::cast), then check that this is either an ast::Fn, ast::Static or ast::Const since those are the only valid ones (nothing else should contain expressions atm).

Then with that item at hand we can do item.syntax().parent().and_then(ast::Item::cast) and check if thats an ast::Impl or ast::Trait.

With those two things we can then fetch the generics and where clauses instead. Thats a bit nicer and that way we also dont grab unrelated generics and where clauses

Okay, I'll try that out! It wasn't clear to me how specific or general I should be when looking up through the ancestors, I'll try to be more specific in the future.

Can you think up an example of an "unrelated" generic that this approach would capture? If so, I'll add it as a unit test.

fn foo<T>() { fn bar<T>() { $0 } }

It would probably be fine either way, but its better to be a bit more precise here.

Ah, I see! I had been thinking that was a case we wanted to catch, but I just tried it out locally and learned about a new compiler error: https://doc.rust-lang.org/error-index.html#E0401

I'll make this change to be more precise now, thanks for the help!

Okay, I've tried this now, what do you think?

This change attempts to resolve issue rust-lang#7636: Extract into Function does not create a generic function with constraints when extracting generic code. In `FunctionBody::analyze_container`, we now traverse the `ancestors` in search of `AnyHasGenericParams`, and attach any `GenericParamList`s and `WhereClause`s we find to the `ContainerInfo`. Later, in `format_function`, we collect all the `GenericParam`s and `WherePred`s from the container, and filter them to keep only types matching `TypeParam`s used within the newly extracted function body or param list. We can then include the new `GenericParamList` and `WhereClause` in the new function definition. This change only impacts `TypeParam`s. `LifetimeParam`s and `ConstParam`s are out of scope for this change.

Veykril · 2022-07-14T14:22:27Z

Thanks!
@bors r+

bors · 2022-07-14T14:22:28Z

📌 Commit 796641b has been approved by Veykril

It is now in the queue for this repository.

bors · 2022-07-14T14:29:40Z

⌛ Testing commit 796641b with merge 073b325...

bors · 2022-07-14T14:40:15Z

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

DorianListens commented Jun 16, 2022

View reviewed changes

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

Veykril self-assigned this Jun 21, 2022

Veykril reviewed Jun 22, 2022

View reviewed changes

DorianListens commented Jun 22, 2022

View reviewed changes

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

DorianListens marked this pull request as draft June 22, 2022 19:39

DorianListens force-pushed the dscheidt/generic-extract branch 4 times, most recently from f36d076 to 1237f53 Compare June 23, 2022 17:07

DorianListens commented Jun 23, 2022

View reviewed changes

DorianListens force-pushed the dscheidt/generic-extract branch from 1237f53 to 2e33349 Compare June 30, 2022 16:26

DorianListens marked this pull request as ready for review June 30, 2022 16:29

DorianListens force-pushed the dscheidt/generic-extract branch 3 times, most recently from 8af51b5 to 14b3a23 Compare July 6, 2022 22:35

DorianListens force-pushed the dscheidt/generic-extract branch 2 times, most recently from da4aec8 to 56b8e89 Compare July 7, 2022 06:11

DorianListens force-pushed the dscheidt/generic-extract branch from 56b8e89 to 5f94f60 Compare July 8, 2022 15:06

DorianListens force-pushed the dscheidt/generic-extract branch from 5f94f60 to 24a4b34 Compare July 8, 2022 15:14

DorianListens force-pushed the dscheidt/generic-extract branch from 24a4b34 to 139b02b Compare July 8, 2022 17:08

Veykril reviewed Jul 13, 2022

View reviewed changes

DorianListens force-pushed the dscheidt/generic-extract branch from 139b02b to 50b79a9 Compare July 13, 2022 15:23

DorianListens added 2 commits July 13, 2022 14:54

Make search for applicable generics more precise

796641b

DorianListens force-pushed the dscheidt/generic-extract branch from 50b79a9 to 796641b Compare July 13, 2022 19:54

bors merged commit 073b325 into rust-lang:master Jul 14, 2022

lnicola mentioned this pull request Aug 27, 2024

Extract into Function does not create a generic function with constraints when extracting generic code #7636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Support generics in extract_function assist #12556

fix: Support generics in extract_function assist #12556

DorianListens commented Jun 16, 2022 •

edited by Veykril

Loading

Veykril left a comment •

edited

Loading

DorianListens commented Jun 22, 2022

DorianListens commented Jun 23, 2022

DorianListens Jun 23, 2022

DorianListens Jun 30, 2022

DorianListens Jun 30, 2022

DorianListens commented Jul 6, 2022

bors commented Jul 8, 2022

bors commented Jul 8, 2022

DorianListens commented Jul 8, 2022

Veykril Jul 13, 2022

DorianListens Jul 13, 2022

Veykril Jul 13, 2022

DorianListens Jul 13, 2022

DorianListens Jul 13, 2022

Veykril commented Jul 14, 2022

bors commented Jul 14, 2022

bors commented Jul 14, 2022

bors commented Jul 14, 2022

fix: Support generics in extract_function assist #12556

fix: Support generics in extract_function assist #12556

Conversation

DorianListens commented Jun 16, 2022 • edited by Veykril Loading

Veykril left a comment • edited Loading

Choose a reason for hiding this comment

DorianListens commented Jun 22, 2022

DorianListens commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DorianListens commented Jul 6, 2022

bors commented Jul 8, 2022

bors commented Jul 8, 2022

DorianListens commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veykril commented Jul 14, 2022

bors commented Jul 14, 2022

bors commented Jul 14, 2022

bors commented Jul 14, 2022

DorianListens commented Jun 16, 2022 •

edited by Veykril

Loading

Veykril left a comment •

edited

Loading