Rollup merge of #121000 - Nadrieril:keep_all_fields, r=compiler-errors

GuillaumeGomez · web-flow · commit 9df7f26b1b0b · 2024-02-29T17:08:37.000+01:00
pattern_analysis: rework how we hide empty private fields

Consider this:
```rust
mod foo {
  pub struct Bar {
    pub a: bool,
    b: !,
  }
}

fn match_a_bar(bar: foo::Bar) -&gt; bool {
  match bar {
    Bar { a, .. } =&gt; a,
  }
}
```

Because the field `b` is private, matches outside the module are not allowed to observe the fact that `Bar` is empty. In particular `match bar {}` is valid within the module `foo` but an error outside (assuming `exhaustive_patterns`).

We currently handle this by hiding the field `b` when it's both private and empty. This means that the pattern `Bar { a, .. }` is lowered to `Bar(a, _)` if we're inside of `foo` and to `Bar(a)` outside. This involves a bit of a dance to keep field indices straight. But most importantly this makes pattern lowering depend on the module.

In this PR, I instead do nothing special when lowering. Only during analysis do we track whether a place must be skipped.

r? `@compiler-errors`
diff --git a/compiler/rustc_pattern_analysis/src/constructor.rs b/compiler/rustc_pattern_analysis/src/constructor.rs
@@ -688,6 +688,9 @@ pub enum Constructor<Cx: TypeCx> {
     /// Fake extra constructor for constructors that are not seen in the matrix, as explained at the
     /// top of the file.
     Missing,
+    /// Fake extra constructor that indicates and empty field that is private. When we encounter one
+    /// we skip the column entirely so we don't observe its emptiness. Only used for specialization.
+    PrivateUninhabited,
 }
 
 impl<Cx: TypeCx> Clone for Constructor<Cx> {
@@ -709,6 +712,7 @@ impl<Cx: TypeCx> Clone for Constructor<Cx> {
             Constructor::NonExhaustive => Constructor::NonExhaustive,
             Constructor::Hidden => Constructor::Hidden,
             Constructor::Missing => Constructor::Missing,
+            Constructor::PrivateUninhabited => Constructor::PrivateUninhabited,
         }
     }
 }
@@ -763,6 +767,8 @@ impl<Cx: TypeCx> Constructor<Cx> {
             }
             // Wildcards cover anything
             (_, Wildcard) => true,
+            // `PrivateUninhabited` skips everything.
+            (PrivateUninhabited, _) => true,
             // Only a wildcard pattern can match these special constructors.
             (Missing { .. } | NonExhaustive | Hidden, _) => false,
 
diff --git a/compiler/rustc_pattern_analysis/src/lib.rs b/compiler/rustc_pattern_analysis/src/lib.rs
@@ -82,6 +82,11 @@ use crate::usefulness::{compute_match_usefulness, ValidityConstraint};
 pub trait Captures<'a> {}
 impl<'a, T: ?Sized> Captures<'a> for T {}
 
+/// `bool` newtype that indicates whether this is a privately uninhabited field that we should skip
+/// during analysis.
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+pub struct PrivateUninhabitedField(pub bool);
+
 /// Context that provides type information about constructors.
 ///
 /// Most of the crate is parameterized on a type that implements this trait.
@@ -105,13 +110,12 @@ pub trait TypeCx: Sized + fmt::Debug {
     /// The number of fields for this constructor.
     fn ctor_arity(&self, ctor: &Constructor<Self>, ty: &Self::Ty) -> usize;
 
-    /// The types of the fields for this constructor. The result must have a length of
-    /// `ctor_arity()`.
+    /// The types of the fields for this constructor. The result must contain `ctor_arity()` fields.
     fn ctor_sub_tys<'a>(
         &'a self,
         ctor: &'a Constructor<Self>,
         ty: &'a Self::Ty,
-    ) -> impl Iterator<Item = Self::Ty> + ExactSizeIterator + Captures<'a>;
+    ) -> impl Iterator<Item = (Self::Ty, PrivateUninhabitedField)> + ExactSizeIterator + Captures<'a>;
 
     /// The set of all the constructors for `ty`.
     ///
diff --git a/compiler/rustc_pattern_analysis/src/pat.rs b/compiler/rustc_pattern_analysis/src/pat.rs
@@ -5,7 +5,7 @@ use std::fmt;
 use smallvec::{smallvec, SmallVec};
 
 use crate::constructor::{Constructor, Slice, SliceKind};
-use crate::TypeCx;
+use crate::{PrivateUninhabitedField, TypeCx};
 
 use self::Constructor::*;
 
@@ -23,11 +23,6 @@ impl PatId {
 /// Values and patterns can be represented as a constructor applied to some fields. This represents
 /// a pattern in this form. A `DeconstructedPat` will almost always come from user input; the only
 /// exception are some `Wildcard`s introduced during pattern lowering.
-///
-/// Note that the number of fields may not match the fields declared in the original struct/variant.
-/// This happens if a private or `non_exhaustive` field is uninhabited, because the code mustn't
-/// observe that it is uninhabited. In that case that field is not included in `fields`. Care must
-/// be taken when converting to/from `thir::Pat`.
 pub struct DeconstructedPat<Cx: TypeCx> {
     ctor: Constructor<Cx>,
     fields: Vec<DeconstructedPat<Cx>>,
@@ -84,6 +79,8 @@ impl<Cx: TypeCx> DeconstructedPat<Cx> {
         match (&self.ctor, other_ctor) {
             // Return a wildcard for each field of `other_ctor`.
             (Wildcard, _) => wildcard_sub_tys(),
+            // Skip this column.
+            (_, PrivateUninhabited) => smallvec![],
             // The only non-trivial case: two slices of different arity. `other_slice` is
             // guaranteed to have a larger arity, so we fill the middle part with enough
             // wildcards to reach the length of the new, larger slice.
@@ -192,7 +189,9 @@ impl<Cx: TypeCx> fmt::Debug for DeconstructedPat<Cx> {
                 }
                 Ok(())
             }
-            Wildcard | Missing { .. } | NonExhaustive | Hidden => write!(f, "_ : {:?}", pat.ty()),
+            Wildcard | Missing | NonExhaustive | Hidden | PrivateUninhabited => {
+                write!(f, "_ : {:?}", pat.ty())
+            }
         }
     }
 }
@@ -300,7 +299,11 @@ impl<Cx: TypeCx> WitnessPat<Cx> {
     /// For example, if `ctor` is a `Constructor::Variant` for `Option::Some`, we get the pattern
     /// `Some(_)`.
     pub(crate) fn wild_from_ctor(cx: &Cx, ctor: Constructor<Cx>, ty: Cx::Ty) -> Self {
-        let fields = cx.ctor_sub_tys(&ctor, &ty).map(|ty| Self::wildcard(ty)).collect();
+        let fields = cx
+            .ctor_sub_tys(&ctor, &ty)
+            .filter(|(_, PrivateUninhabitedField(skip))| !skip)
+            .map(|(ty, _)| Self::wildcard(ty))
+            .collect();
         Self::new(ctor, fields, ty)
     }
 
diff --git a/compiler/rustc_pattern_analysis/src/rustc.rs b/compiler/rustc_pattern_analysis/src/rustc.rs
@@ -10,15 +10,15 @@ use rustc_middle::mir::interpret::Scalar;
 use rustc_middle::mir::{self, Const};
 use rustc_middle::thir::{FieldPat, Pat, PatKind, PatRange, PatRangeBoundary};
 use rustc_middle::ty::layout::IntegerExt;
-use rustc_middle::ty::{self, OpaqueTypeKey, Ty, TyCtxt, TypeVisitableExt, VariantDef};
+use rustc_middle::ty::{self, FieldDef, OpaqueTypeKey, Ty, TyCtxt, TypeVisitableExt, VariantDef};
 use rustc_session::lint;
 use rustc_span::{ErrorGuaranteed, Span, DUMMY_SP};
 use rustc_target::abi::{FieldIdx, Integer, VariantIdx, FIRST_VARIANT};
 
 use crate::constructor::{
     IntRange, MaybeInfiniteInt, OpaqueId, RangeEnd, Slice, SliceKind, VariantVisibility,
 };
-use crate::{errors, Captures, TypeCx};
+use crate::{errors, Captures, PrivateUninhabitedField, TypeCx};
 
 use crate::constructor::Constructor::*;
 
@@ -158,34 +158,19 @@ impl<'p, 'tcx: 'p> RustcMatchCheckCtxt<'p, 'tcx> {
         }
     }
 
-    // In the cases of either a `#[non_exhaustive]` field list or a non-public field, we hide
-    // uninhabited fields in order not to reveal the uninhabitedness of the whole variant.
-    // This lists the fields we keep along with their types.
-    pub(crate) fn list_variant_nonhidden_fields(
+    pub(crate) fn variant_sub_tys(
         &self,
         ty: RevealedTy<'tcx>,
         variant: &'tcx VariantDef,
-    ) -> impl Iterator<Item = (FieldIdx, RevealedTy<'tcx>)> + Captures<'p> + Captures<'_> {
-        let cx = self;
-        let ty::Adt(adt, args) = ty.kind() else { bug!() };
-        // Whether we must not match the fields of this variant exhaustively.
-        let is_non_exhaustive = variant.is_field_list_non_exhaustive() && !adt.did().is_local();
-
-        variant.fields.iter().enumerate().filter_map(move |(i, field)| {
-            let ty = field.ty(cx.tcx, args);
+    ) -> impl Iterator<Item = (&'tcx FieldDef, RevealedTy<'tcx>)> + Captures<'p> + Captures<'_>
+    {
+        let ty::Adt(_, args) = ty.kind() else { bug!() };
+        variant.fields.iter().map(move |field| {
+            let ty = field.ty(self.tcx, args);
             // `field.ty()` doesn't normalize after instantiating.
-            let ty = cx.tcx.normalize_erasing_regions(cx.param_env, ty);
-            let is_visible = adt.is_enum() || field.vis.is_accessible_from(cx.module, cx.tcx);
-            let is_uninhabited = (cx.tcx.features().exhaustive_patterns
-                || cx.tcx.features().min_exhaustive_patterns)
-                && cx.is_uninhabited(ty);
-
-            if is_uninhabited && (!is_visible || is_non_exhaustive) {
-                None
-            } else {
-                let ty = cx.reveal_opaque_ty(ty);
-                Some((FieldIdx::new(i), ty))
-            }
+            let ty = self.tcx.normalize_erasing_regions(self.param_env, ty);
+            let ty = self.reveal_opaque_ty(ty);
+            (field, ty)
         })
     }
 
@@ -210,12 +195,17 @@ impl<'p, 'tcx: 'p> RustcMatchCheckCtxt<'p, 'tcx> {
         &'a self,
         ctor: &'a Constructor<'p, 'tcx>,
         ty: RevealedTy<'tcx>,
-    ) -> impl Iterator<Item = RevealedTy<'tcx>> + ExactSizeIterator + Captures<'a> {
+    ) -> impl Iterator<Item = (RevealedTy<'tcx>, PrivateUninhabitedField)>
+    + ExactSizeIterator
+    + Captures<'a> {
         fn reveal_and_alloc<'a, 'tcx>(
             cx: &'a RustcMatchCheckCtxt<'_, 'tcx>,
             iter: impl Iterator<Item = Ty<'tcx>>,
-        ) -> &'a [RevealedTy<'tcx>] {
-            cx.dropless_arena.alloc_from_iter(iter.map(|ty| cx.reveal_opaque_ty(ty)))
+        ) -> &'a [(RevealedTy<'tcx>, PrivateUninhabitedField)] {
+            cx.dropless_arena.alloc_from_iter(
+                iter.map(|ty| cx.reveal_opaque_ty(ty))
+                    .map(|ty| (ty, PrivateUninhabitedField(false))),
+            )
         }
         let cx = self;
         let slice = match ctor {
@@ -229,7 +219,21 @@ impl<'p, 'tcx: 'p> RustcMatchCheckCtxt<'p, 'tcx> {
                     } else {
                         let variant =
                             &adt.variant(RustcMatchCheckCtxt::variant_index_for_adt(&ctor, *adt));
-                        let tys = cx.list_variant_nonhidden_fields(ty, variant).map(|(_, ty)| ty);
+
+                        // In the cases of either a `#[non_exhaustive]` field list or a non-public
+                        // field, we skip uninhabited fields in order not to reveal the
+                        // uninhabitedness of the whole variant.
+                        let is_non_exhaustive =
+                            variant.is_field_list_non_exhaustive() && !adt.did().is_local();
+                        let tys = cx.variant_sub_tys(ty, variant).map(|(field, ty)| {
+                            let is_visible =
+                                adt.is_enum() || field.vis.is_accessible_from(cx.module, cx.tcx);
+                            let is_uninhabited = (cx.tcx.features().exhaustive_patterns
+                                || cx.tcx.features().min_exhaustive_patterns)
+                                && cx.is_uninhabited(*ty);
+                            let skip = is_uninhabited && (!is_visible || is_non_exhaustive);
+                            (ty, PrivateUninhabitedField(skip))
+                        });
                         cx.dropless_arena.alloc_from_iter(tys)
                     }
                 }
@@ -246,16 +250,8 @@ impl<'p, 'tcx: 'p> RustcMatchCheckCtxt<'p, 'tcx> {
                 }
                 _ => bug!("bad slice pattern {:?} {:?}", ctor, ty),
             },
-            Bool(..)
-            | IntRange(..)
-            | F32Range(..)
-            | F64Range(..)
-            | Str(..)
-            | Opaque(..)
-            | NonExhaustive
-            | Hidden
-            | Missing { .. }
-            | Wildcard => &[],
+            Bool(..) | IntRange(..) | F32Range(..) | F64Range(..) | Str(..) | Opaque(..)
+            | NonExhaustive | Hidden | Missing | PrivateUninhabited | Wildcard => &[],
             Or => {
                 bug!("called `Fields::wildcards` on an `Or` ctor")
             }
@@ -274,25 +270,16 @@ impl<'p, 'tcx: 'p> RustcMatchCheckCtxt<'p, 'tcx> {
                         // patterns. If we're here we can assume this is a box pattern.
                         1
                     } else {
-                        let variant =
-                            &adt.variant(RustcMatchCheckCtxt::variant_index_for_adt(&ctor, *adt));
-                        self.list_variant_nonhidden_fields(ty, variant).count()
+                        let variant_idx = RustcMatchCheckCtxt::variant_index_for_adt(&ctor, *adt);
+                        adt.variant(variant_idx).fields.len()
                     }
                 }
                 _ => bug!("Unexpected type for constructor `{ctor:?}`: {ty:?}"),
             },
             Ref => 1,
             Slice(slice) => slice.arity(),
-            Bool(..)
-            | IntRange(..)
-            | F32Range(..)
-            | F64Range(..)
-            | Str(..)
-            | Opaque(..)
-            | NonExhaustive
-            | Hidden
-            | Missing { .. }
-            | Wildcard => 0,
+            Bool(..) | IntRange(..) | F32Range(..) | F64Range(..) | Str(..) | Opaque(..)
+            | NonExhaustive | Hidden | Missing | PrivateUninhabited | Wildcard => 0,
             Or => bug!("The `Or` constructor doesn't have a fixed arity"),
         }
     }
@@ -520,20 +507,12 @@ impl<'p, 'tcx: 'p> RustcMatchCheckCtxt<'p, 'tcx> {
                         };
                         let variant =
                             &adt.variant(RustcMatchCheckCtxt::variant_index_for_adt(&ctor, *adt));
-                        // For each field in the variant, we store the relevant index into `self.fields` if any.
-                        let mut field_id_to_id: Vec<Option<usize>> =
-                            (0..variant.fields.len()).map(|_| None).collect();
-                        let tys = cx.list_variant_nonhidden_fields(ty, variant).enumerate().map(
-                            |(i, (field, ty))| {
-                                field_id_to_id[field.index()] = Some(i);
-                                ty
-                            },
-                        );
-                        fields = tys.map(|ty| DeconstructedPat::wildcard(ty)).collect();
+                        fields = cx
+                            .variant_sub_tys(ty, variant)
+                            .map(|(_, ty)| DeconstructedPat::wildcard(ty))
+                            .collect();
                         for pat in subpatterns {
-                            if let Some(i) = field_id_to_id[pat.field.index()] {
-                                fields[i] = self.lower_pat(&pat.pattern);
-                            }
+                            fields[pat.field.index()] = self.lower_pat(&pat.pattern);
                         }
                     }
                     _ => bug!("pattern has unexpected type: pat: {:?}, ty: {:?}", pat, ty),
@@ -775,11 +754,9 @@ impl<'p, 'tcx: 'p> RustcMatchCheckCtxt<'p, 'tcx> {
                 ty::Adt(adt_def, args) => {
                     let variant_index =
                         RustcMatchCheckCtxt::variant_index_for_adt(&pat.ctor(), *adt_def);
-                    let variant = &adt_def.variant(variant_index);
-                    let subpatterns = cx
-                        .list_variant_nonhidden_fields(*pat.ty(), variant)
-                        .zip(subpatterns)
-                        .map(|((field, _ty), pattern)| FieldPat { field, pattern })
+                    let subpatterns = subpatterns
+                        .enumerate()
+                        .map(|(i, pattern)| FieldPat { field: FieldIdx::new(i), pattern })
                         .collect();
 
                     if adt_def.is_enum() {
@@ -830,7 +807,7 @@ impl<'p, 'tcx: 'p> RustcMatchCheckCtxt<'p, 'tcx> {
                 }
             }
             &Str(value) => PatKind::Constant { value },
-            Wildcard | NonExhaustive | Hidden => PatKind::Wild,
+            Wildcard | NonExhaustive | Hidden | PrivateUninhabited => PatKind::Wild,
             Missing { .. } => bug!(
                 "trying to convert a `Missing` constructor into a `Pat`; this is probably a bug,
                 `Missing` should have been processed in `apply_constructors`"
@@ -866,7 +843,8 @@ impl<'p, 'tcx: 'p> TypeCx for RustcMatchCheckCtxt<'p, 'tcx> {
         &'a self,
         ctor: &'a crate::constructor::Constructor<Self>,
         ty: &'a Self::Ty,
-    ) -> impl Iterator<Item = Self::Ty> + ExactSizeIterator + Captures<'a> {
+    ) -> impl Iterator<Item = (Self::Ty, PrivateUninhabitedField)> + ExactSizeIterator + Captures<'a>
+    {
         self.ctor_sub_tys(ctor, *ty)
     }
     fn ctors_for_ty(
diff --git a/compiler/rustc_pattern_analysis/src/usefulness.rs b/compiler/rustc_pattern_analysis/src/usefulness.rs
@@ -716,7 +716,7 @@ use std::fmt;
 
 use crate::constructor::{Constructor, ConstructorSet, IntRange};
 use crate::pat::{DeconstructedPat, PatId, PatOrWild, WitnessPat};
-use crate::{Captures, MatchArm, TypeCx};
+use crate::{Captures, MatchArm, PrivateUninhabitedField, TypeCx};
 
 use self::ValidityConstraint::*;
 
@@ -817,6 +817,9 @@ impl fmt::Display for ValidityConstraint {
 struct PlaceInfo<Cx: TypeCx> {
     /// The type of the place.
     ty: Cx::Ty,
+    /// Whether the place is a private uninhabited field. If so we skip this field during analysis
+    /// so that we don't observe its emptiness.
+    private_uninhabited: bool,
     /// Whether the place is known to contain valid data.
     validity: ValidityConstraint,
     /// Whether the place is the scrutinee itself or a subplace of it.
@@ -833,8 +836,9 @@ impl<Cx: TypeCx> PlaceInfo<Cx> {
     ) -> impl Iterator<Item = Self> + ExactSizeIterator + Captures<'a> {
         let ctor_sub_tys = cx.ctor_sub_tys(ctor, &self.ty);
         let ctor_sub_validity = self.validity.specialize(ctor);
-        ctor_sub_tys.map(move |ty| PlaceInfo {
+        ctor_sub_tys.map(move |(ty, PrivateUninhabitedField(private_uninhabited))| PlaceInfo {
             ty,
+            private_uninhabited,
             validity: ctor_sub_validity,
             is_scrutinee: false,
         })
@@ -856,6 +860,11 @@ impl<Cx: TypeCx> PlaceInfo<Cx> {
     where
         Cx: 'a,
     {
+        if self.private_uninhabited {
+            // Skip the whole column
+            return Ok((smallvec![Constructor::PrivateUninhabited], vec![]));
+        }
+
         let ctors_for_ty = cx.ctors_for_ty(&self.ty)?;
 
         // We treat match scrutinees of type `!` or `EmptyEnum` differently.
@@ -914,7 +923,12 @@ impl<Cx: TypeCx> PlaceInfo<Cx> {
 
 impl<Cx: TypeCx> Clone for PlaceInfo<Cx> {
     fn clone(&self) -> Self {
-        Self { ty: self.ty.clone(), validity: self.validity, is_scrutinee: self.is_scrutinee }
+        Self {
+            ty: self.ty.clone(),
+            private_uninhabited: self.private_uninhabited,
+            validity: self.validity,
+            is_scrutinee: self.is_scrutinee,
+        }
     }
 }
 
@@ -1121,7 +1135,12 @@ impl<'p, Cx: TypeCx> Matrix<'p, Cx> {
         scrut_ty: Cx::Ty,
         scrut_validity: ValidityConstraint,
     ) -> Self {
-        let place_info = PlaceInfo { ty: scrut_ty, validity: scrut_validity, is_scrutinee: true };
+        let place_info = PlaceInfo {
+            ty: scrut_ty,
+            private_uninhabited: false,
+            validity: scrut_validity,
+            is_scrutinee: true,
+        };
         let mut matrix = Matrix {
             rows: Vec::with_capacity(arms.len()),
             place_info: smallvec![place_info],

Original file line number	Diff line number	Diff line change
`@@ -688,6 +688,9 @@ pub enum Constructor<Cx: TypeCx> {`
`688`	`688`	`/// Fake extra constructor for constructors that are not seen in the matrix, as explained at the`
`689`	`689`	`/// top of the file.`
`690`	`690`	`Missing,`
	`691`	`+ /// Fake extra constructor that indicates and empty field that is private. When we encounter one`
	`692`	`+ /// we skip the column entirely so we don't observe its emptiness. Only used for specialization.`
	`693`	`+ PrivateUninhabited,`
`691`	`694`	`}`
`692`	`695`
`693`	`696`	`impl<Cx: TypeCx> Clone for Constructor<Cx> {`
`@@ -709,6 +712,7 @@ impl<Cx: TypeCx> Clone for Constructor<Cx> {`
`709`	`712`	`Constructor::NonExhaustive => Constructor::NonExhaustive,`
`710`	`713`	`Constructor::Hidden => Constructor::Hidden,`
`711`	`714`	`Constructor::Missing => Constructor::Missing,`
	`715`	`+ Constructor::PrivateUninhabited => Constructor::PrivateUninhabited,`
`712`	`716`	`}`
`713`	`717`	`}`
`714`	`718`	`}`
`@@ -763,6 +767,8 @@ impl<Cx: TypeCx> Constructor<Cx> {`
`763`	`767`	`}`
`764`	`768`	`// Wildcards cover anything`
`765`	`769`	`(_, Wildcard) => true,`
	`770`	+ // `PrivateUninhabited` skips everything.
	`771`	`+ (PrivateUninhabited, _) => true,`
`766`	`772`	`// Only a wildcard pattern can match these special constructors.`
`767`	`773`	`(Missing { .. } \| NonExhaustive \| Hidden, _) => false,`
`768`	`774`