Add generic parameter assist

[anarachnid]

Closes #12531

Supported features

Add a generic type parameter to anything that can take one. This includes struct, enum, union, fn, trait, type alias, trait alias, use alias.

Note that this produces code that does not compile, as the generic parameter must be used, and the assist does not add a member to the struct/enum/etc.

All of these examples are listed in the tests section at the bottom of the assist file (crates/ide-assists/src/handlers/add_generic_parameter.rs), but are some duplicated here for exposition:

Basic nesting (works same for enums, unions, traits)

struct Channel$0(u8);
struct Color([Channel; 3]);
struct Image(Vec<Color>);

struct Channel<T1>(u8);
struct Color<T1>([Channel<T1>; 3]);
struct Image<T1>(Vec<Color<T1>>);

It works for cyclic structs/etc, and adds the parameter on impls.

struct Cons$0(i32, List);
impl Cons{ fn len(&self){42} }
struct List(Option<Box<Cons>>);
impl List{ fn len(&self){42} }

struct Cons<T1>(i32, List<T1>);
impl<T1> Cons<T1>{ fn len(&self){42} }
struct List<T1>(Option<Box<Cons<T1>>>);
impl<T1> List<T1>{ fn len(&self){42} }

For traits, it adds the generic to the trait, rather than the function. This seems to generally be the desired outcome

trait HasFood$0 {}
trait CanMakeFood{ fn make() -> impl HasFood; }

trait HasFood<T1> {}
trait CanMakeFood<T1>{ fn make() -> impl HasFood<T1>; }

For impls, it uses the impl's generic, but if there is none, it adds to the innermost (likely a function)

struct Color$0(u8);
struct MakeColor;
impl MakeColor { fn make() -> Color { Color(0) }}
struct MakeColor2(Color);
impl MakeColor2 { fn make() -> Color { Color(0) }}

struct Color<T1>(u8);
struct MakeColor;
impl MakeColor { fn make<T1>() -> Color<T1> { Color(0) }}
struct MakeColor2<T1>(Color<T1>);
impl<T1> MakeColor2<T1> { fn make() -> Color<T1> { Color(0) }}

Use aliases:

struct ColorImpl$0(u8);
use ColorImpl as Color;
struct Image(Vec<Color>);

struct ColorImpl<T1>(u8);
use ColorImpl as Color;
struct Image<T1>(Vec<Color<T1>>);

Type/trait aliases:

trait Debug2$0 = Debug;
trait SuperDebug: Debug2 {}

trait Debug2<T1> = Debug;
trait SuperDebug<T1>: Debug2<T1> {}

More complicated interaction through a trait:

struct Food$0;
trait MakeFood{ fn make() -> Food; }
impl MakeFood for Farm{ fn make() -> Food { Food }}

struct Food<T1>;
trait MakeFood<T1>{ fn make() -> Food<T1>; }
impl<T1> MakeFood<T1> for Farm{ fn make() -> Food<T1> { Food }}

I've tested this on toy AST's and it works fine, but I haven't tried larger scenarios. Larger things might try to modify things (e.g. traits) that aren't owned by the user, which could be problematic.

Showcase

• none, look at examples above, not much interactivity (yet).

Future

• I'd like to have the user pick the generic parameter name. I just use T1 right now.

• I haven't implemented solving name collisions, like the following, which would add a duplicate the T1 parameter:

  • struct Thing;
    trait DoStuff{ fn stuff<T1>(hello: Thing$0); }

• I'd like to add support for bounds and more kinds of generics (const, lifetime), but it is unclear how to add it to the editor/lsp, read below.

  • [edit 2023/08/25] Possible implementation:

    // show assist for any part of this attribute
    // note: this attribute isn't defined, its just for the assist to look at
    #[assist::add_generic_parameters$0]
    struct T<'a, T, const N: usize>;
    // will add T's generics to Thing (possibly deleting struct T)
    struct Thing;

    • a similar strategy could be used in other assists for more advanced things:
      • apply assist for all descendant nodes applicable
      • change fn signature
      • move definition to some module

• test for edge cases since many different kinds of things can have generics and interact with each other.

• Is there anything I'm not seeing that could be improved, changed?

• I'd like some external direction in how to workflow, contribute better/easier, optimize, clean up, etc.

About the coding

I have a few difficulty/uncertainty points when making this:

• rust-analyzer questions

  • syntax/ast api:
    • confusing when they get invalidated: if you modify a previous ast node, then it makes a later ast node have invalid offsets, but only if you don't make_mut on the later node
    • when to call clone_for_update/clone_subtree/make_mut
    • My solution was to buffer up everything I wanted to make_mut, then make_mut and modify.
  • There were a few missing pieces in the library code, do I add it? e.g. a trait impl that seemed obvious (I assumed so, so i added them)
  • Should I pull request a mostly done thing? are fixmes fine? if they're fine, how many are allowed?

• general assist questions:

  • input fuzzer? check for panic'ing inputs
  • how to do multi-file input tests
  • error reporting
    • silently fail?
    • print an error somewhere?
  • Is there a way to add more powerful assists (require more user input)?
    • make a command?
    • powerful assist examples:
      • "extract (possibly multiple) selected expressions as parameters"
      • "add multiple generic parameters, with bounds: <'a, T: Clone, const N: usize>"
      • "reorder function parameters (x,y,z) -> (z,x,y)"

• how are small details discussed?

  • when assist should be applicable
  • assist specific details
    • placeholder names (e.g. have T1 be a placeholder selected by the user after assist execution)
    • I have a bunch of these in FIXME comments

• specific to add_generic_parameter:

  • customize the parameter name? snippet?
  • many options: lifetime, const
    • can these be in a menu?
  • remove generic parameter?
  • what should Assist.target be? (Right now it is the entire file, since this is as far reaching as a rename, which is pretty far)

[bors]

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

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

• 09551c5

[anarachnid]

I'm not sure what I'm supposed to do to make this PR mergeable.
First, I followed the "Resolve conflicts" button (at the bottom "This branch has conflicts that must be resolved" panel), but that created a merge commit. This, I discovered, is not allowed, per the no merge policy.
I deleted all the commits after my last real commit, and tried to manually "merge" by making my own commit:
The only conflict I saw was here.
So I manually changed the comments on my branch to match.
Now it tells me again that "This branch has conflicts that must be resolved" so I'm stuck now. (see here)
I've only been able to

• have conflicts
• add a merge commit to remove the conflicts, but then have a (prohibited) merge commit

What should I do to make this pull request mergeable?

[HKalbasi]

Thanks for the PR! For fixing the merge commit problem, reset your branch to the original commit before the merge commit, then run git fetch upstream (replace upstream with the name of https://github.com/rust-lang/rust-analyzer/ remote) to download the latest change your branch has conflict with, then run git rebase -i upstream/master, resolve merge conflicts, finish the rebase using git rebase --continue, and finally force push it into your fork. If it turns out to be too troublesome, you can reapply your diff in a clean clone and create another PR.

how to do multi-file input tests

See tests in this file:

rust-analyzer/crates/ide-diagnostics/src/handlers/unlinked_file.rs

Line 254 in b1f89a8

mod tests {

print an error somewhere?

I think just omitting the assist if it is not applicable due some problem is fine. If the error indicates some bug in the implementation, you can use the never! macro.

Is there a way to add more powerful assists (require more user input)?

The idea is that assists provide a simple and atomic ability, and user can combine them by repeatedly invoking them. For example for "add multiple generic parameters, with bounds: <'a, T: Clone, const N: usize>" user can repeatedly invoke the "add generic parameter", and another imaginary assist "add bound to this generic parameter" to achieve this behavior.

placeholder names (e.g. have T1 be a placeholder selected by the user after assist execution)

I think it is fine, and it is used in other assists (for example "Extract to function" uses fun_name) and user can rename the symbol by another assist. An alternative is to make this assist a fix on "unresolved type name" diagnostic, which uses the unresolved name instead of T1, but that diagnostic doesn't exist yet.

[anarachnid]

Reply:
git:
Thank you, the git advice worked perfectly.
👍

multi file:
I copied the "//- /main.rs" syntax into my check_assist tests and that worked.
👍

powerful assists:
Theres lifetime, type, and const parameters, which in my opinion would clutter the assist dropdown. However, there are people better than me to decide if that is cluttered. Continuing with my opinion, a submenu would be nice, but I'm not sure of ide support for this.

error:
👍

placeholder names:
Rename works well for other assists, but for this one it's less ideal.

struct A<T1>(); struct B<T1>(A<T1>);
         ^^              ^^----^^

these are independent symbols, so one would have to change A's and B's parameter (2 renames by the user).

new:
When I did my multi file test in vs code, it complains about:
overly long loop turn took 136.828ms (event handling took 42.2µs): Notification { method: "textDocument/didChange" }
Obviously I could try to optimize. Should I? Another orthogonal solution would be to asyncify the assist so it doesn't block, and/or add a timeout.

I had to modify code in search.rs to support adding generic parameters through aliases. Should this be in a different PR?
i.e. 1 PR adds the feature, then 1 PR adds alias support

[Veykril]

Sorry for not having looked at this for so long, I just dread reviewing PRs that have a massive diff like this (and I dread reviewing assists generally as well 😅). I still haven't given the assist code a review either yet, but the one thing that immediately stood out are the unrelated changes so it would be good to revert that first after which I'll take a look.

[Veykril]

Let's revert all of the changes in search.rs. import renames are in generally not yet supported and this PR should not attempt to fix that

[Veykril]

I'll close this for now due to inactivity, if you get back to this feel free to re-open