Rollup merge of #136577 - dianne:simple-pat-migration-simplification, r=Nadrieril

Pattern Migration 2024: try to suggest eliding redundant binding modifiers

This is based on #136475. Only the last commit is new.

This is a simpler, more restrictive alternative to #136496, meant to partially address #136047. If a pattern can be migrated to Rust 2024 solely by removing redundant binding modifiers, this will make that suggestion; otherwise, it uses the old suggestion of making the pattern fully explicit.

Relevant tracking issue: #131414

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

r? ``@Nadrieril``

Author: matthiaskrgr

compiler/rustc_hir_typeck/src/pat.rs

@@ -812,7 +812,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()
@@ -834,22 +834,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
@@ -2386,9 +2386,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,
                     )
                 }
             }
@@ -2778,33 +2778,65 @@ 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)
+            .span_extend_prev_while(cutoff_span, |c| c.is_whitespace() || c == '(')
             .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());
+
+        let mut typeck_results = self.typeck_results.borrow_mut();
+        let mut table = typeck_results.rust_2024_migration_desugared_pats_mut();
+        // FIXME(ref_pat_eat_one_layer_2024): The migration diagnostic doesn't know how to track the
+        // default binding mode in the presence of Rule 3 or Rule 5. As a consequence, the labels it
+        // gives for default binding modes are wrong, as well as suggestions based on the default
+        // binding mode. This keeps it from making those suggestions, as doing so could panic.
+        let info = table.entry(pat_id).or_insert_with(|| ty::Rust2024IncompatiblePatInfo {
+            primary_labels: Vec::new(),
+            bad_modifiers: false,
+            bad_ref_pats: false,
+            suggest_eliding_modes: !self.tcx.features().ref_pat_eat_one_layer_2024()
+                && !self.tcx.features().ref_pat_eat_one_layer_2024_structural(),
+        });
 
         // 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 from_expansion = subpat.span.from_expansion();
+        let primary_label = if from_expansion {
+            // NB: This wording assumes the only expansions that can produce problematic reference
+            // patterns and bindings are macros. If a desugaring or AST pass is added that can do
+            // so, we may want to inspect the span's source callee or macro backtrace.
+            "occurs within macro expansion".to_owned()
         } else {
-            detailed_label
+            let pat_kind = if let PatKind::Binding(user_bind_annot, _, _, _) = subpat.kind {
+                info.bad_modifiers |= true;
+                // If the user-provided binding modifier doesn't match the default binding mode, we'll
+                // need to suggest reference patterns, which can affect other bindings.
+                // For simplicity, we opt to suggest making the pattern fully explicit.
+                info.suggest_eliding_modes &=
+                    user_bind_annot == BindingMode(ByRef::Yes(def_br_mutbl), Mutability::Not);
+                "binding modifier"
+            } else {
+                info.bad_ref_pats |= true;
+                // For simplicity, we don't try to suggest eliding reference patterns. Thus, we'll
+                // suggest adding them instead, which can affect the types assigned to bindings.
+                // As such, we opt to suggest making the pattern fully explicit.
+                info.suggest_eliding_modes = false;
+                "reference pattern"
+            };
+            let dbm_str = match def_br_mutbl {
+                Mutability::Not => "ref",
+                Mutability::Mut => "ref mut",
+            };
+            format!("{pat_kind} not allowed under `{dbm_str}` default binding mode")
         };
-
-        self.typeck_results
-            .borrow_mut()
-            .rust_2024_migration_desugared_pats_mut()
-            .entry(pat_id)
-            .or_default()
-            .push((trimmed_span, desc.to_owned()));
+        info.primary_labels.push((trimmed_span, primary_label));
     }
 }

compiler/rustc_middle/src/ty/mod.rs

@@ -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};

compiler/rustc_middle/src/ty/typeck_results.rs

@@ -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,
@@ -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,
@@ -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,
@@ -811,3 +811,17 @@ 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)]
+pub struct Rust2024IncompatiblePatInfo {
+    /// Labeled spans for `&`s, `&mut`s, and binding modifiers incompatible with Rust 2024.
+    pub primary_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,
+    /// If `true`, we can give a simpler suggestion solely by eliding explicit binding modifiers.
+    pub suggest_eliding_modes: bool,
+}

compiler/rustc_mir_build/messages.ftl

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

compiler/rustc_mir_build/src/errors.rs

@@ -1,3 +1,4 @@
+use rustc_data_structures::fx::FxIndexMap;
 use rustc_errors::codes::*;
 use rustc_errors::{
     Applicability, Diag, DiagArgValue, DiagCtxtHandle, Diagnostic, EmissionGuarantee, Level,
@@ -1097,41 +1098,70 @@ 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 {
+    /// If true, our suggestion is to elide explicit binding modifiers.
+    /// If false, our suggestion is to make the pattern fully explicit.
+    pub(crate) suggest_eliding_modes: bool,
     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)],
+    /// Internal state: the ref-mutability of the default binding mode at the subpattern being
+    /// lowered, with the span where it was introduced. `None` for a by-value default mode.
+    pub(crate) default_mode_span: Option<(Span, ty::Mutability)>,
+    /// Labels for where incompatibility-causing by-ref default binding modes were introduced.
+    pub(crate) default_mode_labels: FxIndexMap<Span, ty::Mutability>,
 }
 
-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>,
         _f: &F,
     ) {
+        // Format and emit explanatory notes about default binding modes. Reversing the spans' order
+        // means if we have nested spans, the innermost ones will be visited first.
+        for (span, def_br_mutbl) in self.default_mode_labels.into_iter().rev() {
+            // Don't point to a macro call site.
+            if !span.from_expansion() {
+                let note_msg = "matching on a reference type with a non-reference pattern changes the default binding mode";
+                let label_msg =
+                    format!("this matches on type `{}_`", def_br_mutbl.ref_prefix_str());
+                let mut label = MultiSpan::from(span);
+                label.push_span_label(span, label_msg);
+                diag.span_note(label, note_msg);
+            }
+        }
+
+        // Format and emit the suggestion.
         let applicability =
             if self.suggestion.iter().all(|(span, _)| span.can_be_used_for_suggestions()) {
                 Applicability::MachineApplicable
             } else {
                 Applicability::MaybeIncorrect
             };
-        let plural_derefs = pluralize!(self.ref_pattern_count);
-        let and_modes = if self.binding_mode_count > 0 {
-            format!(" and variable binding mode{}", pluralize!(self.binding_mode_count))
+        let msg = if self.suggest_eliding_modes {
+            let plural_modes = pluralize!(self.binding_mode_count);
+            format!("remove the unnecessary binding modifier{plural_modes}")
         } else {
-            String::new()
+            let plural_derefs = pluralize!(self.ref_pattern_count);
+            let and_modes = if self.binding_mode_count > 0 {
+                format!(" and variable binding mode{}", pluralize!(self.binding_mode_count))
+            } else {
+                String::new()
+            };
+            format!("make the implied reference pattern{plural_derefs}{and_modes} explicit")
         };
-        diag.multipart_suggestion_verbose(
-            format!("make the implied reference pattern{plural_derefs}{and_modes} explicit"),
-            self.suggestion,
-            applicability,
-        );
+        // FIXME(dianne): for peace of mind, don't risk emitting a 0-part suggestion (that panics!)
+        if !self.suggestion.is_empty() {
+            diag.multipart_suggestion_verbose(msg, self.suggestion, applicability);
+        }
     }
 }