Regression in diagnostic quality for unimplemented traits on arrays.

[BGR360]

Commit dcd716f (#86986) appears to have regressed the quality of compiler diagnostics produced when an array type does not satisfy a trait but its slice version does (there may be other scenarios that see similar regressions, but this is where I've seen it).

This is currently blocking #91314.

Motivating example (playground):

trait Read {}

impl Read for &[u8] {}

fn wants_read(_: impl Read) {}

fn main() {
    wants_read([0u8]);
}

When compiled on dcd716f~1 (83b32f2), rustc provides a help message telling the user that the trait is implemented for slices:

error[E0277]: the trait bound `[u8; 1]: Read` is not satisfied
 --> ../scratch/issue-90528-unsizing-suggestion-1.rs:8:16
  |
8 |     wants_read([0u8]);
  |     ---------- ^^^^^ the trait `Read` is not implemented for `[u8; 1]`
  |     |
  |     required by a bound introduced by this call
  |
  = help: the following implementations were found:
            <&[u8] as Read>
note: required by a bound in `wants_read`
 --> ../scratch/issue-90528-unsizing-suggestion-1.rs:5:23
  |
5 | fn wants_read(_: impl Read) {}
  |                       ^^^^ required by this bound in `wants_read`

Compiled on dcd716f, this help message no longer appears:

error[E0277]: the trait bound `[u8; 1]: Read` is not satisfied
 --> ../scratch/issue-90528-unsizing-suggestion-1.rs:8:16
  |
8 |     wants_read([0u8]);
  |     ---------- ^^^^^ the trait `Read` is not implemented for `[u8; 1]`
  |     |
  |     required by a bound introduced by this call
  |
note: required by a bound in `wants_read`
 --> ../scratch/issue-90528-unsizing-suggestion-1.rs:5:23
  |
5 | fn wants_read(_: impl Read) {}
  |                       ^^^^ required by this bound in `wants_read`

Inspiration for regression tests can be taken from #91314.

@rustbot modify labels: +A-diagnostics +T-compiler +D-terse

cc @lcnr

[BGR360]

@rustbot modify labels: -regression-untriaged +regression-from-stable-to-nightly

[lcnr]

While I think mentioning <&[u8] as Read> here is useful, I do not believe that reverting the change in #86986 is the right fix here.

I think that we could actually change these diagnostics to more generally deal with stuff like deref impls by explicitly looking for potential impls instead of looking at the existing impl candidates.

So when encountering a "trait bound is not satisfied" error for some type T, we look at impls for type &T, &mut T and [U] (if T = [U; N]) and then mention these impls with a note like "considering using a reference".

Do you want to look at this yourself, if not I should hopefully be able to make some time for this over the next few weeks

cc @estebank

[BGR360]

I do not believe that reverting the change in #86986 is the right fix here.

Certainly not. I'm a little bummed that the work I did in #91314 is kaput, but I agree with you.

explicitly looking for potential impls instead of looking at the existing impl candidates

I like that idea, but it seems like we're lacking tests to tell us what else might have lost information as a result of #86986, which will make it harder to know exactly which types of potential impls we should look for. The work I was doing in #91314 obviously tells us one category of potential impls we need to check: [U] if T = [U; N], but how can we know what else may have regressed? I'll have to lean on your understanding of the work you did.

Do you want to look at this yourself

I was just poking around in there, but this seems like a larger time commitment compared to the very directed suggestion generation of #91314.

With guidance, I'd be happy to try out doing just the [U; N] -> [U] searching, and seeing how it goes from there. My main worry is that it sounds like this may not be something we've done before in rustc_trait_selection (that is, constructing a new type and querying for impls on that type). But if there is an example you can point me to, then that would probably make it a lot easier.

@rustbot label +A-traits

[lcnr]

alright 👍 i also believe that we aren't doing anything like that, but i think the required changes won't be too large though they do require more knowledge about how rustc works 😅 .

Going to write some instructions this week, feel free to ping me either here or on zulip in case i forget

[lcnr]

so, we collect the impl candidates in

rust/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

Lines 1440 to 1443 in e100ec5

	fn find_similar_impl_candidates(
	&self,
	trait_ref: ty::PolyTraitRef<'tcx>,
	) -> Vec<ty::TraitRef<'tcx>> {

The reason we don't consider slice impls anymore is this comparision

rust/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

Lines 1471 to 1473 in e100ec5

	if simp != imp_simp {
	return None;
	}

we want some fuzzy equality here instead of comparing their simplified types. How exactly that fuzzy equality should work requires a bit of fiddling so that's something you probably have to experiment with yourself. I would expect that directly using Ty here is better than using SimplifiedTypes. It should end up looking similar to

rust/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

Line 1389 in e100ec5

fn fuzzy_match_tys(&self, a: Ty<'tcx>, b: Ty<'tcx>) -> bool {

which you can probably reuse 😅

[apiraino]

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-low

[steffahn]

@rustbot label regression-from-stable-to-beta, -regression-from-stable-to-nightly

[BGR360]

This is fixed on stable. Not sure exactly what fixed it, but I suspect #93298.

error[E0277]: the trait bound `[u8; 1]: Read` is not satisfied
 --> src/main.rs:8:16
  |
8 |     wants_read([0u8]);
  |     ---------- ^^^^^ the trait `Read` is not implemented for `[u8; 1]`
  |     |
  |     required by a bound introduced by this call
  |
  = help: the trait `Read` is implemented for `&[u8]`
note: required by a bound in `wants_read`
 --> src/main.rs:5:23
  |
5 | fn wants_read(_: impl Read) {}
  |                       ^^^^ required by this bound in `wants_read`