From feb1e13960ab435ba14a37e237a3c5aaee8b9558 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sun, 25 Oct 2020 21:59:59 +0000 Subject: [PATCH 01/16] Split `split_grouped_constructor` into smaller functions --- .../src/thir/pattern/_match.rs | 674 +++++++++--------- 1 file changed, 345 insertions(+), 329 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 0449e14983150..29356cba50efe 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -284,7 +284,7 @@ //! disjunction over every range. This is a bit more tricky to deal with: essentially we need //! to form equivalence classes of subranges of the constructor range for which the behaviour //! of the matrix `P` and new pattern `p` are the same. This is described in more -//! detail in `split_grouped_constructors`. +//! detail in `Constructor::split`. //! + If some constructors are missing from the matrix, it turns out we don't need to do //! anything special (because we know none of the integers are actually wildcards: i.e., we //! can't span wildcards using ranges). @@ -409,7 +409,7 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { /// This computes `S(constructor, self)`. See top of the file for explanations. fn specialize_constructor( &self, - cx: &mut MatchCheckCtxt<'p, 'tcx>, + cx: &MatchCheckCtxt<'p, 'tcx>, constructor: &Constructor<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, is_my_head_ctor: bool, @@ -581,7 +581,7 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { /// This computes `S(constructor, self)`. See top of the file for explanations. fn specialize_constructor( &self, - cx: &mut MatchCheckCtxt<'p, 'tcx>, + cx: &MatchCheckCtxt<'p, 'tcx>, constructor: &Constructor<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, ) -> Matrix<'p, 'tcx> { @@ -832,6 +832,139 @@ impl Slice { fn arity(self) -> u64 { self.pattern_kind().arity() } + + /// The exhaustiveness-checking paper does not include any details on + /// checking variable-length slice patterns. However, they are matched + /// by an infinite collection of fixed-length array patterns. + /// + /// Checking the infinite set directly would take an infinite amount + /// of time. However, it turns out that for each finite set of + /// patterns `P`, all sufficiently large array lengths are equivalent: + /// + /// Each slice `s` with a "sufficiently-large" length `l ≥ L` that applies + /// to exactly the subset `Pₜ` of `P` can be transformed to a slice + /// `sₘ` for each sufficiently-large length `m` that applies to exactly + /// the same subset of `P`. + /// + /// Because of that, each witness for reachability-checking from one + /// of the sufficiently-large lengths can be transformed to an + /// equally-valid witness from any other length, so we only have + /// to check slice lengths from the "minimal sufficiently-large length" + /// and below. + /// + /// Note that the fact that there is a *single* `sₘ` for each `m` + /// not depending on the specific pattern in `P` is important: if + /// you look at the pair of patterns + /// `[true, ..]` + /// `[.., false]` + /// Then any slice of length ≥1 that matches one of these two + /// patterns can be trivially turned to a slice of any + /// other length ≥1 that matches them and vice-versa - for + /// but the slice from length 2 `[false, true]` that matches neither + /// of these patterns can't be turned to a slice from length 1 that + /// matches neither of these patterns, so we have to consider + /// slices from length 2 there. + /// + /// Now, to see that that length exists and find it, observe that slice + /// patterns are either "fixed-length" patterns (`[_, _, _]`) or + /// "variable-length" patterns (`[_, .., _]`). + /// + /// For fixed-length patterns, all slices with lengths *longer* than + /// the pattern's length have the same outcome (of not matching), so + /// as long as `L` is greater than the pattern's length we can pick + /// any `sₘ` from that length and get the same result. + /// + /// For variable-length patterns, the situation is more complicated, + /// because as seen above the precise value of `sₘ` matters. + /// + /// However, for each variable-length pattern `p` with a prefix of length + /// `plₚ` and suffix of length `slₚ`, only the first `plₚ` and the last + /// `slₚ` elements are examined. + /// + /// Therefore, as long as `L` is positive (to avoid concerns about empty + /// types), all elements after the maximum prefix length and before + /// the maximum suffix length are not examined by any variable-length + /// pattern, and therefore can be added/removed without affecting + /// them - creating equivalent patterns from any sufficiently-large + /// length. + /// + /// Of course, if fixed-length patterns exist, we must be sure + /// that our length is large enough to miss them all, so + /// we can pick `L = max(max(FIXED_LEN)+1, max(PREFIX_LEN) + max(SUFFIX_LEN))` + /// + /// for example, with the above pair of patterns, all elements + /// but the first and last can be added/removed, so any + /// witness of length ≥2 (say, `[false, false, true]`) can be + /// turned to a witness from any other length ≥2. + fn split<'p, 'tcx>( + self, + cx: &MatchCheckCtxt<'p, 'tcx>, + matrix: &Matrix<'p, 'tcx>, + ) -> SmallVec<[Constructor<'tcx>; 1]> { + let (array_len, self_prefix, self_suffix) = match self { + Slice { array_len, kind: VarLen(self_prefix, self_suffix) } => { + (array_len, self_prefix, self_suffix) + } + _ => return smallvec![Slice(self)], + }; + + let head_ctors = + matrix.heads().filter_map(|pat| pat_constructor(cx.tcx, cx.param_env, pat)); + + let mut max_prefix_len = self_prefix; + let mut max_suffix_len = self_suffix; + let mut max_fixed_len = 0; + + for ctor in head_ctors { + if let Slice(slice) = ctor { + match slice.pattern_kind() { + FixedLen(len) => { + max_fixed_len = cmp::max(max_fixed_len, len); + } + VarLen(prefix, suffix) => { + max_prefix_len = cmp::max(max_prefix_len, prefix); + max_suffix_len = cmp::max(max_suffix_len, suffix); + } + } + } + } + + // For diagnostics, we keep the prefix and suffix lengths separate, so in the case + // where `max_fixed_len + 1` is the largest, we adapt `max_prefix_len` accordingly, + // so that `L = max_prefix_len + max_suffix_len`. + 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 also guaranteed to be larger than its previous + // value. + max_prefix_len = max_fixed_len + 1 - max_suffix_len; + } + + match array_len { + Some(len) => { + let kind = if max_prefix_len + max_suffix_len < len { + VarLen(max_prefix_len, max_suffix_len) + } else { + FixedLen(len) + }; + smallvec![Slice(Slice { array_len, kind })] + } + None => { + // `ctor` originally covered the range `(self_prefix + + // self_suffix..infinity)`. We now split it into two: lengths smaller than + // `max_prefix_len + max_suffix_len` are treated independently as + // fixed-lengths slices, and lengths above are captured by a final VarLen + // constructor. + let smaller_lengths = + (self_prefix + self_suffix..max_prefix_len + max_suffix_len).map(FixedLen); + let final_slice = VarLen(max_prefix_len, max_suffix_len); + smaller_lengths + .chain(Some(final_slice)) + .map(|kind| Slice { array_len, kind }) + .map(Slice) + .collect() + } + } + } } /// A value can be decomposed into a constructor applied to some fields. This struct represents @@ -960,6 +1093,45 @@ impl<'tcx> Constructor<'tcx> { } } + /// Some constructors (namely IntRange and Slice) actually stand for a set of actual + /// constructors (integers and fixed-sized slices). When specializing for these + /// constructors, we want to be specialising for the actual underlying constructors. + /// Naively, we would simply return the list of constructors they correspond to. We instead are + /// more clever: if there are constructors that we know will behave the same wrt the current + /// matrix, we keep them grouped. For example, all slices of a sufficiently large length + /// will either be all useful or all non-useful with a given matrix. + /// + /// See the branches for details on how the splitting is done. + /// + /// This function may discard some irrelevant constructors if this preserves behavior and + /// diagnostics. Eg. for the `_` case, we ignore the constructors already present in the + /// matrix, unless all of them are. + /// + /// `hir_id` is `None` when we're evaluating the wildcard pattern. In that case we do not want + /// to lint for overlapping ranges. + fn split<'p>( + self, + cx: &MatchCheckCtxt<'p, 'tcx>, + pcx: PatCtxt<'tcx>, + matrix: &Matrix<'p, 'tcx>, + hir_id: Option, + ) -> SmallVec<[Self; 1]> { + debug!("Constructor::split({:#?}, {:#?})", self, matrix); + + match self { + // Fast-track if the range is trivial. In particular, we don't do the overlapping + // ranges check. + IntRange(ctor_range) + if ctor_range.treat_exhaustively(cx.tcx) && !ctor_range.is_singleton() => + { + ctor_range.split(cx, pcx, matrix, hir_id) + } + Slice(slice @ Slice { kind: VarLen(..), .. }) => slice.split(cx, matrix), + // Any other constructor can be used unchanged. + _ => smallvec![self], + } + } + /// Apply a constructor to a list of patterns, yielding a new pattern. `pats` /// must have as many elements as this constructor's arity. /// @@ -1492,7 +1664,7 @@ impl<'tcx> Witness<'tcx> { /// Invariant: this returns an empty `Vec` if and only if the type is uninhabited (as determined by /// `cx.is_uninhabited()`). fn all_constructors<'a, 'tcx>( - cx: &mut MatchCheckCtxt<'a, 'tcx>, + cx: &MatchCheckCtxt<'a, 'tcx>, pcx: PatCtxt<'tcx>, ) -> Vec> { debug!("all_constructors({:?})", pcx.ty); @@ -1837,6 +2009,153 @@ impl<'tcx> IntRange<'tcx> { // This is a brand new pattern, so we don't reuse `self.span`. Pat { ty: self.ty, span: DUMMY_SP, kind: Box::new(kind) } } + + /// For exhaustive integer matching, some constructors are grouped within other constructors + /// (namely integer typed values are grouped within ranges). However, when specialising these + /// constructors, we want to be specialising for the underlying constructors (the integers), not + /// the groups (the ranges). Thus we need to split the groups up. Splitting them up naïvely would + /// mean creating a separate constructor for every single value in the range, which is clearly + /// impractical. However, observe that for some ranges of integers, the specialisation will be + /// identical across all values in that range (i.e., there are equivalence classes of ranges of + /// constructors based on their `U(S(c, P), S(c, p))` outcome). These classes are grouped by + /// the patterns that apply to them (in the matrix `P`). We can split the range whenever the + /// patterns that apply to that range (specifically: the patterns that *intersect* with that range) + /// change. + /// Our solution, therefore, is to split the range constructor into subranges at every single point + /// the group of intersecting patterns changes (using the method described below). + /// And voilà! We're testing precisely those ranges that we need to, without any exhaustive matching + /// on actual integers. The nice thing about this is that the number of subranges is linear in the + /// number of rows in the matrix (i.e., the number of cases in the `match` statement), so we don't + /// need to be worried about matching over gargantuan ranges. + /// + /// Essentially, given the first column of a matrix representing ranges, looking like the following: + /// + /// |------| |----------| |-------| || + /// |-------| |-------| |----| || + /// |---------| + /// + /// We split the ranges up into equivalence classes so the ranges are no longer overlapping: + /// + /// |--|--|||-||||--||---|||-------| |-|||| || + /// + /// The logic for determining how to split the ranges is fairly straightforward: we calculate + /// boundaries for each interval range, sort them, then create constructors for each new interval + /// between every pair of boundary points. (This essentially sums up to performing the intuitive + /// merging operation depicted above.) + fn split<'p>( + self, + cx: &MatchCheckCtxt<'p, 'tcx>, + pcx: PatCtxt<'tcx>, + matrix: &Matrix<'p, 'tcx>, + hir_id: Option, + ) -> SmallVec<[Constructor<'tcx>; 1]> { + let ty = pcx.ty; + + /// Represents a border between 2 integers. Because the intervals spanning borders + /// must be able to cover every integer, we need to be able to represent + /// 2^128 + 1 such borders. + #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug)] + enum Border { + JustBefore(u128), + AfterMax, + } + + // A function for extracting the borders of an integer interval. + fn range_borders(r: IntRange<'_>) -> impl Iterator { + let (lo, hi) = r.range.into_inner(); + let from = Border::JustBefore(lo); + let to = match hi.checked_add(1) { + Some(m) => Border::JustBefore(m), + None => Border::AfterMax, + }; + vec![from, to].into_iter() + } + + // Collect the span and range of all the intersecting ranges to lint on likely + // incorrect range patterns. (#63987) + let mut overlaps = vec![]; + // `borders` is the set of borders between equivalence classes: each equivalence + // class lies between 2 borders. + let row_borders = matrix + .patterns + .iter() + .flat_map(|row| { + IntRange::from_pat(cx.tcx, cx.param_env, row.head()).map(|r| (r, row.len())) + }) + .flat_map(|(range, row_len)| { + let intersection = self.intersection(cx.tcx, &range); + let should_lint = self.suspicious_intersection(&range); + if let (Some(range), 1, true) = (&intersection, row_len, should_lint) { + // FIXME: for now, only check for overlapping ranges on simple range + // patterns. Otherwise with the current logic the following is detected + // as overlapping: + // match (10u8, true) { + // (0 ..= 125, false) => {} + // (126 ..= 255, false) => {} + // (0 ..= 255, true) => {} + // } + overlaps.push(range.clone()); + } + intersection + }) + .flat_map(range_borders); + let self_borders = range_borders(self.clone()); + let mut borders: Vec<_> = row_borders.chain(self_borders).collect(); + borders.sort_unstable(); + + self.lint_overlapping_patterns(cx.tcx, hir_id, ty, overlaps); + + // We're going to iterate through every adjacent pair of borders, making sure that + // each represents an interval of nonnegative length, and convert each such + // interval into a constructor. + borders + .array_windows() + .filter_map(|&pair| match pair { + [Border::JustBefore(n), Border::JustBefore(m)] => { + if n < m { + Some(n..=(m - 1)) + } else { + None + } + } + [Border::JustBefore(n), Border::AfterMax] => Some(n..=u128::MAX), + [Border::AfterMax, _] => None, + }) + .map(|range| IntRange { range, ty, span: pcx.span }) + .map(IntRange) + .collect() + } + + fn lint_overlapping_patterns( + self, + tcx: TyCtxt<'tcx>, + hir_id: Option, + ty: Ty<'tcx>, + overlaps: Vec>, + ) { + if let (true, Some(hir_id)) = (!overlaps.is_empty(), hir_id) { + tcx.struct_span_lint_hir( + lint::builtin::OVERLAPPING_PATTERNS, + hir_id, + self.span, + |lint| { + let mut err = lint.build("multiple patterns covering the same range"); + err.span_label(self.span, "overlapping patterns"); + for int_range in overlaps { + // Use the real type for user display of the ranges: + err.span_label( + int_range.span, + &format!( + "this range overlaps on `{}`", + IntRange { range: int_range.range, ty, span: DUMMY_SP }.to_pat(tcx), + ), + ); + } + err.emit(); + }, + ); + } + } } /// Ignore spans when comparing, they don't carry semantic information as they are only for lints. @@ -1906,7 +2225,7 @@ impl<'tcx> fmt::Debug for MissingConstructors<'tcx> { /// has one it must not be inserted into the matrix. This shouldn't be /// relied on for soundness. crate fn is_useful<'p, 'tcx>( - cx: &mut MatchCheckCtxt<'p, 'tcx>, + cx: &MatchCheckCtxt<'p, 'tcx>, matrix: &Matrix<'p, 'tcx>, v: &PatStack<'p, 'tcx>, witness_preference: WitnessPreference, @@ -1993,30 +2312,23 @@ crate fn is_useful<'p, 'tcx>( let ret = if let Some(constructor) = pat_constructor(cx.tcx, cx.param_env, v.head()) { debug!("is_useful - expanding constructor: {:#?}", constructor); - split_grouped_constructors( - cx.tcx, - cx.param_env, - pcx, - vec![constructor], - matrix, - pcx.span, - Some(hir_id), - ) - .into_iter() - .map(|c| { - is_useful_specialized( - cx, - matrix, - v, - c, - pcx.ty, - witness_preference, - hir_id, - is_under_guard, - ) - }) - .find(|result| result.is_useful()) - .unwrap_or(NotUseful) + constructor + .split(cx, pcx, matrix, Some(hir_id)) + .into_iter() + .map(|c| { + is_useful_specialized( + cx, + matrix, + v, + c, + pcx.ty, + witness_preference, + hir_id, + is_under_guard, + ) + }) + .find(|result| result.is_useful()) + .unwrap_or(NotUseful) } else { debug!("is_useful - expanding wildcard"); @@ -2045,8 +2357,9 @@ crate fn is_useful<'p, 'tcx>( if missing_ctors.is_empty() { let (all_ctors, _) = missing_ctors.into_inner(); - split_grouped_constructors(cx.tcx, cx.param_env, pcx, all_ctors, matrix, DUMMY_SP, None) + all_ctors .into_iter() + .flat_map(|ctor| ctor.split(cx, pcx, matrix, None)) .map(|c| { is_useful_specialized( cx, @@ -2119,7 +2432,7 @@ crate fn is_useful<'p, 'tcx>( /// A shorthand for the `U(S(c, P), S(c, q))` operation from the paper. I.e., `is_useful` applied /// to the specialised version of both the pattern matrix `P` and the new pattern `q`. fn is_useful_specialized<'p, 'tcx>( - cx: &mut MatchCheckCtxt<'p, 'tcx>, + cx: &MatchCheckCtxt<'p, 'tcx>, matrix: &Matrix<'p, 'tcx>, v: &PatStack<'p, 'tcx>, ctor: Constructor<'tcx>, @@ -2200,303 +2513,6 @@ fn pat_constructor<'tcx>( } } -/// For exhaustive integer matching, some constructors are grouped within other constructors -/// (namely integer typed values are grouped within ranges). However, when specialising these -/// constructors, we want to be specialising for the underlying constructors (the integers), not -/// the groups (the ranges). Thus we need to split the groups up. Splitting them up naïvely would -/// mean creating a separate constructor for every single value in the range, which is clearly -/// impractical. However, observe that for some ranges of integers, the specialisation will be -/// identical across all values in that range (i.e., there are equivalence classes of ranges of -/// constructors based on their `is_useful_specialized` outcome). These classes are grouped by -/// the patterns that apply to them (in the matrix `P`). We can split the range whenever the -/// patterns that apply to that range (specifically: the patterns that *intersect* with that range) -/// change. -/// Our solution, therefore, is to split the range constructor into subranges at every single point -/// the group of intersecting patterns changes (using the method described below). -/// And voilà! We're testing precisely those ranges that we need to, without any exhaustive matching -/// on actual integers. The nice thing about this is that the number of subranges is linear in the -/// number of rows in the matrix (i.e., the number of cases in the `match` statement), so we don't -/// need to be worried about matching over gargantuan ranges. -/// -/// Essentially, given the first column of a matrix representing ranges, looking like the following: -/// -/// |------| |----------| |-------| || -/// |-------| |-------| |----| || -/// |---------| -/// -/// We split the ranges up into equivalence classes so the ranges are no longer overlapping: -/// -/// |--|--|||-||||--||---|||-------| |-|||| || -/// -/// The logic for determining how to split the ranges is fairly straightforward: we calculate -/// boundaries for each interval range, sort them, then create constructors for each new interval -/// between every pair of boundary points. (This essentially sums up to performing the intuitive -/// merging operation depicted above.) -/// -/// `hir_id` is `None` when we're evaluating the wildcard pattern, do not lint for overlapping in -/// ranges that case. -/// -/// This also splits variable-length slices into fixed-length slices. -fn split_grouped_constructors<'p, 'tcx>( - tcx: TyCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, - pcx: PatCtxt<'tcx>, - ctors: Vec>, - matrix: &Matrix<'p, 'tcx>, - span: Span, - hir_id: Option, -) -> Vec> { - let ty = pcx.ty; - let mut split_ctors = Vec::with_capacity(ctors.len()); - debug!("split_grouped_constructors({:#?}, {:#?})", matrix, ctors); - - for ctor in ctors.into_iter() { - match ctor { - IntRange(ctor_range) if ctor_range.treat_exhaustively(tcx) => { - // Fast-track if the range is trivial. In particular, don't do the overlapping - // ranges check. - if ctor_range.is_singleton() { - split_ctors.push(IntRange(ctor_range)); - continue; - } - - /// Represents a border between 2 integers. Because the intervals spanning borders - /// must be able to cover every integer, we need to be able to represent - /// 2^128 + 1 such borders. - #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug)] - enum Border { - JustBefore(u128), - AfterMax, - } - - // A function for extracting the borders of an integer interval. - fn range_borders(r: IntRange<'_>) -> impl Iterator { - let (lo, hi) = r.range.into_inner(); - let from = Border::JustBefore(lo); - let to = match hi.checked_add(1) { - Some(m) => Border::JustBefore(m), - None => Border::AfterMax, - }; - vec![from, to].into_iter() - } - - // Collect the span and range of all the intersecting ranges to lint on likely - // incorrect range patterns. (#63987) - let mut overlaps = vec![]; - // `borders` is the set of borders between equivalence classes: each equivalence - // class lies between 2 borders. - let row_borders = matrix - .patterns - .iter() - .flat_map(|row| { - IntRange::from_pat(tcx, param_env, row.head()).map(|r| (r, row.len())) - }) - .flat_map(|(range, row_len)| { - let intersection = ctor_range.intersection(tcx, &range); - let should_lint = ctor_range.suspicious_intersection(&range); - if let (Some(range), 1, true) = (&intersection, row_len, should_lint) { - // FIXME: for now, only check for overlapping ranges on simple range - // patterns. Otherwise with the current logic the following is detected - // as overlapping: - // match (10u8, true) { - // (0 ..= 125, false) => {} - // (126 ..= 255, false) => {} - // (0 ..= 255, true) => {} - // } - overlaps.push(range.clone()); - } - intersection - }) - .flat_map(range_borders); - let ctor_borders = range_borders(ctor_range.clone()); - let mut borders: Vec<_> = row_borders.chain(ctor_borders).collect(); - borders.sort_unstable(); - - lint_overlapping_patterns(tcx, hir_id, ctor_range, ty, overlaps); - - // We're going to iterate through every adjacent pair of borders, making sure that - // each represents an interval of nonnegative length, and convert each such - // interval into a constructor. - split_ctors.extend( - borders - .array_windows() - .filter_map(|&pair| match pair { - [Border::JustBefore(n), Border::JustBefore(m)] => { - if n < m { - Some(IntRange { range: n..=(m - 1), ty, span }) - } else { - None - } - } - [Border::JustBefore(n), Border::AfterMax] => { - Some(IntRange { range: n..=u128::MAX, ty, span }) - } - [Border::AfterMax, _] => None, - }) - .map(IntRange), - ); - } - Slice(Slice { array_len, kind: VarLen(self_prefix, self_suffix) }) => { - // The exhaustiveness-checking paper does not include any details on - // checking variable-length slice patterns. However, they are matched - // by an infinite collection of fixed-length array patterns. - // - // Checking the infinite set directly would take an infinite amount - // of time. However, it turns out that for each finite set of - // patterns `P`, all sufficiently large array lengths are equivalent: - // - // Each slice `s` with a "sufficiently-large" length `l ≥ L` that applies - // to exactly the subset `Pₜ` of `P` can be transformed to a slice - // `sₘ` for each sufficiently-large length `m` that applies to exactly - // the same subset of `P`. - // - // Because of that, each witness for reachability-checking from one - // of the sufficiently-large lengths can be transformed to an - // equally-valid witness from any other length, so we only have - // to check slice lengths from the "minimal sufficiently-large length" - // and below. - // - // Note that the fact that there is a *single* `sₘ` for each `m` - // not depending on the specific pattern in `P` is important: if - // you look at the pair of patterns - // `[true, ..]` - // `[.., false]` - // Then any slice of length ≥1 that matches one of these two - // patterns can be trivially turned to a slice of any - // other length ≥1 that matches them and vice-versa - for - // but the slice from length 2 `[false, true]` that matches neither - // of these patterns can't be turned to a slice from length 1 that - // matches neither of these patterns, so we have to consider - // slices from length 2 there. - // - // Now, to see that that length exists and find it, observe that slice - // patterns are either "fixed-length" patterns (`[_, _, _]`) or - // "variable-length" patterns (`[_, .., _]`). - // - // For fixed-length patterns, all slices with lengths *longer* than - // the pattern's length have the same outcome (of not matching), so - // as long as `L` is greater than the pattern's length we can pick - // any `sₘ` from that length and get the same result. - // - // For variable-length patterns, the situation is more complicated, - // because as seen above the precise value of `sₘ` matters. - // - // However, for each variable-length pattern `p` with a prefix of length - // `plₚ` and suffix of length `slₚ`, only the first `plₚ` and the last - // `slₚ` elements are examined. - // - // Therefore, as long as `L` is positive (to avoid concerns about empty - // types), all elements after the maximum prefix length and before - // the maximum suffix length are not examined by any variable-length - // pattern, and therefore can be added/removed without affecting - // them - creating equivalent patterns from any sufficiently-large - // length. - // - // Of course, if fixed-length patterns exist, we must be sure - // that our length is large enough to miss them all, so - // we can pick `L = max(max(FIXED_LEN)+1, max(PREFIX_LEN) + max(SUFFIX_LEN))` - // - // for example, with the above pair of patterns, all elements - // but the first and last can be added/removed, so any - // witness of length ≥2 (say, `[false, false, true]`) can be - // turned to a witness from any other length ≥2. - - let mut max_prefix_len = self_prefix; - let mut max_suffix_len = self_suffix; - let mut max_fixed_len = 0; - - let head_ctors = - matrix.heads().filter_map(|pat| pat_constructor(tcx, param_env, pat)); - for ctor in head_ctors { - if let Slice(slice) = ctor { - match slice.pattern_kind() { - FixedLen(len) => { - max_fixed_len = cmp::max(max_fixed_len, len); - } - VarLen(prefix, suffix) => { - max_prefix_len = cmp::max(max_prefix_len, prefix); - max_suffix_len = cmp::max(max_suffix_len, suffix); - } - } - } - } - - // For diagnostics, we keep the prefix and suffix lengths separate, so in the case - // where `max_fixed_len + 1` is the largest, we adapt `max_prefix_len` accordingly, - // so that `L = max_prefix_len + max_suffix_len`. - 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 also guaranteed to be larger than its previous - // value. - max_prefix_len = max_fixed_len + 1 - max_suffix_len; - } - - match array_len { - Some(len) => { - let kind = if max_prefix_len + max_suffix_len < len { - VarLen(max_prefix_len, max_suffix_len) - } else { - FixedLen(len) - }; - split_ctors.push(Slice(Slice { array_len, kind })); - } - None => { - // `ctor` originally covered the range `(self_prefix + - // self_suffix..infinity)`. We now split it into two: lengths smaller than - // `max_prefix_len + max_suffix_len` are treated independently as - // fixed-lengths slices, and lengths above are captured by a final VarLen - // constructor. - split_ctors.extend( - (self_prefix + self_suffix..max_prefix_len + max_suffix_len) - .map(|len| Slice(Slice { array_len, kind: FixedLen(len) })), - ); - split_ctors.push(Slice(Slice { - array_len, - kind: VarLen(max_prefix_len, max_suffix_len), - })); - } - } - } - // Any other constructor can be used unchanged. - _ => split_ctors.push(ctor), - } - } - - debug!("split_grouped_constructors(..)={:#?}", split_ctors); - split_ctors -} - -fn lint_overlapping_patterns<'tcx>( - tcx: TyCtxt<'tcx>, - hir_id: Option, - ctor_range: IntRange<'tcx>, - ty: Ty<'tcx>, - overlaps: Vec>, -) { - if let (true, Some(hir_id)) = (!overlaps.is_empty(), hir_id) { - tcx.struct_span_lint_hir( - lint::builtin::OVERLAPPING_PATTERNS, - hir_id, - ctor_range.span, - |lint| { - let mut err = lint.build("multiple patterns covering the same range"); - err.span_label(ctor_range.span, "overlapping patterns"); - for int_range in overlaps { - // Use the real type for user display of the ranges: - err.span_label( - int_range.span, - &format!( - "this range overlaps on `{}`", - IntRange { range: int_range.range, ty, span: DUMMY_SP }.to_pat(tcx), - ), - ); - } - err.emit(); - }, - ); - } -} - /// This is the main specialization step. It expands the pattern /// into `arity` patterns based on the constructor. For most patterns, the step is trivial, /// for instance tuple patterns are flattened and box patterns expand into their inner pattern. @@ -2509,7 +2525,7 @@ fn lint_overlapping_patterns<'tcx>( /// /// This is roughly the inverse of `Constructor::apply`. fn specialize_one_pattern<'p, 'tcx>( - cx: &mut MatchCheckCtxt<'p, 'tcx>, + cx: &MatchCheckCtxt<'p, 'tcx>, pat: &'p Pat<'tcx>, constructor: &Constructor<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, From 7c4f94be4859ee6b6bc8fd50fa75328b3d30c9aa Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 19 Oct 2020 23:58:04 +0100 Subject: [PATCH 02/16] Use pat_constructor to simplify specialize_one_pattern --- .../src/thir/pattern/_match.rs | 176 +++++++----------- 1 file changed, 71 insertions(+), 105 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 29356cba50efe..3c9b8d5da1d24 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -2529,17 +2529,69 @@ fn specialize_one_pattern<'p, 'tcx>( pat: &'p Pat<'tcx>, constructor: &Constructor<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, - is_its_own_ctor: bool, // Whether `ctor` is known to be derived from `pat` + is_its_own_ctor: bool, // Whether `constructor` is known to be derived from `pat` ) -> Option> { - if let NonExhaustive = constructor { - // Only a wildcard pattern can match the special extra constructor. - if !pat.is_wildcard() { - return None; - } - return Some(Fields::empty()); + if pat.is_wildcard() { + return Some(ctor_wild_subpatterns.clone()); } - if let Opaque = constructor { + let ty = pat.ty; + // `unwrap` is safe because `pat` is not a wildcard. + let pat_ctor = pat_constructor(cx.tcx, cx.param_env, pat).unwrap(); + + let result = match (constructor, &pat_ctor, pat.kind.as_ref()) { + (Single, Single, PatKind::Leaf { subpatterns }) => { + Some(ctor_wild_subpatterns.replace_with_fieldpats(subpatterns)) + } + (Single, Single, PatKind::Deref { subpattern }) => { + Some(Fields::from_single_pattern(subpattern)) + } + (Variant(_), Variant(_), _) if constructor != &pat_ctor => None, + (Variant(_), Variant(_), PatKind::Variant { subpatterns, .. }) => { + Some(ctor_wild_subpatterns.replace_with_fieldpats(subpatterns)) + } + + (IntRange(ctor_range), IntRange(pat_range), _) => { + ctor_range.intersection(cx.tcx, &pat_range)?; + // Constructor splitting should ensure that all intersections we encounter + // are actually inclusions. + assert!(ctor_range.is_subrange(&pat_range)); + Some(Fields::empty()) + } + (FloatRange(ctor_from, ctor_to, ctor_end), FloatRange(pat_from, pat_to, pat_end), _) => { + let to = compare_const_vals(cx.tcx, ctor_to, pat_to, cx.param_env, ty)?; + let from = compare_const_vals(cx.tcx, ctor_from, pat_from, cx.param_env, ty)?; + let intersects = (from == Ordering::Greater || from == Ordering::Equal) + && (to == Ordering::Less || (pat_end == ctor_end && to == Ordering::Equal)); + if intersects { Some(Fields::empty()) } else { None } + } + (Str(ctor_val), Str(pat_val), _) => { + // FIXME: there's probably a more direct way of comparing for equality + let comparison = compare_const_vals(cx.tcx, ctor_val, pat_val, cx.param_env, ty)?; + if comparison == Ordering::Equal { Some(Fields::empty()) } else { None } + } + + (Slice(ctor_slice), Slice(pat_slice), _) + if !pat_slice.pattern_kind().covers_length(ctor_slice.arity()) => + { + None + } + ( + Slice(ctor_slice), + Slice(_), + PatKind::Array { prefix, suffix, .. } | PatKind::Slice { prefix, suffix, .. }, + ) => { + // Number of subpatterns for the constructor + let ctor_arity = ctor_slice.arity(); + + // Replace the prefix and the suffix with the given patterns, leaving wildcards in + // the middle if there was a subslice pattern `..`. + let prefix = prefix.iter().enumerate(); + let suffix = + suffix.iter().enumerate().map(|(i, p)| (ctor_arity as usize - suffix.len() + i, p)); + Some(ctor_wild_subpatterns.replace_fields_indexed(prefix.chain(suffix))) + } + // Only a wildcard pattern can match an opaque constant, unless we're specializing the // value against its own constructor. That happens when we call // `v.specialize_constructor(ctor)` with `ctor` obtained from `pat_constructor(v.head())`. @@ -2559,109 +2611,23 @@ fn specialize_one_pattern<'p, 'tcx>( // (FOO, false) => {} // } // ``` - if is_its_own_ctor || pat.is_wildcard() { - return Some(Fields::empty()); - } else { - return None; - } - } - - let result = match *pat.kind { - PatKind::AscribeUserType { .. } => bug!(), // Handled by `expand_pattern` - - PatKind::Binding { .. } | PatKind::Wild => Some(ctor_wild_subpatterns.clone()), - - PatKind::Variant { adt_def, variant_index, ref subpatterns, .. } => { - let variant = &adt_def.variants[variant_index]; - if constructor != &Variant(variant.def_id) { - return None; - } - Some(ctor_wild_subpatterns.replace_with_fieldpats(subpatterns)) - } - - PatKind::Leaf { ref subpatterns } => { - Some(ctor_wild_subpatterns.replace_with_fieldpats(subpatterns)) - } - - PatKind::Deref { ref subpattern } => Some(Fields::from_single_pattern(subpattern)), - - PatKind::Constant { .. } | PatKind::Range { .. } => { - match constructor { - IntRange(ctor) => { - let pat = IntRange::from_pat(cx.tcx, cx.param_env, pat)?; - ctor.intersection(cx.tcx, &pat)?; - // Constructor splitting should ensure that all intersections we encounter - // are actually inclusions. - assert!(ctor.is_subrange(&pat)); - } - FloatRange(ctor_from, ctor_to, ctor_end) => { - let (pat_from, pat_to, pat_end, ty) = match *pat.kind { - PatKind::Constant { value } => (value, value, RangeEnd::Included, value.ty), - PatKind::Range(PatRange { lo, hi, end }) => (lo, hi, end, lo.ty), - _ => unreachable!(), // This is ensured by the branch we're in - }; - let to = compare_const_vals(cx.tcx, ctor_to, pat_to, cx.param_env, ty)?; - let from = compare_const_vals(cx.tcx, ctor_from, pat_from, cx.param_env, ty)?; - let intersects = (from == Ordering::Greater || from == Ordering::Equal) - && (to == Ordering::Less - || (pat_end == *ctor_end && to == Ordering::Equal)); - if !intersects { - return None; - } - } - Str(ctor_value) => { - let pat_value = match *pat.kind { - PatKind::Constant { value } => value, - _ => span_bug!( - pat.span, - "unexpected range pattern {:?} for constant value ctor", - pat - ), - }; - - // FIXME: there's probably a more direct way of comparing for equality - if compare_const_vals(cx.tcx, ctor_value, pat_value, cx.param_env, pat.ty)? - != Ordering::Equal - { - return None; - } - } - _ => { - // If we reach here, we must be trying to inspect an opaque constant. Thus we skip - // the row. - return None; - } - } - Some(Fields::empty()) - } - - PatKind::Array { ref prefix, ref slice, ref suffix } - | PatKind::Slice { ref prefix, ref slice, ref suffix } => match *constructor { - Slice(_) => { - // Number of subpatterns for this pattern - let pat_len = prefix.len() + suffix.len(); - // Number of subpatterns for this constructor - let arity = ctor_wild_subpatterns.len(); - - if (slice.is_none() && arity != pat_len) || pat_len > arity { - return None; - } - - // Replace the prefix and the suffix with the given patterns, leaving wildcards in - // the middle if there was a subslice pattern `..`. - let prefix = prefix.iter().enumerate(); - let suffix = suffix.iter().enumerate().map(|(i, p)| (arity - suffix.len() + i, p)); - Some(ctor_wild_subpatterns.replace_fields_indexed(prefix.chain(suffix))) - } - _ => span_bug!(pat.span, "unexpected ctor {:?} for slice pat", constructor), - }, + (Opaque, Opaque, _) if is_its_own_ctor => Some(Fields::empty()), + // We are trying to inspect an opaque constant. Thus we skip the row. + (Opaque, _, _) | (_, Opaque, _) => None, + // Only a wildcard pattern can match the special extra constructor. + (NonExhaustive, _, _) => None, - PatKind::Or { .. } => bug!("Or-pattern should have been expanded earlier on."), + _ => bug!("trying to specialize pattern {:?} with constructor {:?}", pat, constructor), }; + debug!( "specialize({:#?}, {:#?}, {:#?}) = {:#?}", pat, constructor, ctor_wild_subpatterns, result ); + if let Some(fields) = &result { + debug_assert_eq!(fields.len(), ctor_wild_subpatterns.len()); + } + result } From 6ad9f44a50fdfdb2388850fbd9b07db66f452ec7 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sun, 25 Oct 2020 22:42:43 +0000 Subject: [PATCH 03/16] Clarify specialization into two steps First is checking for constructor overlap, second is extracting the resulting fields. --- .../src/thir/pattern/_match.rs | 104 ++++++++---------- 1 file changed, 48 insertions(+), 56 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 3c9b8d5da1d24..aee1320ec89f8 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -2527,9 +2527,9 @@ fn pat_constructor<'tcx>( fn specialize_one_pattern<'p, 'tcx>( cx: &MatchCheckCtxt<'p, 'tcx>, pat: &'p Pat<'tcx>, - constructor: &Constructor<'tcx>, + ctor: &Constructor<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, - is_its_own_ctor: bool, // Whether `constructor` is known to be derived from `pat` + is_its_own_ctor: bool, // Whether `ctor` is known to be derived from `pat` ) -> Option> { if pat.is_wildcard() { return Some(ctor_wild_subpatterns.clone()); @@ -2539,57 +2539,34 @@ fn specialize_one_pattern<'p, 'tcx>( // `unwrap` is safe because `pat` is not a wildcard. let pat_ctor = pat_constructor(cx.tcx, cx.param_env, pat).unwrap(); - let result = match (constructor, &pat_ctor, pat.kind.as_ref()) { - (Single, Single, PatKind::Leaf { subpatterns }) => { - Some(ctor_wild_subpatterns.replace_with_fieldpats(subpatterns)) - } - (Single, Single, PatKind::Deref { subpattern }) => { - Some(Fields::from_single_pattern(subpattern)) - } - (Variant(_), Variant(_), _) if constructor != &pat_ctor => None, - (Variant(_), Variant(_), PatKind::Variant { subpatterns, .. }) => { - Some(ctor_wild_subpatterns.replace_with_fieldpats(subpatterns)) - } + let ctor_covered_by_pat = match (ctor, &pat_ctor) { + (Single, Single) => true, + (Variant(ctor_id), Variant(pat_id)) => ctor_id == pat_id, - (IntRange(ctor_range), IntRange(pat_range), _) => { - ctor_range.intersection(cx.tcx, &pat_range)?; - // Constructor splitting should ensure that all intersections we encounter - // are actually inclusions. - assert!(ctor_range.is_subrange(&pat_range)); - Some(Fields::empty()) + (IntRange(ctor_range), IntRange(pat_range)) => { + if ctor_range.intersection(cx.tcx, pat_range).is_some() { + // Constructor splitting should ensure that all intersections we encounter + // are actually inclusions. + assert!(ctor_range.is_subrange(pat_range)); + true + } else { + false + } } - (FloatRange(ctor_from, ctor_to, ctor_end), FloatRange(pat_from, pat_to, pat_end), _) => { + (FloatRange(ctor_from, ctor_to, ctor_end), FloatRange(pat_from, pat_to, pat_end)) => { let to = compare_const_vals(cx.tcx, ctor_to, pat_to, cx.param_env, ty)?; let from = compare_const_vals(cx.tcx, ctor_from, pat_from, cx.param_env, ty)?; - let intersects = (from == Ordering::Greater || from == Ordering::Equal) - && (to == Ordering::Less || (pat_end == ctor_end && to == Ordering::Equal)); - if intersects { Some(Fields::empty()) } else { None } + (from == Ordering::Greater || from == Ordering::Equal) + && (to == Ordering::Less || (pat_end == ctor_end && to == Ordering::Equal)) } - (Str(ctor_val), Str(pat_val), _) => { + (Str(ctor_val), Str(pat_val)) => { // FIXME: there's probably a more direct way of comparing for equality let comparison = compare_const_vals(cx.tcx, ctor_val, pat_val, cx.param_env, ty)?; - if comparison == Ordering::Equal { Some(Fields::empty()) } else { None } - } - - (Slice(ctor_slice), Slice(pat_slice), _) - if !pat_slice.pattern_kind().covers_length(ctor_slice.arity()) => - { - None + comparison == Ordering::Equal } - ( - Slice(ctor_slice), - Slice(_), - PatKind::Array { prefix, suffix, .. } | PatKind::Slice { prefix, suffix, .. }, - ) => { - // Number of subpatterns for the constructor - let ctor_arity = ctor_slice.arity(); - // Replace the prefix and the suffix with the given patterns, leaving wildcards in - // the middle if there was a subslice pattern `..`. - let prefix = prefix.iter().enumerate(); - let suffix = - suffix.iter().enumerate().map(|(i, p)| (ctor_arity as usize - suffix.len() + i, p)); - Some(ctor_wild_subpatterns.replace_fields_indexed(prefix.chain(suffix))) + (Slice(ctor_slice), Slice(pat_slice)) => { + pat_slice.pattern_kind().covers_length(ctor_slice.arity()) } // Only a wildcard pattern can match an opaque constant, unless we're specializing the @@ -2611,23 +2588,38 @@ fn specialize_one_pattern<'p, 'tcx>( // (FOO, false) => {} // } // ``` - (Opaque, Opaque, _) if is_its_own_ctor => Some(Fields::empty()), + (Opaque, Opaque) if is_its_own_ctor => true, // We are trying to inspect an opaque constant. Thus we skip the row. - (Opaque, _, _) | (_, Opaque, _) => None, + (Opaque, _) | (_, Opaque) => false, // Only a wildcard pattern can match the special extra constructor. - (NonExhaustive, _, _) => None, + (NonExhaustive, _) => false, - _ => bug!("trying to specialize pattern {:?} with constructor {:?}", pat, constructor), + _ => bug!("trying to specialize pattern {:?} with constructor {:?}", pat, ctor), }; - debug!( - "specialize({:#?}, {:#?}, {:#?}) = {:#?}", - pat, constructor, ctor_wild_subpatterns, result - ); - - if let Some(fields) = &result { - debug_assert_eq!(fields.len(), ctor_wild_subpatterns.len()); + if !ctor_covered_by_pat { + return None; } - result + let fields = match pat.kind.as_ref() { + PatKind::Deref { subpattern } => Fields::from_single_pattern(subpattern), + PatKind::Leaf { subpatterns } | PatKind::Variant { subpatterns, .. } => { + ctor_wild_subpatterns.replace_with_fieldpats(subpatterns) + } + PatKind::Array { prefix, suffix, .. } | PatKind::Slice { prefix, suffix, .. } => { + // Number of subpatterns for the constructor + let ctor_arity = ctor_wild_subpatterns.len(); + + // Replace the prefix and the suffix with the given patterns, leaving wildcards in + // the middle if there was a subslice pattern `..`. + let prefix = prefix.iter().enumerate(); + let suffix = suffix.iter().enumerate().map(|(i, p)| (ctor_arity - suffix.len() + i, p)); + ctor_wild_subpatterns.replace_fields_indexed(prefix.chain(suffix)) + } + _ => ctor_wild_subpatterns.clone(), + }; + + debug!("specialize({:#?}, {:#?}, {:#?}) = {:#?}", pat, ctor, ctor_wild_subpatterns, fields); + + Some(fields) } From c511955a9ff89089bff313cd8a87a6e62e2783f5 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sun, 25 Oct 2020 22:51:50 +0000 Subject: [PATCH 04/16] Factor out the two specialization steps --- .../src/thir/pattern/_match.rs | 187 ++++++++++-------- 1 file changed, 107 insertions(+), 80 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index aee1320ec89f8..7dbde71b2a60e 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -1132,6 +1132,66 @@ impl<'tcx> Constructor<'tcx> { } } + /// Returns whether `self` is covered by `other`, ie whether `self` is a subset of `other`. For + /// the simple cases, this is simply checking for equality. For the "grouped" constructors, + /// this checks for inclusion. + fn is_covered_by<'p>( + &self, + cx: &MatchCheckCtxt<'p, 'tcx>, + other: &Constructor<'tcx>, + ty: Ty<'tcx>, + ) -> bool { + match (self, other) { + (Single, Single) => true, + (Variant(self_id), Variant(other_id)) => self_id == other_id, + + (IntRange(self_range), IntRange(other_range)) => { + if self_range.intersection(cx.tcx, other_range).is_some() { + // Constructor splitting should ensure that all intersections we encounter + // are actually inclusions. + assert!(self_range.is_subrange(other_range)); + true + } else { + false + } + } + ( + FloatRange(self_from, self_to, self_end), + FloatRange(other_from, other_to, other_end), + ) => { + match ( + compare_const_vals(cx.tcx, self_to, other_to, cx.param_env, ty), + compare_const_vals(cx.tcx, self_from, other_from, cx.param_env, ty), + ) { + (Some(to), Some(from)) => { + (from == Ordering::Greater || from == Ordering::Equal) + && (to == Ordering::Less + || (other_end == self_end && to == Ordering::Equal)) + } + _ => false, + } + } + (Str(self_val), Str(other_val)) => { + // FIXME: there's probably a more direct way of comparing for equality + match compare_const_vals(cx.tcx, self_val, other_val, cx.param_env, ty) { + Some(comparison) => comparison == Ordering::Equal, + None => false, + } + } + + (Slice(self_slice), Slice(other_slice)) => { + other_slice.pattern_kind().covers_length(self_slice.arity()) + } + + // We are trying to inspect an opaque constant. Thus we skip the row. + (Opaque, _) | (_, Opaque) => false, + // Only a wildcard pattern can match the special extra constructor. + (NonExhaustive, _) => false, + + _ => bug!("trying to compare incompatible constructors {:?} and {:?}", self, other), + } + } + /// Apply a constructor to a list of patterns, yielding a new pattern. `pats` /// must have as many elements as this constructor's arity. /// @@ -1461,6 +1521,41 @@ impl<'p, 'tcx> Fields<'p, 'tcx> { } } + /// Replaces contained fields with the arguments of the given pattern. Only use on a pattern + /// that is compatible with the constructor used to build `self`. + /// This is meant to be used on the result of `Fields::wildcards()`. The idea is that + /// `wildcards` constructs a list of fields where all entries are wildcards, and the pattern + /// provided to this function fills some of the fields with non-wildcards. + /// In the following example `Fields::wildcards` would return `[_, _, _, _]`. If we call + /// `replace_with_pattern_arguments` on it with the pattern, the result will be `[Some(0), _, + /// _, _]`. + /// ```rust + /// let x: [Option; 4] = foo(); + /// match x { + /// [Some(0), ..] => {} + /// } + /// ``` + fn replace_with_pattern_arguments(&self, pat: &'p Pat<'tcx>) -> Self { + match pat.kind.as_ref() { + PatKind::Deref { subpattern } => Self::from_single_pattern(subpattern), + PatKind::Leaf { subpatterns } | PatKind::Variant { subpatterns, .. } => { + self.replace_with_fieldpats(subpatterns) + } + PatKind::Array { prefix, suffix, .. } | PatKind::Slice { prefix, suffix, .. } => { + // Number of subpatterns for the constructor + let ctor_arity = self.len(); + + // Replace the prefix and the suffix with the given patterns, leaving wildcards in + // the middle if there was a subslice pattern `..`. + let prefix = prefix.iter().enumerate(); + let suffix = + suffix.iter().enumerate().map(|(i, p)| (ctor_arity - suffix.len() + i, p)); + self.replace_fields_indexed(prefix.chain(suffix)) + } + _ => self.clone(), + } + } + fn push_on_patstack(self, stack: &[&'p Pat<'tcx>]) -> PatStack<'p, 'tcx> { let pats: SmallVec<_> = match self { Fields::Slice(pats) => pats.iter().chain(stack.iter().copied()).collect(), @@ -2535,89 +2630,21 @@ fn specialize_one_pattern<'p, 'tcx>( return Some(ctor_wild_subpatterns.clone()); } - let ty = pat.ty; - // `unwrap` is safe because `pat` is not a wildcard. - let pat_ctor = pat_constructor(cx.tcx, cx.param_env, pat).unwrap(); - - let ctor_covered_by_pat = match (ctor, &pat_ctor) { - (Single, Single) => true, - (Variant(ctor_id), Variant(pat_id)) => ctor_id == pat_id, - - (IntRange(ctor_range), IntRange(pat_range)) => { - if ctor_range.intersection(cx.tcx, pat_range).is_some() { - // Constructor splitting should ensure that all intersections we encounter - // are actually inclusions. - assert!(ctor_range.is_subrange(pat_range)); - true - } else { - false - } + // We return `None` if `ctor` is not covered by `pat`. If `ctor` is known to be derived from + // `pat` then we don't need to check; otherwise, we compute the constructor of `pat` and check + // for constructor inclusion. + // Note that this shortcut is also necessary for correctness: a pattern should always be + // specializable with its own constructor, even in cases where we refuse to inspect values like + // opaque constants. + if !is_its_own_ctor { + // `unwrap` is safe because `pat` is not a wildcard. + let pat_ctor = pat_constructor(cx.tcx, cx.param_env, pat).unwrap(); + if !ctor.is_covered_by(cx, &pat_ctor, pat.ty) { + return None; } - (FloatRange(ctor_from, ctor_to, ctor_end), FloatRange(pat_from, pat_to, pat_end)) => { - let to = compare_const_vals(cx.tcx, ctor_to, pat_to, cx.param_env, ty)?; - let from = compare_const_vals(cx.tcx, ctor_from, pat_from, cx.param_env, ty)?; - (from == Ordering::Greater || from == Ordering::Equal) - && (to == Ordering::Less || (pat_end == ctor_end && to == Ordering::Equal)) - } - (Str(ctor_val), Str(pat_val)) => { - // FIXME: there's probably a more direct way of comparing for equality - let comparison = compare_const_vals(cx.tcx, ctor_val, pat_val, cx.param_env, ty)?; - comparison == Ordering::Equal - } - - (Slice(ctor_slice), Slice(pat_slice)) => { - pat_slice.pattern_kind().covers_length(ctor_slice.arity()) - } - - // Only a wildcard pattern can match an opaque constant, unless we're specializing the - // value against its own constructor. That happens when we call - // `v.specialize_constructor(ctor)` with `ctor` obtained from `pat_constructor(v.head())`. - // For example, in the following match, when we are dealing with the third branch, we will - // specialize with an `Opaque` ctor. We want to ignore the second branch because opaque - // constants should not be inspected, but we don't want to ignore the current (third) - // branch, as that would cause us to always conclude that such a branch is unreachable. - // ```rust - // #[derive(PartialEq)] - // struct Foo(i32); - // impl Eq for Foo {} - // const FOO: Foo = Foo(42); - // - // match (Foo(0), true) { - // (_, true) => {} - // (FOO, true) => {} - // (FOO, false) => {} - // } - // ``` - (Opaque, Opaque) if is_its_own_ctor => true, - // We are trying to inspect an opaque constant. Thus we skip the row. - (Opaque, _) | (_, Opaque) => false, - // Only a wildcard pattern can match the special extra constructor. - (NonExhaustive, _) => false, - - _ => bug!("trying to specialize pattern {:?} with constructor {:?}", pat, ctor), - }; - - if !ctor_covered_by_pat { - return None; } - let fields = match pat.kind.as_ref() { - PatKind::Deref { subpattern } => Fields::from_single_pattern(subpattern), - PatKind::Leaf { subpatterns } | PatKind::Variant { subpatterns, .. } => { - ctor_wild_subpatterns.replace_with_fieldpats(subpatterns) - } - PatKind::Array { prefix, suffix, .. } | PatKind::Slice { prefix, suffix, .. } => { - // Number of subpatterns for the constructor - let ctor_arity = ctor_wild_subpatterns.len(); - - // Replace the prefix and the suffix with the given patterns, leaving wildcards in - // the middle if there was a subslice pattern `..`. - let prefix = prefix.iter().enumerate(); - let suffix = suffix.iter().enumerate().map(|(i, p)| (ctor_arity - suffix.len() + i, p)); - ctor_wild_subpatterns.replace_fields_indexed(prefix.chain(suffix)) - } - _ => ctor_wild_subpatterns.clone(), - }; + let fields = ctor_wild_subpatterns.replace_with_pattern_arguments(pat); debug!("specialize({:#?}, {:#?}, {:#?}) = {:#?}", pat, ctor, ctor_wild_subpatterns, fields); From 41e7ca499d51913131a5afa5fa76db914cb672cd Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sun, 25 Oct 2020 23:03:15 +0000 Subject: [PATCH 05/16] Inline `specialize_one_pattern` --- .../src/thir/pattern/_match.rs | 90 ++++++++----------- 1 file changed, 39 insertions(+), 51 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 7dbde71b2a60e..d3602dac9e854 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -407,20 +407,51 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { } /// This computes `S(constructor, self)`. See top of the file for explanations. + /// + /// This is the main specialization step. It expands the pattern + /// into `arity` patterns based on the constructor. For most patterns, the step is trivial, + /// for instance tuple patterns are flattened and box patterns expand into their inner pattern. + /// Returns `None` if the pattern does not have the given constructor. + /// + /// OTOH, slice patterns with a subslice pattern (tail @ ..) can be expanded into multiple + /// different patterns. + /// Structure patterns with a partial wild pattern (Foo { a: 42, .. }) have their missing + /// fields filled with wild patterns. + /// + /// This is roughly the inverse of `Constructor::apply`. fn specialize_constructor( &self, cx: &MatchCheckCtxt<'p, 'tcx>, - constructor: &Constructor<'tcx>, + ctor: &Constructor<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, is_my_head_ctor: bool, ) -> Option> { - let new_fields = specialize_one_pattern( - cx, + // We return `None` if `ctor` is not covered by `self.head()`. If `ctor` is known to be + // derived from `self.head()`, or if `self.head()` is a wildcard, then we don't need to + // check; otherwise, we compute the constructor of `self.head()` and check for constructor + // inclusion. + // Note that this shortcut is also necessary for correctness: a pattern should always be + // specializable with its own constructor, even in cases where we refuse to inspect values like + // opaque constants. + if !self.head().is_wildcard() && !is_my_head_ctor { + // `unwrap` is safe because `pat` is not a wildcard. + let head_ctor = pat_constructor(cx.tcx, cx.param_env, self.head()).unwrap(); + if !ctor.is_covered_by(cx, &head_ctor, self.head().ty) { + return None; + } + } + let new_fields = ctor_wild_subpatterns.replace_with_pattern_arguments(self.head()); + + debug!( + "specialize_constructor({:#?}, {:#?}, {:#?}) = {:#?}", self.head(), - constructor, + ctor, ctor_wild_subpatterns, - is_my_head_ctor, - )?; + new_fields + ); + + // We pop the head pattern and push the new fields extracted from the arguments of + // `self.head()`. Some(new_fields.push_on_patstack(&self.0[1..])) } } @@ -971,7 +1002,7 @@ impl Slice { /// the constructor. See also `Fields`. /// /// `pat_constructor` retrieves the constructor corresponding to a pattern. -/// `specialize_one_pattern` returns the list of fields corresponding to a pattern, given a +/// `specialize_constructor` returns the list of fields corresponding to a pattern, given a /// constructor. `Constructor::apply` reconstructs the pattern from a pair of `Constructor` and /// `Fields`. #[derive(Clone, Debug, PartialEq)] @@ -1195,7 +1226,7 @@ impl<'tcx> Constructor<'tcx> { /// Apply a constructor to a list of patterns, yielding a new pattern. `pats` /// must have as many elements as this constructor's arity. /// - /// This is roughly the inverse of `specialize_one_pattern`. + /// This is roughly the inverse of `specialize_constructor`. /// /// Examples: /// `self`: `Constructor::Single` @@ -2607,46 +2638,3 @@ fn pat_constructor<'tcx>( PatKind::Or { .. } => bug!("Or-pattern should have been expanded earlier on."), } } - -/// This is the main specialization step. It expands the pattern -/// into `arity` patterns based on the constructor. For most patterns, the step is trivial, -/// for instance tuple patterns are flattened and box patterns expand into their inner pattern. -/// Returns `None` if the pattern does not have the given constructor. -/// -/// OTOH, slice patterns with a subslice pattern (tail @ ..) can be expanded into multiple -/// different patterns. -/// Structure patterns with a partial wild pattern (Foo { a: 42, .. }) have their missing -/// fields filled with wild patterns. -/// -/// This is roughly the inverse of `Constructor::apply`. -fn specialize_one_pattern<'p, 'tcx>( - cx: &MatchCheckCtxt<'p, 'tcx>, - pat: &'p Pat<'tcx>, - ctor: &Constructor<'tcx>, - ctor_wild_subpatterns: &Fields<'p, 'tcx>, - is_its_own_ctor: bool, // Whether `ctor` is known to be derived from `pat` -) -> Option> { - if pat.is_wildcard() { - return Some(ctor_wild_subpatterns.clone()); - } - - // We return `None` if `ctor` is not covered by `pat`. If `ctor` is known to be derived from - // `pat` then we don't need to check; otherwise, we compute the constructor of `pat` and check - // for constructor inclusion. - // Note that this shortcut is also necessary for correctness: a pattern should always be - // specializable with its own constructor, even in cases where we refuse to inspect values like - // opaque constants. - if !is_its_own_ctor { - // `unwrap` is safe because `pat` is not a wildcard. - let pat_ctor = pat_constructor(cx.tcx, cx.param_env, pat).unwrap(); - if !ctor.is_covered_by(cx, &pat_ctor, pat.ty) { - return None; - } - } - - let fields = ctor_wild_subpatterns.replace_with_pattern_arguments(pat); - - debug!("specialize({:#?}, {:#?}, {:#?}) = {:#?}", pat, ctor, ctor_wild_subpatterns, fields); - - Some(fields) -} From 833089fbc9d800813686d4d9b228c0dfe2aabd7b Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Thu, 22 Oct 2020 12:02:17 +0100 Subject: [PATCH 06/16] Unify the two kinds of specialization by adding a Wildcard ctor --- .../src/thir/pattern/_match.rs | 177 ++++++++---------- 1 file changed, 80 insertions(+), 97 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index d3602dac9e854..c2b0d8f52e34f 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -137,8 +137,8 @@ //! S(c, (r_1, p_2, .., p_n)) //! S(c, (r_2, p_2, .., p_n)) //! -//! 2. We can pop a wildcard off the top of the stack. This is called `D(p)`, where `p` is -//! a pattern-stack. +//! 2. We can pop a wildcard off the top of the stack. This is called `S(_, p)`, where `p` is +//! a pattern-stack. Note: the paper calls this `D(p)`. //! This is used when we know there are missing constructor cases, but there might be //! existing wildcard patterns, so to check the usefulness of the matrix, we have to check //! all its *other* components. @@ -150,8 +150,8 @@ //! p_2, .., p_n //! 2.3. `p_1 = r_1 | r_2`. We expand the OR-pattern and then recurse on each resulting //! stack. -//! D((r_1, p_2, .., p_n)) -//! D((r_2, p_2, .., p_n)) +//! S(_, (r_1, p_2, .., p_n)) +//! S(_, (r_2, p_2, .., p_n)) //! //! Note that the OR-patterns are not always used directly in Rust, but are used to derive the //! exhaustive integer matching rules, so they're written here for posterity. @@ -205,7 +205,7 @@ //! That's almost correct, but only works if there were no wildcards in those first //! components. So we need to check that `p` is useful with respect to the rows that //! start with a wildcard, if there are any. This is where `D` comes in: -//! `U(P, p) := U(D(P), D(p))` +//! `U(P, p) := U(S(_, P), S(_, p))` //! //! For example, if `P` is: //! @@ -358,10 +358,6 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { PatStack(vec) } - fn from_slice(s: &[&'p Pat<'tcx>]) -> Self { - PatStack(SmallVec::from_slice(s)) - } - fn is_empty(&self) -> bool { self.0.is_empty() } @@ -374,10 +370,6 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { self.0[0] } - fn to_tail(&self) -> Self { - PatStack::from_slice(&self.0[1..]) - } - fn iter(&self) -> impl Iterator> { self.0.iter().copied() } @@ -401,11 +393,6 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { } } - /// This computes `D(self)`. See top of the file for explanations. - fn specialize_wildcard(&self) -> Option { - if self.head().is_wildcard() { Some(self.to_tail()) } else { None } - } - /// This computes `S(constructor, self)`. See top of the file for explanations. /// /// This is the main specialization step. It expands the pattern @@ -427,15 +414,13 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { is_my_head_ctor: bool, ) -> Option> { // We return `None` if `ctor` is not covered by `self.head()`. If `ctor` is known to be - // derived from `self.head()`, or if `self.head()` is a wildcard, then we don't need to - // check; otherwise, we compute the constructor of `self.head()` and check for constructor - // inclusion. + // derived from `self.head()`, then we don't need to check; otherwise, we compute the + // constructor of `self.head()` and check for constructor inclusion. // Note that this shortcut is also necessary for correctness: a pattern should always be // specializable with its own constructor, even in cases where we refuse to inspect values like // opaque constants. - if !self.head().is_wildcard() && !is_my_head_ctor { - // `unwrap` is safe because `pat` is not a wildcard. - let head_ctor = pat_constructor(cx.tcx, cx.param_env, self.head()).unwrap(); + if !is_my_head_ctor { + let head_ctor = pat_constructor(cx.tcx, cx.param_env, self.head()); if !ctor.is_covered_by(cx, &head_ctor, self.head().ty) { return None; } @@ -480,8 +465,8 @@ enum SpecializationCache { /// so it is possible to precompute the result of `Matrix::specialize_constructor` at a /// lower computational complexity. /// `lookup` is responsible for holding the precomputed result of - /// `Matrix::specialize_constructor`, while `wilds` is used for two purposes: the first one is - /// the precomputed result of `Matrix::specialize_wildcard`, and the second is to be used as a + /// specialization, while `wilds` is used for two purposes: the first one is + /// the precomputed result of specialization with a wildcard, and the second is to be used as a /// fallback for `Matrix::specialize_constructor` when it tries to apply a constructor that /// has not been seen in the `Matrix`. See `update_cache` for further explanations. Variants { lookup: FxHashMap>, wilds: SmallVec<[usize; 1]> }, @@ -553,9 +538,9 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { v.push(idx); } // Per rule 2.1 and 2.2 in the top-level comments, only wildcard patterns - // are included in the result of `specialize_wildcard`. + // are included in the result of specialization with a wildcard. // What we do here is to track the wildcards we have seen; so in addition to - // acting as the precomputed result of `specialize_wildcard`, `wilds` also + // acting as the precomputed result of specialization with a wildcard, `wilds` also // serves as the default value of `specialize_constructor` for constructors // that are not in `lookup`. wilds.push(idx); @@ -585,30 +570,6 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { self.patterns.iter().map(|r| r.head()) } - /// This computes `D(self)`. See top of the file for explanations. - fn specialize_wildcard(&self) -> Self { - match &self.cache { - SpecializationCache::Variants { wilds, .. } => { - let result = - wilds.iter().filter_map(|&i| self.patterns[i].specialize_wildcard()).collect(); - // When debug assertions are enabled, check the results against the "slow path" - // result. - debug_assert_eq!( - result, - Self { - patterns: self.patterns.clone(), - cache: SpecializationCache::Incompatible - } - .specialize_wildcard() - ); - result - } - SpecializationCache::Incompatible => { - self.patterns.iter().filter_map(|r| r.specialize_wildcard()).collect() - } - } - } - /// This computes `S(constructor, self)`. See top of the file for explanations. fn specialize_constructor( &self, @@ -618,24 +579,30 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { ) -> Matrix<'p, 'tcx> { match &self.cache { SpecializationCache::Variants { lookup, wilds } => { - let result: Self = if let Constructor::Variant(id) = constructor { + let cached = if let Constructor::Variant(id) = constructor { lookup .get(id) // Default to `wilds` for absent keys. See `update_cache` for an explanation. .unwrap_or(&wilds) - .iter() - .filter_map(|&i| { - self.patterns[i].specialize_constructor( - cx, - constructor, - ctor_wild_subpatterns, - false, - ) - }) - .collect() + } else if let Wildcard = constructor { + &wilds } else { - unreachable!() + bug!( + "unexpected constructor encountered while dealing with matrix cache: {:?}", + constructor + ); }; + let result: Self = cached + .iter() + .filter_map(|&i| { + self.patterns[i].specialize_constructor( + cx, + constructor, + ctor_wild_subpatterns, + false, + ) + }) + .collect(); // When debug assertions are enabled, check the results against the "slow path" // result. debug_assert_eq!( @@ -939,8 +906,10 @@ impl Slice { _ => return smallvec![Slice(self)], }; - let head_ctors = - matrix.heads().filter_map(|pat| pat_constructor(cx.tcx, cx.param_env, pat)); + let head_ctors = matrix + .heads() + .map(|p| pat_constructor(cx.tcx, cx.param_env, p)) + .filter(|c| !c.is_wildcard()); let mut max_prefix_len = self_prefix; let mut max_suffix_len = self_suffix; @@ -1026,9 +995,18 @@ enum Constructor<'tcx> { Opaque, /// Fake extra constructor for enums that aren't allowed to be matched exhaustively. NonExhaustive, + /// Wildcard pattern. + Wildcard, } impl<'tcx> Constructor<'tcx> { + fn is_wildcard(&self) -> bool { + match self { + Wildcard => true, + _ => false, + } + } + fn variant_index_for_adt(&self, adt: &'tcx ty::AdtDef) -> VariantIdx { match *self { Variant(id) => adt.variant_index_with_id(id), @@ -1120,7 +1098,8 @@ impl<'tcx> Constructor<'tcx> { } // This constructor is never covered by anything else NonExhaustive => vec![NonExhaustive], - Opaque => bug!("unexpected opaque ctor {:?} found in all_ctors", self), + Opaque => bug!("found unexpected opaque ctor in all_ctors"), + Wildcard => bug!("found unexpected wildcard ctor in all_ctors"), } } @@ -1173,6 +1152,11 @@ impl<'tcx> Constructor<'tcx> { ty: Ty<'tcx>, ) -> bool { match (self, other) { + // Wildcards cover anything + (_, Wildcard) => true, + // Wildcards are only covered by wildcards + (Wildcard, _) => false, + (Single, Single) => true, (Variant(self_id), Variant(other_id)) => self_id == other_id, @@ -1302,7 +1286,8 @@ impl<'tcx> Constructor<'tcx> { &FloatRange(lo, hi, end) => PatKind::Range(PatRange { lo, hi, end }), IntRange(range) => return range.to_pat(cx.tcx), NonExhaustive => PatKind::Wild, - Opaque => bug!("we should not try to apply an opaque constructor {:?}", self), + Opaque => bug!("we should not try to apply an opaque constructor"), + Wildcard => bug!("we should not try to apply a wildcard constructor"), }; Pat { ty, span: DUMMY_SP, kind: Box::new(pat) } @@ -1454,7 +1439,9 @@ impl<'p, 'tcx> Fields<'p, 'tcx> { } _ => bug!("bad slice pattern {:?} {:?}", constructor, ty), }, - Str(..) | FloatRange(..) | IntRange(..) | NonExhaustive | Opaque => Fields::empty(), + Str(..) | FloatRange(..) | IntRange(..) | NonExhaustive | Opaque | Wildcard => { + Fields::empty() + } }; debug!("Fields::wildcards({:?}, {:?}) = {:#?}", constructor, ty, ret); ret @@ -2011,19 +1998,7 @@ impl<'tcx> IntRange<'tcx> { ) -> Option> { // This MUST be kept in sync with `pat_constructor`. match *pat.kind { - PatKind::AscribeUserType { .. } => bug!(), // Handled by `expand_pattern` - PatKind::Or { .. } => bug!("Or-pattern should have been expanded earlier on."), - - PatKind::Binding { .. } - | PatKind::Wild - | PatKind::Leaf { .. } - | PatKind::Deref { .. } - | PatKind::Variant { .. } - | PatKind::Array { .. } - | PatKind::Slice { .. } => None, - PatKind::Constant { value } => Self::from_const(tcx, param_env, value, pat.span), - PatKind::Range(PatRange { lo, hi, end }) => { let ty = lo.ty; Self::from_range( @@ -2035,6 +2010,7 @@ impl<'tcx> IntRange<'tcx> { pat.span, ) } + _ => None, } } @@ -2436,7 +2412,8 @@ crate fn is_useful<'p, 'tcx>( debug!("is_useful_expand_first_col: pcx={:#?}, expanding {:#?}", pcx, v.head()); - let ret = if let Some(constructor) = pat_constructor(cx.tcx, cx.param_env, v.head()) { + let constructor = pat_constructor(cx.tcx, cx.param_env, v.head()); + let ret = if !constructor.is_wildcard() { debug!("is_useful - expanding constructor: {:#?}", constructor); constructor .split(cx, pcx, matrix, Some(hir_id)) @@ -2458,8 +2435,11 @@ crate fn is_useful<'p, 'tcx>( } else { debug!("is_useful - expanding wildcard"); - let used_ctors: Vec> = - matrix.heads().filter_map(|p| pat_constructor(cx.tcx, cx.param_env, p)).collect(); + let used_ctors: Vec> = matrix + .heads() + .map(|p| pat_constructor(cx.tcx, cx.param_env, p)) + .filter(|c| !c.is_wildcard()) + .collect(); debug!("is_useful_used_ctors = {:#?}", used_ctors); // `all_ctors` are all the constructors for the given type, which // should all be represented (or caught with the wild pattern `_`). @@ -2501,8 +2481,11 @@ crate fn is_useful<'p, 'tcx>( .find(|result| result.is_useful()) .unwrap_or(NotUseful) } else { - let matrix = matrix.specialize_wildcard(); - let v = v.to_tail(); + let ctor_wild_subpatterns = Fields::empty(); + let matrix = matrix.specialize_constructor(cx, &constructor, &ctor_wild_subpatterns); + // Unwrap is ok: v can always be specialized with its own constructor. + let v = + v.specialize_constructor(cx, &constructor, &ctor_wild_subpatterns, true).unwrap(); let usefulness = is_useful(cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false); @@ -2584,26 +2567,26 @@ fn pat_constructor<'tcx>( tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>, pat: &Pat<'tcx>, -) -> Option> { +) -> Constructor<'tcx> { // This MUST be kept in sync with `IntRange::from_pat`. match *pat.kind { PatKind::AscribeUserType { .. } => bug!(), // Handled by `expand_pattern` - PatKind::Binding { .. } | PatKind::Wild => None, - PatKind::Leaf { .. } | PatKind::Deref { .. } => Some(Single), + PatKind::Binding { .. } | PatKind::Wild => Wildcard, + PatKind::Leaf { .. } | PatKind::Deref { .. } => Single, PatKind::Variant { adt_def, variant_index, .. } => { - Some(Variant(adt_def.variants[variant_index].def_id)) + Variant(adt_def.variants[variant_index].def_id) } PatKind::Constant { value } => { if let Some(int_range) = IntRange::from_const(tcx, param_env, value, pat.span) { - Some(IntRange(int_range)) + IntRange(int_range) } else { match value.ty.kind() { - ty::Float(_) => Some(FloatRange(value, value, RangeEnd::Included)), - ty::Ref(_, t, _) if t.is_str() => Some(Str(value)), + ty::Float(_) => FloatRange(value, value, RangeEnd::Included), + ty::Ref(_, t, _) if t.is_str() => Str(value), // All constants that can be structurally matched have already been expanded // into the corresponding `Pat`s by `const_to_pat`. Constants that remain are // opaque. - _ => Some(Opaque), + _ => Opaque, } } } @@ -2617,9 +2600,9 @@ fn pat_constructor<'tcx>( &end, pat.span, ) { - Some(IntRange(int_range)) + IntRange(int_range) } else { - Some(FloatRange(lo, hi, end)) + FloatRange(lo, hi, end) } } PatKind::Array { ref prefix, ref slice, ref suffix } @@ -2633,7 +2616,7 @@ fn pat_constructor<'tcx>( let suffix = suffix.len() as u64; let kind = if slice.is_some() { VarLen(prefix, suffix) } else { FixedLen(prefix + suffix) }; - Some(Slice(Slice { array_len, kind })) + Slice(Slice { array_len, kind }) } PatKind::Or { .. } => bug!("Or-pattern should have been expanded earlier on."), } From 1190e7275cd1eadc0ef17b564fc84551973b6d85 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Fri, 23 Oct 2020 22:49:26 +0100 Subject: [PATCH 07/16] Cache head constructor in PatStack Since the constructor is recomputed a lot, caching is worth it. --- compiler/rustc_mir_build/src/lib.rs | 1 + .../src/thir/pattern/_match.rs | 130 +++++++++--------- 2 files changed, 63 insertions(+), 68 deletions(-) diff --git a/compiler/rustc_mir_build/src/lib.rs b/compiler/rustc_mir_build/src/lib.rs index 714041ad4e874..0866892265bd9 100644 --- a/compiler/rustc_mir_build/src/lib.rs +++ b/compiler/rustc_mir_build/src/lib.rs @@ -9,6 +9,7 @@ #![feature(control_flow_enum)] #![feature(crate_visibility_modifier)] #![feature(bool_to_option)] +#![feature(once_cell)] #![feature(or_patterns)] #![recursion_limit = "256"] diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index c2b0d8f52e34f..30529ef5e1bb8 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -295,6 +295,7 @@ use self::WitnessPreference::*; use rustc_data_structures::captures::Captures; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_data_structures::sync::OnceCell; use rustc_index::vec::Idx; use super::{compare_const_vals, PatternFoldable, PatternFolder}; @@ -346,32 +347,40 @@ impl<'tcx> Pat<'tcx> { /// A row of a matrix. Rows of len 1 are very common, which is why `SmallVec[_; 2]` /// works well. -#[derive(Debug, Clone, PartialEq)] -crate struct PatStack<'p, 'tcx>(SmallVec<[&'p Pat<'tcx>; 2]>); +#[derive(Debug, Clone)] +crate struct PatStack<'p, 'tcx> { + pats: SmallVec<[&'p Pat<'tcx>; 2]>, + /// Cache for the constructor of the head + head_ctor: OnceCell>, +} impl<'p, 'tcx> PatStack<'p, 'tcx> { crate fn from_pattern(pat: &'p Pat<'tcx>) -> Self { - PatStack(smallvec![pat]) + Self::from_vec(smallvec![pat]) } fn from_vec(vec: SmallVec<[&'p Pat<'tcx>; 2]>) -> Self { - PatStack(vec) + PatStack { pats: vec, head_ctor: OnceCell::new() } } fn is_empty(&self) -> bool { - self.0.is_empty() + self.pats.is_empty() } fn len(&self) -> usize { - self.0.len() + self.pats.len() } fn head(&self) -> &'p Pat<'tcx> { - self.0[0] + self.pats[0] + } + + fn head_ctor<'a>(&'a self, cx: &MatchCheckCtxt<'p, 'tcx>) -> &'a Constructor<'tcx> { + self.head_ctor.get_or_init(|| pat_constructor(cx, self.head())) } fn iter(&self) -> impl Iterator> { - self.0.iter().copied() + self.pats.iter().copied() } // If the first pattern is an or-pattern, expand this pattern. Otherwise, return `None`. @@ -383,7 +392,7 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { pats.iter() .map(|pat| { let mut new_patstack = PatStack::from_pattern(pat); - new_patstack.0.extend_from_slice(&self.0[1..]); + new_patstack.pats.extend_from_slice(&self.pats[1..]); new_patstack }) .collect(), @@ -414,16 +423,13 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { is_my_head_ctor: bool, ) -> Option> { // We return `None` if `ctor` is not covered by `self.head()`. If `ctor` is known to be - // derived from `self.head()`, then we don't need to check; otherwise, we compute the - // constructor of `self.head()` and check for constructor inclusion. + // derived from `self.head()`, then we don't need to check; otherwise, we check for + // constructor inclusion. // Note that this shortcut is also necessary for correctness: a pattern should always be // specializable with its own constructor, even in cases where we refuse to inspect values like // opaque constants. - if !is_my_head_ctor { - let head_ctor = pat_constructor(cx.tcx, cx.param_env, self.head()); - if !ctor.is_covered_by(cx, &head_ctor, self.head().ty) { - return None; - } + if !is_my_head_ctor && !ctor.is_covered_by(cx, self.head_ctor(cx), self.head().ty) { + return None; } let new_fields = ctor_wild_subpatterns.replace_with_pattern_arguments(self.head()); @@ -437,13 +443,19 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { // We pop the head pattern and push the new fields extracted from the arguments of // `self.head()`. - Some(new_fields.push_on_patstack(&self.0[1..])) + Some(new_fields.push_on_patstack(&self.pats[1..])) } } impl<'p, 'tcx> Default for PatStack<'p, 'tcx> { fn default() -> Self { - PatStack(smallvec![]) + Self::from_vec(smallvec![]) + } +} + +impl<'p, 'tcx> PartialEq for PatStack<'p, 'tcx> { + fn eq(&self, other: &Self) -> bool { + self.pats == other.pats } } @@ -452,7 +464,7 @@ impl<'p, 'tcx> FromIterator<&'p Pat<'tcx>> for PatStack<'p, 'tcx> { where T: IntoIterator>, { - PatStack(iter.into_iter().collect()) + Self::from_vec(iter.into_iter().collect()) } } @@ -570,6 +582,14 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { self.patterns.iter().map(|r| r.head()) } + /// Iterate over the first constructor of each row + fn head_ctors<'a>( + &'a self, + cx: &'a MatchCheckCtxt<'p, 'tcx>, + ) -> impl Iterator> + Captures<'a> + Captures<'p> { + self.patterns.iter().map(move |r| r.head_ctor(cx)) + } + /// This computes `S(constructor, self)`. See top of the file for explanations. fn specialize_constructor( &self, @@ -906,10 +926,7 @@ impl Slice { _ => return smallvec![Slice(self)], }; - let head_ctors = matrix - .heads() - .map(|p| pat_constructor(cx.tcx, cx.param_env, p)) - .filter(|c| !c.is_wildcard()); + let head_ctors = matrix.head_ctors(cx).filter(|c| !c.is_wildcard()); let mut max_prefix_len = self_prefix; let mut max_suffix_len = self_suffix; @@ -1120,7 +1137,7 @@ impl<'tcx> Constructor<'tcx> { /// `hir_id` is `None` when we're evaluating the wildcard pattern. In that case we do not want /// to lint for overlapping ranges. fn split<'p>( - self, + &self, cx: &MatchCheckCtxt<'p, 'tcx>, pcx: PatCtxt<'tcx>, matrix: &Matrix<'p, 'tcx>, @@ -1138,7 +1155,7 @@ impl<'tcx> Constructor<'tcx> { } Slice(slice @ Slice { kind: VarLen(..), .. }) => slice.split(cx, matrix), // Any other constructor can be used unchanged. - _ => smallvec![self], + _ => smallvec![self.clone()], } } @@ -1991,25 +2008,9 @@ impl<'tcx> IntRange<'tcx> { } } - fn from_pat( - tcx: TyCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, - pat: &Pat<'tcx>, - ) -> Option> { - // This MUST be kept in sync with `pat_constructor`. - match *pat.kind { - PatKind::Constant { value } => Self::from_const(tcx, param_env, value, pat.span), - PatKind::Range(PatRange { lo, hi, end }) => { - let ty = lo.ty; - Self::from_range( - tcx, - lo.eval_bits(tcx, param_env, lo.ty), - hi.eval_bits(tcx, param_env, hi.ty), - ty, - &end, - pat.span, - ) - } + fn from_ctor<'a>(ctor: &'a Constructor<'tcx>) -> Option<&'a IntRange<'tcx>> { + match ctor { + IntRange(range) => Some(range), _ => None, } } @@ -2145,7 +2146,7 @@ impl<'tcx> IntRange<'tcx> { /// between every pair of boundary points. (This essentially sums up to performing the intuitive /// merging operation depicted above.) fn split<'p>( - self, + &self, cx: &MatchCheckCtxt<'p, 'tcx>, pcx: PatCtxt<'tcx>, matrix: &Matrix<'p, 'tcx>, @@ -2176,15 +2177,13 @@ impl<'tcx> IntRange<'tcx> { // Collect the span and range of all the intersecting ranges to lint on likely // incorrect range patterns. (#63987) let mut overlaps = vec![]; + let row_len = matrix.patterns.get(0).map(|r| r.len()).unwrap_or(0); // `borders` is the set of borders between equivalence classes: each equivalence // class lies between 2 borders. let row_borders = matrix - .patterns - .iter() - .flat_map(|row| { - IntRange::from_pat(cx.tcx, cx.param_env, row.head()).map(|r| (r, row.len())) - }) - .flat_map(|(range, row_len)| { + .head_ctors(cx) + .filter_map(|ctor| IntRange::from_ctor(ctor)) + .filter_map(|range| { let intersection = self.intersection(cx.tcx, &range); let should_lint = self.suspicious_intersection(&range); if let (Some(range), 1, true) = (&intersection, row_len, should_lint) { @@ -2229,7 +2228,7 @@ impl<'tcx> IntRange<'tcx> { } fn lint_overlapping_patterns( - self, + &self, tcx: TyCtxt<'tcx>, hir_id: Option, ty: Ty<'tcx>, @@ -2412,7 +2411,7 @@ crate fn is_useful<'p, 'tcx>( debug!("is_useful_expand_first_col: pcx={:#?}, expanding {:#?}", pcx, v.head()); - let constructor = pat_constructor(cx.tcx, cx.param_env, v.head()); + let constructor = v.head_ctor(cx); let ret = if !constructor.is_wildcard() { debug!("is_useful - expanding constructor: {:#?}", constructor); constructor @@ -2435,11 +2434,8 @@ crate fn is_useful<'p, 'tcx>( } else { debug!("is_useful - expanding wildcard"); - let used_ctors: Vec> = matrix - .heads() - .map(|p| pat_constructor(cx.tcx, cx.param_env, p)) - .filter(|c| !c.is_wildcard()) - .collect(); + let used_ctors: Vec> = + matrix.head_ctors(cx).cloned().filter(|c| !c.is_wildcard()).collect(); debug!("is_useful_used_ctors = {:#?}", used_ctors); // `all_ctors` are all the constructors for the given type, which // should all be represented (or caught with the wild pattern `_`). @@ -2563,12 +2559,10 @@ fn is_useful_specialized<'p, 'tcx>( /// Determines the constructor that the given pattern can be specialized to. /// Returns `None` in case of a catch-all, which can't be specialized. -fn pat_constructor<'tcx>( - tcx: TyCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, - pat: &Pat<'tcx>, +fn pat_constructor<'p, 'tcx>( + cx: &MatchCheckCtxt<'p, 'tcx>, + pat: &'p Pat<'tcx>, ) -> Constructor<'tcx> { - // This MUST be kept in sync with `IntRange::from_pat`. match *pat.kind { PatKind::AscribeUserType { .. } => bug!(), // Handled by `expand_pattern` PatKind::Binding { .. } | PatKind::Wild => Wildcard, @@ -2577,7 +2571,7 @@ fn pat_constructor<'tcx>( Variant(adt_def.variants[variant_index].def_id) } PatKind::Constant { value } => { - if let Some(int_range) = IntRange::from_const(tcx, param_env, value, pat.span) { + if let Some(int_range) = IntRange::from_const(cx.tcx, cx.param_env, value, pat.span) { IntRange(int_range) } else { match value.ty.kind() { @@ -2593,9 +2587,9 @@ fn pat_constructor<'tcx>( PatKind::Range(PatRange { lo, hi, end }) => { let ty = lo.ty; if let Some(int_range) = IntRange::from_range( - tcx, - lo.eval_bits(tcx, param_env, lo.ty), - hi.eval_bits(tcx, param_env, hi.ty), + cx.tcx, + lo.eval_bits(cx.tcx, cx.param_env, lo.ty), + hi.eval_bits(cx.tcx, cx.param_env, hi.ty), ty, &end, pat.span, @@ -2608,7 +2602,7 @@ fn pat_constructor<'tcx>( PatKind::Array { ref prefix, ref slice, ref suffix } | PatKind::Slice { ref prefix, ref slice, ref suffix } => { let array_len = match pat.ty.kind() { - ty::Array(_, length) => Some(length.eval_usize(tcx, param_env)), + ty::Array(_, length) => Some(length.eval_usize(cx.tcx, cx.param_env)), ty::Slice(_) => None, _ => span_bug!(pat.span, "bad ty {:?} for slice pattern", pat.ty), }; From cdafd1e1bda60f19c41540302da9543069cc1f66 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 26 Oct 2020 17:35:59 +0000 Subject: [PATCH 08/16] Let MissingConstructors handle the subtleties of missing constructors --- .../src/thir/pattern/_match.rs | 161 ++++++++---------- 1 file changed, 72 insertions(+), 89 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 30529ef5e1bb8..e61578afd7815 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -1309,11 +1309,6 @@ impl<'tcx> Constructor<'tcx> { Pat { ty, span: DUMMY_SP, kind: Box::new(pat) } } - - /// Like `apply`, but where all the subpatterns are wildcards `_`. - fn apply_wildcards<'a>(&self, cx: &MatchCheckCtxt<'a, 'tcx>, ty: Ty<'tcx>) -> Pat<'tcx> { - self.apply(cx, ty, Fields::wildcards(cx, self, ty)) - } } /// Some fields need to be explicitly hidden away in certain cases; see the comment above the @@ -1649,35 +1644,15 @@ impl<'tcx> Usefulness<'tcx> { } } - fn apply_wildcard(self, ty: Ty<'tcx>) -> Self { - match self { - UsefulWithWitness(witnesses) => { - let wild = Pat::wildcard_from_ty(ty); - UsefulWithWitness( - witnesses - .into_iter() - .map(|mut witness| { - witness.0.push(wild.clone()); - witness - }) - .collect(), - ) - } - x => x, - } - } - - fn apply_missing_ctors( + fn apply_wildcard<'p>( self, - cx: &MatchCheckCtxt<'_, 'tcx>, - ty: Ty<'tcx>, - missing_ctors: &MissingConstructors<'tcx>, + cx: &MatchCheckCtxt<'p, 'tcx>, + pcx: PatCtxt<'tcx>, + missing_ctors: MissingConstructors<'tcx>, ) -> Self { match self { UsefulWithWitness(witnesses) => { - let new_patterns: Vec<_> = - missing_ctors.iter().map(|ctor| ctor.apply_wildcards(cx, ty)).collect(); - // Add the new patterns to each witness + let new_patterns = missing_ctors.report_patterns(cx, pcx); UsefulWithWitness( witnesses .into_iter() @@ -2270,11 +2245,21 @@ impl<'tcx> std::cmp::PartialEq for IntRange<'tcx> { struct MissingConstructors<'tcx> { all_ctors: Vec>, used_ctors: Vec>, + is_top_level: bool, } impl<'tcx> MissingConstructors<'tcx> { - fn new(all_ctors: Vec>, used_ctors: Vec>) -> Self { - MissingConstructors { all_ctors, used_ctors } + fn new<'p>( + cx: &MatchCheckCtxt<'p, 'tcx>, + pcx: PatCtxt<'tcx>, + matrix: &Matrix<'p, 'tcx>, + is_top_level: bool, + ) -> Self { + let used_ctors: Vec> = + matrix.head_ctors(cx).cloned().filter(|c| !c.is_wildcard()).collect(); + let all_ctors = all_constructors(cx, pcx); + + MissingConstructors { all_ctors, used_ctors, is_top_level } } fn into_inner(self) -> (Vec>, Vec>) { @@ -2284,16 +2269,64 @@ impl<'tcx> MissingConstructors<'tcx> { fn is_empty(&self) -> bool { self.iter().next().is_none() } - /// Whether this contains all the constructors for the given type or only a - /// subset. - fn all_ctors_are_missing(&self) -> bool { - self.used_ctors.is_empty() - } /// Iterate over all_ctors \ used_ctors fn iter<'a>(&'a self) -> impl Iterator> + Captures<'a> { self.all_ctors.iter().flat_map(move |req_ctor| req_ctor.subtract_ctors(&self.used_ctors)) } + + /// List the patterns corresponding to the missing constructors. In some cases, instead of + /// listing all constructors of a given type, we prefer to simply report a wildcard. + fn report_patterns<'p>( + &self, + cx: &MatchCheckCtxt<'p, 'tcx>, + pcx: PatCtxt<'tcx>, + ) -> SmallVec<[Pat<'tcx>; 1]> { + // There are 2 ways we can report a witness here. + // Commonly, we can report all the "free" + // constructors as witnesses, e.g., if we have: + // + // ``` + // enum Direction { N, S, E, W } + // let Direction::N = ...; + // ``` + // + // we can report 3 witnesses: `S`, `E`, and `W`. + // + // However, there is a case where we don't want + // to do this and instead report a single `_` witness: + // if the user didn't actually specify a constructor + // in this arm, e.g., in + // + // ``` + // let x: (Direction, Direction, bool) = ...; + // let (_, _, false) = x; + // ``` + // + // we don't want to show all 16 possible witnesses + // `(, , true)` - we are + // satisfied with `(_, _, true)`. In this case, + // `used_ctors` is empty. + // The exception is: if we are at the top-level, for example in an empty match, we + // sometimes prefer reporting the list of constructors instead of just `_`. + let report_when_all_missing = self.is_top_level && !IntRange::is_integral(pcx.ty); + if self.used_ctors.is_empty() && !report_when_all_missing { + // All constructors are unused. Report only a wildcard + // rather than each individual constructor. + smallvec![Pat::wildcard_from_ty(pcx.ty)] + } else { + // Construct for each missing constructor a "wild" version of this + // constructor, that matches everything that can be built with + // it. For example, if `ctor` is a `Constructor::Variant` for + // `Option::Some`, we get the pattern `Some(_)`. + self.iter() + .map(|missing_ctor| { + let fields = Fields::wildcards(cx, &missing_ctor, pcx.ty); + missing_ctor.apply(cx, pcx.ty, fields) + }) + .collect() + } + } } impl<'tcx> fmt::Debug for MissingConstructors<'tcx> { @@ -2434,14 +2467,6 @@ crate fn is_useful<'p, 'tcx>( } else { debug!("is_useful - expanding wildcard"); - let used_ctors: Vec> = - matrix.head_ctors(cx).cloned().filter(|c| !c.is_wildcard()).collect(); - debug!("is_useful_used_ctors = {:#?}", used_ctors); - // `all_ctors` are all the constructors for the given type, which - // should all be represented (or caught with the wild pattern `_`). - let all_ctors = all_constructors(cx, pcx); - debug!("is_useful_all_ctors = {:#?}", all_ctors); - // `missing_ctors` is the set of constructors from the same type as the // first column of `matrix` that are matched only by wildcard patterns // from the first column. @@ -2453,7 +2478,7 @@ crate fn is_useful<'p, 'tcx>( // Missing constructors are those that are not matched by any non-wildcard patterns in the // current column. We only fully construct them on-demand, because they're rarely used and // can be big. - let missing_ctors = MissingConstructors::new(all_ctors, used_ctors); + let missing_ctors = MissingConstructors::new(cx, pcx, matrix, is_top_level); debug!("is_useful_missing_ctors.empty()={:#?}", missing_ctors.is_empty(),); @@ -2485,49 +2510,7 @@ crate fn is_useful<'p, 'tcx>( let usefulness = is_useful(cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false); - // In this case, there's at least one "free" - // constructor that is only matched against by - // wildcard patterns. - // - // There are 2 ways we can report a witness here. - // Commonly, we can report all the "free" - // constructors as witnesses, e.g., if we have: - // - // ``` - // enum Direction { N, S, E, W } - // let Direction::N = ...; - // ``` - // - // we can report 3 witnesses: `S`, `E`, and `W`. - // - // However, there is a case where we don't want - // to do this and instead report a single `_` witness: - // if the user didn't actually specify a constructor - // in this arm, e.g., in - // - // ``` - // let x: (Direction, Direction, bool) = ...; - // let (_, _, false) = x; - // ``` - // - // we don't want to show all 16 possible witnesses - // `(, , true)` - we are - // satisfied with `(_, _, true)`. In this case, - // `used_ctors` is empty. - // The exception is: if we are at the top-level, for example in an empty match, we - // sometimes prefer reporting the list of constructors instead of just `_`. - let report_ctors_rather_than_wildcard = is_top_level && !IntRange::is_integral(pcx.ty); - if missing_ctors.all_ctors_are_missing() && !report_ctors_rather_than_wildcard { - // All constructors are unused. Add a wild pattern - // rather than each individual constructor. - usefulness.apply_wildcard(pcx.ty) - } else { - // Construct for each missing constructor a "wild" version of this - // constructor, that matches everything that can be built with - // it. For example, if `ctor` is a `Constructor::Variant` for - // `Option::Some`, we get the pattern `Some(_)`. - usefulness.apply_missing_ctors(cx, pcx.ty, &missing_ctors) - } + usefulness.apply_wildcard(cx, pcx, missing_ctors) } }; debug!("is_useful::returns({:#?}, {:#?}) = {:?}", matrix, v, ret); From b49f90760d0c464e6e7c3da06dc6a1eb122552ec Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 26 Oct 2020 18:13:30 +0000 Subject: [PATCH 09/16] Pass more things through `PatCtxt` This is even a perf improvement on the match-heavy benchmarks. --- .../src/thir/pattern/_match.rs | 203 +++++++----------- 1 file changed, 75 insertions(+), 128 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index e61578afd7815..850ce1847dbaa 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -417,7 +417,7 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { /// This is roughly the inverse of `Constructor::apply`. fn specialize_constructor( &self, - cx: &MatchCheckCtxt<'p, 'tcx>, + pcx: PatCtxt<'_, 'p, 'tcx>, ctor: &Constructor<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, is_my_head_ctor: bool, @@ -428,7 +428,7 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { // Note that this shortcut is also necessary for correctness: a pattern should always be // specializable with its own constructor, even in cases where we refuse to inspect values like // opaque constants. - if !is_my_head_ctor && !ctor.is_covered_by(cx, self.head_ctor(cx), self.head().ty) { + if !is_my_head_ctor && !ctor.is_covered_by(pcx, self.head_ctor(pcx.cx)) { return None; } let new_fields = ctor_wild_subpatterns.replace_with_pattern_arguments(self.head()); @@ -593,7 +593,7 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { /// This computes `S(constructor, self)`. See top of the file for explanations. fn specialize_constructor( &self, - cx: &MatchCheckCtxt<'p, 'tcx>, + pcx: PatCtxt<'_, 'p, 'tcx>, constructor: &Constructor<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, ) -> Matrix<'p, 'tcx> { @@ -616,7 +616,7 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { .iter() .filter_map(|&i| { self.patterns[i].specialize_constructor( - cx, + pcx, constructor, ctor_wild_subpatterns, false, @@ -632,7 +632,7 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { cache: SpecializationCache::Incompatible } .specialize_constructor( - cx, + pcx, constructor, ctor_wild_subpatterns ) @@ -643,7 +643,7 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { .patterns .iter() .filter_map(|r| { - r.specialize_constructor(cx, constructor, ctor_wild_subpatterns, false) + r.specialize_constructor(pcx, constructor, ctor_wild_subpatterns, false) }) .collect(), } @@ -914,11 +914,7 @@ impl Slice { /// but the first and last can be added/removed, so any /// witness of length ≥2 (say, `[false, false, true]`) can be /// turned to a witness from any other length ≥2. - fn split<'p, 'tcx>( - self, - cx: &MatchCheckCtxt<'p, 'tcx>, - matrix: &Matrix<'p, 'tcx>, - ) -> SmallVec<[Constructor<'tcx>; 1]> { + fn split<'p, 'tcx>(self, pcx: PatCtxt<'_, 'p, 'tcx>) -> SmallVec<[Constructor<'tcx>; 1]> { let (array_len, self_prefix, self_suffix) = match self { Slice { array_len, kind: VarLen(self_prefix, self_suffix) } => { (array_len, self_prefix, self_suffix) @@ -926,7 +922,7 @@ impl Slice { _ => return smallvec![Slice(self)], }; - let head_ctors = matrix.head_ctors(cx).filter(|c| !c.is_wildcard()); + let head_ctors = pcx.matrix.head_ctors(pcx.cx).filter(|c| !c.is_wildcard()); let mut max_prefix_len = self_prefix; let mut max_suffix_len = self_suffix; @@ -1136,24 +1132,18 @@ impl<'tcx> Constructor<'tcx> { /// /// `hir_id` is `None` when we're evaluating the wildcard pattern. In that case we do not want /// to lint for overlapping ranges. - fn split<'p>( - &self, - cx: &MatchCheckCtxt<'p, 'tcx>, - pcx: PatCtxt<'tcx>, - matrix: &Matrix<'p, 'tcx>, - hir_id: Option, - ) -> SmallVec<[Self; 1]> { - debug!("Constructor::split({:#?}, {:#?})", self, matrix); + fn split<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>, hir_id: Option) -> SmallVec<[Self; 1]> { + debug!("Constructor::split({:#?}, {:#?})", self, pcx.matrix); match self { // Fast-track if the range is trivial. In particular, we don't do the overlapping // ranges check. IntRange(ctor_range) - if ctor_range.treat_exhaustively(cx.tcx) && !ctor_range.is_singleton() => + if ctor_range.treat_exhaustively(pcx.cx.tcx) && !ctor_range.is_singleton() => { - ctor_range.split(cx, pcx, matrix, hir_id) + ctor_range.split(pcx, hir_id) } - Slice(slice @ Slice { kind: VarLen(..), .. }) => slice.split(cx, matrix), + Slice(slice @ Slice { kind: VarLen(..), .. }) => slice.split(pcx), // Any other constructor can be used unchanged. _ => smallvec![self.clone()], } @@ -1162,12 +1152,7 @@ impl<'tcx> Constructor<'tcx> { /// Returns whether `self` is covered by `other`, ie whether `self` is a subset of `other`. For /// the simple cases, this is simply checking for equality. For the "grouped" constructors, /// this checks for inclusion. - fn is_covered_by<'p>( - &self, - cx: &MatchCheckCtxt<'p, 'tcx>, - other: &Constructor<'tcx>, - ty: Ty<'tcx>, - ) -> bool { + fn is_covered_by<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>, other: &Constructor<'tcx>) -> bool { match (self, other) { // Wildcards cover anything (_, Wildcard) => true, @@ -1178,7 +1163,7 @@ impl<'tcx> Constructor<'tcx> { (Variant(self_id), Variant(other_id)) => self_id == other_id, (IntRange(self_range), IntRange(other_range)) => { - if self_range.intersection(cx.tcx, other_range).is_some() { + if self_range.intersection(pcx.cx.tcx, other_range).is_some() { // Constructor splitting should ensure that all intersections we encounter // are actually inclusions. assert!(self_range.is_subrange(other_range)); @@ -1192,8 +1177,8 @@ impl<'tcx> Constructor<'tcx> { FloatRange(other_from, other_to, other_end), ) => { match ( - compare_const_vals(cx.tcx, self_to, other_to, cx.param_env, ty), - compare_const_vals(cx.tcx, self_from, other_from, cx.param_env, ty), + compare_const_vals(pcx.cx.tcx, self_to, other_to, pcx.cx.param_env, pcx.ty), + compare_const_vals(pcx.cx.tcx, self_from, other_from, pcx.cx.param_env, pcx.ty), ) { (Some(to), Some(from)) => { (from == Ordering::Greater || from == Ordering::Equal) @@ -1205,7 +1190,8 @@ impl<'tcx> Constructor<'tcx> { } (Str(self_val), Str(other_val)) => { // FIXME: there's probably a more direct way of comparing for equality - match compare_const_vals(cx.tcx, self_val, other_val, cx.param_env, ty) { + match compare_const_vals(pcx.cx.tcx, self_val, other_val, pcx.cx.param_env, pcx.ty) + { Some(comparison) => comparison == Ordering::Equal, None => false, } @@ -1239,23 +1225,18 @@ impl<'tcx> Constructor<'tcx> { /// `ty`: `Option` /// `pats`: `[false]` /// returns `Some(false)` - fn apply<'p>( - &self, - cx: &MatchCheckCtxt<'p, 'tcx>, - ty: Ty<'tcx>, - fields: Fields<'p, 'tcx>, - ) -> Pat<'tcx> { + fn apply<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>, fields: Fields<'p, 'tcx>) -> Pat<'tcx> { let mut subpatterns = fields.all_patterns(); let pat = match self { - Single | Variant(_) => match ty.kind() { + Single | Variant(_) => match pcx.ty.kind() { ty::Adt(..) | ty::Tuple(..) => { let subpatterns = subpatterns .enumerate() .map(|(i, p)| FieldPat { field: Field::new(i), pattern: p }) .collect(); - if let ty::Adt(adt, substs) = ty.kind() { + if let ty::Adt(adt, substs) = pcx.ty.kind() { if adt.is_enum() { PatKind::Variant { adt_def: adt, @@ -1271,7 +1252,7 @@ impl<'tcx> Constructor<'tcx> { } } ty::Ref(..) => PatKind::Deref { subpattern: subpatterns.next().unwrap() }, - ty::Slice(_) | ty::Array(..) => bug!("bad slice pattern {:?} {:?}", self, ty), + ty::Slice(_) | ty::Array(..) => bug!("bad slice pattern {:?} {:?}", self, pcx.ty), _ => PatKind::Wild, }, Slice(slice) => match slice.pattern_kind() { @@ -1295,19 +1276,19 @@ impl<'tcx> Constructor<'tcx> { } else { subpatterns.collect() }; - let wild = Pat::wildcard_from_ty(ty); + let wild = Pat::wildcard_from_ty(pcx.ty); PatKind::Slice { prefix, slice: Some(wild), suffix } } }, &Str(value) => PatKind::Constant { value }, &FloatRange(lo, hi, end) => PatKind::Range(PatRange { lo, hi, end }), - IntRange(range) => return range.to_pat(cx.tcx), + IntRange(range) => return range.to_pat(pcx.cx.tcx), NonExhaustive => PatKind::Wild, Opaque => bug!("we should not try to apply an opaque constructor"), Wildcard => bug!("we should not try to apply a wildcard constructor"), }; - Pat { ty, span: DUMMY_SP, kind: Box::new(pat) } + Pat { ty: pcx.ty, span: DUMMY_SP, kind: Box::new(pat) } } } @@ -1385,11 +1366,9 @@ impl<'p, 'tcx> Fields<'p, 'tcx> { } /// Creates a new list of wildcard fields for a given constructor. - fn wildcards( - cx: &MatchCheckCtxt<'p, 'tcx>, - constructor: &Constructor<'tcx>, - ty: Ty<'tcx>, - ) -> Self { + fn wildcards(pcx: PatCtxt<'_, 'p, 'tcx>, constructor: &Constructor<'tcx>) -> Self { + let ty = pcx.ty; + let cx = pcx.cx; let wildcard_from_ty = |ty| &*cx.pattern_arena.alloc(Pat::wildcard_from_ty(ty)); let ret = match constructor { @@ -1628,16 +1607,15 @@ impl<'tcx> Usefulness<'tcx> { fn apply_constructor<'p>( self, - cx: &MatchCheckCtxt<'p, 'tcx>, + pcx: PatCtxt<'_, 'p, 'tcx>, ctor: &Constructor<'tcx>, - ty: Ty<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, ) -> Self { match self { UsefulWithWitness(witnesses) => UsefulWithWitness( witnesses .into_iter() - .map(|witness| witness.apply_constructor(cx, &ctor, ty, ctor_wild_subpatterns)) + .map(|witness| witness.apply_constructor(pcx, &ctor, ctor_wild_subpatterns)) .collect(), ), x => x, @@ -1646,13 +1624,12 @@ impl<'tcx> Usefulness<'tcx> { fn apply_wildcard<'p>( self, - cx: &MatchCheckCtxt<'p, 'tcx>, - pcx: PatCtxt<'tcx>, + pcx: PatCtxt<'_, 'p, 'tcx>, missing_ctors: MissingConstructors<'tcx>, ) -> Self { match self { UsefulWithWitness(witnesses) => { - let new_patterns = missing_ctors.report_patterns(cx, pcx); + let new_patterns = missing_ctors.report_patterns(pcx); UsefulWithWitness( witnesses .into_iter() @@ -1677,9 +1654,14 @@ crate enum WitnessPreference { LeaveOutWitness, } -#[derive(Copy, Clone, Debug)] -struct PatCtxt<'tcx> { +#[derive(Copy, Clone)] +struct PatCtxt<'a, 'p, 'tcx> { + cx: &'a MatchCheckCtxt<'p, 'tcx>, + /// Current state of the matrix. + matrix: &'a Matrix<'p, 'tcx>, + /// Type of the current column under investigation. ty: Ty<'tcx>, + /// Span of the current pattern under investigation. span: Span, } @@ -1740,17 +1722,16 @@ impl<'tcx> Witness<'tcx> { /// pats: [(false, "foo"), 42] => X { a: (false, "foo"), b: 42 } fn apply_constructor<'p>( mut self, - cx: &MatchCheckCtxt<'p, 'tcx>, + pcx: PatCtxt<'_, 'p, 'tcx>, ctor: &Constructor<'tcx>, - ty: Ty<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, ) -> Self { let pat = { let len = self.0.len(); let arity = ctor_wild_subpatterns.len(); let pats = self.0.drain((len - arity)..).rev(); - let fields = ctor_wild_subpatterns.replace_fields(cx, pats); - ctor.apply(cx, ty, fields) + let fields = ctor_wild_subpatterns.replace_fields(pcx.cx, pats); + ctor.apply(pcx, fields) }; self.0.push(pat); @@ -1768,11 +1749,9 @@ impl<'tcx> Witness<'tcx> { /// `Option`, we do not include `Some(_)` in the returned list of constructors. /// Invariant: this returns an empty `Vec` if and only if the type is uninhabited (as determined by /// `cx.is_uninhabited()`). -fn all_constructors<'a, 'tcx>( - cx: &MatchCheckCtxt<'a, 'tcx>, - pcx: PatCtxt<'tcx>, -) -> Vec> { +fn all_constructors<'p, 'tcx>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Vec> { debug!("all_constructors({:?})", pcx.ty); + let cx = pcx.cx; let make_range = |start, end| { IntRange( // `unwrap()` is ok because we know the type is an integer. @@ -2122,9 +2101,7 @@ impl<'tcx> IntRange<'tcx> { /// merging operation depicted above.) fn split<'p>( &self, - cx: &MatchCheckCtxt<'p, 'tcx>, - pcx: PatCtxt<'tcx>, - matrix: &Matrix<'p, 'tcx>, + pcx: PatCtxt<'_, 'p, 'tcx>, hir_id: Option, ) -> SmallVec<[Constructor<'tcx>; 1]> { let ty = pcx.ty; @@ -2152,14 +2129,15 @@ impl<'tcx> IntRange<'tcx> { // Collect the span and range of all the intersecting ranges to lint on likely // incorrect range patterns. (#63987) let mut overlaps = vec![]; - let row_len = matrix.patterns.get(0).map(|r| r.len()).unwrap_or(0); + let row_len = pcx.matrix.patterns.get(0).map(|r| r.len()).unwrap_or(0); // `borders` is the set of borders between equivalence classes: each equivalence // class lies between 2 borders. - let row_borders = matrix - .head_ctors(cx) + let row_borders = pcx + .matrix + .head_ctors(pcx.cx) .filter_map(|ctor| IntRange::from_ctor(ctor)) .filter_map(|range| { - let intersection = self.intersection(cx.tcx, &range); + let intersection = self.intersection(pcx.cx.tcx, &range); let should_lint = self.suspicious_intersection(&range); if let (Some(range), 1, true) = (&intersection, row_len, should_lint) { // FIXME: for now, only check for overlapping ranges on simple range @@ -2179,7 +2157,7 @@ impl<'tcx> IntRange<'tcx> { let mut borders: Vec<_> = row_borders.chain(self_borders).collect(); borders.sort_unstable(); - self.lint_overlapping_patterns(cx.tcx, hir_id, ty, overlaps); + self.lint_overlapping_patterns(pcx.cx.tcx, hir_id, ty, overlaps); // We're going to iterate through every adjacent pair of borders, making sure that // each represents an interval of nonnegative length, and convert each such @@ -2249,15 +2227,10 @@ struct MissingConstructors<'tcx> { } impl<'tcx> MissingConstructors<'tcx> { - fn new<'p>( - cx: &MatchCheckCtxt<'p, 'tcx>, - pcx: PatCtxt<'tcx>, - matrix: &Matrix<'p, 'tcx>, - is_top_level: bool, - ) -> Self { + fn new<'p>(pcx: PatCtxt<'_, 'p, 'tcx>, is_top_level: bool) -> Self { let used_ctors: Vec> = - matrix.head_ctors(cx).cloned().filter(|c| !c.is_wildcard()).collect(); - let all_ctors = all_constructors(cx, pcx); + pcx.matrix.head_ctors(pcx.cx).cloned().filter(|c| !c.is_wildcard()).collect(); + let all_ctors = all_constructors(pcx); MissingConstructors { all_ctors, used_ctors, is_top_level } } @@ -2277,11 +2250,7 @@ impl<'tcx> MissingConstructors<'tcx> { /// List the patterns corresponding to the missing constructors. In some cases, instead of /// listing all constructors of a given type, we prefer to simply report a wildcard. - fn report_patterns<'p>( - &self, - cx: &MatchCheckCtxt<'p, 'tcx>, - pcx: PatCtxt<'tcx>, - ) -> SmallVec<[Pat<'tcx>; 1]> { + fn report_patterns<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>) -> SmallVec<[Pat<'tcx>; 1]> { // There are 2 ways we can report a witness here. // Commonly, we can report all the "free" // constructors as witnesses, e.g., if we have: @@ -2321,8 +2290,8 @@ impl<'tcx> MissingConstructors<'tcx> { // `Option::Some`, we get the pattern `Some(_)`. self.iter() .map(|missing_ctor| { - let fields = Fields::wildcards(cx, &missing_ctor, pcx.ty); - missing_ctor.apply(cx, pcx.ty, fields) + let fields = Fields::wildcards(pcx, &missing_ctor); + missing_ctor.apply(pcx, fields) }) .collect() } @@ -2440,28 +2409,17 @@ crate fn is_useful<'p, 'tcx>( // FIXME(Nadrieril): Hack to work around type normalization issues (see #72476). let ty = matrix.heads().next().map(|r| r.ty).unwrap_or(v.head().ty); - let pcx = PatCtxt { ty, span: v.head().span }; + let pcx = PatCtxt { cx, matrix, ty, span: v.head().span }; - debug!("is_useful_expand_first_col: pcx={:#?}, expanding {:#?}", pcx, v.head()); + debug!("is_useful_expand_first_col: ty={:#?}, expanding {:#?}", pcx.ty, v.head()); let constructor = v.head_ctor(cx); let ret = if !constructor.is_wildcard() { debug!("is_useful - expanding constructor: {:#?}", constructor); constructor - .split(cx, pcx, matrix, Some(hir_id)) + .split(pcx, Some(hir_id)) .into_iter() - .map(|c| { - is_useful_specialized( - cx, - matrix, - v, - c, - pcx.ty, - witness_preference, - hir_id, - is_under_guard, - ) - }) + .map(|c| is_useful_specialized(pcx, v, c, witness_preference, hir_id, is_under_guard)) .find(|result| result.is_useful()) .unwrap_or(NotUseful) } else { @@ -2478,7 +2436,7 @@ crate fn is_useful<'p, 'tcx>( // Missing constructors are those that are not matched by any non-wildcard patterns in the // current column. We only fully construct them on-demand, because they're rarely used and // can be big. - let missing_ctors = MissingConstructors::new(cx, pcx, matrix, is_top_level); + let missing_ctors = MissingConstructors::new(pcx, is_top_level); debug!("is_useful_missing_ctors.empty()={:#?}", missing_ctors.is_empty(),); @@ -2486,31 +2444,22 @@ crate fn is_useful<'p, 'tcx>( let (all_ctors, _) = missing_ctors.into_inner(); all_ctors .into_iter() - .flat_map(|ctor| ctor.split(cx, pcx, matrix, None)) + .flat_map(|ctor| ctor.split(pcx, None)) .map(|c| { - is_useful_specialized( - cx, - matrix, - v, - c, - pcx.ty, - witness_preference, - hir_id, - is_under_guard, - ) + is_useful_specialized(pcx, v, c, witness_preference, hir_id, is_under_guard) }) .find(|result| result.is_useful()) .unwrap_or(NotUseful) } else { let ctor_wild_subpatterns = Fields::empty(); - let matrix = matrix.specialize_constructor(cx, &constructor, &ctor_wild_subpatterns); + let matrix = matrix.specialize_constructor(pcx, &constructor, &ctor_wild_subpatterns); // Unwrap is ok: v can always be specialized with its own constructor. let v = - v.specialize_constructor(cx, &constructor, &ctor_wild_subpatterns, true).unwrap(); + v.specialize_constructor(pcx, &constructor, &ctor_wild_subpatterns, true).unwrap(); let usefulness = is_useful(cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false); - usefulness.apply_wildcard(cx, pcx, missing_ctors) + usefulness.apply_wildcard(pcx, missing_ctors) } }; debug!("is_useful::returns({:#?}, {:#?}) = {:?}", matrix, v, ret); @@ -2520,23 +2469,21 @@ crate fn is_useful<'p, 'tcx>( /// A shorthand for the `U(S(c, P), S(c, q))` operation from the paper. I.e., `is_useful` applied /// to the specialised version of both the pattern matrix `P` and the new pattern `q`. fn is_useful_specialized<'p, 'tcx>( - cx: &MatchCheckCtxt<'p, 'tcx>, - matrix: &Matrix<'p, 'tcx>, + pcx: PatCtxt<'_, 'p, 'tcx>, v: &PatStack<'p, 'tcx>, ctor: Constructor<'tcx>, - ty: Ty<'tcx>, witness_preference: WitnessPreference, hir_id: HirId, is_under_guard: bool, ) -> Usefulness<'tcx> { - debug!("is_useful_specialized({:#?}, {:#?}, {:?})", v, ctor, ty); + debug!("is_useful_specialized({:#?}, {:#?}, {:?})", v, ctor, pcx.ty); // We cache the result of `Fields::wildcards` because it is used a lot. - let ctor_wild_subpatterns = Fields::wildcards(cx, &ctor, ty); - let matrix = matrix.specialize_constructor(cx, &ctor, &ctor_wild_subpatterns); - v.specialize_constructor(cx, &ctor, &ctor_wild_subpatterns, true) - .map(|v| is_useful(cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false)) - .map(|u| u.apply_constructor(cx, &ctor, ty, &ctor_wild_subpatterns)) + let ctor_wild_subpatterns = Fields::wildcards(pcx, &ctor); + let matrix = pcx.matrix.specialize_constructor(pcx, &ctor, &ctor_wild_subpatterns); + v.specialize_constructor(pcx, &ctor, &ctor_wild_subpatterns, true) + .map(|v| is_useful(pcx.cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false)) + .map(|u| u.apply_constructor(pcx, &ctor, &ctor_wild_subpatterns)) .unwrap_or(NotUseful) } From c96bd28ab375e6e273bac174e86860c9d3a27510 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 26 Oct 2020 18:41:31 +0000 Subject: [PATCH 10/16] Recompute `MissingConstructors` when needed This only happens in a slow (diagnostics) path, so the code clarity gain is worth it. --- .../src/thir/pattern/_match.rs | 95 +++++++++++-------- 1 file changed, 56 insertions(+), 39 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 850ce1847dbaa..948c441f2c903 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -1285,7 +1285,9 @@ impl<'tcx> Constructor<'tcx> { IntRange(range) => return range.to_pat(pcx.cx.tcx), NonExhaustive => PatKind::Wild, Opaque => bug!("we should not try to apply an opaque constructor"), - Wildcard => bug!("we should not try to apply a wildcard constructor"), + Wildcard => bug!( + "trying to apply a wildcard constructor; this should have been done in `apply_constructors`" + ), }; Pat { ty: pcx.ty, span: DUMMY_SP, kind: Box::new(pat) } @@ -1610,27 +1612,13 @@ impl<'tcx> Usefulness<'tcx> { pcx: PatCtxt<'_, 'p, 'tcx>, ctor: &Constructor<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, - ) -> Self { - match self { - UsefulWithWitness(witnesses) => UsefulWithWitness( - witnesses - .into_iter() - .map(|witness| witness.apply_constructor(pcx, &ctor, ctor_wild_subpatterns)) - .collect(), - ), - x => x, - } - } - - fn apply_wildcard<'p>( - self, - pcx: PatCtxt<'_, 'p, 'tcx>, - missing_ctors: MissingConstructors<'tcx>, + is_top_level: bool, ) -> Self { match self { UsefulWithWitness(witnesses) => { - let new_patterns = missing_ctors.report_patterns(pcx); - UsefulWithWitness( + let new_witnesses = if ctor.is_wildcard() { + let missing_ctors = MissingConstructors::new(pcx, is_top_level); + let new_patterns = missing_ctors.report_patterns(pcx); witnesses .into_iter() .flat_map(|witness| { @@ -1640,8 +1628,14 @@ impl<'tcx> Usefulness<'tcx> { witness }) }) - .collect(), - ) + .collect() + } else { + witnesses + .into_iter() + .map(|witness| witness.apply_constructor(pcx, &ctor, ctor_wild_subpatterns)) + .collect() + }; + UsefulWithWitness(new_witnesses) } x => x, } @@ -2419,7 +2413,17 @@ crate fn is_useful<'p, 'tcx>( constructor .split(pcx, Some(hir_id)) .into_iter() - .map(|c| is_useful_specialized(pcx, v, c, witness_preference, hir_id, is_under_guard)) + .map(|c| { + is_useful_specialized( + pcx, + v, + &c, + witness_preference, + hir_id, + is_under_guard, + is_top_level, + ) + }) .find(|result| result.is_useful()) .unwrap_or(NotUseful) } else { @@ -2446,20 +2450,31 @@ crate fn is_useful<'p, 'tcx>( .into_iter() .flat_map(|ctor| ctor.split(pcx, None)) .map(|c| { - is_useful_specialized(pcx, v, c, witness_preference, hir_id, is_under_guard) + is_useful_specialized( + pcx, + v, + &c, + witness_preference, + hir_id, + is_under_guard, + is_top_level, + ) }) .find(|result| result.is_useful()) .unwrap_or(NotUseful) } else { - let ctor_wild_subpatterns = Fields::empty(); - let matrix = matrix.specialize_constructor(pcx, &constructor, &ctor_wild_subpatterns); - // Unwrap is ok: v can always be specialized with its own constructor. - let v = - v.specialize_constructor(pcx, &constructor, &ctor_wild_subpatterns, true).unwrap(); - let usefulness = - is_useful(cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false); - - usefulness.apply_wildcard(pcx, missing_ctors) + // Some constructors are missing, thus we can specialize with the wildcard constructor, + // which will stand for those constructors that are missing, and behaves like any of + // them. + is_useful_specialized( + pcx, + v, + constructor, + witness_preference, + hir_id, + is_under_guard, + is_top_level, + ) } }; debug!("is_useful::returns({:#?}, {:#?}) = {:?}", matrix, v, ret); @@ -2471,20 +2486,22 @@ crate fn is_useful<'p, 'tcx>( fn is_useful_specialized<'p, 'tcx>( pcx: PatCtxt<'_, 'p, 'tcx>, v: &PatStack<'p, 'tcx>, - ctor: Constructor<'tcx>, + ctor: &Constructor<'tcx>, witness_preference: WitnessPreference, hir_id: HirId, is_under_guard: bool, + is_top_level: bool, ) -> Usefulness<'tcx> { debug!("is_useful_specialized({:#?}, {:#?}, {:?})", v, ctor, pcx.ty); // We cache the result of `Fields::wildcards` because it is used a lot. - let ctor_wild_subpatterns = Fields::wildcards(pcx, &ctor); - let matrix = pcx.matrix.specialize_constructor(pcx, &ctor, &ctor_wild_subpatterns); - v.specialize_constructor(pcx, &ctor, &ctor_wild_subpatterns, true) - .map(|v| is_useful(pcx.cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false)) - .map(|u| u.apply_constructor(pcx, &ctor, &ctor_wild_subpatterns)) - .unwrap_or(NotUseful) + let ctor_wild_subpatterns = Fields::wildcards(pcx, ctor); + let matrix = pcx.matrix.specialize_constructor(pcx, ctor, &ctor_wild_subpatterns); + // Unwrap is ok: v can always be specialized with its own constructor. + let v = v.specialize_constructor(pcx, ctor, &ctor_wild_subpatterns, true).unwrap(); + let usefulness = + is_useful(pcx.cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false); + usefulness.apply_constructor(pcx, ctor, &ctor_wild_subpatterns, is_top_level) } /// Determines the constructor that the given pattern can be specialized to. From 54fa70290d8ab65e33116eb888278f7a64ca6da4 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Tue, 20 Oct 2020 02:56:40 +0100 Subject: [PATCH 11/16] Unify the paths through `is_useful` --- .../src/thir/pattern/_match.rs | 156 ++++++------------ 1 file changed, 53 insertions(+), 103 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 948c441f2c903..c7f09ea4ad94a 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -181,7 +181,6 @@ //! we ignore all the patterns in the first column of `P` that involve other constructors. //! This is where `S(c, P)` comes in: //! `U(P, p) := U(S(c, P), S(c, p))` -//! This special case is handled in `is_useful_specialized`. //! //! For example, if `P` is: //! @@ -1116,8 +1115,8 @@ impl<'tcx> Constructor<'tcx> { } } - /// Some constructors (namely IntRange and Slice) actually stand for a set of actual - /// constructors (integers and fixed-sized slices). When specializing for these + /// Some constructors (namely Wildcard, IntRange and Slice) actually stand for a set of actual + /// constructors (like variants, integers or fixed-sized slices). When specializing for these /// constructors, we want to be specialising for the actual underlying constructors. /// Naively, we would simply return the list of constructors they correspond to. We instead are /// more clever: if there are constructors that we know will behave the same wrt the current @@ -1136,6 +1135,7 @@ impl<'tcx> Constructor<'tcx> { debug!("Constructor::split({:#?}, {:#?})", self, pcx.matrix); match self { + Wildcard => Constructor::split_wildcard(pcx), // Fast-track if the range is trivial. In particular, we don't do the overlapping // ranges check. IntRange(ctor_range) @@ -1149,6 +1149,30 @@ impl<'tcx> Constructor<'tcx> { } } + /// For wildcards, there are two groups of constructors: there are the constructors actually + /// present in the matrix (`head_ctors`), and the constructors not present (`missing_ctors`). + /// Two constructors that are not in the matrix will either both be catched (by a wildcard), or + /// both not be catched. Therefore we can keep the missing constructors grouped together. + fn split_wildcard<'p>(pcx: PatCtxt<'_, 'p, 'tcx>) -> SmallVec<[Self; 1]> { + // Missing constructors are those that are not matched by any non-wildcard patterns in the + // current column. We only fully construct them on-demand, because they're rarely used and + // can be big. + let missing_ctors = MissingConstructors::new(pcx); + + if missing_ctors.is_empty() { + // All the constructors are present in the matrix, so we just go through them all. + // We must also split them first. + // Since `all_ctors` never contains wildcards, this won't recurse more than once. + let (all_ctors, _) = missing_ctors.into_inner(); + all_ctors.into_iter().flat_map(|ctor| ctor.split(pcx, None)).collect() + } else { + // Some constructors are missing, thus we can specialize with the wildcard constructor, + // which will stand for those constructors that are missing, and behaves like any of + // them. + smallvec![Wildcard] + } + } + /// Returns whether `self` is covered by `other`, ie whether `self` is a subset of `other`. For /// the simple cases, this is simply checking for equality. For the "grouped" constructors, /// this checks for inclusion. @@ -1617,8 +1641,8 @@ impl<'tcx> Usefulness<'tcx> { match self { UsefulWithWitness(witnesses) => { let new_witnesses = if ctor.is_wildcard() { - let missing_ctors = MissingConstructors::new(pcx, is_top_level); - let new_patterns = missing_ctors.report_patterns(pcx); + let missing_ctors = MissingConstructors::new(pcx); + let new_patterns = missing_ctors.report_patterns(pcx, is_top_level); witnesses .into_iter() .flat_map(|witness| { @@ -2217,16 +2241,15 @@ impl<'tcx> std::cmp::PartialEq for IntRange<'tcx> { struct MissingConstructors<'tcx> { all_ctors: Vec>, used_ctors: Vec>, - is_top_level: bool, } impl<'tcx> MissingConstructors<'tcx> { - fn new<'p>(pcx: PatCtxt<'_, 'p, 'tcx>, is_top_level: bool) -> Self { + fn new<'p>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Self { let used_ctors: Vec> = pcx.matrix.head_ctors(pcx.cx).cloned().filter(|c| !c.is_wildcard()).collect(); let all_ctors = all_constructors(pcx); - MissingConstructors { all_ctors, used_ctors, is_top_level } + MissingConstructors { all_ctors, used_ctors } } fn into_inner(self) -> (Vec>, Vec>) { @@ -2244,7 +2267,11 @@ impl<'tcx> MissingConstructors<'tcx> { /// List the patterns corresponding to the missing constructors. In some cases, instead of /// listing all constructors of a given type, we prefer to simply report a wildcard. - fn report_patterns<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>) -> SmallVec<[Pat<'tcx>; 1]> { + fn report_patterns<'p>( + &self, + pcx: PatCtxt<'_, 'p, 'tcx>, + is_top_level: bool, + ) -> SmallVec<[Pat<'tcx>; 1]> { // There are 2 ways we can report a witness here. // Commonly, we can report all the "free" // constructors as witnesses, e.g., if we have: @@ -2272,7 +2299,7 @@ impl<'tcx> MissingConstructors<'tcx> { // `used_ctors` is empty. // The exception is: if we are at the top-level, for example in an empty match, we // sometimes prefer reporting the list of constructors instead of just `_`. - let report_when_all_missing = self.is_top_level && !IntRange::is_integral(pcx.ty); + let report_when_all_missing = is_top_level && !IntRange::is_integral(pcx.ty); if self.used_ctors.is_empty() && !report_when_all_missing { // All constructors are unused. Report only a wildcard // rather than each individual constructor. @@ -2407,103 +2434,26 @@ crate fn is_useful<'p, 'tcx>( debug!("is_useful_expand_first_col: ty={:#?}, expanding {:#?}", pcx.ty, v.head()); - let constructor = v.head_ctor(cx); - let ret = if !constructor.is_wildcard() { - debug!("is_useful - expanding constructor: {:#?}", constructor); - constructor - .split(pcx, Some(hir_id)) - .into_iter() - .map(|c| { - is_useful_specialized( - pcx, - v, - &c, - witness_preference, - hir_id, - is_under_guard, - is_top_level, - ) - }) - .find(|result| result.is_useful()) - .unwrap_or(NotUseful) - } else { - debug!("is_useful - expanding wildcard"); - - // `missing_ctors` is the set of constructors from the same type as the - // first column of `matrix` that are matched only by wildcard patterns - // from the first column. - // - // Therefore, if there is some pattern that is unmatched by `matrix`, - // it will still be unmatched if the first constructor is replaced by - // any of the constructors in `missing_ctors` - - // Missing constructors are those that are not matched by any non-wildcard patterns in the - // current column. We only fully construct them on-demand, because they're rarely used and - // can be big. - let missing_ctors = MissingConstructors::new(pcx, is_top_level); - - debug!("is_useful_missing_ctors.empty()={:#?}", missing_ctors.is_empty(),); - - if missing_ctors.is_empty() { - let (all_ctors, _) = missing_ctors.into_inner(); - all_ctors - .into_iter() - .flat_map(|ctor| ctor.split(pcx, None)) - .map(|c| { - is_useful_specialized( - pcx, - v, - &c, - witness_preference, - hir_id, - is_under_guard, - is_top_level, - ) - }) - .find(|result| result.is_useful()) - .unwrap_or(NotUseful) - } else { - // Some constructors are missing, thus we can specialize with the wildcard constructor, - // which will stand for those constructors that are missing, and behaves like any of - // them. - is_useful_specialized( - pcx, - v, - constructor, - witness_preference, - hir_id, - is_under_guard, - is_top_level, - ) - } - }; + let ret = v + .head_ctor(cx) + .split(pcx, Some(hir_id)) + .into_iter() + .map(|ctor| { + // We cache the result of `Fields::wildcards` because it is used a lot. + let ctor_wild_subpatterns = Fields::wildcards(pcx, &ctor); + let matrix = pcx.matrix.specialize_constructor(pcx, &ctor, &ctor_wild_subpatterns); + // Unwrap is ok: v can always be specialized with its own constructor. + let v = v.specialize_constructor(pcx, &ctor, &ctor_wild_subpatterns, true).unwrap(); + let usefulness = + is_useful(pcx.cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false); + usefulness.apply_constructor(pcx, &ctor, &ctor_wild_subpatterns, is_top_level) + }) + .find(|result| result.is_useful()) + .unwrap_or(NotUseful); debug!("is_useful::returns({:#?}, {:#?}) = {:?}", matrix, v, ret); ret } -/// A shorthand for the `U(S(c, P), S(c, q))` operation from the paper. I.e., `is_useful` applied -/// to the specialised version of both the pattern matrix `P` and the new pattern `q`. -fn is_useful_specialized<'p, 'tcx>( - pcx: PatCtxt<'_, 'p, 'tcx>, - v: &PatStack<'p, 'tcx>, - ctor: &Constructor<'tcx>, - witness_preference: WitnessPreference, - hir_id: HirId, - is_under_guard: bool, - is_top_level: bool, -) -> Usefulness<'tcx> { - debug!("is_useful_specialized({:#?}, {:#?}, {:?})", v, ctor, pcx.ty); - - // We cache the result of `Fields::wildcards` because it is used a lot. - let ctor_wild_subpatterns = Fields::wildcards(pcx, ctor); - let matrix = pcx.matrix.specialize_constructor(pcx, ctor, &ctor_wild_subpatterns); - // Unwrap is ok: v can always be specialized with its own constructor. - let v = v.specialize_constructor(pcx, ctor, &ctor_wild_subpatterns, true).unwrap(); - let usefulness = - is_useful(pcx.cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false); - usefulness.apply_constructor(pcx, ctor, &ctor_wild_subpatterns, is_top_level) -} - /// Determines the constructor that the given pattern can be specialized to. /// Returns `None` in case of a catch-all, which can't be specialized. fn pat_constructor<'p, 'tcx>( From db9a8480c481deb0936508c224dc0514e4fe0e80 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Tue, 27 Oct 2020 02:30:10 +0000 Subject: [PATCH 12/16] Simplify specialize_constructor Also removes the ugly caching that was introduced in #76918. It was bolted on without deeper knowledge of the workings of the algorithm. This commit manages to be more performant without any of the complexity. It should be better on representative workloads too. --- .../src/thir/pattern/_match.rs | 188 ++---------------- 1 file changed, 13 insertions(+), 175 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index c7f09ea4ad94a..966d3c747f6e1 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -293,7 +293,7 @@ use self::Usefulness::*; use self::WitnessPreference::*; use rustc_data_structures::captures::Captures; -use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::sync::OnceCell; use rustc_index::vec::Idx; @@ -401,48 +401,17 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { } } - /// This computes `S(constructor, self)`. See top of the file for explanations. - /// - /// This is the main specialization step. It expands the pattern - /// into `arity` patterns based on the constructor. For most patterns, the step is trivial, - /// for instance tuple patterns are flattened and box patterns expand into their inner pattern. - /// Returns `None` if the pattern does not have the given constructor. + /// This computes `S(self.head_ctor(), self)`. See top of the file for explanations. /// - /// OTOH, slice patterns with a subslice pattern (tail @ ..) can be expanded into multiple - /// different patterns. /// Structure patterns with a partial wild pattern (Foo { a: 42, .. }) have their missing /// fields filled with wild patterns. /// /// This is roughly the inverse of `Constructor::apply`. - fn specialize_constructor( - &self, - pcx: PatCtxt<'_, 'p, 'tcx>, - ctor: &Constructor<'tcx>, - ctor_wild_subpatterns: &Fields<'p, 'tcx>, - is_my_head_ctor: bool, - ) -> Option> { - // We return `None` if `ctor` is not covered by `self.head()`. If `ctor` is known to be - // derived from `self.head()`, then we don't need to check; otherwise, we check for - // constructor inclusion. - // Note that this shortcut is also necessary for correctness: a pattern should always be - // specializable with its own constructor, even in cases where we refuse to inspect values like - // opaque constants. - if !is_my_head_ctor && !ctor.is_covered_by(pcx, self.head_ctor(pcx.cx)) { - return None; - } - let new_fields = ctor_wild_subpatterns.replace_with_pattern_arguments(self.head()); - - debug!( - "specialize_constructor({:#?}, {:#?}, {:#?}) = {:#?}", - self.head(), - ctor, - ctor_wild_subpatterns, - new_fields - ); - + fn pop_head_constructor(&self, ctor_wild_subpatterns: &Fields<'p, 'tcx>) -> PatStack<'p, 'tcx> { // We pop the head pattern and push the new fields extracted from the arguments of // `self.head()`. - Some(new_fields.push_on_patstack(&self.pats[1..])) + let new_fields = ctor_wild_subpatterns.replace_with_pattern_arguments(self.head()); + new_fields.push_on_patstack(&self.pats[1..]) } } @@ -467,36 +436,15 @@ impl<'p, 'tcx> FromIterator<&'p Pat<'tcx>> for PatStack<'p, 'tcx> { } } -/// Depending on the match patterns, the specialization process might be able to use a fast path. -/// Tracks whether we can use the fast path and the lookup table needed in those cases. -#[derive(Clone, Debug, PartialEq)] -enum SpecializationCache { - /// Patterns consist of only enum variants. - /// Variant patterns does not intersect with each other (in contrast to range patterns), - /// so it is possible to precompute the result of `Matrix::specialize_constructor` at a - /// lower computational complexity. - /// `lookup` is responsible for holding the precomputed result of - /// specialization, while `wilds` is used for two purposes: the first one is - /// the precomputed result of specialization with a wildcard, and the second is to be used as a - /// fallback for `Matrix::specialize_constructor` when it tries to apply a constructor that - /// has not been seen in the `Matrix`. See `update_cache` for further explanations. - Variants { lookup: FxHashMap>, wilds: SmallVec<[usize; 1]> }, - /// Does not belong to the cases above, use the slow path. - Incompatible, -} - /// A 2D matrix. #[derive(Clone, PartialEq)] crate struct Matrix<'p, 'tcx> { patterns: Vec>, - cache: SpecializationCache, } impl<'p, 'tcx> Matrix<'p, 'tcx> { crate fn empty() -> Self { - // Use `SpecializationCache::Incompatible` as a placeholder; we will initialize it on the - // first call to `push`. See the first half of `update_cache`. - Matrix { patterns: vec![], cache: SpecializationCache::Incompatible } + Matrix { patterns: vec![] } } /// Pushes a new row to the matrix. If the row starts with an or-pattern, this expands it. @@ -509,70 +457,6 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { } } else { self.patterns.push(row); - self.update_cache(self.patterns.len() - 1); - } - } - - fn update_cache(&mut self, idx: usize) { - let row = &self.patterns[idx]; - // We don't know which kind of cache could be used until we see the first row; therefore an - // empty `Matrix` is initialized with `SpecializationCache::Empty`, then the cache is - // assigned the appropriate variant below on the first call to `push`. - if self.patterns.is_empty() { - self.cache = if row.is_empty() { - SpecializationCache::Incompatible - } else { - match *row.head().kind { - PatKind::Variant { .. } => SpecializationCache::Variants { - lookup: FxHashMap::default(), - wilds: SmallVec::new(), - }, - // Note: If the first pattern is a wildcard, then all patterns after that is not - // useful. The check is simple enough so we treat it as the same as unsupported - // patterns. - _ => SpecializationCache::Incompatible, - } - }; - } - // Update the cache. - match &mut self.cache { - SpecializationCache::Variants { ref mut lookup, ref mut wilds } => { - let head = row.head(); - match *head.kind { - _ if head.is_wildcard() => { - // Per rule 1.3 in the top-level comments, a wildcard pattern is included in - // the result of `specialize_constructor` for *any* `Constructor`. - // We push the wildcard pattern to the precomputed result for constructors - // that we have seen before; results for constructors we have not yet seen - // defaults to `wilds`, which is updated right below. - for (_, v) in lookup.iter_mut() { - v.push(idx); - } - // Per rule 2.1 and 2.2 in the top-level comments, only wildcard patterns - // are included in the result of specialization with a wildcard. - // What we do here is to track the wildcards we have seen; so in addition to - // acting as the precomputed result of specialization with a wildcard, `wilds` also - // serves as the default value of `specialize_constructor` for constructors - // that are not in `lookup`. - wilds.push(idx); - } - PatKind::Variant { adt_def, variant_index, .. } => { - // Handle the cases of rule 1.1 and 1.2 in the top-level comments. - // A variant pattern can only be included in the results of - // `specialize_constructor` for a particular constructor, therefore we are - // using a HashMap to track that. - lookup - .entry(adt_def.variants[variant_index].def_id) - // Default to `wilds` for absent keys. See above for an explanation. - .or_insert_with(|| wilds.clone()) - .push(idx); - } - _ => { - self.cache = SpecializationCache::Incompatible; - } - } - } - SpecializationCache::Incompatible => {} } } @@ -593,59 +477,14 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { fn specialize_constructor( &self, pcx: PatCtxt<'_, 'p, 'tcx>, - constructor: &Constructor<'tcx>, + ctor: &Constructor<'tcx>, ctor_wild_subpatterns: &Fields<'p, 'tcx>, ) -> Matrix<'p, 'tcx> { - match &self.cache { - SpecializationCache::Variants { lookup, wilds } => { - let cached = if let Constructor::Variant(id) = constructor { - lookup - .get(id) - // Default to `wilds` for absent keys. See `update_cache` for an explanation. - .unwrap_or(&wilds) - } else if let Wildcard = constructor { - &wilds - } else { - bug!( - "unexpected constructor encountered while dealing with matrix cache: {:?}", - constructor - ); - }; - let result: Self = cached - .iter() - .filter_map(|&i| { - self.patterns[i].specialize_constructor( - pcx, - constructor, - ctor_wild_subpatterns, - false, - ) - }) - .collect(); - // When debug assertions are enabled, check the results against the "slow path" - // result. - debug_assert_eq!( - result, - Matrix { - patterns: self.patterns.clone(), - cache: SpecializationCache::Incompatible - } - .specialize_constructor( - pcx, - constructor, - ctor_wild_subpatterns - ) - ); - result - } - SpecializationCache::Incompatible => self - .patterns - .iter() - .filter_map(|r| { - r.specialize_constructor(pcx, constructor, ctor_wild_subpatterns, false) - }) - .collect(), - } + self.patterns + .iter() + .filter(|r| ctor.is_covered_by(pcx, r.head_ctor(pcx.cx))) + .map(|r| r.pop_head_constructor(ctor_wild_subpatterns)) + .collect() } } @@ -2442,8 +2281,7 @@ crate fn is_useful<'p, 'tcx>( // We cache the result of `Fields::wildcards` because it is used a lot. let ctor_wild_subpatterns = Fields::wildcards(pcx, &ctor); let matrix = pcx.matrix.specialize_constructor(pcx, &ctor, &ctor_wild_subpatterns); - // Unwrap is ok: v can always be specialized with its own constructor. - let v = v.specialize_constructor(pcx, &ctor, &ctor_wild_subpatterns, true).unwrap(); + let v = v.pop_head_constructor(&ctor_wild_subpatterns); let usefulness = is_useful(pcx.cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false); usefulness.apply_constructor(pcx, &ctor, &ctor_wild_subpatterns, is_top_level) From 1fab669f8dbc1138509fe3a28c200cab5c54db21 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Tue, 20 Oct 2020 02:31:21 +0100 Subject: [PATCH 13/16] Be honest about being able to list constructors The test change is because we used to treat `&str` like other `&T`s, ie as having a single constructor. That's not quite true though since we consider `&str` constants as atomic instead of refs to `str` constants. --- .../src/thir/pattern/_match.rs | 74 ++++++++++--------- src/test/ui/issues/issue-30240.rs | 4 +- src/test/ui/issues/issue-30240.stderr | 8 +- 3 files changed, 47 insertions(+), 39 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 966d3c747f6e1..b6b1491e36321 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -846,6 +846,9 @@ enum Constructor<'tcx> { Opaque, /// Fake extra constructor for enums that aren't allowed to be matched exhaustively. NonExhaustive, + /// Fake constructor for those types for which we can't list constructors explicitely, like + /// `f64` and `&str`. + Unlistable, /// Wildcard pattern. Wildcard, } @@ -949,6 +952,9 @@ impl<'tcx> Constructor<'tcx> { } // This constructor is never covered by anything else NonExhaustive => vec![NonExhaustive], + // This constructor is only covered by `Single`s + Unlistable if other_ctors.iter().any(|c| *c == Single) => vec![], + Unlistable => vec![Unlistable], Opaque => bug!("found unexpected opaque ctor in all_ctors"), Wildcard => bug!("found unexpected wildcard ctor in all_ctors"), } @@ -1068,6 +1074,11 @@ impl<'tcx> Constructor<'tcx> { (Opaque, _) | (_, Opaque) => false, // Only a wildcard pattern can match the special extra constructor. (NonExhaustive, _) => false, + // If we encounter a `Single` here, this means there was only one constructor for this + // type after all. + (Unlistable, Single) => true, + // Otherwise, only a wildcard pattern can match the special extra constructor. + (Unlistable, _) => false, _ => bug!("trying to compare incompatible constructors {:?} and {:?}", self, other), } @@ -1146,7 +1157,7 @@ impl<'tcx> Constructor<'tcx> { &Str(value) => PatKind::Constant { value }, &FloatRange(lo, hi, end) => PatKind::Range(PatRange { lo, hi, end }), IntRange(range) => return range.to_pat(pcx.cx.tcx), - NonExhaustive => PatKind::Wild, + NonExhaustive | Unlistable => PatKind::Wild, Opaque => bug!("we should not try to apply an opaque constructor"), Wildcard => bug!( "trying to apply a wildcard constructor; this should have been done in `apply_constructors`" @@ -1286,7 +1297,7 @@ impl<'p, 'tcx> Fields<'p, 'tcx> { } } } - _ => Fields::empty(), + _ => bug!("Unexpected type for `Single` constructor: {:?}", ty), }, Slice(slice) => match *ty.kind() { ty::Slice(ty) | ty::Array(ty, _) => { @@ -1295,9 +1306,8 @@ impl<'p, 'tcx> Fields<'p, 'tcx> { } _ => bug!("bad slice pattern {:?} {:?}", constructor, ty), }, - Str(..) | FloatRange(..) | IntRange(..) | NonExhaustive | Opaque | Wildcard => { - Fields::empty() - } + Str(..) | FloatRange(..) | IntRange(..) | NonExhaustive | Opaque | Unlistable + | Wildcard => Fields::empty(), }; debug!("Fields::wildcards({:?}, {:?}) = {:#?}", constructor, ty, ret); ret @@ -1616,9 +1626,9 @@ fn all_constructors<'p, 'tcx>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Vec vec![make_range(0, 1)], - ty::Array(ref sub_ty, len) if len.try_eval_usize(cx.tcx, cx.param_env).is_some() => { + ty::Array(sub_ty, len) if len.try_eval_usize(cx.tcx, cx.param_env).is_some() => { let len = len.eval_usize(cx.tcx, cx.param_env); if len != 0 && cx.is_uninhabited(sub_ty) { vec![] @@ -1627,26 +1637,11 @@ fn all_constructors<'p, 'tcx>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Vec { + ty::Array(sub_ty, _) | ty::Slice(sub_ty) => { let kind = if cx.is_uninhabited(sub_ty) { FixedLen(0) } else { VarLen(0, 0) }; vec![Slice(Slice { array_len: None, kind })] } ty::Adt(def, substs) if def.is_enum() => { - let ctors: Vec<_> = if cx.tcx.features().exhaustive_patterns { - // If `exhaustive_patterns` is enabled, we exclude variants known to be - // uninhabited. - def.variants - .iter() - .filter(|v| { - !v.uninhabited_from(cx.tcx, substs, def.adt_kind(), cx.param_env) - .contains(cx.tcx, cx.module) - }) - .map(|v| Variant(v.def_id)) - .collect() - } else { - def.variants.iter().map(|v| Variant(v.def_id)).collect() - }; - // 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 @@ -1672,7 +1667,22 @@ fn all_constructors<'p, 'tcx>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Vec { vec![ @@ -1690,24 +1700,22 @@ fn all_constructors<'p, 'tcx>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Vec { + &ty::Int(ity) => { let bits = Integer::from_attr(&cx.tcx, SignedInt(ity)).size().bits() as u128; let min = 1u128 << (bits - 1); let max = min - 1; vec![make_range(min, max)] } - ty::Uint(uty) => { + &ty::Uint(uty) => { let size = Integer::from_attr(&cx.tcx, UnsignedInt(uty)).size(); let max = truncate(u128::MAX, size); vec![make_range(0, max)] } - _ => { - if cx.is_uninhabited(pcx.ty) { - vec![] - } else { - vec![Single] - } - } + _ if cx.is_uninhabited(pcx.ty) => vec![], + ty::Adt(..) | ty::Tuple(..) => vec![Single], + ty::Ref(_, t, _) if !t.is_str() => vec![Single], + // This type is one for which we don't know how to list constructors, like &str of f64. + _ => vec![Unlistable], } } diff --git a/src/test/ui/issues/issue-30240.rs b/src/test/ui/issues/issue-30240.rs index a0c0d1626ec45..8075532c37d76 100644 --- a/src/test/ui/issues/issue-30240.rs +++ b/src/test/ui/issues/issue-30240.rs @@ -1,9 +1,9 @@ fn main() { - match "world" { //~ ERROR non-exhaustive patterns: `&_` + match "world" { //~ ERROR non-exhaustive patterns: `_` "hello" => {} } - match "world" { //~ ERROR non-exhaustive patterns: `&_` + match "world" { //~ ERROR non-exhaustive patterns: `_` ref _x if false => {} "hello" => {} } diff --git a/src/test/ui/issues/issue-30240.stderr b/src/test/ui/issues/issue-30240.stderr index a2c58d6e051b5..71a8bcb50cda4 100644 --- a/src/test/ui/issues/issue-30240.stderr +++ b/src/test/ui/issues/issue-30240.stderr @@ -1,17 +1,17 @@ -error[E0004]: non-exhaustive patterns: `&_` not covered +error[E0004]: non-exhaustive patterns: `_` not covered --> $DIR/issue-30240.rs:2:11 | LL | match "world" { - | ^^^^^^^ pattern `&_` not covered + | ^^^^^^^ pattern `_` not covered | = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms = note: the matched value is of type `&str` -error[E0004]: non-exhaustive patterns: `&_` not covered +error[E0004]: non-exhaustive patterns: `_` not covered --> $DIR/issue-30240.rs:6:11 | LL | match "world" { - | ^^^^^^^ pattern `&_` not covered + | ^^^^^^^ pattern `_` not covered | = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms = note: the matched value is of type `&str` From cd4c7144de75e4789dd6dac5f6020aecb1e8e0b0 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Tue, 27 Oct 2020 01:59:36 +0000 Subject: [PATCH 14/16] Deduplicate work between splitting and subtraction After splitting, subtraction becomes much simpler --- .../src/thir/pattern/_match.rs | 303 +++++------------- 1 file changed, 87 insertions(+), 216 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index b6b1491e36321..2e7b3626e8705 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -608,46 +608,6 @@ impl SliceKind { VarLen(prefix, suffix) => prefix + suffix <= other_len, } } - - /// Returns a collection of slices that spans the values covered by `self`, subtracted by the - /// values covered by `other`: i.e., `self \ other` (in set notation). - fn subtract(self, other: Self) -> SmallVec<[Self; 1]> { - // Remember, `VarLen(i, j)` covers the union of `FixedLen` from `i + j` to infinity. - // Naming: we remove the "neg" constructors from the "pos" ones. - match self { - FixedLen(pos_len) => { - if other.covers_length(pos_len) { - smallvec![] - } else { - smallvec![self] - } - } - VarLen(pos_prefix, pos_suffix) => { - let pos_len = pos_prefix + pos_suffix; - match other { - FixedLen(neg_len) => { - if neg_len < pos_len { - smallvec![self] - } else { - (pos_len..neg_len) - .map(FixedLen) - // We know that `neg_len + 1 >= pos_len >= pos_suffix`. - .chain(Some(VarLen(neg_len + 1 - pos_suffix, pos_suffix))) - .collect() - } - } - VarLen(neg_prefix, neg_suffix) => { - let neg_len = neg_prefix + neg_suffix; - if neg_len <= pos_len { - smallvec![] - } else { - (pos_len..neg_len).map(FixedLen).collect() - } - } - } - } - } - } } /// A constructor for array and slice patterns. @@ -662,7 +622,7 @@ struct Slice { impl Slice { /// Returns what patterns this constructor covers: either fixed-length patterns or /// variable-length patterns. - fn pattern_kind(self) -> SliceKind { + fn kind(self) -> SliceKind { match self { Slice { array_len: Some(len), kind: VarLen(prefix, suffix) } if prefix + suffix == len => @@ -673,20 +633,8 @@ impl Slice { } } - /// Returns what values this constructor covers: either values of only one given length, or - /// values of length above a given length. - /// This is different from `pattern_kind()` because in some cases the pattern only takes into - /// account a subset of the entries of the array, but still only captures values of a given - /// length. - fn value_kind(self) -> SliceKind { - match self { - Slice { array_len: Some(len), kind: VarLen(_, _) } => FixedLen(len), - _ => self.kind, - } - } - fn arity(self) -> u64 { - self.pattern_kind().arity() + self.kind().arity() } /// The exhaustiveness-checking paper does not include any details on @@ -768,7 +716,7 @@ impl Slice { for ctor in head_ctors { if let Slice(slice) = ctor { - match slice.pattern_kind() { + match slice.kind() { FixedLen(len) => { max_fixed_len = cmp::max(max_fixed_len, len); } @@ -816,6 +764,11 @@ impl Slice { } } } + + /// See `Constructor::is_covered_by` + fn is_covered_by(self, other: Self) -> bool { + other.kind().covers_length(self.arity()) + } } /// A value can be decomposed into a constructor applied to some fields. This struct represents @@ -861,6 +814,20 @@ impl<'tcx> Constructor<'tcx> { } } + fn as_intrange(&self) -> Option<&IntRange<'tcx>> { + match self { + IntRange(range) => Some(range), + _ => None, + } + } + + fn as_slice(&self) -> Option { + match self { + Slice(slice) => Some(*slice), + _ => None, + } + } + fn variant_index_for_adt(&self, adt: &'tcx ty::AdtDef) -> VariantIdx { match *self { Variant(id) => adt.variant_index_with_id(id), @@ -872,94 +839,6 @@ impl<'tcx> Constructor<'tcx> { } } - // Returns the set of constructors covered by `self` but not by - // anything in `other_ctors`. - fn subtract_ctors(&self, other_ctors: &Vec>) -> Vec> { - if other_ctors.is_empty() { - return vec![self.clone()]; - } - - match self { - // Those constructors can only match themselves. - Single | Variant(_) | Str(..) | FloatRange(..) => { - if other_ctors.iter().any(|c| c == self) { vec![] } else { vec![self.clone()] } - } - &Slice(slice) => { - let mut other_slices = other_ctors - .iter() - .filter_map(|c: &Constructor<'_>| match c { - Slice(slice) => Some(*slice), - _ => bug!("bad slice pattern constructor {:?}", c), - }) - .map(Slice::value_kind); - - match slice.value_kind() { - FixedLen(self_len) => { - if other_slices.any(|other_slice| other_slice.covers_length(self_len)) { - vec![] - } else { - vec![Slice(slice)] - } - } - kind @ VarLen(..) => { - let mut remaining_slices = vec![kind]; - - // For each used slice, subtract from the current set of slices. - for other_slice in other_slices { - remaining_slices = remaining_slices - .into_iter() - .flat_map(|remaining_slice| remaining_slice.subtract(other_slice)) - .collect(); - - // If the constructors that have been considered so far already cover - // the entire range of `self`, no need to look at more constructors. - if remaining_slices.is_empty() { - break; - } - } - - remaining_slices - .into_iter() - .map(|kind| Slice { array_len: slice.array_len, kind }) - .map(Slice) - .collect() - } - } - } - IntRange(self_range) => { - let mut remaining_ranges = vec![self_range.clone()]; - for other_ctor in other_ctors { - if let IntRange(other_range) = other_ctor { - if other_range == self_range { - // If the `self` range appears directly in a `match` arm, we can - // eliminate it straight away. - remaining_ranges = vec![]; - } else { - // Otherwise explicitly compute the remaining ranges. - remaining_ranges = other_range.subtract_from(remaining_ranges); - } - - // If the ranges that have been considered so far already cover the entire - // range of values, we can return early. - if remaining_ranges.is_empty() { - break; - } - } - } - - // Convert the ranges back into constructors. - remaining_ranges.into_iter().map(IntRange).collect() - } - // This constructor is never covered by anything else - NonExhaustive => vec![NonExhaustive], - // This constructor is only covered by `Single`s - Unlistable if other_ctors.iter().any(|c| *c == Single) => vec![], - Unlistable => vec![Unlistable], - Opaque => bug!("found unexpected opaque ctor in all_ctors"), - Wildcard => bug!("found unexpected wildcard ctor in all_ctors"), - } - } - /// Some constructors (namely Wildcard, IntRange and Slice) actually stand for a set of actual /// constructors (like variants, integers or fixed-sized slices). When specializing for these /// constructors, we want to be specialising for the actual underlying constructors. @@ -1003,13 +882,10 @@ impl<'tcx> Constructor<'tcx> { // current column. We only fully construct them on-demand, because they're rarely used and // can be big. let missing_ctors = MissingConstructors::new(pcx); - - if missing_ctors.is_empty() { + if missing_ctors.is_empty(pcx) { // All the constructors are present in the matrix, so we just go through them all. // We must also split them first. - // Since `all_ctors` never contains wildcards, this won't recurse more than once. - let (all_ctors, _) = missing_ctors.into_inner(); - all_ctors.into_iter().flat_map(|ctor| ctor.split(pcx, None)).collect() + missing_ctors.all_ctors } else { // Some constructors are missing, thus we can specialize with the wildcard constructor, // which will stand for those constructors that are missing, and behaves like any of @@ -1021,7 +897,7 @@ impl<'tcx> Constructor<'tcx> { /// Returns whether `self` is covered by `other`, ie whether `self` is a subset of `other`. For /// the simple cases, this is simply checking for equality. For the "grouped" constructors, /// this checks for inclusion. - fn is_covered_by<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>, other: &Constructor<'tcx>) -> bool { + fn is_covered_by<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>, other: &Self) -> bool { match (self, other) { // Wildcards cover anything (_, Wildcard) => true, @@ -1032,14 +908,7 @@ impl<'tcx> Constructor<'tcx> { (Variant(self_id), Variant(other_id)) => self_id == other_id, (IntRange(self_range), IntRange(other_range)) => { - if self_range.intersection(pcx.cx.tcx, other_range).is_some() { - // Constructor splitting should ensure that all intersections we encounter - // are actually inclusions. - assert!(self_range.is_subrange(other_range)); - true - } else { - false - } + self_range.is_covered_by(pcx, other_range) } ( FloatRange(self_from, self_to, self_end), @@ -1066,9 +935,7 @@ impl<'tcx> Constructor<'tcx> { } } - (Slice(self_slice), Slice(other_slice)) => { - other_slice.pattern_kind().covers_length(self_slice.arity()) - } + (Slice(self_slice), Slice(other_slice)) => self_slice.is_covered_by(*other_slice), // We are trying to inspect an opaque constant. Thus we skip the row. (Opaque, _) | (_, Opaque) => false, @@ -1084,6 +951,39 @@ impl<'tcx> Constructor<'tcx> { } } + /// Faster version of `is_covered_by` when applied to many constructors. `used_ctors` is + /// assumed to be built from `matrix.head_ctors()`, and `self` is assumed to have been split. + fn is_covered_by_any<'p>( + &self, + pcx: PatCtxt<'_, 'p, 'tcx>, + used_ctors: &[Constructor<'tcx>], + ) -> bool { + if used_ctors.is_empty() { + return false; + } + + match self { + // `used_ctors` cannot contain anything else than `Single`s. + Single => !used_ctors.is_empty(), + Variant(_) => used_ctors.iter().any(|c| c == self), + IntRange(range) => used_ctors + .iter() + .filter_map(|c| c.as_intrange()) + .any(|other| range.is_covered_by(pcx, other)), + Slice(slice) => used_ctors + .iter() + .filter_map(|c| c.as_slice()) + .any(|other| slice.is_covered_by(other)), + // This constructor is never covered by anything else + NonExhaustive => false, + // This constructor is only covered by `Single`s + Unlistable => used_ctors.iter().any(|c| *c == Single), + Str(..) | FloatRange(..) | Opaque | Wildcard => { + bug!("found unexpected ctor in all_ctors: {:?}", self) + } + } + } + /// Apply a constructor to a list of patterns, yielding a new pattern. `pats` /// must have as many elements as this constructor's arity. /// @@ -1129,7 +1029,7 @@ impl<'tcx> Constructor<'tcx> { ty::Slice(_) | ty::Array(..) => bug!("bad slice pattern {:?} {:?}", self, pcx.ty), _ => PatKind::Wild, }, - Slice(slice) => match slice.pattern_kind() { + Slice(slice) => match slice.kind() { FixedLen(_) => { PatKind::Slice { prefix: subpatterns.collect(), slice: None, suffix: vec![] } } @@ -1827,13 +1727,6 @@ impl<'tcx> IntRange<'tcx> { } } - fn from_ctor<'a>(ctor: &'a Constructor<'tcx>) -> Option<&'a IntRange<'tcx>> { - match ctor { - IntRange(range) => Some(range), - _ => None, - } - } - // The return value of `signed_bias` should be XORed with an endpoint to encode/decode it. fn signed_bias(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> u128 { match *ty.kind() { @@ -1845,35 +1738,6 @@ impl<'tcx> IntRange<'tcx> { } } - /// Returns a collection of ranges that spans the values covered by `ranges`, subtracted - /// by the values covered by `self`: i.e., `ranges \ self` (in set notation). - fn subtract_from(&self, ranges: Vec>) -> Vec> { - let mut remaining_ranges = vec![]; - let ty = self.ty; - let span = self.span; - let (lo, hi) = self.boundaries(); - for subrange in ranges { - let (subrange_lo, subrange_hi) = subrange.range.into_inner(); - if lo > subrange_hi || subrange_lo > hi { - // The pattern doesn't intersect with the subrange at all, - // so the subrange remains untouched. - remaining_ranges.push(IntRange { range: subrange_lo..=subrange_hi, ty, span }); - } else { - if lo > subrange_lo { - // The pattern intersects an upper section of the - // subrange, so a lower section will remain. - remaining_ranges.push(IntRange { range: subrange_lo..=(lo - 1), ty, span }); - } - if hi < subrange_hi { - // The pattern intersects a lower section of the - // subrange, so an upper section will remain. - remaining_ranges.push(IntRange { range: (hi + 1)..=subrange_hi, ty, span }); - } - } - } - remaining_ranges - } - fn is_subrange(&self, other: &Self) -> bool { other.range.start() <= self.range.start() && self.range.end() <= other.range.end() } @@ -2000,7 +1864,7 @@ impl<'tcx> IntRange<'tcx> { let row_borders = pcx .matrix .head_ctors(pcx.cx) - .filter_map(|ctor| IntRange::from_ctor(ctor)) + .filter_map(|ctor| ctor.as_intrange()) .filter_map(|range| { let intersection = self.intersection(pcx.cx.tcx, &range); let should_lint = self.suspicious_intersection(&range); @@ -2075,6 +1939,18 @@ impl<'tcx> IntRange<'tcx> { ); } } + + /// See `Constructor::is_covered_by` + fn is_covered_by<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>, other: &Self) -> bool { + if self.intersection(pcx.cx.tcx, other).is_some() { + // Constructor splitting should ensure that all intersections we encounter are actually + // inclusions. + assert!(self.is_subrange(other)); + true + } else { + false + } + } } /// Ignore spans when comparing, they don't carry semantic information as they are only for lints. @@ -2085,8 +1961,9 @@ impl<'tcx> std::cmp::PartialEq for IntRange<'tcx> { } // A struct to compute a set of constructors equivalent to `all_ctors \ used_ctors`. +#[derive(Debug)] struct MissingConstructors<'tcx> { - all_ctors: Vec>, + all_ctors: SmallVec<[Constructor<'tcx>; 1]>, used_ctors: Vec>, } @@ -2094,22 +1971,23 @@ impl<'tcx> MissingConstructors<'tcx> { fn new<'p>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Self { let used_ctors: Vec> = pcx.matrix.head_ctors(pcx.cx).cloned().filter(|c| !c.is_wildcard()).collect(); - let all_ctors = all_constructors(pcx); + // Since `all_ctors` never contains wildcards, this won't recurse further. + let all_ctors = + all_constructors(pcx).into_iter().flat_map(|ctor| ctor.split(pcx, None)).collect(); MissingConstructors { all_ctors, used_ctors } } - fn into_inner(self) -> (Vec>, Vec>) { - (self.all_ctors, self.used_ctors) - } - - fn is_empty(&self) -> bool { - self.iter().next().is_none() + fn is_empty<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>) -> bool { + self.iter(pcx).next().is_none() } /// Iterate over all_ctors \ used_ctors - fn iter<'a>(&'a self) -> impl Iterator> + Captures<'a> { - self.all_ctors.iter().flat_map(move |req_ctor| req_ctor.subtract_ctors(&self.used_ctors)) + fn iter<'a, 'p>( + &'a self, + pcx: PatCtxt<'a, 'p, 'tcx>, + ) -> impl Iterator> + Captures<'p> { + self.all_ctors.iter().filter(move |ctor| !ctor.is_covered_by_any(pcx, &self.used_ctors)) } /// List the patterns corresponding to the missing constructors. In some cases, instead of @@ -2156,7 +2034,7 @@ impl<'tcx> MissingConstructors<'tcx> { // constructor, that matches everything that can be built with // it. For example, if `ctor` is a `Constructor::Variant` for // `Option::Some`, we get the pattern `Some(_)`. - self.iter() + self.iter(pcx) .map(|missing_ctor| { let fields = Fields::wildcards(pcx, &missing_ctor); missing_ctor.apply(pcx, fields) @@ -2166,13 +2044,6 @@ impl<'tcx> MissingConstructors<'tcx> { } } -impl<'tcx> fmt::Debug for MissingConstructors<'tcx> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let ctors: Vec<_> = self.iter().collect(); - write!(f, "{:?}", ctors) - } -} - /// Algorithm from http://moscova.inria.fr/~maranget/papers/warn/index.html. /// The algorithm from the paper has been modified to correctly handle empty /// types. The changes are: From 766ab78a1cb243293eb84efd073186aa28724802 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Thu, 22 Oct 2020 10:45:24 +0100 Subject: [PATCH 15/16] Simplify slice splitting a bit --- .../src/thir/pattern/_match.rs | 69 ++++++++----------- 1 file changed, 28 insertions(+), 41 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 2e7b3626e8705..73f905647b94b 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -620,21 +620,17 @@ struct Slice { } impl Slice { - /// Returns what patterns this constructor covers: either fixed-length patterns or - /// variable-length patterns. - fn kind(self) -> SliceKind { - match self { - Slice { array_len: Some(len), kind: VarLen(prefix, suffix) } - if prefix + suffix == len => - { - FixedLen(len) - } - _ => self.kind, - } + 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), + _ => kind, + }; + Slice { array_len, kind } } fn arity(self) -> u64 { - self.kind().arity() + self.kind.arity() } /// The exhaustiveness-checking paper does not include any details on @@ -701,10 +697,8 @@ impl Slice { /// witness of length ≥2 (say, `[false, false, true]`) can be /// turned to a witness from any other length ≥2. fn split<'p, 'tcx>(self, pcx: PatCtxt<'_, 'p, 'tcx>) -> SmallVec<[Constructor<'tcx>; 1]> { - let (array_len, self_prefix, self_suffix) = match self { - Slice { array_len, kind: VarLen(self_prefix, self_suffix) } => { - (array_len, self_prefix, self_suffix) - } + let (self_prefix, self_suffix) = match self.kind { + VarLen(self_prefix, self_suffix) => (self_prefix, self_suffix), _ => return smallvec![Slice(self)], }; @@ -716,7 +710,7 @@ impl Slice { for ctor in head_ctors { if let Slice(slice) = ctor { - match slice.kind() { + match slice.kind { FixedLen(len) => { max_fixed_len = cmp::max(max_fixed_len, len); } @@ -725,6 +719,8 @@ impl Slice { max_suffix_len = cmp::max(max_suffix_len, suffix); } } + } else { + bug!("unexpected ctor for slice type: {:?}", ctor); } } @@ -738,27 +734,19 @@ impl Slice { max_prefix_len = max_fixed_len + 1 - max_suffix_len; } - match array_len { - Some(len) => { - let kind = if max_prefix_len + max_suffix_len < len { - VarLen(max_prefix_len, max_suffix_len) - } else { - FixedLen(len) - }; - smallvec![Slice(Slice { array_len, kind })] - } + let final_slice = VarLen(max_prefix_len, max_suffix_len); + let final_slice = Slice::new(self.array_len, final_slice); + match self.array_len { + Some(_) => smallvec![Slice(final_slice)], None => { - // `ctor` originally covered the range `(self_prefix + - // self_suffix..infinity)`. We now split it into two: lengths smaller than - // `max_prefix_len + max_suffix_len` are treated independently as - // fixed-lengths slices, and lengths above are captured by a final VarLen - // constructor. - let smaller_lengths = - (self_prefix + self_suffix..max_prefix_len + max_suffix_len).map(FixedLen); - let final_slice = VarLen(max_prefix_len, max_suffix_len); + // `self` originally covered the range `(self.arity()..infinity)`. We split that + // range into two: lengths smaller than `final_slice.arity()` are treated + // independently as fixed-lengths slices, and lengths above are captured by + // `final_slice`. + let smaller_lengths = (self.arity()..final_slice.arity()).map(FixedLen); smaller_lengths + .map(|kind| Slice::new(self.array_len, kind)) .chain(Some(final_slice)) - .map(|kind| Slice { array_len, kind }) .map(Slice) .collect() } @@ -767,7 +755,7 @@ impl Slice { /// See `Constructor::is_covered_by` fn is_covered_by(self, other: Self) -> bool { - other.kind().covers_length(self.arity()) + other.kind.covers_length(self.arity()) } } @@ -934,7 +922,6 @@ impl<'tcx> Constructor<'tcx> { None => false, } } - (Slice(self_slice), Slice(other_slice)) => self_slice.is_covered_by(*other_slice), // We are trying to inspect an opaque constant. Thus we skip the row. @@ -1029,7 +1016,7 @@ impl<'tcx> Constructor<'tcx> { ty::Slice(_) | ty::Array(..) => bug!("bad slice pattern {:?} {:?}", self, pcx.ty), _ => PatKind::Wild, }, - Slice(slice) => match slice.kind() { + Slice(slice) => match slice.kind { FixedLen(_) => { PatKind::Slice { prefix: subpatterns.collect(), slice: None, suffix: vec![] } } @@ -1533,13 +1520,13 @@ fn all_constructors<'p, 'tcx>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Vec { let kind = if cx.is_uninhabited(sub_ty) { FixedLen(0) } else { VarLen(0, 0) }; - vec![Slice(Slice { array_len: None, kind })] + vec![Slice(Slice::new(None, kind))] } ty::Adt(def, substs) if def.is_enum() => { // If the enum is declared as `#[non_exhaustive]`, we treat it as if it had an @@ -2224,7 +2211,7 @@ fn pat_constructor<'p, 'tcx>( let suffix = suffix.len() as u64; let kind = if slice.is_some() { VarLen(prefix, suffix) } else { FixedLen(prefix + suffix) }; - Slice(Slice { array_len, kind }) + Slice(Slice::new(array_len, kind)) } PatKind::Or { .. } => bug!("Or-pattern should have been expanded earlier on."), } From 41a74ace4aeba292ac524dccaa594c2c8aeb19c0 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Wed, 28 Oct 2020 19:03:49 +0000 Subject: [PATCH 16/16] Apply suggestions from code review Co-authored-by: Who? Me?! Co-authored-by: varkor --- .../src/thir/pattern/_match.rs | 43 +++++++++---------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/_match.rs b/compiler/rustc_mir_build/src/thir/pattern/_match.rs index 73f905647b94b..843a6c0e461f9 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/_match.rs @@ -203,7 +203,7 @@ //! before. //! That's almost correct, but only works if there were no wildcards in those first //! components. So we need to check that `p` is useful with respect to the rows that -//! start with a wildcard, if there are any. This is where `D` comes in: +//! start with a wildcard, if there are any. This is where `S(_, x)` comes in: //! `U(P, p) := U(S(_, P), S(_, p))` //! //! For example, if `P` is: @@ -634,8 +634,8 @@ impl Slice { } /// The exhaustiveness-checking paper does not include any details on - /// checking variable-length slice patterns. However, they are matched - /// by an infinite collection of fixed-length array patterns. + /// checking variable-length slice patterns. However, they may be + /// matched by an infinite collection of fixed-length array patterns. /// /// Checking the infinite set directly would take an infinite amount /// of time. However, it turns out that for each finite set of @@ -646,11 +646,11 @@ impl Slice { /// `sₘ` for each sufficiently-large length `m` that applies to exactly /// the same subset of `P`. /// - /// Because of that, each witness for reachability-checking from one + /// Because of that, each witness for reachability-checking of one /// of the sufficiently-large lengths can be transformed to an - /// equally-valid witness from any other length, so we only have - /// to check slice lengths from the "minimal sufficiently-large length" - /// and below. + /// equally-valid witness of any other length, so we only have + /// to check slices of the "minimal sufficiently-large length" + /// and less. /// /// Note that the fact that there is a *single* `sₘ` for each `m` /// not depending on the specific pattern in `P` is important: if @@ -659,8 +659,8 @@ impl Slice { /// `[.., false]` /// Then any slice of length ≥1 that matches one of these two /// patterns can be trivially turned to a slice of any - /// other length ≥1 that matches them and vice-versa - for - /// but the slice from length 2 `[false, true]` that matches neither + /// other length ≥1 that matches them and vice-versa, + /// but the slice of length 2 `[false, true]` that matches neither /// of these patterns can't be turned to a slice from length 1 that /// matches neither of these patterns, so we have to consider /// slices from length 2 there. @@ -787,7 +787,7 @@ enum Constructor<'tcx> { Opaque, /// Fake extra constructor for enums that aren't allowed to be matched exhaustively. NonExhaustive, - /// Fake constructor for those types for which we can't list constructors explicitely, like + /// Fake constructor for those types for which we can't list constructors explicitly, like /// `f64` and `&str`. Unlistable, /// Wildcard pattern. @@ -796,13 +796,10 @@ enum Constructor<'tcx> { impl<'tcx> Constructor<'tcx> { fn is_wildcard(&self) -> bool { - match self { - Wildcard => true, - _ => false, - } + matches!(self, Wildcard) } - fn as_intrange(&self) -> Option<&IntRange<'tcx>> { + fn as_int_range(&self) -> Option<&IntRange<'tcx>> { match self { IntRange(range) => Some(range), _ => None, @@ -827,7 +824,7 @@ impl<'tcx> Constructor<'tcx> { } } - /// Some constructors (namely Wildcard, IntRange and Slice) actually stand for a set of actual + /// Some constructors (namely `Wildcard`, `IntRange` and `Slice`) actually stand for a set of actual /// constructors (like variants, integers or fixed-sized slices). When specializing for these /// constructors, we want to be specialising for the actual underlying constructors. /// Naively, we would simply return the list of constructors they correspond to. We instead are @@ -863,8 +860,8 @@ impl<'tcx> Constructor<'tcx> { /// For wildcards, there are two groups of constructors: there are the constructors actually /// present in the matrix (`head_ctors`), and the constructors not present (`missing_ctors`). - /// Two constructors that are not in the matrix will either both be catched (by a wildcard), or - /// both not be catched. Therefore we can keep the missing constructors grouped together. + /// Two constructors that are not in the matrix will either both be caught (by a wildcard), or + /// both not be caught. Therefore we can keep the missing constructors grouped together. fn split_wildcard<'p>(pcx: PatCtxt<'_, 'p, 'tcx>) -> SmallVec<[Self; 1]> { // Missing constructors are those that are not matched by any non-wildcard patterns in the // current column. We only fully construct them on-demand, because they're rarely used and @@ -882,8 +879,8 @@ impl<'tcx> Constructor<'tcx> { } } - /// Returns whether `self` is covered by `other`, ie whether `self` is a subset of `other`. For - /// the simple cases, this is simply checking for equality. For the "grouped" constructors, + /// Returns whether `self` is covered by `other`, i.e. whether `self` is a subset of `other`. + /// For the simple cases, this is simply checking for equality. For the "grouped" constructors, /// this checks for inclusion. fn is_covered_by<'p>(&self, pcx: PatCtxt<'_, 'p, 'tcx>, other: &Self) -> bool { match (self, other) { @@ -955,7 +952,7 @@ impl<'tcx> Constructor<'tcx> { Variant(_) => used_ctors.iter().any(|c| c == self), IntRange(range) => used_ctors .iter() - .filter_map(|c| c.as_intrange()) + .filter_map(|c| c.as_int_range()) .any(|other| range.is_covered_by(pcx, other)), Slice(slice) => used_ctors .iter() @@ -1601,7 +1598,7 @@ fn all_constructors<'p, 'tcx>(pcx: PatCtxt<'_, 'p, 'tcx>) -> Vec vec![], ty::Adt(..) | ty::Tuple(..) => vec![Single], ty::Ref(_, t, _) if !t.is_str() => vec![Single], - // This type is one for which we don't know how to list constructors, like &str of f64. + // This type is one for which we don't know how to list constructors, like `&str` or `f64`. _ => vec![Unlistable], } } @@ -1851,7 +1848,7 @@ impl<'tcx> IntRange<'tcx> { let row_borders = pcx .matrix .head_ctors(pcx.cx) - .filter_map(|ctor| ctor.as_intrange()) + .filter_map(|ctor| ctor.as_int_range()) .filter_map(|range| { let intersection = self.intersection(pcx.cx.tcx, &range); let should_lint = self.suspicious_intersection(&range);