From 3a95a65d095027b5f77c3930016c449aae70ad17 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sun, 29 Oct 2023 19:18:18 +0100 Subject: [PATCH] Correctly handle empty constructors - `ConstructorSet` knows about both empty and nonempty constructors; - If an empty constructor is present in the column, we output it in `split().present`. --- .../src/thir/pattern/deconstruct_pat.rs | 373 ++++++++++-------- .../src/thir/pattern/usefulness.rs | 6 +- 2 files changed, 217 insertions(+), 162 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs b/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs index 4ef2109168e1..2a5a80c88a8c 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs @@ -10,6 +10,7 @@ //! precisely here. //! //! +//! //! # Constructor grouping and splitting //! //! As explained in the corresponding section in [`super::usefulness`], to make usefulness tractable @@ -49,6 +50,7 @@ //! [`IntRange::split`]) and slice splitting (see [`Slice::split`]). //! //! +//! //! # The `Missing` constructor //! //! We detail a special case of constructor splitting that is a bit subtle. Take the following: @@ -77,6 +79,64 @@ //! //! //! +//! ## Empty types, empty constructors, and the `exhaustive_patterns` feature +//! +//! An empty type is a type that has no valid value, like `!`, `enum Void {}`, or `Result`. +//! They require careful handling. +//! +//! First, for soundness reasons related to the possible existence of invalid values, by default we +//! don't treat empty types as empty. We force them to be matched with wildcards. Except if the +//! `exhaustive_patterns` feature is turned on, in which case we do treat them as empty. And also +//! except if the type has no constructors (like `enum Void {}` but not like `Result`), we +//! specifically allow `match void {}` to be exhaustive. There are additionally considerations of +//! place validity that are handled in `super::usefulness`. Yes this is a bit tricky. +//! +//! The second thing is that regardless of the above, it is always allowed to use all the +//! constructors of a type. For example, all the following is ok: +//! +//! ```rust,ignore(example) +//! fn foo(x: Option) { +//! match x { +//! None => {} +//! Some(_) => {} +//! } +//! } +//! fn bar(x: &[!]) { +//! match x { +//! [] => 1 +//! [_] => 2 +//! [_, _] => 3 +//! } +//! } +//! ``` +//! +//! Moreover, take the following: +//! +//! ```rust,ignore(example) +//! match x { +//! None => {} +//! } +//! ``` +//! +//! On a normal type, we would identify `Some` as missing and tell the user. If `x: Option` +//! however (and `exhaustive_patterns` is on), it's ok to omit `Some`. When listing the constructors +//! of a type, we must therefore track which can be omitted. +//! +//! Let's call "empty" a constructor that matches no valid value for the type, like `Some` for the +//! type `Option`. What this all means is that `ConstructorSet` must know which constructors are +//! empty. The difference between empty and nonempty constructors is that empty constructors need +//! not be present for the match to be exhaustive. +//! +//! A final remark: empty constructors of arity 0 break specialization, we must avoid them. The +//! reason is that if we specialize by them, nothing remains to witness the emptiness; the rest of +//! the algorithm can't distinguish them from a nonempty constructor. The only known case where this +//! could happen is the `[..]` pattern on `[!; N]` with `N > 0` so we must take care to not emit it. +//! +//! This is all handled by [`ConstructorSet::for_ty`] and [`ConstructorSet::split`]. The invariants +//! of [`SplitConstructorSet`] are also of interest. +//! +//! +//! //! ## Opaque patterns //! //! Some patterns, such as constants that are not allowed to be matched structurally, cannot be @@ -95,7 +155,7 @@ use rustc_apfloat::ieee::{DoubleS, IeeeFloat, SingleS}; use rustc_data_structures::captures::Captures; use rustc_data_structures::fx::FxHashSet; use rustc_hir::RangeEnd; -use rustc_index::Idx; +use rustc_index::{Idx, IndexVec}; use rustc_middle::middle::stability::EvalResult; use rustc_middle::mir; use rustc_middle::mir::interpret::Scalar; @@ -483,8 +543,12 @@ pub(super) struct Slice { impl Slice { fn new(array_len: Option, kind: SliceKind) -> Self { let kind = match (array_len, kind) { - // If the middle `..` is empty, we effectively have a fixed-length pattern. - (Some(len), VarLen(prefix, suffix)) if prefix + suffix >= len => FixedLen(len), + // If the middle `..` has length 0, we effectively have a fixed-length pattern. + (Some(len), VarLen(prefix, suffix)) if prefix + suffix == len => FixedLen(len), + (Some(len), VarLen(prefix, suffix)) if prefix + suffix > len => bug!( + "Slice pattern of length {} longer than its array length {len}", + prefix + suffix + ), _ => kind, }; Slice { array_len, kind } @@ -584,17 +648,18 @@ impl Slice { let mut seen_fixed_lens = FxHashSet::default(); match &mut max_slice { VarLen(max_prefix_len, max_suffix_len) => { + // A length larger than any fixed-length slice encountered. + // We start at 1 in case the subtype is empty because in that case the zero-length + // slice must be treated separately from the rest. + let mut fixed_len_upper_bound = 1; // We grow `max_slice` to be larger than all slices encountered, as described above. - // For diagnostics, we keep the prefix and suffix lengths separate, but grow them so that - // `L = max_prefix_len + max_suffix_len`. - let mut max_fixed_len = 0; + // `L` is `max_slice.arity()`. For diagnostics, we keep the prefix and suffix + // lengths separate. for slice in column_slices { match slice.kind { FixedLen(len) => { - max_fixed_len = cmp::max(max_fixed_len, len); - if arity <= len { - seen_fixed_lens.insert(len); - } + fixed_len_upper_bound = cmp::max(fixed_len_upper_bound, len + 1); + seen_fixed_lens.insert(len); } VarLen(prefix, suffix) => { *max_prefix_len = cmp::max(*max_prefix_len, prefix); @@ -603,12 +668,11 @@ impl Slice { } } } - // We want `L = max(L, max_fixed_len + 1)`, modulo the fact that we keep prefix and - // suffix separate. - if max_fixed_len + 1 >= *max_prefix_len + *max_suffix_len { - // The subtraction can't overflow thanks to the above check. - // The new `max_prefix_len` is larger than its previous value. - *max_prefix_len = max_fixed_len + 1 - *max_suffix_len; + // If `fixed_len_upper_bound >= L`, we set `L` to `fixed_len_upper_bound`. + if let Some(delta) = + fixed_len_upper_bound.checked_sub(*max_prefix_len + *max_suffix_len) + { + *max_prefix_len += delta } // We cap the arity of `max_slice` at the array size. @@ -857,34 +921,48 @@ impl<'tcx> Constructor<'tcx> { } } -/// Describes the set of all constructors for a type. +#[derive(Debug, Clone, Copy)] +pub(super) enum VariantVisibility { + /// Variant that doesn't fit the other cases, i.e. most variants. + Visible, + /// Variant behind an unstable gate or with the `#[doc(hidden)]` attribute. It will not be + /// mentioned in diagnostics unless the user mentioned it first. + Hidden, + /// Variant that matches no value. E.g. `Some::>` if the `exhaustive_patterns` feature + /// is enabled. Like `Hidden`, it will not be mentioned in diagnostics unless the user mentioned + /// it first. + Empty, +} + +/// Describes the set of all constructors for a type. For details, in particular about the emptiness +/// of constructors, see the top of the file. +/// +/// In terms of division of responsibility, [`ConstructorSet::split`] handles all of the +/// `exhaustive_patterns` feature. #[derive(Debug)] pub(super) enum ConstructorSet { - /// The type has a single constructor, e.g. `&T` or a struct. - Single, - /// This type has the following list of constructors. - /// Some variants are hidden, which means they won't be mentioned in diagnostics unless the user - /// mentioned them first. We use this for variants behind an unstable gate as well as - /// `#[doc(hidden)]` ones. - Variants { - visible_variants: Vec, - hidden_variants: Vec, - non_exhaustive: bool, - }, + /// The type has a single constructor, e.g. `&T` or a struct. `empty` tracks whether the + /// constructor is empty. + Single { empty: bool }, + /// This type has the following list of constructors. If `variants` is empty and + /// `non_exhaustive` is false, don't use this; use `NoConstructors` instead. + Variants { variants: IndexVec, non_exhaustive: bool }, /// Booleans. Bool, /// The type is spanned by integer values. The range or ranges give the set of allowed values. /// The second range is only useful for `char`. Integers { range_1: IntRange, range_2: Option }, - /// The type is matched by slices. The usize is the compile-time length of the array, if known. - Slice(Option), - /// The type is matched by slices whose elements are uninhabited. - SliceOfEmpty, + /// The type is matched by slices. `array_len` is the compile-time length of the array, if + /// known. If `subtype_is_empty`, all constructors are empty except possibly the zero-length + /// slice `[]`. + Slice { array_len: Option, subtype_is_empty: bool }, /// The constructors cannot be listed, and the type cannot be matched exhaustively. E.g. `str`, - /// floats. - Unlistable, - /// The type has no inhabitants. - Uninhabited, + /// floats. `empty` tracks whether *all the constructors* are empty. We don't know anything + /// about individual constructors' emptiness. + /// FIXME(Nadrieril): is there a type where `empty` is true? + Unlistable { empty: bool }, + /// The type has no constructors (not even empty ones). This is `!` and empty enums. + NoConstructors, } /// Describes the result of analyzing the constructors in a column of a match. @@ -914,6 +992,8 @@ pub(super) struct SplitConstructorSet<'tcx> { impl ConstructorSet { /// Creates a set that represents all the constructors of `ty`. + /// + /// See at the top of the file for considerations of emptiness. #[instrument(level = "debug", skip(cx), ret)] pub(super) fn for_ty<'p, 'tcx>(cx: &MatchCheckCtxt<'p, 'tcx>, ty: Ty<'tcx>) -> Self { let make_range = |start, end| { @@ -925,12 +1005,6 @@ impl ConstructorSet { }; // This determines the set of all possible constructors for the type `ty`. For numbers, // arrays and slices we use ranges and variable-length slices when appropriate. - // - // If the `exhaustive_patterns` feature is enabled, we make sure to omit constructors that - // are statically impossible. E.g., for `Option`, we do not include `Some(_)` in the - // returned list of constructors. - // Invariant: this is `Uninhabited` if and only if the type is uninhabited (as determined by - // `cx.is_uninhabited()`). match ty.kind() { ty::Bool => Self::Bool, ty::Char => { @@ -970,82 +1044,74 @@ impl ConstructorSet { }; Self::Integers { range_1: range, range_2: None } } - ty::Array(sub_ty, len) if len.try_eval_target_usize(cx.tcx, cx.param_env).is_some() => { - let len = len.eval_target_usize(cx.tcx, cx.param_env) as usize; - if len != 0 && cx.is_uninhabited(*sub_ty) { - Self::Uninhabited - } else { - Self::Slice(Some(len)) - } + ty::Slice(sub_ty) => { + Self::Slice { array_len: None, subtype_is_empty: cx.is_uninhabited(*sub_ty) } } - // Treat arrays of a constant but unknown length like slices. - ty::Array(sub_ty, _) | ty::Slice(sub_ty) => { - if cx.is_uninhabited(*sub_ty) { - Self::SliceOfEmpty - } else { - Self::Slice(None) + ty::Array(sub_ty, len) => { + // We treat arrays of a constant but unknown length like slices. + Self::Slice { + array_len: len.try_eval_target_usize(cx.tcx, cx.param_env).map(|l| l as usize), + subtype_is_empty: cx.is_uninhabited(*sub_ty), } } ty::Adt(def, args) if def.is_enum() => { - // If the enum is declared as `#[non_exhaustive]`, we treat it as if it had an - // additional "unknown" constructor. - // There is no point in enumerating all possible variants, because the user can't - // actually match against them all themselves. So we always return only the fictitious - // constructor. - // E.g., in an example like: - // - // ``` - // let err: io::ErrorKind = ...; - // match err { - // io::ErrorKind::NotFound => {}, - // } - // ``` - // - // we don't want to show every possible IO error, but instead have only `_` as the - // witness. let is_declared_nonexhaustive = cx.is_foreign_non_exhaustive_enum(ty); - if def.variants().is_empty() && !is_declared_nonexhaustive { - Self::Uninhabited + Self::NoConstructors } else { - let is_exhaustive_pat_feature = cx.tcx.features().exhaustive_patterns; - let (hidden_variants, visible_variants) = def - .variants() - .iter_enumerated() - .filter(|(_, v)| { - // If `exhaustive_patterns` is enabled, we exclude variants known to be - // uninhabited. - !is_exhaustive_pat_feature - || v.inhabited_predicate(cx.tcx, *def) - .instantiate(cx.tcx, args) - .apply(cx.tcx, cx.param_env, cx.module) - }) - .map(|(idx, _)| idx) - .partition(|idx| { - let variant_def_id = def.variant(*idx).def_id; - // Filter variants that depend on a disabled unstable feature. - let is_unstable = matches!( - cx.tcx.eval_stability(variant_def_id, None, DUMMY_SP, None), - EvalResult::Deny { .. } - ); - // Filter foreign `#[doc(hidden)]` variants. - let is_doc_hidden = - cx.tcx.is_doc_hidden(variant_def_id) && !variant_def_id.is_local(); - is_unstable || is_doc_hidden - }); - - Self::Variants { - visible_variants, - hidden_variants, - non_exhaustive: is_declared_nonexhaustive, + let mut variants = + IndexVec::from_elem(VariantVisibility::Visible, def.variants()); + for (idx, v) in def.variants().iter_enumerated() { + let variant_def_id = def.variant(idx).def_id; + // Visibly uninhabited variants. + let is_inhabited = v + .inhabited_predicate(cx.tcx, *def) + .instantiate(cx.tcx, args) + .apply(cx.tcx, cx.param_env, cx.module); + // Variants that depend on a disabled unstable feature. + let is_unstable = matches!( + cx.tcx.eval_stability(variant_def_id, None, DUMMY_SP, None), + EvalResult::Deny { .. } + ); + // Foreign `#[doc(hidden)]` variants. + let is_doc_hidden = + cx.tcx.is_doc_hidden(variant_def_id) && !variant_def_id.is_local(); + let visibility = if !is_inhabited { + // FIXME: handle empty+hidden + VariantVisibility::Empty + } else if is_unstable || is_doc_hidden { + VariantVisibility::Hidden + } else { + VariantVisibility::Visible + }; + variants[idx] = visibility; } + + Self::Variants { variants, non_exhaustive: is_declared_nonexhaustive } } } - ty::Never => Self::Uninhabited, - _ if cx.is_uninhabited(ty) => Self::Uninhabited, - ty::Adt(..) | ty::Tuple(..) | ty::Ref(..) => Self::Single, + ty::Adt(..) | ty::Tuple(..) | ty::Ref(..) => { + Self::Single { empty: cx.is_uninhabited(ty) } + } + ty::Never => Self::NoConstructors, // This type is one for which we cannot list constructors, like `str` or `f64`. - _ => Self::Unlistable, + // FIXME(Nadrieril): which of these are actually allowed? + ty::Float(_) + | ty::Str + | ty::Foreign(_) + | ty::RawPtr(_) + | ty::FnDef(_, _) + | ty::FnPtr(_) + | ty::Dynamic(_, _, _) + | ty::Closure(_, _) + | ty::Coroutine(_, _, _) + | ty::CoroutineWitness(_, _) + | ty::Alias(_, _) + | ty::Param(_) + | ty::Bound(_, _) + | ty::Placeholder(_) + | ty::Infer(_) + | ty::Error(_) => Self::Unlistable { empty: cx.is_uninhabited(ty) }, } } @@ -1063,6 +1129,9 @@ impl ConstructorSet { 'tcx: 'a, { let mut present: SmallVec<[_; 1]> = SmallVec::new(); + // Empty constructors found missing. + let mut missing_empty = Vec::new(); + // Nonempty constructors found missing. let mut missing = Vec::new(); // Constructors in `ctors`, except wildcards and opaques. let mut seen = Vec::new(); @@ -1075,38 +1144,36 @@ impl ConstructorSet { } match self { - ConstructorSet::Single => { - if seen.is_empty() { - missing.push(Single); - } else { + ConstructorSet::Single { empty } => { + if !seen.is_empty() { present.push(Single); + } else if *empty { + missing_empty.push(Single); + } else { + missing.push(Single); } } - ConstructorSet::Variants { visible_variants, hidden_variants, non_exhaustive } => { + ConstructorSet::Variants { variants, non_exhaustive } => { let seen_set: FxHashSet<_> = seen.iter().map(|c| c.as_variant().unwrap()).collect(); let mut skipped_a_hidden_variant = false; - for variant in visible_variants { - let ctor = Variant(*variant); - if seen_set.contains(variant) { + for (idx, visibility) in variants.iter_enumerated() { + let ctor = Variant(idx); + if seen_set.contains(&idx) { present.push(ctor); } else { - missing.push(ctor); + // We only put visible variants directly into `missing`. + match visibility { + VariantVisibility::Visible => missing.push(ctor), + VariantVisibility::Hidden => skipped_a_hidden_variant = true, + VariantVisibility::Empty => missing_empty.push(ctor), + } } } - for variant in hidden_variants { - let ctor = Variant(*variant); - if seen_set.contains(variant) { - present.push(ctor); - } else { - skipped_a_hidden_variant = true; - } - } if skipped_a_hidden_variant { missing.push(Hidden); } - if *non_exhaustive { missing.push(NonExhaustive); } @@ -1150,53 +1217,45 @@ impl ConstructorSet { } } } - &ConstructorSet::Slice(array_len) => { - let seen_slices = seen.iter().map(|c| c.as_slice().unwrap()); - let base_slice = Slice::new(array_len, VarLen(0, 0)); - for (seen, splitted_slice) in base_slice.split(seen_slices) { - let ctor = Slice(splitted_slice); - match seen { - Presence::Unseen => missing.push(ctor), - Presence::Seen => present.push(ctor), - } - } - } - ConstructorSet::SliceOfEmpty => { - // This one is tricky because even though there's only one possible value of this - // type (namely `[]`), slice patterns of all lengths are allowed, they're just - // unreachable if length != 0. - // We still gather the seen constructors in `present`, but the only slice that can - // go in `missing` is `[]`. + ConstructorSet::Slice { array_len, subtype_is_empty } => { let seen_slices = seen.iter().map(|c| c.as_slice().unwrap()); - let base_slice = Slice::new(None, VarLen(0, 0)); + let base_slice = Slice::new(*array_len, VarLen(0, 0)); for (seen, splitted_slice) in base_slice.split(seen_slices) { let ctor = Slice(splitted_slice); match seen { Presence::Seen => present.push(ctor), - Presence::Unseen if splitted_slice.arity() == 0 => { - missing.push(Slice(Slice::new(None, FixedLen(0)))) + Presence::Unseen => { + if *subtype_is_empty && splitted_slice.arity() != 0 { + // We have subpatterns of an empty type, so the constructor is + // empty. + missing_empty.push(ctor); + } else { + missing.push(ctor); + } } - Presence::Unseen => {} } } } - ConstructorSet::Unlistable => { + ConstructorSet::Unlistable { empty } => { // Since we can't list constructors, we take the ones in the column. This might list // some constructors several times but there's not much we can do. present.extend(seen); - missing.push(NonExhaustive); + if *empty { + missing_empty.push(NonExhaustive); + } else { + missing.push(NonExhaustive); + } } - // If `exhaustive_patterns` is disabled and our scrutinee is an empty type, we cannot - // expose its emptiness. The exception is if the pattern is at the top level, because we - // want empty matches to be considered exhaustive. - ConstructorSet::Uninhabited - if !pcx.cx.tcx.features().exhaustive_patterns && !pcx.is_top_level => - { - missing.push(NonExhaustive); + ConstructorSet::NoConstructors => { + if !pcx.is_top_level { + missing_empty.push(NonExhaustive); + } } - ConstructorSet::Uninhabited => {} } + if !pcx.cx.tcx.features().exhaustive_patterns { + missing.extend(missing_empty); + } SplitConstructorSet { present, missing } } } @@ -1270,7 +1329,7 @@ impl<'p, 'tcx> Fields<'p, 'tcx> { // `field.ty()` doesn't normalize after substituting. 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.is_uninhabited(ty); + let is_uninhabited = cx.tcx.features().exhaustive_patterns && cx.is_uninhabited(ty); if is_uninhabited && (!is_visible || is_non_exhaustive) { None diff --git a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs index 7bb32cdadb43..6c2f633709de 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs @@ -595,11 +595,7 @@ pub(crate) struct MatchCheckCtxt<'p, 'tcx> { impl<'a, 'tcx> MatchCheckCtxt<'a, 'tcx> { pub(super) fn is_uninhabited(&self, ty: Ty<'tcx>) -> bool { - if self.tcx.features().exhaustive_patterns { - !ty.is_inhabited_from(self.tcx, self.module, self.param_env) - } else { - false - } + !ty.is_inhabited_from(self.tcx, self.module, self.param_env) } /// Returns whether the given type is an enum from another crate declared `#[non_exhaustive]`.