Show similar trait implementations if no matching impl is found

[fhahn]

This PR for #21659 uses DefId.for_each_relevant_impl() to show other possible implementations in the "trait not implemented" message.

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[nikomatsakis]

Interesting idea! The code here seems good. I'm a bit torn on how to present this information though: we've already got quite a few things going on when there is a trait resolution failure. This change seems to further demote the custom error message, as well, which is not clearly a win, since the whole point of #[rustc_on_unimplemented] is to make a message the user is more likely to understand.

cc @rust-lang/compiler @rust-lang/tools I'd like some feedback on how this error message ought to look.

[Aatch]

I'm not sure how useful the number is here. I think just indenting the name slightly should work well.

Also, I'd like to limit the number of output traits to some small-ish number, say three or four. This is because sometimes there might be a lot of "applicable" traits, say implementations of From, for example. In one of my libraries, I have an implementation of From<T> for T = all integer types. If you tried to use, say, a float by accident, that would print out 8 implementations. Use of macros can easily produce a large number of potential implementations.

I'd like an output like this:

the following implementations were found:
  Foo<A>,
  Foo<B>,
  Foo<C>,
  Foo<D>
and 4 others

[Aatch]

I'd like to see a few more tests for edge cases, like when there aren't any applicable implementations at all and when there is a generic bounded impl, e.g. impl<T: Clone> Foo for Bar<T>.

As for the interaction with #[rustc_on_unimplemented], I share some of @nikomatsakis concerns. Maybe print out one or the other, but not both? So if there is an #[rustc_on_unimplemented] custom error, it doesn't show the "other applicable traits" help.

[fhahn]

@Aatch thanks for your extensive feedback. I have pushed 2 commits which should address it. The relevant trait impl help should now only be displayed, if ther is not #[rustc_on_unimplemented].

[alexcrichton]

For help messages like this my general rule of thumb is that they're find so long as they're easy to skip over. This is likely to not actually print out the right set of types in all circumstances, but in the cases it does it's quite valuable! Along those lines I think the colorization we do of error messages and the limiting to 4 here works for making this a reasonable addition.

From an automation point of view I don't think we verify help messages at all in compiletest, and it'd be nice to do that, but not critical.

[arielb1]

I am not sure that using for_each_relevant_impl directly is right here - the function is part off rustc internals and may change at any moment. I don't want to see error message changes because I made the logic there smarter/stupider.

[nikomatsakis]

@arielb1 I certainly see your concerns, it'd be nice to have a notion of "similar implementations" that was independent. I'm not so concerned about the message changing though as I am about the idea that if we evolve the impl strategy in some way this help message might be hard to maintain; but then I guess we can just remove it.

[arielb1]

@nikomatsakis

We can always keep the algorithm with a stupid linear search - that will not be too slow for error messages. I just don't want to get random "the error used to show my impl but now it does not" bug reports.

[fhahn]

@nikomatsakis @arielb1 I am not sure how to continue here. Should create a copy of for_each_relevant_impl just for this note? Or leave it as is?

[nikomatsakis]

@fhahn I'm not really sure what @arielb1 meant by this, actually:

We can always keep the algorithm with a stupid linear search - that will not be too slow for error messages.

I think @arielb1 was proposing that we do a linear search through the impls and compare the simplified types. But I'm not sure if that makes sense, since it will yield the same results as the hashtable lookup, and that is clearly faster. I would have thought that the main concern would be that we might tighten how "simplified types" work to make them more precise, which might then in turn cause the error message to become less useful (since the point here is that, indeed, the types are NOT an exact match). So @arielb1 what precisely did you have in mind? (Another variant one might consider is doing a linear search but checking the simplified type across all type operands, not just Self.)

[pnkfelix]

@nikomatsakis I can't speak for @arielb1 , but one issue is that this introduces a dependency on the ordering of results from `for_each_relevant_impl (since only the first four are reported). So maybe the point was that a dumb search is more stable long term? (Another option could be to normalize the result vector before taking its prefix -- e.g. maybe just sort it? )

[nikomatsakis]

@pnkfelix it doesn't seem that there IS any inherent ordering here, so I don't see how iteration would be more stable. Sorting by some metric (strings?) would be, of course.

[arielb1]

@nikomatsakis

I was suggesting doing the linear search manually, because that won't randomly change the next time we touch for_each_relevant_impl.

[nikomatsakis]

Ah, I guess the concern is because we just pick the first 5? (As opposed to the concern that we might tighten up or change the approximations in use?)

[fhahn]

@arielb1 I've added a new commit which iterates over the trait def's blankt impls and its nonblanket impls. For the nonblanket impls, it first checks if the type can be simplified and if it can it filters all types that do not match it from the nonblanket impls. Is this along the lines what you had in mind?

[arielb1]

@Ftahn

I would prefer just using for_each_impl and filtering that manually.

[fhahn]

@arielb1 I see. I've pushed another commit, which uses for_each_impl and does the filtering manually. I am not entirely sure it completely matches what for_each_relevant_impl does at the moment though.

[arielb1]

Please use a match instead of a nest of ifs.

[arielb1]

Also, I would prefer not to flood the user with impls if our self-type does not simplify (consider <&mut _ as Clone>). That's one of the kinds of things that get much easier when we are not stuck with reusing some random function.

[arielb1]

Though I suppose that kind of rate-limiting should be done externally. it is already done.

[bors]

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

[fhahn]

@arielb1 I've pushed a commit refactoring the candidate selection. It still uses an if let with an nested if, because there's only one interesting case. About not flooding the user with multiple impls: the simplest way seems to be just using a hash set for the outputs.

[arielb1]

@bors r+

[bors]

📌 Commit a8d6070 has been approved by arielb1

[bors]

⌛ Testing commit a8d6070 with merge cae9267...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt