Pattern Migration 2024: reword to make sense on all Editions

[dianne]

This aims to fix #136456 by doing a few things:

• The main message of the diagnostic now says what went wrong, using wording similar to that in the Edition Guide.
• The diagnostic's primary message only mentions Rust 2024 if it's not a hard error.
• Since the primary message now covers what used to be handled by the labels, I've instead made it show the default binding mode at those points, to (hopefully!) aid in understanding. The additional information might be a bit much, admittedly, so possibly some other label text would be better.

An example of the migration lint:

error: reference patterns may only be written when the default binding mode is `move` in Rust 2024
  --> $DIR/migration_lint.rs:57:13
   |
LL |     let Foo(&x) = &Foo(&0);
   |             ^ default binding mode is `ref`
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/match-ergonomics.html>
help: make the implied reference pattern explicit
   |
LL |     let &Foo(&x) = &Foo(&0);
   |         +

An example of the hard error:

error: binding modifiers may only be written when the default binding mode is `move`
  --> $DIR/min_match_ergonomics_fail.rs:34:20
   |
LL | test_pat_on_type![(ref mut x,): &mut (T,)];
   |                    ^^^^^^^ default binding mode is `ref mut`
   |
   = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/match-ergonomics.html>
help: make the implied reference pattern explicit
   |
LL | test_pat_on_type![&mut (ref mut x,): &mut (T,)];
   |                   ++++

I'm not confident in the wording; if anyone has thoughts there, please let me know.

Relevant tracking issue: #131414

@rustbot label A-diagnostics A-patterns A-edition-2024

[rustbot]

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in match checking

cc @Nadrieril

[compiler-errors]

r? nadrieril

[traviscross]

Looks great. Nice touch on showing the default binding mode in the diagnostic.

[Nadrieril]

I hoped we could avoid mentioning default binding modes to users but I don't know how else to explain what's happening. The wordings you chose sound good to me.

[Nadrieril]

The default binding mode is really a property of the whole subpattern. Could you change this label to something like "this reference/binding mode" and add another label that points to the whole subpattern with "default binding mode is X"?

[dianne]

Done! I'm a little wary about labeling the whole subpattern for reference patterns, though. It looks good for bindings, but with reference patterns it kind of looks like it's highlighting the inside of the pattern, where the default binding mode will be different and can potentially overlap other labels. I've added a test to illustrate:

LL |     let [&mut [ref a]] = &mut [&mut &[0]];
   |          ^^^^--^^^---
   |          |     |
   |          |     this binding modifier
   |          |     default binding mode is `ref`
   |          this reference pattern
   |          default binding mode is `ref mut`

I wonder if it'd look good to instead put the "default binding mode is X" label where the default binding mode is introduced.. that'd give more information to users too, but I'm not sure how readable it'd be. I'll experiment and fix another small formatting issue before marking this as ready for review.

[Nadrieril]

Hm that's not very legible indeed. I think there might be an option so that labels are kept more separate than this for legibility? I'm not sure though.

I wonder if it'd look good to instead put the "default binding mode is X" label where the default binding mode is introduced

Good point, that could be good yes! I'm thinking this might work well if we could separate it so we can explain a bit more, e.g.

LL |     let [&mut [ref a]] = &mut [&mut &[0]];
   |          ^^^^
   |          |
   |          reference pattern not allowed under `ref mut` default binding mode
   |
LL |     let [&mut [ref a]] = &mut [&mut &[0]];
   |         ^^^^^^^^^^^^^^
   |         |
   |         the default binding mode is `ref mut`, introduced here
help: the default binding mode changed to `ref mut` because this has type `&mut _`

[dianne]

I think I know how to do that! I like the help too. Would each labeled reference pattern / binding modifier also get its own note? Would they be grouped if their default binding modes are introduced at the same span? I'll experiment and see how it looks. Maybe the underline could disappear where the default binding mode changes, too.. that might just be more confusing though.

[traviscross]

We could show each transition of the default binding mode leading all the way down to the binding. I'm not really sure how serious I am about that suggestion. On the one hand, it seems maybe a bit much, but on the other, it probably would improve understanding.

We have other errors, like our cycle errors, that carefully show each step. It's maybe not inconceivable.

(Though to really make sense of it, we'd probably have to elaborate the type as well. Anyway, keeping it simple is fine too. Seeing the default binding mode annotated at all made me smile.)

[dianne]

@dianne I think the "this" is not very clear when it's directly in the diagnostic message. maybe something like:

How does this look? I wanted to get something that worked for both mutable and shared references, in case we're labeling both ref and ref mut default binding modes, so we can have just one help after all the notes (any more feels excessive):

note: the default binding mode changed to `ref`
  --> $DIR/migration_lint.rs:57:9
   |
LL |     let Foo(&x) = &Foo(&0);
   |         ^^^^^^^ this matches on type `&_`
   = help: matching on a reference type with a non-reference pattern changes the default binding mode

Looking at it as a whole though, the note's message feels a bit redundant with the labels on the main diagnostic, so I tried moving the help into the note again:

note: matching on type `&_` with a non-`&` pattern changes the default binding mode
  --> $DIR/migration_lint.rs:57:9
   |
LL |     let Foo(&x) = &Foo(&0);
   |         ^^^^^^^ this matches on type `&_`

But then ref is missing.. maybe it could go at the end of the note or the label? e.g. "matching on type &_ with a non-& pattern changes the default binding mode to ref". It's a bit long, but all the information's there, at least.

I'm worried we might be overdoing this x)

Likely, yeah. ^^;

[Nadrieril]

I like "matching on a reference type with a non-reference pattern changes the default binding mode", it's very clear. do we actually need to say which binding mode there is? saying "non-default binding mode" could be enough? except if the user doesn't know what a binding mode is I guess ><

[dianne]

Ah, good point. I'm not too worried about terminology; the diagnostic links to the edition guide, which explains it. I think hoping people follow the link if they don't know the terms is the best we can do.

[dianne]

as of a5cc4cb, it now looks like this:

error: reference patterns may only be written when the default binding mode is `move` in Rust 2024
  --> $DIR/migration_lint.rs:57:13
   |
LL |     let Foo(&x) = &Foo(&0);
   |             ^ reference pattern not allowed under `ref` default binding mode
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/match-ergonomics.html>
note: matching on a reference type with a non-reference pattern changes the default binding mode
  --> $DIR/migration_lint.rs:57:9
   |
LL |     let Foo(&x) = &Foo(&0);
   |         ^^^^^^^ this matches on type `&_`
help: make the implied reference pattern explicit
   |
LL |     let &Foo(&x) = &Foo(&0);
   |         +

[traviscross]

Looks great.

[dianne]

I've experimentally added a commit that labels the full span of the default binding mode. It's unfortunately a bit hard to read in complex cases. Making it only point to where the default binding mode is introduced (rather than underlining the whole subpattern) would reduce some of the clutter but I found it makes it harder to tell what's going on. I'm not sure what the right tradeoff is. The code's also a bit messy since it was an experiment; the organization and bookkeeping done in #136496 would hopefully make it cleaner and keep it out of the way (that PR moves all the migration logic THIR construction to a separate file). In any case, @rustbot ready

I also snuck in a slight tweak to the labels' spans: previously if you had excess parens like in let [&(_)] = &[&0];, the label for the & would also highlight the (. Now it doesn't.

[Nadrieril]

I replied about the spans, I think we can make it more legible by splitting the diagnostic in two, though I'm not clear on how to tell the diagnostics API to do that.

Regarding #136496, I'm hoping to merge this first so we can backport it to beta before the edition goes stable on feb 20th. I'm fine with temporarily messy diagnostic code. I hope this isn't too complex to be backported.