Skip to content

Support generic function in generate_function assist #14065

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
Feb 2, 2023

Conversation

lowr
Copy link
Contributor

@lowr lowr commented Jan 31, 2023

Part of #3639

This PR adds support for generic function generation in generate_function assist. Now the assist looks for generic parameters and trait bounds in scope, filters out irrelevant ones, and generates new function with them.

See fn_generic_params() for the outline of the procedure, and see comments on filter_unnecessary_bounds() for criteria for filtering. I think it's good criteria for most cases, but I'm open to opinions and suggestions.

The diff is pretty big, but it should run in linear time w.r.t. the number of nodes we operate on and should be fast enough.

Some notes:

  • When we generate function in an existing impl, generic parameters may cause name conflict. While we can detect the conflict and rename conflicting params, I didn't find it worthwhile mainly because it's really easy to resolve on IDE: use Rename functionality.
  • I've implemented graph structure myself, because we don't have graph library as a dependency and we only need the simplest one.
    • Although petgraph is in our dependency graph and I was initially looking to use it, we don't actually depend on it AFAICT since it's only used in chalk's specialization graph handling, which we don't use. I'd be happy to replace my implementation with petgraph if it's okay to use it though.
  • There are some caveats that I consider out of scope of this PR. See FIXME notes on added tests.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 31, 2023
Comment on lines +623 to +624
/// Returns unique placeholders for types and consts contained in `value`.
pub fn collect_placeholders<T>(value: &T, db: &dyn HirDatabase) -> Vec<TypeOrConstParamId>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure where to place this function. hir_ty::utils says it's for "Helper functions for working with def, which (...) can't be computed directly from *Data" and it doesn't seem to fit there?

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Not using petgraph here seems okay since this doesn't seem to need a lot of graph-like functionaly

@Veykril
Copy link
Member

Veykril commented Feb 1, 2023

r=me with the 2 unwraps replaced
@bors delegate+

@bors
Copy link
Contributor

bors commented Feb 1, 2023

✌️ @lowr can now approve this pull request

@lowr
Copy link
Contributor Author

lowr commented Feb 2, 2023

Two rather random thoughts:

  • extract_function assist may benefit from functions this PR adds
  • I feel it might make sense to add a variant of the assist: "Generate function/method without trait bounds"

@bors r=Veykril

@bors
Copy link
Contributor

bors commented Feb 2, 2023

📌 Commit 493cabb has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 2, 2023

⌛ Testing commit 493cabb with merge eeceba7...

@bors
Copy link
Contributor

bors commented Feb 2, 2023

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

@bors bors merged commit eeceba7 into rust-lang:master Feb 2, 2023
@bors bors mentioned this pull request Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants