Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pattern Migration 2024: reword to make sense on all Editions #136475

Closed
49 changes: 28 additions & 21 deletions compiler/rustc_hir_typeck/src/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -804,7 +804,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

// Determine the binding mode...
let bm = match user_bind_annot {
BindingMode(ByRef::No, Mutability::Mut) if matches!(def_br, ByRef::Yes(_)) => {
BindingMode(ByRef::No, Mutability::Mut) if let ByRef::Yes(def_br_mutbl) = def_br => {
// Only mention the experimental `mut_ref` feature if if we're in edition 2024 and
// using other experimental matching features compatible with it.
if pat.span.at_least_rust_2024()
Expand All @@ -826,22 +826,22 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// `mut` resets the binding mode on edition <= 2021
self.add_rust_2024_migration_desugared_pat(
pat_info.top_info.hir_id,
pat.span,
pat,
ident.span,
"requires binding by-value, but the implicit default is by-reference",
def_br_mutbl,
);
BindingMode(ByRef::No, Mutability::Mut)
}
}
BindingMode(ByRef::No, mutbl) => BindingMode(def_br, mutbl),
BindingMode(ByRef::Yes(_), _) => {
if matches!(def_br, ByRef::Yes(_)) {
if let ByRef::Yes(def_br_mutbl) = def_br {
// `ref`/`ref mut` overrides the binding mode on edition <= 2021
self.add_rust_2024_migration_desugared_pat(
pat_info.top_info.hir_id,
pat.span,
pat,
ident.span,
"cannot override to bind by-reference when that is the implicit default",
def_br_mutbl,
);
}
user_bind_annot
Expand Down Expand Up @@ -2378,9 +2378,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
pat_info.binding_mode = ByRef::No;
self.add_rust_2024_migration_desugared_pat(
pat_info.top_info.hir_id,
pat.span,
pat,
inner.span,
"cannot implicitly match against multiple layers of reference",
inh_mut,
)
}
}
Expand Down Expand Up @@ -2770,33 +2770,40 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fn add_rust_2024_migration_desugared_pat(
&self,
pat_id: HirId,
subpat_span: Span,
subpat: &'tcx Pat<'tcx>,
cutoff_span: Span,
detailed_label: &str,
def_br_mutbl: Mutability,
) {
// Try to trim the span we're labeling to just the `&` or binding mode that's an issue.
// If the subpattern's span is is from an expansion, the emitted label will not be trimmed.
let source_map = self.tcx.sess.source_map();
let cutoff_span = source_map
.span_extend_prev_while(cutoff_span, char::is_whitespace)
.unwrap_or(cutoff_span);
// Ensure we use the syntax context and thus edition of `subpat_span`; this will be a hard
// Ensure we use the syntax context and thus edition of `subpat.span`; this will be a hard
// error if the subpattern is of edition >= 2024.
let trimmed_span = subpat_span.until(cutoff_span).with_ctxt(subpat_span.ctxt());
let trimmed_span = subpat.span.until(cutoff_span).with_ctxt(subpat.span.ctxt());

// Only provide a detailed label if the problematic subpattern isn't from an expansion.
// In the case that it's from a macro, we'll add a more detailed note in the emitter.
let desc = if subpat_span.from_expansion() {
"default binding mode is reset within expansion"
let desc = if subpat.span.from_expansion() {
"occurs within expansion"
Nadrieril marked this conversation as resolved.
Show resolved Hide resolved
} else {
detailed_label
match def_br_mutbl {
Mutability::Not => "default binding mode is `ref`",
Mutability::Mut => "default binding mode is `ref mut`",
}
};

self.typeck_results
.borrow_mut()
.rust_2024_migration_desugared_pats_mut()
.entry(pat_id)
.or_default()
.push((trimmed_span, desc.to_owned()));
let mut typeck_results = self.typeck_results.borrow_mut();
let mut table = typeck_results.rust_2024_migration_desugared_pats_mut();
let info = table.entry(pat_id).or_default();

info.labels.push((trimmed_span, desc.to_owned()));
if matches!(subpat.kind, PatKind::Binding(_, _, _, _)) {
info.bad_modifiers |= true;
} else {
info.bad_ref_pats |= true;
}
}
}
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ pub use self::sty::{
pub use self::trait_def::TraitDef;
pub use self::typeck_results::{
CanonicalUserType, CanonicalUserTypeAnnotation, CanonicalUserTypeAnnotations, IsIdentity,
TypeckResults, UserType, UserTypeAnnotationIndex, UserTypeKind,
Rust2024IncompatiblePatInfo, TypeckResults, UserType, UserTypeAnnotationIndex, UserTypeKind,
};
pub use self::visit::{TypeSuperVisitable, TypeVisitable, TypeVisitableExt, TypeVisitor};
use crate::error::{OpaqueHiddenTypeMismatch, TypeMismatchReason};
Expand Down
22 changes: 17 additions & 5 deletions compiler/rustc_middle/src/ty/typeck_results.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@ pub struct TypeckResults<'tcx> {
/// Stores the actual binding mode for all instances of [`BindingMode`].
pat_binding_modes: ItemLocalMap<BindingMode>,

/// Top-level patterns whose match ergonomics need to be desugared by the Rust 2021 -> 2024
/// migration lint. Problematic subpatterns are stored in the `Vec` for the lint to highlight.
rust_2024_migration_desugared_pats: ItemLocalMap<Vec<(Span, String)>>,
/// Top-level patterns incompatible with Rust 2024's match ergonomics. These will be translated
/// to a form valid in all Editions, either as a lint diagnostic or hard error.
rust_2024_migration_desugared_pats: ItemLocalMap<Rust2024IncompatiblePatInfo>,

/// Stores the types which were implicitly dereferenced in pattern binding modes
/// for later usage in THIR lowering. For example,
Expand Down Expand Up @@ -420,7 +420,7 @@ impl<'tcx> TypeckResults<'tcx> {

pub fn rust_2024_migration_desugared_pats(
&self,
) -> LocalTableInContext<'_, Vec<(Span, String)>> {
) -> LocalTableInContext<'_, Rust2024IncompatiblePatInfo> {
LocalTableInContext {
hir_owner: self.hir_owner,
data: &self.rust_2024_migration_desugared_pats,
Expand All @@ -429,7 +429,7 @@ impl<'tcx> TypeckResults<'tcx> {

pub fn rust_2024_migration_desugared_pats_mut(
&mut self,
) -> LocalTableInContextMut<'_, Vec<(Span, String)>> {
) -> LocalTableInContextMut<'_, Rust2024IncompatiblePatInfo> {
LocalTableInContextMut {
hir_owner: self.hir_owner,
data: &mut self.rust_2024_migration_desugared_pats,
Expand Down Expand Up @@ -811,3 +811,15 @@ impl<'tcx> std::fmt::Display for UserTypeKind<'tcx> {
}
}
}

/// Information on a pattern incompatible with Rust 2024, for use by the error/migration diagnostic
/// emitted during THIR construction.
#[derive(TyEncodable, TyDecodable, Debug, HashStable, Default)]
pub struct Rust2024IncompatiblePatInfo {
/// Labels for subpatterns incompatible with Rust 2024.
pub labels: Vec<(Span, String)>,
/// Whether any binding modifiers occur under a non-`move` default binding mode.
pub bad_modifiers: bool,
/// Whether any `&` or `&mut` patterns occur under a non-`move` default binding mode.
pub bad_ref_pats: bool,
}
11 changes: 10 additions & 1 deletion compiler/rustc_mir_build/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,16 @@ mir_build_pointer_pattern = function pointers and raw pointers not derived from

mir_build_privately_uninhabited = pattern `{$witness_1}` is currently uninhabited, but this variant contains private fields which may become inhabited in the future

mir_build_rust_2024_incompatible_pat = this pattern relies on behavior which may change in edition 2024
mir_build_rust_2024_incompatible_pat = {$bad_modifiers ->
*[true] binding modifiers{$bad_ref_pats ->
*[true] {" "}and reference patterns
[false] {""}
}
[false] reference patterns
} may only be written when the default binding mode is `move`{$is_hard_error ->
*[true] {""}
[false] {" "}in Rust 2024
}

mir_build_static_in_pattern = statics cannot be referenced in patterns
.label = can't be used in patterns
Expand Down
13 changes: 7 additions & 6 deletions compiler/rustc_mir_build/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1097,20 +1097,21 @@ pub(crate) enum MiscPatternSuggestion {

#[derive(LintDiagnostic)]
#[diag(mir_build_rust_2024_incompatible_pat)]
pub(crate) struct Rust2024IncompatiblePat<'a> {
pub(crate) struct Rust2024IncompatiblePat {
#[subdiagnostic]
pub(crate) sugg: Rust2024IncompatiblePatSugg<'a>,
pub(crate) sugg: Rust2024IncompatiblePatSugg,
pub(crate) bad_modifiers: bool,
pub(crate) bad_ref_pats: bool,
pub(crate) is_hard_error: bool,
}

pub(crate) struct Rust2024IncompatiblePatSugg<'a> {
pub(crate) struct Rust2024IncompatiblePatSugg {
pub(crate) suggestion: Vec<(Span, String)>,
pub(crate) ref_pattern_count: usize,
pub(crate) binding_mode_count: usize,
/// Labeled spans for subpatterns invalid in Rust 2024.
pub(crate) labels: &'a [(Span, String)],
}

impl<'a> Subdiagnostic for Rust2024IncompatiblePatSugg<'a> {
impl Subdiagnostic for Rust2024IncompatiblePatSugg {
fn add_to_diag_with<G: EmissionGuarantee, F: SubdiagMessageOp<G>>(
self,
diag: &mut Diag<'_, G>,
Expand Down
38 changes: 22 additions & 16 deletions compiler/rustc_mir_build/src/thir/pattern/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ struct PatCtxt<'a, 'tcx> {
typeck_results: &'a ty::TypeckResults<'tcx>,

/// Used by the Rust 2024 migration lint.
rust_2024_migration_suggestion: Option<Rust2024IncompatiblePatSugg<'a>>,
rust_2024_migration_suggestion: Option<Rust2024IncompatiblePatSugg>,
}

pub(super) fn pat_from_hir<'a, 'tcx>(
Expand All @@ -44,44 +44,50 @@ pub(super) fn pat_from_hir<'a, 'tcx>(
typeck_results: &'a ty::TypeckResults<'tcx>,
pat: &'tcx hir::Pat<'tcx>,
) -> Box<Pat<'tcx>> {
let migration_info = typeck_results.rust_2024_migration_desugared_pats().get(pat.hir_id);
let mut pcx = PatCtxt {
tcx,
typing_env,
typeck_results,
rust_2024_migration_suggestion: typeck_results
.rust_2024_migration_desugared_pats()
.get(pat.hir_id)
.map(|labels| Rust2024IncompatiblePatSugg {
suggestion: Vec::new(),
ref_pattern_count: 0,
binding_mode_count: 0,
labels: labels.as_slice(),
}),
rust_2024_migration_suggestion: migration_info.and(Some(Rust2024IncompatiblePatSugg {
suggestion: Vec::new(),
ref_pattern_count: 0,
binding_mode_count: 0,
})),
};
let result = pcx.lower_pattern(pat);
debug!("pat_from_hir({:?}) = {:?}", pat, result);
if let Some(sugg) = pcx.rust_2024_migration_suggestion {
let mut spans = MultiSpan::from_spans(sugg.labels.iter().map(|(span, _)| *span).collect());
for (span, label) in sugg.labels {
if let Some(info) = migration_info {
let sugg = pcx.rust_2024_migration_suggestion.expect("suggestion should be present");
let mut spans = MultiSpan::from_spans(info.labels.iter().map(|(span, _)| *span).collect());
for (span, label) in &info.labels {
spans.push_span_label(*span, label.clone());
}
// If a relevant span is from at least edition 2024, this is a hard error.
let is_hard_error = spans.primary_spans().iter().any(|span| span.at_least_rust_2024());
if is_hard_error {
let mut err =
tcx.dcx().struct_span_err(spans, fluent::mir_build_rust_2024_incompatible_pat);
if let Some(info) = lint::builtin::RUST_2024_INCOMPATIBLE_PAT.future_incompatible {
if let Some(lint_info) = lint::builtin::RUST_2024_INCOMPATIBLE_PAT.future_incompatible {
// provide the same reference link as the lint
err.note(format!("for more information, see {}", info.reference));
err.note(format!("for more information, see {}", lint_info.reference));
}
err.arg("bad_modifiers", info.bad_modifiers);
err.arg("bad_ref_pats", info.bad_ref_pats);
err.arg("is_hard_error", true);
err.subdiagnostic(sugg);
err.emit();
} else {
tcx.emit_node_span_lint(
lint::builtin::RUST_2024_INCOMPATIBLE_PAT,
pat.hir_id,
spans,
Rust2024IncompatiblePat { sugg },
Rust2024IncompatiblePat {
sugg,
bad_modifiers: info.bad_modifiers,
bad_ref_pats: info.bad_ref_pats,
is_hard_error,
},
);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0308]: mismatched types
--> $DIR/ref-binding-on-inh-ref-errors.rs:60:10
--> $DIR/ref-binding-on-inh-ref-errors.rs:54:10
|
LL | let [&mut ref x] = &[&mut 0];
| ^^^^^
Expand All @@ -10,11 +10,11 @@ help: replace this `&mut` pattern with `&`
LL | let [&ref x] = &[&mut 0];
| ~

error: this pattern relies on behavior which may change in edition 2024
--> $DIR/ref-binding-on-inh-ref-errors.rs:74:10
error: binding modifiers may only be written when the default binding mode is `move`
--> $DIR/ref-binding-on-inh-ref-errors.rs:67:10
|
LL | let [ref mut x] = &[0];
| ^^^^^^^ cannot override to bind by-reference when that is the implicit default
| ^^^^^^^ default binding mode is `ref`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Contributor Author

@dianne dianne Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Nadrieril Nadrieril Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 _`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@traviscross traviscross Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. ^^;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ><

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
   |         +

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

|
= 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
Expand All @@ -23,40 +23,40 @@ LL | let &[ref mut x] = &[0];
| +

error[E0596]: cannot borrow data in a `&` reference as mutable
--> $DIR/ref-binding-on-inh-ref-errors.rs:74:10
--> $DIR/ref-binding-on-inh-ref-errors.rs:67:10
|
LL | let [ref mut x] = &[0];
| ^^^^^^^^^ cannot borrow as mutable

error: this pattern relies on behavior which may change in edition 2024
--> $DIR/ref-binding-on-inh-ref-errors.rs:83:10
error: binding modifiers may only be written when the default binding mode is `move`
--> $DIR/ref-binding-on-inh-ref-errors.rs:75:10
|
LL | let [ref x] = &[0];
| ^^^ cannot override to bind by-reference when that is the implicit default
| ^^^ default binding mode is `ref`
|
= 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 &[ref x] = &[0];
| +

error: this pattern relies on behavior which may change in edition 2024
--> $DIR/ref-binding-on-inh-ref-errors.rs:88:10
error: binding modifiers may only be written when the default binding mode is `move`
--> $DIR/ref-binding-on-inh-ref-errors.rs:79:10
|
LL | let [ref x] = &mut [0];
| ^^^ cannot override to bind by-reference when that is the implicit default
| ^^^ 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 | let &mut [ref x] = &mut [0];
| ++++

error: this pattern relies on behavior which may change in edition 2024
--> $DIR/ref-binding-on-inh-ref-errors.rs:93:10
error: binding modifiers may only be written when the default binding mode is `move`
--> $DIR/ref-binding-on-inh-ref-errors.rs:83:10
|
LL | let [ref mut x] = &mut [0];
| ^^^^^^^ cannot override to bind by-reference when that is the implicit default
| ^^^^^^^ 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
Expand Down
Loading
Loading