Suggest correct code for associated fn with type parameters

[estebank]

#68689 partially addresses #50734, but we need to resugar the predicates to unify the output of a trait obligation with related projection obligations (associated types).

For example

   = help: implement the missing item: `fn from_iter<T>(_: T) -> Self where T: std::iter::IntoIterator, std::iter::IntoIterator::Item = A { unimplemented!() }`

should be

   = help: implement the missing item: `fn from_iter<T>(_: T) -> Self where T: std::iter::IntoIterator<Item = A> { unimplemented!() }`

[alarsyo]

I hit this today, couldn't remember the exact prototype so I wanted the compiler to suggest it to me (typing this, I realize how amazing it is that Rust gets most of these suggestions right. Thanks for all the work in this area!). Playground:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=470d102838165432f30154680cc4bc23

Copy-pasting the suggestion leads to this:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=e0c32aeffcc38450d39f7144c5bd9e70

About the second error:

error[E0412]: cannot find type `A` in this scope
 --> src/lib.rs:9:41
  |
6 |     fn from_iter<T>(_: T) -> Self
  |                  - similarly named type parameter `T` defined here
...
9 |         std::iter::IntoIterator::Item = A,
  |                                         ^ help: a type parameter with a similar name exists: `T`

Couldn't the compiler also deduce that A is in fact bool here? I couldn't find an open issue specific to this.

For the resugaring error, I'd be willing to tackle this, if you think this is something achievable for a relatively new contributor!

[estebank]

For the resugaring error, I'd be willing to tackle this, if you think this is something achievable for a relatively new contributor!

@alarsyo there are different parts that could potentially be tackled of different level of difficulty. The underlying problem is that by the time the compile error happens, we no longer have access to the original bound information. We know that the method requires T to be IntoIterator, and that <T as IntoIterator>>::Item = *something*, but we no longer have the metadata that ties those two bounds together, as far as rustc is concerned, they are two different things, which is why the output looks the way it does. Extending the metadata to keep those two things mergeable at that stage is probably a medium sized undertaking requiring to touch a fair amount of the compiler. I would advice against trying that as a newcomer 😅

An alternative to the "correct" fix above would be to try to unify multiple bounds into a single one when possible. Doing that is concentrated in a single part of the compiler, which can be easier to understand, but will require tricky logic. I would look at all the traits bounds (Predicates in rustc parlance) and see if an associated type equality constraint has a self type mentioned in a trait bound on the RHS of the bound. Meaning, if you have T: Foo, <_ as Foo>::Bar = Baz, look for Foo in all bounds. If found, you have to figure out how to "print" it. There might be some existing code that can be reused to make this as resilient as any other part, potentially building a hir::Path with all the necessary parts, but I'd be happy enough with "one off" for this use case with a FIXME comment.

Finally, I would love to handle the equality constraint std::iter::IntoIterator::Item = T case in a more user friendly way, similar to #70908: we should detect that this is constraining an associated type and provide the same suggestion as in the previously mentioned case. If that is too hard to do, we could at least mention what the solution is given IntoIterator::Item is an associated type in a help (not a structured suggestion).

What do you think? Is any of this something of the scope size you'd like to tackle?

---

Edit: Looking at #70908, it seems to me like what would be necessary to accomplish the last part of my comments above would be to pass all the predicates to this function and perform the "unification" I mentioned for the previous cases, so doing either the first or second approach, you can get the last case almost for free 😃

[alarsyo]

Hi @estebank, sorry for the late reply.

What do you think? Is any of this something of the scope size you'd like to tackle?

I think I can try and build something for the alternative to the "correct" fix you mention, i.e. manually merge an associated type equality constraint bound with another bound if the other bound is about Foo as well.

Finally, I would love to handle the equality constraint std::iter::IntoIterator::Item = T case in a more user friendly way, similar to #70908: we should detect that this is constraining an associated type and provide the same suggestion as in the previously mentioned case. If that is too hard to do, we could at least mention what the solution is given IntoIterator::Item is an associated type in a help (not a structured suggestion).

Making sure I understand this correctly, the difference with the previous idea would be correcting the user when he writes the equality constraint, rather fixing the suggestion provided by the compiler? (both things being complementary to each other, of course). Did I get this right? 😄

Edit: Looking at #70908, it seems to me like what would be necessary to accomplish the last part of my comments above would be to pass all the predicates to this function and perform the "unification" I mentioned for the previous cases, so doing either the first or second approach, you can get the last case almost for free

So starting with this seems like it'd be a good path? If I understand correctly, implementing the "manual merge" of related bounds would allow its use both to fix the "method impl" suggestion I hit with IntoIterator above, as well as add a new helpful suggestion when the user writes an equality constraint?

[alarsyo]

I'll start working on that "unification". Looking at #70908, I'm having a hard time identifying where I'd get started though 😅 rustc_middle, rustc_typeck, rustc_ast_passes...

[alarsyo]

@estebank friendly ping in case this went past you (nothing pressing if you're not available though!)

[estebank]

Making sure I understand this correctly, the difference with the previous idea would be correcting the user when he writes the equality constraint, rather fixing the suggestion provided by the compiler? (both things being complementary to each other, of course). Did I get this right? 😄

I think you got it right, we would want to support both cases independently (likely in separate PRs). One is reducing the likelihood of hitting the equality case, but anyone could try to write it without waiting for rustc to point them in that direction, so ideally we cover both cases.

So starting with this seems like it'd be a good path? If I understand correctly, implementing the "manual merge" of related bounds would allow its use both to fix the "method impl" suggestion I hit with IntoIterator above, as well as add a new helpful suggestion when the user writes an equality constraint?

Correct. It would be what we need and would work for most cases (but not 100% because we're rejoining things that got separated for a reason, but it would work well enough for all the common cases).

I'll start working on that "unification". Looking at #70908, I'm having a hard time identifying where I'd get started though 😅 rustc_middle, rustc_typeck, rustc_ast_passes...

Now with a fresh set of eyes I'm thinking now that you can probably try to unify things just with the information you have in bounds_from_generic_predicates and deny_equality_constraints by looking at the ty::GenericPredicates<'_> and try to "merge" the ones that are related.

@estebank friendly ping in case this went past you (nothing pressing if you're not available though!)

Sorry! The previous notifications did indeed get drowned and am currently with intermittent internet access. Do not hesitate to ping me again if you need any further help though!

[estebank]

Current output:

error[E0046]: not all trait items implemented, missing: `from_iter`
 --> src/lib.rs:5:1
  |
5 | impl FromIterator<bool> for Test {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `from_iter` in implementation
  |
  = help: implement the missing item: `fn from_iter<T: IntoIterator<Item = bool>>(_: T) -> Self { todo!() }`

error: equality constraints are not yet supported in `where` clauses
 --> src/lib.rs:9:9
  |
9 |         std::iter::IntoIterator::Item = A,
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not supported
  |
  = note: see issue #20041 <https://github.com/rust-lang/rust/issues/20041> for more information

error[E0412]: cannot find type `A` in this scope
 --> src/lib.rs:9:41
  |
6 |     fn from_iter<T>(_: T) -> Self
  |                  - similarly named type parameter `T` defined here
...
9 |         std::iter::IntoIterator::Item = A,
  |                                         ^
  |
help: there is an enum variant `futures_0_1_31::future::Either::A`; try using the variant's enum
  |
9 |         std::iter::IntoIterator::Item = futures_0_1_31::future::Either,
  |                                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
help: a type parameter with a similar name exists
  |
9 |         std::iter::IntoIterator::Item = T,
  |                                         ~

We need to fix how we handle the incorrect syntax, but the initial suggestion is now fixed.