Clarify the match ergonomics 2024 migration lint's output

[dianne]

This makes a few changes:

• Rather than using the whole pattern as a span for the lint, this collects spans for each problematic default binding mode reset and labels them with why they're problems.
• The lint's suggestions are now verbose-styled, so that it's clear what's being suggested vs. what's problematic.
• The wording is now less technical, and the hard error version of this diagnostic now links to the same reference material as the lint (currently an unwritten page of the edition guide).

I'm not totally confident in the wording or formatting, so I'd appreciate feedback on that in particular. I tried to draw a connection with word choice between the labels and the suggestion, but it might be imprecise, unclear, or cluttered. If so, it might be worth making the labels more terse and adding notes that explain them, but that's harder to read in a way too.

cc @Nadrieril @Jules-Bertholet

Closes #133854. For reference, the error from that issue becomes:

error: pattern uses features incompatible with edition 2024
  --> $DIR/remove-me.rs:6:25
   |
LL |     map.iter().filter(|(&(_x, _y), &_c)| false);
   |                         ^          ^ cannot implicitly match against multiple layers of reference
   |                         |
   |                         cannot implicitly match against multiple layers of reference
   |
help: make the implied reference pattern explicit
   |
LL |     map.iter().filter(|&(&(_x, _y), &_c)| false);
   |                        +

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
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]

I'm not totally certain if the span logic is resilient enough to ensure that a trimmed/modified span will always have the same edition as one of the input spans. Like, pat_span may be from edition 2024 and inner_span may be from 2021, and it's not totally deterministic what we'd be doing in that case.

To avoid worrying about this subtle change in behavior, could you record the pat_span (which is the span from which we get the definition in the implementation today) or perhaps just its edition in this table too, so we don't implicitly rely on trimmed_span's edition which may be totally random depending on the impl of Span::until and SourceMap::span_extend_prev_while.

[dianne]

ah, yeah. good point. I've opted to re-add the syntax context from pat.span to the trimmed span, to avoid needing a separate span or boolean field (and thus a tuple or struct). using Span::trim_end would be a less roundabout way of doing this, since it just uses SpanData::with_hi to do the trimming, but that would rely on Span::trim_end keeping this behavior. we could also inline it to use with_hi directly, but that's less readable.

[compiler-errors]

maybe we should say "this pattern relies on behavior which may change in edition 2024" ?

[dianne]

done; I like that wording. at first I was unsure about using it for hard errors too, but I think it works there (until exactly what's happening with patterns in edition 2024 gets sorted out, but it'll need changing regardless then)

[bors]

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

[dianne]

Rebased: #134368 changed the migration lint's reference link to point to the edition guide, so this now uses that.

That's the only change to the initial commit, but I've added some additional commits as well. They're each fairly small, so I can squash them if they look good.

• When determining whether we should emit a hard error, ensure we check the editions of the subpattern(s) we've found issues with.
• Improve the primary message for the diagnostic.
• Since the edition guide seems good to link to when emitting an error as well, I've added the reference note from the lint to it.

There's a few other changes I'm considering, but which I'm less sure about, so they're not included:

• Currently, if any of the migration's suggested changes point to spans within an expansion, its applicability is downgraded to MaybeIncorrect. I'm wondering if there should be any other indication; applicability is a bit subtle from a user's perspective. This may be more of a diagnostic infra issue, though.
• The emitter already adds a note if an error originates within a macro expansion, so it may be apparent that if you get such an error in editions ≤ 2021 it's because of the macro's edition. I'm not sure, though; it may warrant another note explaining that (though think the ordering would be a bit awkward unless it can somehow be added after or as part of the emitter's note).
• If multiple subpatterns of a macro expansion reset the default binding mode, this will produce multiple instances of the "default binding mode is reset within expansion" label appearing to point to the macro's call site. It's not hard to filter the duplicates out, but they are distinct within the expansion; if you're compiling with -Z macro-backtrace on nightly it would only show the first one. Changing the diagnostic's output based on that option feels too subtle, but the current compromise is less than ideal in either case. I think the ideal for viewing the macro backtrace would be to keep the labels separate and use the more specific label wordings that get applied to spans not from expansions. Maybe that would be fine always, given the emitter's note pointing to the error being within the macro expansion? But labeling the call site with references to its internals without -Z macro-backtrace feels a bit awkward. Is there any precedent for a particular approach here?

[compiler-errors]

this looks pretty good to me. i don't have much feedback regarding the follow-ups (i'm recovering from an illness so i'd rather just r+ this now; feel free to put up additional changes separately).

@bors r+ rollup

[bors]

📌 Commit 28c6d0b has been approved by compiler-errors

It is now in the queue for this repository.

[jieyouxu]

• Currently, if any of the migration's suggested changes point to spans within an expansion, its applicability is downgraded to MaybeIncorrect. I'm wondering if there should be any other indication; applicability is a bit subtle from a user's perspective. This may be more of a diagnostic infra issue, though.

AFAIK applicability is mostly intended for tooling, especially rustfix, in relation to what suggestions can be automatically applied. To a user, this distinction should not matter -- the user won't get automatic suggestion application, which is a good thing, as it warrants user manual review and intervention.

• The emitter already adds a note if an error originates within a macro expansion, so it may be apparent that if you get such an error in editions ≤ 2021 it's because of the macro's edition. I'm not sure, though; it may warrant another note explaining that (though think the ordering would be a bit awkward unless it can somehow be added after or as part of the emitter's note).

I think this is fine. If there are follow-up reports that this is still confusing, we can address those separately. This PR as-is is already a nice improvement.

• If multiple subpatterns of a macro expansion reset the default binding mode, this will produce multiple instances of the "default binding mode is reset within expansion" label appearing to point to the macro's call site. It's not hard to filter the duplicates out, but they are distinct within the expansion; if you're compiling with -Z macro-backtrace on nightly it would only show the first one. Changing the diagnostic's output based on that option feels too subtle, but the current compromise is less than ideal in either case. I think the ideal for viewing the macro backtrace would be to keep the labels separate and use the more specific label wordings that get applied to spans not from expansions. Maybe that would be fine always, given the emitter's note pointing to the error being within the macro expansion? But labeling the call site with references to its internals without -Z macro-backtrace feels a bit awkward. Is there any precedent for a particular approach here?

I seem to recall that being tricky. If you really want to pursue that, you may have to do some fancy span ancestor searching, which, when combined with macros (which can be from other macros), usually has a whole bunch of edge cases.