Add suggestion to diagnostic when user has array but trait wants slice.

[BGR360]

Resolves #90528.

Before:

After:

Alternatively, I could make it look like this:

The latter more closely mirrors other suggestions like "consider changing this borrow's mutability," but the former kinda looks better. I'm open to other ideas as well.

The code ended up being a little more complicated than I had hoped. Had to account for different mutabilities and variables vs. literals. See the tests.

Since I found it impractical to provide a machine applicable suggestion, is it maybe not worth bothering with the complicated mutability logic?

cc @binajmen

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[BGR360]

r? @estebank

This seems up your alley :)

[BGR360]

r? rust-lang/diagnostics

[davidtwco]

Apologies for the delay in having this reviewed, this looks good to me, I've left one suggestion that you can change if you'd like, but otherwise we can see this merged.

[davidtwco]

nit: you could probably collapse these tests down into fewer files - perhaps cases where a suggestion is made and where a suggestion isn't made - and then you can add // run-rustfix to the test to confirm the suggestion works.

[BGR360]

Sure. What should I do about the scenarios where the suggestion is wrong (see anywhere I put bad suggestion)?

[davidtwco]

We could split these out into another test, as long as the suggestion is MaybeIncorrect then it's fine that it's sometimes wrong, it'll just need to be in a different test if we want // run-rustfix to work on the cases that do have correct suggestions.

[BGR360]

I'm still new to rustfix. Will it apply MaybeIncorrect changes if the test is annotated with // run-rustfix? As written, my code does not know when its suggestion is MachineApplicable and when it is MaybeIncorrect, so I just always give MaybeIncorrect.

[davidtwco]

I actually can't remember, if it isn't doing it when you're trying this locally then we can just approve this - apologies for the runaround.

[estebank]

I think that in tests MaybeIncorrect ones do get applied. If they don't what we normally do is separate all the cases that can be fixed into its own file and another for the rest.

[BGR360]

@estebank looks like you're right: https://rustc-dev-guide.rust-lang.org/tests/adding.html#other-header-commands

rustfix-only-machine-applicable is equivalent to run-rustfix except it will only apply MachineApplicable suggestions. run-rustfix will apply all suggestions.

[BGR360]

I went to go and apply the CR feedback today, but after rebasing with upstream, it seems my changes no longer work (i.e., no suggestions are being produced).

Some very rudimentary debugging has revealed that the candidate_impls list is now empty. Git reveals no changes to find_similar_impl_candidates during that time:

$ git log -L 1447,+50:compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
commit 023b56572f54f5ada2d298c59622646c63177b14
Author: lcnr <rust@lcnr.de>
Date:   Fri Nov 19 14:21:21 2021 +0100

    add some comments

diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
...

Shall I go bisecting to figure out where things went wrong? Or does anybody have an idea as to what changed between Nov 28 and today that could be causing this?

[BGR360]

Bisect points to dcd716f. Prior to that commit, rustc will suggest the traitref of <&[u8] as Read>. After, it does not.

Given:

trait Read {}

impl Read for &[u8] {}

fn wants_read(_: impl Read) {}

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

Compiled on dcd716f~1 (83b32f2):

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:

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`

I'll open another issue

[BGR360]

Ah crap I accidentally clicked the re-request review button, sorry @estebank

[davidtwco]

Looks like the current status of this is that #92223 fixes #92113 which unblocks this (just to save me some time next time I check this assigned pull request).

[BGR360]

Someone (maybe me, eventually) should check the state of the diagnostics now that #93298 is in. Just to confirm that this PR is even necessary.

[compiler-errors]

@BGR360 Heads up, I rebased this really quick and I think the suggestion may be showing up correctly now. You might be able to get this landed now, unless I'm mistaken.

If you do rebase this PR, maybe you want to make this into a multipart-suggestion that suggests prepending & and adding [..].

[BGR360]

Finally looking at this again.

If you do rebase this PR, maybe you want to make this into a multipart-suggestion that suggests prepending & and adding [..].

@compiler-errors why would that be preferable in your opinion?

[compiler-errors]

@BGR360 it's just conventional to avoid span_to_snippet calls when they can be avoided, and in this code they can definitely be avoided with multipart_suggestion. A multipart suggestion renders more cleanly, with +++ underlining just the added parts as opposed to ~~~ underlining the whole replaced expression.

[estebank]

To clarify the rationale:

• the output is slightly nicer with the patch output
• it avoids breaking in the face of macros
• lowers the likelihood of multiple applied suggestions clobbering each other

[estebank]

Feel free to ping me once you've rebased.

[Dylan-DPC]

Unblocking as #92113 is resolved

[jackh726]

@BGR360 are you still interested in working on this? I could potentially bring this over the finish line by addressing the final reviews if you no longer are able to.

[BGR360]

@jackh726 All yours if you want it! Thanks :)

[jackh726]

Closing in favor of #108841. Thanks @BGR360 for the work here!