fix: add generic TypeBoundList in generated derivable impl

[rami3l]

Potentially fixes #13727.

Continuing with the work in #13732, this fix tries to add correct type bounds in the generated impl block:

  enum Either<T, U> {
      Left(T),
      Right(U),
  }
  
- impl<T, U> PartialEq for Either<T, U> {
+ impl<T: PartialEq, U: PartialEq> PartialEq for Either<T, U> {
      fn eq(&self, other: &Self) -> bool {
          match (self, other) {
              (Self::Left(l0), Self::Left(r0)) => l0 == r0,
              (Self::Right(l0), Self::Right(r0)) => l0 == r0,
              _ => false,
          }
      }
  }

[rami3l]

@lowr The new testcase (against the original example in #13727) works alright now.

However, this fix also influences the following test, which got me thinking: maybe fn generate_impl_text_inner() is "too generic" and needs to be refined in terms of e.g. whether to add TypeBoundList.

  enum Generic<T, U: Clone> { One(T), Two(U) }

- impl<T, U: Clone> From<T> for Generic<T, U> {
+ impl<T: From<T>, U: Clone + From<T>> From<T> for Generic<T, U> {
      fn from(v: T) -> Self {
          Self::One(v)
      }
  }

... T: From<T> seems correct but redundant, U: From<T> is not correct at all.

(I guess in this case of enums, From is a bit like Default where there is a bias towards a certain variant.)

---

Update: I finally special-cased From<T> but it looks like a hack way too much... Please don't hesitate to ask me to add more test cases if nessary :)

---

Update: The special-casing of From<T> has been refactored away in cece895.

[lowr]

Hmm, generate_impl_text_inner() is intended to be generic, and I feel like it might be too much for it to handle all of these. For this particular instance, I think it's sufficient to add the bounds after generating impl text, leaving generate_impl_text_inner() as is.

Also, could you add a test case where type parameter(s) already has some bounds so we ensure we don't lose the existing bounds? We may also want to check if the bounds already include the trait we're adding so it wouldn't be duplicated.

[rami3l]

@lowr Thanks a lot for your reply!

However, there are still several points that I'm unsure about:

1. add the bounds after generating impl text

   I'm not quite clear about what you meant by this.
   If you meant appending where bounds after the first line, then that's also part of fn generate_impl_text_inner()'s job:

   rust-analyzer/crates/ide-assists/src/utils.rs

   Lines 499 to 502 in 4596847

   	match adt.where_clause() {
   	Some(where_clause) => {
   	format_to!(buf, "\n{where_clause}\n{{\n{code}\n}}");
   	}

   I do think the current code (blacklisting From<T>) is too "hacky", but I don't think leaving the function unchanged is a good idea.

   How about changing the API and make call sites explicitly specify if we want to copy down the trait or not? In that case, the call site generating From<T> can opt out of this.

2. We may also want to check if the bounds already include the trait we're adding so it wouldn't be duplicated.

   This logic has already been handled in the make::type_bound_list function:

   let bounds = bounds.into_iter().map(|it| it.to_string()).unique().join(" + ");

   I assume it's not desired to make a TypeBoundList with duplicates anyway, plus the duplication is detected by .to_string(), so I put this logic in the constructor.

   And there's an existing test guarding against duplications:

   rust-analyzer/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs

   Lines 1182 to 1201 in 4596847

   	#[test]
   	fn add_custom_impl_clone_generic_tuple_struct_with_bounds() {
   	check_assist(
   	replace_derive_with_manual_impl,
   	r#"
   	//- minicore: clone, derive
   	#[derive(Clo$0ne)]
   	struct Foo<T: Clone>(T, usize);
   	"#,
   	r#"
   	struct Foo<T: Clone>(T, usize);
   	impl<T: Clone> Clone for Foo<T> {
   	$0fn clone(&self) -> Self {
   	Self(self.0.clone(), self.1.clone())
   	}
   	}
   	"#,
   	)
   	}

   I'll add a new test case with more bounds to showcase the reservation of existing ones as well as the deduplication, just in case :)

[lowr]

1. add the bounds after generating impl text

I'm not quite clear about what you meant by this.
If you meant appending where bounds after the first line, then that's also part of fn generate_impl_text_inner()'s job:

I meant you can add the bounds in impl_def_from_trait() after calling generate_trait_impl_text(), maybe around here, by directly mutating the AST in-place. See this file if you haven't seen our AST editing API (I'm not sure if we have the API for editing bounds, you may have to add one).

generate_impl_text_inner() is just a utility function after all and I'd say its job is to generate the most generic impl text that can be used in multiple assists as a "starting point", which then you can mutate depending on your needs. Some assists, like generate_from_impl_for_enum, don't need to add trait bounds, and some may want to add bounds to only one type parameter among many. These logics are specific to each assist thus it's for them to handle themselves, not for generate_impl_text_inner() to.

That said, cece895 actually looks good enough (though special casing $0 still bugs me a bit), and I don't strongly oppose to this getting merged if maintainers approve.

2. We may also want to check if the bounds already include the trait we're adding so it wouldn't be duplicated.

This logic has already been handled in the make::type_bound_list function:

Aha, this is clearly my oversight, sorry about that. Nice work! 👍

[rami3l]

@lowr I see what you mean now, thanks 🙏

Let's wait for the moment and see what the maintainers have to say about this change then.

---

Update:

though special casing $0 still bugs me a bit

I also updated the code so that it won't special-case $0 anymore.

[Veykril]

This seems fine to me, a more general API would be in having the boolean parameter be replaced with a list of bound lists that zip up with the generic parameters, but as that is currently not really needed there is no point in going that far.
Mutating the ast would also work but that is always kind of a pain to work with.
@bors r+

[bors]

📌 Commit cfa9149 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit cfa9149 with merge ae65912...

[bors]

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