Rollup merge of #128304 - Zalathar:thir-pat-display, r=Nadrieril

Isolate the diagnostic code that expects `thir::Pat` to be printable

Currently, `thir::Pat` implements `fmt::Display` (and `IntoDiagArg`) directly, for use by a few diagnostics.

That makes it tricky to experiment with alternate representations for THIR patterns, because the patterns currently need to be printable on their own. That immediately rules out possibilities like storing subpatterns as a `PatId` index into a central list (instead of the current directly-owned `Box<Pat>`).

This PR therefore takes an incremental step away from that obstacle, by removing `thir::Pat` from diagnostic structs in `rustc_pattern_analysis`, and hiding the pattern-printing process behind a single public `Pat::to_string` method. Doing so makes it easier to identify and update the code that wants to print patterns, and gives a place to pass in additional context in the future if necessary.

---

I'm currently not sure whether switching over to `PatId` is actually desirable or not, but I think this change makes sense on its own merits, by reducing the coupling between `thir::Pat` and the pattern-analysis error types.

Author: matthiaskrgr

compiler/rustc_middle/src/thir.rs

@@ -13,7 +13,6 @@ use std::fmt;
 use std::ops::Index;
 
 use rustc_ast::{InlineAsmOptions, InlineAsmTemplatePiece};
-use rustc_errors::{DiagArgValue, IntoDiagArg};
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
 use rustc_hir::{BindingMode, ByRef, HirId, MatchSource, RangeEnd};
@@ -702,12 +701,6 @@ impl<'tcx> Pat<'tcx> {
     }
 }
 
-impl<'tcx> IntoDiagArg for Pat<'tcx> {
-    fn into_diag_arg(self) -> DiagArgValue {
-        format!("{self}").into_diag_arg()
-    }
-}
-
 #[derive(Clone, Debug, HashStable, TypeVisitable)]
 pub struct Ascription<'tcx> {
     pub annotation: CanonicalUserTypeAnnotation<'tcx>,
@@ -1080,8 +1073,33 @@ impl<'tcx> PatRangeBoundary<'tcx> {
     }
 }
 
-impl<'tcx> fmt::Display for Pat<'tcx> {
+impl<'tcx> Pat<'tcx> {
+    /// Prints a [`Pat`] to an owned string, for user-facing diagnostics.
+    ///
+    /// If we ever switch over to storing subpatterns as `PatId`, this will also
+    /// need to take a context that can resolve IDs to subpatterns.
+    pub fn to_string(&self) -> String {
+        format!("{}", self.display())
+    }
+
+    /// Used internally by [`fmt::Display`] for [`PatDisplay`].
+    fn display(&self) -> PatDisplay<'_, 'tcx> {
+        PatDisplay { pat: self }
+    }
+}
+
+/// Wrapper around [`&Pat<'tcx>`][`Pat`] that implements [`fmt::Display`].
+///
+/// If we ever switch over to storing subpatterns as `PatId`, this will also
+/// need to hold a context that can resolve IDs to subpatterns.
+struct PatDisplay<'pat, 'tcx> {
+    pat: &'pat Pat<'tcx>,
+}
+
+impl<'pat, 'tcx> fmt::Display for PatDisplay<'pat, 'tcx> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let &Self { pat } = self;
+
         // Printing lists is a chore.
         let mut first = true;
         let mut start_or_continue = |s| {
@@ -1094,20 +1112,22 @@ impl<'tcx> fmt::Display for Pat<'tcx> {
         };
         let mut start_or_comma = || start_or_continue(", ");
 
-        match self.kind {
+        match pat.kind {
             PatKind::Wild => write!(f, "_"),
             PatKind::Never => write!(f, "!"),
-            PatKind::AscribeUserType { ref subpattern, .. } => write!(f, "{subpattern}: _"),
+            PatKind::AscribeUserType { ref subpattern, .. } => {
+                write!(f, "{}: _", subpattern.display())
+            }
             PatKind::Binding { name, mode, ref subpattern, .. } => {
                 f.write_str(mode.prefix_str())?;
                 write!(f, "{name}")?;
                 if let Some(ref subpattern) = *subpattern {
-                    write!(f, " @ {subpattern}")?;
+                    write!(f, " @ {}", subpattern.display())?;
                 }
                 Ok(())
             }
             PatKind::Variant { ref subpatterns, .. } | PatKind::Leaf { ref subpatterns } => {
-                let variant_and_name = match self.kind {
+                let variant_and_name = match pat.kind {
                     PatKind::Variant { adt_def, variant_index, .. } => ty::tls::with(|tcx| {
                         let variant = adt_def.variant(variant_index);
                         let adt_did = adt_def.did();
@@ -1120,7 +1140,7 @@ impl<'tcx> fmt::Display for Pat<'tcx> {
                         };
                         Some((variant, name))
                     }),
-                    _ => self.ty.ty_adt_def().and_then(|adt_def| {
+                    _ => pat.ty.ty_adt_def().and_then(|adt_def| {
                         if !adt_def.is_enum() {
                             ty::tls::with(|tcx| {
                                 Some((adt_def.non_enum_variant(), tcx.def_path_str(adt_def.did())))
@@ -1145,11 +1165,11 @@ impl<'tcx> fmt::Display for Pat<'tcx> {
                                 continue;
                             }
                             let name = variant.fields[p.field].name;
-                            write!(f, "{}{}: {}", start_or_comma(), name, p.pattern)?;
+                            write!(f, "{}{}: {}", start_or_comma(), name, p.pattern.display())?;
                             printed += 1;
                         }
 
-                        let is_union = self.ty.ty_adt_def().is_some_and(|adt| adt.is_union());
+                        let is_union = pat.ty.ty_adt_def().is_some_and(|adt| adt.is_union());
                         if printed < variant.fields.len() && (!is_union || printed == 0) {
                             write!(f, "{}..", start_or_comma())?;
                         }
@@ -1168,14 +1188,14 @@ impl<'tcx> fmt::Display for Pat<'tcx> {
                         // Common case: the field is where we expect it.
                         if let Some(p) = subpatterns.get(i) {
                             if p.field.index() == i {
-                                write!(f, "{}", p.pattern)?;
+                                write!(f, "{}", p.pattern.display())?;
                                 continue;
                             }
                         }
 
                         // Otherwise, we have to go looking for it.
                         if let Some(p) = subpatterns.iter().find(|p| p.field.index() == i) {
-                            write!(f, "{}", p.pattern)?;
+                            write!(f, "{}", p.pattern.display())?;
                         } else {
                             write!(f, "_")?;
                         }
@@ -1186,45 +1206,45 @@ impl<'tcx> fmt::Display for Pat<'tcx> {
                 Ok(())
             }
             PatKind::Deref { ref subpattern } => {
-                match self.ty.kind() {
+                match pat.ty.kind() {
                     ty::Adt(def, _) if def.is_box() => write!(f, "box ")?,
                     ty::Ref(_, _, mutbl) => {
                         write!(f, "&{}", mutbl.prefix_str())?;
                     }
-                    _ => bug!("{} is a bad Deref pattern type", self.ty),
+                    _ => bug!("{} is a bad Deref pattern type", pat.ty),
                 }
-                write!(f, "{subpattern}")
+                write!(f, "{}", subpattern.display())
             }
             PatKind::DerefPattern { ref subpattern, .. } => {
-                write!(f, "deref!({subpattern})")
+                write!(f, "deref!({})", subpattern.display())
             }
             PatKind::Constant { value } => write!(f, "{value}"),
             PatKind::InlineConstant { def: _, ref subpattern } => {
-                write!(f, "{} (from inline const)", subpattern)
+                write!(f, "{} (from inline const)", subpattern.display())
             }
             PatKind::Range(ref range) => write!(f, "{range}"),
             PatKind::Slice { ref prefix, ref slice, ref suffix }
             | PatKind::Array { ref prefix, ref slice, ref suffix } => {
                 write!(f, "[")?;
                 for p in prefix.iter() {
-                    write!(f, "{}{}", start_or_comma(), p)?;
+                    write!(f, "{}{}", start_or_comma(), p.display())?;
                 }
                 if let Some(ref slice) = *slice {
                     write!(f, "{}", start_or_comma())?;
                     match slice.kind {
                         PatKind::Wild => {}
-                        _ => write!(f, "{slice}")?,
+                        _ => write!(f, "{}", slice.display())?,
                     }
                     write!(f, "..")?;
                 }
                 for p in suffix.iter() {
-                    write!(f, "{}{}", start_or_comma(), p)?;
+                    write!(f, "{}{}", start_or_comma(), p.display())?;
                 }
                 write!(f, "]")
             }
             PatKind::Or { ref pats } => {
                 for pat in pats.iter() {
-                    write!(f, "{}{}", start_or_continue(" | "), pat)?;
+                    write!(f, "{}{}", start_or_continue(" | "), pat.display())?;
                 }
                 Ok(())
             }

compiler/rustc_mir_build/src/errors.rs

@@ -858,7 +858,7 @@ pub(crate) struct PatternNotCovered<'s, 'tcx> {
     pub(crate) span: Span,
     pub(crate) origin: &'s str,
     #[subdiagnostic]
-    pub(crate) uncovered: Uncovered<'tcx>,
+    pub(crate) uncovered: Uncovered,
     #[subdiagnostic]
     pub(crate) inform: Option<Inform>,
     #[subdiagnostic]

compiler/rustc_mir_build/src/thir/pattern/check_match.rs

@@ -1078,7 +1078,7 @@ fn report_non_exhaustive_match<'p, 'tcx>(
     let suggested_arm = if suggest_the_witnesses {
         let pattern = witnesses
             .iter()
-            .map(|witness| cx.hoist_witness_pat(witness).to_string())
+            .map(|witness| cx.print_witness_pat(witness))
             .collect::<Vec<String>>()
             .join(" | ");
         if witnesses.iter().all(|p| p.is_never_pattern()) && cx.tcx.features().never_patterns {
@@ -1196,13 +1196,13 @@ fn joined_uncovered_patterns<'p, 'tcx>(
     witnesses: &[WitnessPat<'p, 'tcx>],
 ) -> String {
     const LIMIT: usize = 3;
-    let pat_to_str = |pat: &WitnessPat<'p, 'tcx>| cx.hoist_witness_pat(pat).to_string();
+    let pat_to_str = |pat: &WitnessPat<'p, 'tcx>| cx.print_witness_pat(pat);
     match witnesses {
         [] => bug!(),
-        [witness] => format!("`{}`", cx.hoist_witness_pat(witness)),
+        [witness] => format!("`{}`", cx.print_witness_pat(witness)),
         [head @ .., tail] if head.len() < LIMIT => {
             let head: Vec<_> = head.iter().map(pat_to_str).collect();
-            format!("`{}` and `{}`", head.join("`, `"), cx.hoist_witness_pat(tail))
+            format!("`{}` and `{}`", head.join("`, `"), cx.print_witness_pat(tail))
         }
         _ => {
             let (head, tail) = witnesses.split_at(LIMIT);

compiler/rustc_pattern_analysis/src/errors.rs

@@ -1,45 +1,38 @@
 use rustc_errors::{Diag, EmissionGuarantee, SubdiagMessageOp, Subdiagnostic};
 use rustc_macros::{LintDiagnostic, Subdiagnostic};
-use rustc_middle::thir::Pat;
 use rustc_middle::ty::Ty;
 use rustc_span::Span;
 
 use crate::rustc::{RustcPatCtxt, WitnessPat};
 
 #[derive(Subdiagnostic)]
 #[label(pattern_analysis_uncovered)]
-pub struct Uncovered<'tcx> {
+pub struct Uncovered {
     #[primary_span]
     span: Span,
     count: usize,
-    witness_1: Pat<'tcx>,
-    witness_2: Pat<'tcx>,
-    witness_3: Pat<'tcx>,
+    witness_1: String, // a printed pattern
+    witness_2: String, // a printed pattern
+    witness_3: String, // a printed pattern
     remainder: usize,
 }
 
-impl<'tcx> Uncovered<'tcx> {
-    pub fn new<'p>(
+impl Uncovered {
+    pub fn new<'p, 'tcx>(
         span: Span,
         cx: &RustcPatCtxt<'p, 'tcx>,
         witnesses: Vec<WitnessPat<'p, 'tcx>>,
     ) -> Self
     where
         'tcx: 'p,
     {
-        let witness_1 = cx.hoist_witness_pat(witnesses.get(0).unwrap());
+        let witness_1 = cx.print_witness_pat(witnesses.get(0).unwrap());
         Self {
             span,
             count: witnesses.len(),
             // Substitute dummy values if witnesses is smaller than 3. These will never be read.
-            witness_2: witnesses
-                .get(1)
-                .map(|w| cx.hoist_witness_pat(w))
-                .unwrap_or_else(|| witness_1.clone()),
-            witness_3: witnesses
-                .get(2)
-                .map(|w| cx.hoist_witness_pat(w))
-                .unwrap_or_else(|| witness_1.clone()),
+            witness_2: witnesses.get(1).map(|w| cx.print_witness_pat(w)).unwrap_or_default(),
+            witness_3: witnesses.get(2).map(|w| cx.print_witness_pat(w)).unwrap_or_default(),
             witness_1,
             remainder: witnesses.len().saturating_sub(3),
         }
@@ -49,19 +42,19 @@ impl<'tcx> Uncovered<'tcx> {
 #[derive(LintDiagnostic)]
 #[diag(pattern_analysis_overlapping_range_endpoints)]
 #[note]
-pub struct OverlappingRangeEndpoints<'tcx> {
+pub struct OverlappingRangeEndpoints {
     #[label]
     pub range: Span,
     #[subdiagnostic]
-    pub overlap: Vec<Overlap<'tcx>>,
+    pub overlap: Vec<Overlap>,
 }
 
-pub struct Overlap<'tcx> {
+pub struct Overlap {
     pub span: Span,
-    pub range: Pat<'tcx>,
+    pub range: String, // a printed pattern
 }
 
-impl<'tcx> Subdiagnostic for Overlap<'tcx> {
+impl Subdiagnostic for Overlap {
     fn add_to_diag_with<G: EmissionGuarantee, F: SubdiagMessageOp<G>>(
         self,
         diag: &mut Diag<'_, G>,
@@ -78,38 +71,38 @@ impl<'tcx> Subdiagnostic for Overlap<'tcx> {
 
 #[derive(LintDiagnostic)]
 #[diag(pattern_analysis_excluside_range_missing_max)]
-pub struct ExclusiveRangeMissingMax<'tcx> {
+pub struct ExclusiveRangeMissingMax {
     #[label]
     #[suggestion(code = "{suggestion}", applicability = "maybe-incorrect")]
     /// This is an exclusive range that looks like `lo..max` (i.e. doesn't match `max`).
     pub first_range: Span,
     /// Suggest `lo..=max` instead.
     pub suggestion: String,
-    pub max: Pat<'tcx>,
+    pub max: String, // a printed pattern
 }
 
 #[derive(LintDiagnostic)]
 #[diag(pattern_analysis_excluside_range_missing_gap)]
-pub struct ExclusiveRangeMissingGap<'tcx> {
+pub struct ExclusiveRangeMissingGap {
     #[label]
     #[suggestion(code = "{suggestion}", applicability = "maybe-incorrect")]
     /// This is an exclusive range that looks like `lo..gap` (i.e. doesn't match `gap`).
     pub first_range: Span,
-    pub gap: Pat<'tcx>,
+    pub gap: String, // a printed pattern
     /// Suggest `lo..=gap` instead.
     pub suggestion: String,
     #[subdiagnostic]
     /// All these ranges skipped over `gap` which we think is probably a mistake.
-    pub gap_with: Vec<GappedRange<'tcx>>,
+    pub gap_with: Vec<GappedRange>,
 }
 
-pub struct GappedRange<'tcx> {
+pub struct GappedRange {
     pub span: Span,
-    pub gap: Pat<'tcx>,
-    pub first_range: Pat<'tcx>,
+    pub gap: String,         // a printed pattern
+    pub first_range: String, // a printed pattern
 }
 
-impl<'tcx> Subdiagnostic for GappedRange<'tcx> {
+impl Subdiagnostic for GappedRange {
     fn add_to_diag_with<G: EmissionGuarantee, F: SubdiagMessageOp<G>>(
         self,
         diag: &mut Diag<'_, G>,
@@ -134,7 +127,7 @@ impl<'tcx> Subdiagnostic for GappedRange<'tcx> {
 pub(crate) struct NonExhaustiveOmittedPattern<'tcx> {
     pub scrut_ty: Ty<'tcx>,
     #[subdiagnostic]
-    pub uncovered: Uncovered<'tcx>,
+    pub uncovered: Uncovered,
 }
 
 #[derive(LintDiagnostic)]

compiler/rustc_pattern_analysis/src/rustc.rs

@@ -743,7 +743,7 @@ impl<'p, 'tcx: 'p> RustcPatCtxt<'p, 'tcx> {
     /// Note: it is possible to get `isize/usize::MAX+1` here, as explained in the doc for
     /// [`IntRange::split`]. This cannot be represented as a `Const`, so we represent it with
     /// `PosInfinity`.
-    pub(crate) fn hoist_pat_range_bdy(
+    fn hoist_pat_range_bdy(
         &self,
         miint: MaybeInfiniteInt,
         ty: RevealedTy<'tcx>,
@@ -774,7 +774,7 @@ impl<'p, 'tcx: 'p> RustcPatCtxt<'p, 'tcx> {
     }
 
     /// Convert back to a `thir::Pat` for diagnostic purposes.
-    pub(crate) fn hoist_pat_range(&self, range: &IntRange, ty: RevealedTy<'tcx>) -> Pat<'tcx> {
+    fn hoist_pat_range(&self, range: &IntRange, ty: RevealedTy<'tcx>) -> Pat<'tcx> {
         use MaybeInfiniteInt::*;
         let cx = self;
         let kind = if matches!((range.lo, range.hi), (NegInfinity, PosInfinity)) {
@@ -810,9 +810,17 @@ impl<'p, 'tcx: 'p> RustcPatCtxt<'p, 'tcx> {
 
         Pat { ty: ty.inner(), span: DUMMY_SP, kind }
     }
+
+    /// Prints a [`WitnessPat`] to an owned string, for diagnostic purposes.
+    pub fn print_witness_pat(&self, pat: &WitnessPat<'p, 'tcx>) -> String {
+        // This works by converting the witness pattern back to a `thir::Pat`
+        // and then printing that, but callers don't need to know that.
+        self.hoist_witness_pat(pat).to_string()
+    }
+
     /// Convert back to a `thir::Pat` for diagnostic purposes. This panics for patterns that don't
     /// appear in diagnostics, like float ranges.
-    pub fn hoist_witness_pat(&self, pat: &WitnessPat<'p, 'tcx>) -> Pat<'tcx> {
+    fn hoist_witness_pat(&self, pat: &WitnessPat<'p, 'tcx>) -> Pat<'tcx> {
         let cx = self;
         let is_wildcard = |pat: &Pat<'_>| matches!(pat.kind, PatKind::Wild);
         let mut subpatterns = pat.iter_fields().map(|p| Box::new(cx.hoist_witness_pat(p)));
@@ -965,7 +973,7 @@ impl<'p, 'tcx: 'p> PatCx for RustcPatCtxt<'p, 'tcx> {
         let overlaps: Vec<_> = overlaps_with
             .iter()
             .map(|pat| pat.data().span)
-            .map(|span| errors::Overlap { range: overlap_as_pat.clone(), span })
+            .map(|span| errors::Overlap { range: overlap_as_pat.to_string(), span })
             .collect();
         let pat_span = pat.data().span;
         self.tcx.emit_node_span_lint(
@@ -1013,7 +1021,7 @@ impl<'p, 'tcx: 'p> PatCx for RustcPatCtxt<'p, 'tcx> {
                     // Point at this range.
                     first_range: thir_pat.span,
                     // That's the gap that isn't covered.
-                    max: gap_as_pat.clone(),
+                    max: gap_as_pat.to_string(),
                     // Suggest `lo..=max` instead.
                     suggestion: suggested_range.to_string(),
                 },
@@ -1027,7 +1035,7 @@ impl<'p, 'tcx: 'p> PatCx for RustcPatCtxt<'p, 'tcx> {
                     // Point at this range.
                     first_range: thir_pat.span,
                     // That's the gap that isn't covered.
-                    gap: gap_as_pat.clone(),
+                    gap: gap_as_pat.to_string(),
                     // Suggest `lo..=gap` instead.
                     suggestion: suggested_range.to_string(),
                     // All these ranges skipped over `gap` which we think is probably a
@@ -1036,8 +1044,8 @@ impl<'p, 'tcx: 'p> PatCx for RustcPatCtxt<'p, 'tcx> {
                         .iter()
                         .map(|pat| errors::GappedRange {
                             span: pat.data().span,
-                            gap: gap_as_pat.clone(),
-                            first_range: thir_pat.clone(),
+                            gap: gap_as_pat.to_string(),
+                            first_range: thir_pat.to_string(),
                         })
                         .collect(),
                 },