From 3c1a1c622776da5741f178658c611ccf0f75755f Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sat, 2 Jan 2021 22:38:18 +0000 Subject: [PATCH 1/9] Add tests --- .../exhaustiveness-unreachable-pattern.rs | 16 ++++++++ .../exhaustiveness-unreachable-pattern.stderr | 39 ++++++++++++++----- .../issue-80501-or-pat-and-macro.rs | 27 +++++++++++++ .../issue-80501-or-pat-and-macro.stderr | 18 +++++++++ 4 files changed, 90 insertions(+), 10 deletions(-) create mode 100644 src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.rs create mode 100644 src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.stderr diff --git a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs index 184ffa85c40e2..f51ba683cab2b 100644 --- a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs +++ b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs @@ -48,6 +48,22 @@ fn main() { (1 | 1,) => {} //~ ERROR unreachable _ => {} } + match 0 { + (0 | 1) | 1 => {} //~ ERROR unreachable + _ => {} + } + match 0 { + 0 | (0 | 0) => {} + //~^ ERROR unreachable + _ => {} + } + match None { + // There is only one error that correctly points to the whole subpattern + Some(0) | + Some( //~ ERROR unreachable + 0 | 0) => {} + _ => {} + } match [0; 2] { [0 | 0 //~ ERROR unreachable diff --git a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr index 8b1003b5514a6..f61e7f2281afc 100644 --- a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr +++ b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr @@ -77,58 +77,77 @@ LL | (1 | 1,) => {} | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:53:15 + --> $DIR/exhaustiveness-unreachable-pattern.rs:52:19 + | +LL | (0 | 1) | 1 => {} + | ^ + +error: unreachable pattern + --> $DIR/exhaustiveness-unreachable-pattern.rs:56:14 + | +LL | 0 | (0 | 0) => {} + | ^^^^^ + +error: unreachable pattern + --> $DIR/exhaustiveness-unreachable-pattern.rs:63:13 + | +LL | / Some( +LL | | 0 | 0) => {} + | |______________________^ + +error: unreachable pattern + --> $DIR/exhaustiveness-unreachable-pattern.rs:69:15 | LL | | 0 | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:55:15 + --> $DIR/exhaustiveness-unreachable-pattern.rs:71:15 | LL | | 0] => {} | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:63:10 + --> $DIR/exhaustiveness-unreachable-pattern.rs:79:10 | LL | [1 | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:75:10 + --> $DIR/exhaustiveness-unreachable-pattern.rs:91:10 | LL | [true | ^^^^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:82:36 + --> $DIR/exhaustiveness-unreachable-pattern.rs:98:36 | LL | (true | false, None | Some(true | ^^^^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:98:14 + --> $DIR/exhaustiveness-unreachable-pattern.rs:114:14 | LL | Some(0 | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:117:19 + --> $DIR/exhaustiveness-unreachable-pattern.rs:133:19 | LL | | false) => {} | ^^^^^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:125:15 + --> $DIR/exhaustiveness-unreachable-pattern.rs:141:15 | LL | | true) => {} | ^^^^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:131:15 + --> $DIR/exhaustiveness-unreachable-pattern.rs:147:15 | LL | | true, | ^^^^ -error: aborting due to 21 previous errors +error: aborting due to 24 previous errors diff --git a/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.rs b/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.rs new file mode 100644 index 0000000000000..483edcebd6254 --- /dev/null +++ b/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.rs @@ -0,0 +1,27 @@ +#![deny(unreachable_patterns)] +//~^ NOTE: lint level is defined here +pub enum TypeCtor { + Slice, + Array, +} + +pub struct ApplicationTy(TypeCtor); + +macro_rules! ty_app { + ($ctor:pat) => { + ApplicationTy($ctor) //~ ERROR unreachable pattern + }; +} + +fn _foo(ty: ApplicationTy) { + match ty { + ty_app!(TypeCtor::Array) | ty_app!(TypeCtor::Slice) => {} //~ NOTE: in this expansion + } + + // same as above, with the macro expanded + match ty { + ApplicationTy(TypeCtor::Array) | ApplicationTy(TypeCtor::Slice) => {} + } +} + +fn main() {} diff --git a/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.stderr b/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.stderr new file mode 100644 index 0000000000000..9d12b4a20e953 --- /dev/null +++ b/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.stderr @@ -0,0 +1,18 @@ +error: unreachable pattern + --> $DIR/issue-80501-or-pat-and-macro.rs:12:9 + | +LL | ApplicationTy($ctor) + | ^^^^^^^^^^^^^^^^^^^^ +... +LL | ty_app!(TypeCtor::Array) | ty_app!(TypeCtor::Slice) => {} + | ------------------------ in this macro invocation + | +note: the lint level is defined here + --> $DIR/issue-80501-or-pat-and-macro.rs:1:9 + | +LL | #![deny(unreachable_patterns)] + | ^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to previous error + From 3a4c135a2f3b4a79319bb21405ebd166b1f38a29 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Thu, 31 Dec 2020 18:48:08 +0000 Subject: [PATCH 2/9] Improve the debugging experience --- .../src/thir/pattern/deconstruct_pat.rs | 2 - .../src/thir/pattern/usefulness.rs | 53 +++++++++++-------- 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs b/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs index db2fa5730a338..223767afde14e 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs @@ -724,8 +724,6 @@ impl<'tcx> Constructor<'tcx> { where 'tcx: 'a, { - debug!("Constructor::split({:#?})", self); - match self { Wildcard => { let mut split_wildcard = SplitWildcard::new(pcx); diff --git a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs index d7c08a2d1af6b..d1c9c3ea6bfba 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs @@ -344,6 +344,12 @@ pub(super) struct PatCtxt<'a, 'p, 'tcx> { pub(super) is_top_level: bool, } +impl<'a, 'p, 'tcx> fmt::Debug for PatCtxt<'a, 'p, 'tcx> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("PatCtxt").field("ty", &self.ty).finish() + } +} + crate fn expand_pattern<'tcx>(pat: Pat<'tcx>) -> Pat<'tcx> { LiteralExpander.fold_pattern(&pat) } @@ -383,7 +389,7 @@ 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)] +#[derive(Clone)] struct PatStack<'p, 'tcx> { pats: SmallVec<[&'p Pat<'tcx>; 2]>, /// Cache for the constructor of the head @@ -475,6 +481,17 @@ impl<'p, 'tcx> FromIterator<&'p Pat<'tcx>> for PatStack<'p, 'tcx> { } } +/// Pretty-printing for matrix row. +impl<'p, 'tcx> fmt::Debug for PatStack<'p, 'tcx> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "+")?; + for pat in self.iter() { + write!(f, " {} +", pat)?; + } + Ok(()) + } +} + /// A 2D matrix. #[derive(Clone, PartialEq)] pub(super) struct Matrix<'p, 'tcx> { @@ -543,17 +560,11 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { /// Pretty-printer for matrices of patterns, example: /// /// ```text -/// +++++++++++++++++++++++++++++ /// + _ + [] + -/// +++++++++++++++++++++++++++++ /// + true + [First] + -/// +++++++++++++++++++++++++++++ /// + true + [Second(true)] + -/// +++++++++++++++++++++++++++++ /// + false + [_] + -/// +++++++++++++++++++++++++++++ /// + _ + [_, _, tail @ ..] + -/// +++++++++++++++++++++++++++++ /// ``` impl<'p, 'tcx> fmt::Debug for Matrix<'p, 'tcx> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -561,17 +572,14 @@ impl<'p, 'tcx> fmt::Debug for Matrix<'p, 'tcx> { let Matrix { patterns: m, .. } = self; let pretty_printed_matrix: Vec> = - m.iter().map(|row| row.iter().map(|pat| format!("{:?}", pat)).collect()).collect(); + m.iter().map(|row| row.iter().map(|pat| format!("{}", pat)).collect()).collect(); - let column_count = m.iter().map(|row| row.len()).max().unwrap_or(0); + let column_count = m.iter().map(|row| row.len()).next().unwrap_or(0); assert!(m.iter().all(|row| row.len() == column_count)); let column_widths: Vec = (0..column_count) .map(|col| pretty_printed_matrix.iter().map(|row| row[col].len()).max().unwrap_or(0)) .collect(); - let total_width = column_widths.iter().cloned().sum::() + column_count * 3 + 1; - let br = "+".repeat(total_width); - write!(f, "{}\n", br)?; for row in pretty_printed_matrix { write!(f, "+")?; for (column, pat_str) in row.into_iter().enumerate() { @@ -580,7 +588,6 @@ impl<'p, 'tcx> fmt::Debug for Matrix<'p, 'tcx> { write!(f, " +")?; } write!(f, "\n")?; - write!(f, "{}\n", br)?; } Ok(()) } @@ -924,6 +931,7 @@ impl<'tcx> Witness<'tcx> { /// `is_under_guard` is used to inform if the pattern has a guard. If it /// has one it must not be inserted into the matrix. This shouldn't be /// relied on for soundness. +#[instrument(skip(cx, matrix, witness_preference, hir_id, is_under_guard, is_top_level))] fn is_useful<'p, 'tcx>( cx: &MatchCheckCtxt<'p, 'tcx>, matrix: &Matrix<'p, 'tcx>, @@ -933,8 +941,8 @@ fn is_useful<'p, 'tcx>( is_under_guard: bool, is_top_level: bool, ) -> Usefulness<'tcx> { + debug!("matrix,v={:?}{:?}", matrix, v); let Matrix { patterns: rows, .. } = matrix; - debug!("is_useful({:#?}, {:#?})", matrix, v); // The base case. We are pattern-matching on () and the return value is // based on whether our matrix has a row or not. @@ -942,12 +950,11 @@ fn is_useful<'p, 'tcx>( // first and then, if v is non-empty, the return value is based on whether // the type of the tuple we're checking is inhabited or not. if v.is_empty() { - return if rows.is_empty() { - Usefulness::new_useful(witness_preference) - } else { - NotUseful - }; - }; + let ret = + if rows.is_empty() { Usefulness::new_useful(witness_preference) } else { NotUseful }; + debug!(?ret); + return ret; + } assert!(rows.iter().all(|r| r.len() == v.len())); @@ -955,10 +962,9 @@ fn is_useful<'p, 'tcx>( let ty = matrix.heads().next().map_or(v.head().ty, |r| r.ty); let pcx = PatCtxt { cx, ty, span: v.head().span, is_top_level }; - debug!("is_useful_expand_first_col: ty={:#?}, expanding {:#?}", pcx.ty, v.head()); - // If the first pattern is an or-pattern, expand it. let ret = if let Some(vs) = v.expand_or_pat() { + debug!("expanding or-pattern"); let subspans: Vec<_> = vs.iter().map(|v| v.head().span).collect(); // We expand the or pattern, trying each of its branches in turn and keeping careful track // of possible unreachable sub-branches. @@ -993,6 +999,7 @@ fn is_useful<'p, 'tcx>( // witness the usefulness of `v`. let start_matrix = &matrix; let usefulnesses = split_ctors.into_iter().map(|ctor| { + debug!("specialize({:?})", ctor); // We cache the result of `Fields::wildcards` because it is used a lot. let ctor_wild_subpatterns = Fields::wildcards(pcx, &ctor); let spec_matrix = @@ -1004,7 +1011,7 @@ fn is_useful<'p, 'tcx>( }); Usefulness::merge(usefulnesses) }; - debug!("is_useful::returns({:#?}, {:#?}) = {:?}", matrix, v, ret); + debug!(?ret); ret } From 5547105f6b030c3cfb42850213d0ad94726937a8 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Tue, 22 Dec 2020 11:21:34 +0000 Subject: [PATCH 3/9] Don't expose `Usefulness` in the api --- .../src/thir/pattern/check_match.rs | 12 ++++++------ .../src/thir/pattern/usefulness.rs | 19 ++++++++++++++++--- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs b/compiler/rustc_mir_build/src/thir/pattern/check_match.rs index 397706851cb79..47456f469f104 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/check_match.rs @@ -1,6 +1,6 @@ -use super::usefulness::Usefulness::*; use super::usefulness::{ - compute_match_usefulness, expand_pattern, MatchArm, MatchCheckCtxt, UsefulnessReport, + compute_match_usefulness, expand_pattern, MatchArm, MatchCheckCtxt, Reachability, + UsefulnessReport, }; use super::{PatCtxt, PatKind, PatternError}; @@ -398,10 +398,11 @@ fn report_arm_reachability<'p, 'tcx>( report: &UsefulnessReport<'p, 'tcx>, source: hir::MatchSource, ) { + use Reachability::*; let mut catchall = None; for (arm_index, (arm, is_useful)) in report.arm_usefulness.iter().enumerate() { match is_useful { - NotUseful => { + Unreachable => { match source { hir::MatchSource::WhileDesugar => bug!(), @@ -430,9 +431,9 @@ fn report_arm_reachability<'p, 'tcx>( hir::MatchSource::AwaitDesugar | hir::MatchSource::TryDesugar => {} } } - Useful(unreachables) if unreachables.is_empty() => {} + Reachable(unreachables) if unreachables.is_empty() => {} // The arm is reachable, but contains unreachable subpatterns (from or-patterns). - Useful(unreachables) => { + Reachable(unreachables) => { let mut unreachables: Vec<_> = unreachables.iter().collect(); // Emit lints in the order in which they occur in the file. unreachables.sort_unstable(); @@ -440,7 +441,6 @@ fn report_arm_reachability<'p, 'tcx>( unreachable_pattern(cx.tcx, span, arm.hir_id, None); } } - UsefulWithWitness(_) => bug!(), } if !arm.has_guard && catchall.is_none() && pat_is_catchall(arm.pat) { catchall = Some(arm.pat.span); diff --git a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs index d1c9c3ea6bfba..d8195eee4b817 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs @@ -679,7 +679,7 @@ impl SpanSet { } #[derive(Clone, Debug)] -crate enum Usefulness<'tcx> { +enum Usefulness<'tcx> { /// Pontentially carries a set of sub-branches that have been found to be unreachable. Used /// only in the presence of or-patterns, otherwise it stays empty. Useful(SpanSet), @@ -1024,10 +1024,18 @@ crate struct MatchArm<'p, 'tcx> { crate has_guard: bool, } +#[derive(Clone, Debug)] +crate enum Reachability { + /// Potentially carries a set of sub-branches that have been found to be unreachable. Used only + /// in the presence of or-patterns, otherwise it stays empty. + Reachable(SpanSet), + Unreachable, +} + /// The output of checking a match for exhaustiveness and arm reachability. crate struct UsefulnessReport<'p, 'tcx> { /// For each arm of the input, whether that arm is reachable after the arms above it. - crate arm_usefulness: Vec<(MatchArm<'p, 'tcx>, Usefulness<'tcx>)>, + crate arm_usefulness: Vec<(MatchArm<'p, 'tcx>, Reachability)>, /// If the match is exhaustive, this is empty. If not, this contains witnesses for the lack of /// exhaustiveness. crate non_exhaustiveness_witnesses: Vec>, @@ -1055,7 +1063,12 @@ crate fn compute_match_usefulness<'p, 'tcx>( if !arm.has_guard { matrix.push(v); } - (arm, usefulness) + let reachability = match usefulness { + Useful(spans) => Reachability::Reachable(spans), + NotUseful => Reachability::Unreachable, + UsefulWithWitness(..) => bug!(), + }; + (arm, reachability) }) .collect(); From f4f20c0663b686f76c5722fde481c83acf2dd1e9 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 21 Dec 2020 04:51:46 +0000 Subject: [PATCH 4/9] Reimplement `Usefulness::merge` in terms of a binop --- .../src/thir/pattern/usefulness.rs | 63 +++++++------------ 1 file changed, 24 insertions(+), 39 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs index d8195eee4b817..bb8213a98ac20 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs @@ -696,9 +696,9 @@ impl<'tcx> Usefulness<'tcx> { } } - /// When trying several branches and each returns a `Usefulness`, we need to combine the - /// results together. - fn merge(usefulnesses: impl Iterator) -> Self { + /// Combine usefulnesses from two branches. This is an associative operation and `NotUseful` is + /// a unit. + fn extend(&mut self, other: Self) { // If we have detected some unreachable sub-branches, we only want to keep them when they // were unreachable in _all_ branches. Eg. in the following, the last `true` is unreachable // in the second branch of the first or-pattern, but not otherwise. Therefore we don't want @@ -709,54 +709,39 @@ impl<'tcx> Usefulness<'tcx> { // (false | true, false | true) => {} // } // ``` - // Here however we _do_ want to lint that the last `false` is unreachable. So we don't want - // to intersect the spans that come directly from the or-pattern, since each branch of the - // or-pattern brings a new disjoint pattern. + // Here however we _do_ want to lint that the last `false` is unreachable. In order to + // handle that correctly, each branch of an or-pattern marks the other branches as + // unreachable (see `unsplit_or_pat`). That way, intersecting the results will correctly + // identify unreachable sub-patterns. // ``` // match None { // Some(false) => {} // None | Some(true | false) => {} // } // ``` + match (&mut *self, other) { + (Useful(s), Useful(o)) => s.intersection_mut(&o), + (UsefulWithWitness(s), UsefulWithWitness(o)) => s.extend(o), + (_, NotUseful) => {} + (NotUseful, other) => *self = other, + (UsefulWithWitness(_), Useful(_)) | (Useful(_), UsefulWithWitness(_)) => unreachable!(), + } + } - // Is `None` when no branch was useful. Will often be `Some(Spanset::new())` because the - // sets are only non-empty in the presence of or-patterns. - let mut unreachables: Option = None; - // Witnesses of usefulness, if any. - let mut witnesses = Vec::new(); - + /// When trying several branches and each returns a `Usefulness`, we need to combine the + /// results together. + fn merge(usefulnesses: impl Iterator) -> Self { + let mut ret = NotUseful; for u in usefulnesses { - match u { - Useful(spans) if spans.is_empty() => { + ret.extend(u); + if let Useful(spans) = &ret { + if spans.is_empty() { // Once we reach the empty set, more intersections won't change the result. - return Useful(SpanSet::new()); - } - Useful(spans) => { - if let Some(unreachables) = &mut unreachables { - if !unreachables.is_empty() { - unreachables.intersection_mut(&spans); - } - if unreachables.is_empty() { - return Useful(SpanSet::new()); - } - } else { - unreachables = Some(spans); - } - } - NotUseful => {} - UsefulWithWitness(wits) => { - witnesses.extend(wits); + return ret; } } } - - if !witnesses.is_empty() { - UsefulWithWitness(witnesses) - } else if let Some(unreachables) = unreachables { - Useful(unreachables) - } else { - NotUseful - } + ret } /// After calculating the usefulness for a branch of an or-pattern, call this to make this From 293af417903b1a718795e60a1b6de0dea8fc4af0 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Fri, 1 Jan 2021 21:28:32 +0000 Subject: [PATCH 5/9] Split `Usefulness::NotUseful` into two --- .../src/thir/pattern/usefulness.rs | 84 ++++++++++++------- 1 file changed, 52 insertions(+), 32 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs index bb8213a98ac20..6f94edb9932b0 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs @@ -680,24 +680,32 @@ impl SpanSet { #[derive(Clone, Debug)] enum Usefulness<'tcx> { - /// Pontentially carries a set of sub-branches that have been found to be unreachable. Used + /// Potentially carries a set of sub-branches that have been found to be unreachable. Used /// only in the presence of or-patterns, otherwise it stays empty. - Useful(SpanSet), - /// Carries a list of witnesses of non-exhaustiveness. - UsefulWithWitness(Vec>), - NotUseful, + NoWitnesses(SpanSet), + /// When not carrying witnesses, indicates that the whole pattern is unreachable. + NoWitnessesFull, + /// Carries a list of witnesses of non-exhaustiveness. Non-empty. + WithWitnesses(Vec>), + /// When carrying witnesses, indicates that the whole pattern is unreachable. + WithWitnessesEmpty, } impl<'tcx> Usefulness<'tcx> { fn new_useful(preference: WitnessPreference) -> Self { match preference { - ConstructWitness => UsefulWithWitness(vec![Witness(vec![])]), - LeaveOutWitness => Useful(Default::default()), + ConstructWitness => WithWitnesses(vec![Witness(vec![])]), + LeaveOutWitness => NoWitnesses(Default::default()), + } + } + fn new_not_useful(preference: WitnessPreference) -> Self { + match preference { + ConstructWitness => WithWitnessesEmpty, + LeaveOutWitness => NoWitnessesFull, } } - /// Combine usefulnesses from two branches. This is an associative operation and `NotUseful` is - /// a unit. + /// Combine usefulnesses from two branches. This is an associative operation. fn extend(&mut self, other: Self) { // If we have detected some unreachable sub-branches, we only want to keep them when they // were unreachable in _all_ branches. Eg. in the following, the last `true` is unreachable @@ -720,21 +728,29 @@ impl<'tcx> Usefulness<'tcx> { // } // ``` match (&mut *self, other) { - (Useful(s), Useful(o)) => s.intersection_mut(&o), - (UsefulWithWitness(s), UsefulWithWitness(o)) => s.extend(o), - (_, NotUseful) => {} - (NotUseful, other) => *self = other, - (UsefulWithWitness(_), Useful(_)) | (Useful(_), UsefulWithWitness(_)) => unreachable!(), + (WithWitnesses(s), WithWitnesses(o)) => s.extend(o), + (WithWitnessesEmpty, WithWitnesses(o)) => *self = WithWitnesses(o), + (WithWitnesses(_), WithWitnessesEmpty) => {} + (WithWitnessesEmpty, WithWitnessesEmpty) => {} + + (NoWitnesses(s), NoWitnesses(o)) => s.intersection_mut(&o), + (NoWitnessesFull, NoWitnesses(o)) => *self = NoWitnesses(o), + (NoWitnesses(_), NoWitnessesFull) => {} + (NoWitnessesFull, NoWitnessesFull) => {} + + _ => { + unreachable!() + } } } /// When trying several branches and each returns a `Usefulness`, we need to combine the /// results together. - fn merge(usefulnesses: impl Iterator) -> Self { - let mut ret = NotUseful; + fn merge(pref: WitnessPreference, usefulnesses: impl Iterator) -> Self { + let mut ret = Self::new_not_useful(pref); for u in usefulnesses { ret.extend(u); - if let Useful(spans) = &ret { + if let NoWitnesses(spans) = &ret { if spans.is_empty() { // Once we reach the empty set, more intersections won't change the result. return ret; @@ -748,7 +764,7 @@ impl<'tcx> Usefulness<'tcx> { /// usefulness mergeable with those from the other branches. fn unsplit_or_pat(self, this_span: Span, or_pat_spans: &[Span]) -> Self { match self { - Useful(mut spans) => { + NoWitnesses(mut spans) => { // We register the spans of the other branches of this or-pattern as being // unreachable from this one. This ensures that intersecting together the sets of // spans returns what we want. @@ -759,9 +775,10 @@ impl<'tcx> Usefulness<'tcx> { spans.push_nonintersecting(span); } } - Useful(spans) + NoWitnesses(spans) } - x => x, + NoWitnessesFull => NoWitnessesFull, + WithWitnesses(_) | WithWitnessesEmpty => bug!(), } } @@ -776,7 +793,7 @@ impl<'tcx> Usefulness<'tcx> { ctor_wild_subpatterns: &Fields<'p, 'tcx>, ) -> Self { match self { - UsefulWithWitness(witnesses) => { + WithWitnesses(witnesses) => { let new_witnesses = if matches!(ctor, Constructor::Missing) { let mut split_wildcard = SplitWildcard::new(pcx); split_wildcard.split(pcx, matrix.head_ctors(pcx.cx)); @@ -806,7 +823,7 @@ impl<'tcx> Usefulness<'tcx> { .map(|witness| witness.apply_constructor(pcx, &ctor, ctor_wild_subpatterns)) .collect() }; - UsefulWithWitness(new_witnesses) + WithWitnesses(new_witnesses) } x => x, } @@ -935,8 +952,11 @@ fn is_useful<'p, 'tcx>( // first and then, if v is non-empty, the return value is based on whether // the type of the tuple we're checking is inhabited or not. if v.is_empty() { - let ret = - if rows.is_empty() { Usefulness::new_useful(witness_preference) } else { NotUseful }; + let ret = if rows.is_empty() { + Usefulness::new_useful(witness_preference) + } else { + Usefulness::new_not_useful(witness_preference) + }; debug!(?ret); return ret; } @@ -966,7 +986,7 @@ fn is_useful<'p, 'tcx>( } usefulness.unsplit_or_pat(v_span, &subspans) }); - Usefulness::merge(usefulnesses) + Usefulness::merge(witness_preference, usefulnesses) } else { let v_ctor = v.head_ctor(cx); if let Constructor::IntRange(ctor_range) = &v_ctor { @@ -994,7 +1014,7 @@ fn is_useful<'p, 'tcx>( is_useful(cx, &spec_matrix, &v, witness_preference, hir_id, is_under_guard, false); usefulness.apply_constructor(pcx, start_matrix, &ctor, &ctor_wild_subpatterns) }); - Usefulness::merge(usefulnesses) + Usefulness::merge(witness_preference, usefulnesses) }; debug!(?ret); ret @@ -1049,9 +1069,9 @@ crate fn compute_match_usefulness<'p, 'tcx>( matrix.push(v); } let reachability = match usefulness { - Useful(spans) => Reachability::Reachable(spans), - NotUseful => Reachability::Unreachable, - UsefulWithWitness(..) => bug!(), + NoWitnesses(spans) => Reachability::Reachable(spans), + NoWitnessesFull => Reachability::Unreachable, + WithWitnesses(..) | WithWitnessesEmpty => bug!(), }; (arm, reachability) }) @@ -1061,15 +1081,15 @@ crate fn compute_match_usefulness<'p, 'tcx>( let v = PatStack::from_pattern(wild_pattern); let usefulness = is_useful(cx, &matrix, &v, ConstructWitness, scrut_hir_id, false, true); let non_exhaustiveness_witnesses = match usefulness { - NotUseful => vec![], // Wildcard pattern isn't useful, so the match is exhaustive. - UsefulWithWitness(pats) => { + WithWitnessesEmpty => vec![], // Wildcard pattern isn't useful, so the match is exhaustive. + WithWitnesses(pats) => { if pats.is_empty() { bug!("Exhaustiveness check returned no witnesses") } else { pats.into_iter().map(|w| w.single_pattern()).collect() } } - Useful(_) => bug!(), + NoWitnesses(_) | NoWitnessesFull => bug!(), }; UsefulnessReport { arm_usefulness, non_exhaustiveness_witnesses } } From 0162d603b335e7f08b86cdddbb51197202a5a79a Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Fri, 1 Jan 2021 22:14:22 +0000 Subject: [PATCH 6/9] Factor or-pattern expansion --- .../src/thir/pattern/usefulness.rs | 60 +++++++++++-------- .../exhaustiveness-unreachable-pattern.rs | 3 + .../exhaustiveness-unreachable-pattern.stderr | 32 ++++++---- 3 files changed, 58 insertions(+), 37 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs index 6f94edb9932b0..d31b5f104e5a9 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs @@ -385,6 +385,27 @@ impl<'tcx> Pat<'tcx> { pub(super) fn is_wildcard(&self) -> bool { matches!(*self.kind, PatKind::Binding { subpattern: None, .. } | PatKind::Wild) } + + fn is_or_pat(&self) -> bool { + matches!(*self.kind, PatKind::Or { .. }) + } + + /// Recursively expand this pattern into its subpatterns. Only useful for or-patterns. + fn expand_or_pat(&self) -> Vec<&Self> { + fn expand<'p, 'tcx>(pat: &'p Pat<'tcx>, vec: &mut Vec<&'p Pat<'tcx>>) { + if let PatKind::Or { pats } = pat.kind.as_ref() { + for pat in pats { + expand(pat, vec); + } + } else { + vec.push(pat) + } + } + + let mut pats = Vec::new(); + expand(self, &mut pats); + pats + } } /// A row of a matrix. Rows of len 1 are very common, which is why `SmallVec[_; 2]` @@ -425,23 +446,14 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> { self.pats.iter().copied() } - // If the first pattern is an or-pattern, expand this pattern. Otherwise, return `None`. - fn expand_or_pat(&self) -> Option> { - if self.is_empty() { - None - } else if let PatKind::Or { pats } = &*self.head().kind { - Some( - pats.iter() - .map(|pat| { - let mut new_patstack = PatStack::from_pattern(pat); - new_patstack.pats.extend_from_slice(&self.pats[1..]); - new_patstack - }) - .collect(), - ) - } else { - None - } + // Recursively expand the first pattern into its subpatterns. Only useful if the pattern is an + // or-pattern. Panics if `self` is empty. + fn expand_or_pat<'a>(&'a self) -> impl Iterator> + Captures<'a> { + self.head().expand_or_pat().into_iter().map(move |pat| { + let mut new_patstack = PatStack::from_pattern(pat); + new_patstack.pats.extend_from_slice(&self.pats[1..]); + new_patstack + }) } /// This computes `S(self.head_ctor(), self)`. See top of the file for explanations. @@ -508,13 +520,12 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> { self.patterns.get(0).map(|r| r.len()) } - /// Pushes a new row to the matrix. If the row starts with an or-pattern, this expands it. + /// Pushes a new row to the matrix. If the row starts with an or-pattern, this recursively + /// expands it. fn push(&mut self, row: PatStack<'p, 'tcx>) { - if let Some(rows) = row.expand_or_pat() { - for row in rows { - // We recursively expand the or-patterns of the new rows. - // This is necessary as we might have `0 | (1 | 2)` or e.g., `x @ 0 | x @ (1 | 2)`. - self.push(row) + if !row.is_empty() && row.head().is_or_pat() { + for row in row.expand_or_pat() { + self.patterns.push(row); } } else { self.patterns.push(row); @@ -968,8 +979,9 @@ fn is_useful<'p, 'tcx>( let pcx = PatCtxt { cx, ty, span: v.head().span, is_top_level }; // If the first pattern is an or-pattern, expand it. - let ret = if let Some(vs) = v.expand_or_pat() { + let ret = if v.head().is_or_pat() { debug!("expanding or-pattern"); + let vs: Vec<_> = v.expand_or_pat().collect(); let subspans: Vec<_> = vs.iter().map(|v| v.head().span).collect(); // We expand the or pattern, trying each of its branches in turn and keeping careful track // of possible unreachable sub-branches. diff --git a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs index f51ba683cab2b..9718396ed4822 100644 --- a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs +++ b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs @@ -53,8 +53,11 @@ fn main() { _ => {} } match 0 { + // We get two errors because recursive or-pattern expansion means we don't notice the two + // errors span a whole pattern. This could be better but doesn't matter much 0 | (0 | 0) => {} //~^ ERROR unreachable + //~| ERROR unreachable _ => {} } match None { diff --git a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr index f61e7f2281afc..901a71885eb86 100644 --- a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr +++ b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr @@ -83,71 +83,77 @@ LL | (0 | 1) | 1 => {} | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:56:14 + --> $DIR/exhaustiveness-unreachable-pattern.rs:58:14 | LL | 0 | (0 | 0) => {} - | ^^^^^ + | ^ + +error: unreachable pattern + --> $DIR/exhaustiveness-unreachable-pattern.rs:58:18 + | +LL | 0 | (0 | 0) => {} + | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:63:13 + --> $DIR/exhaustiveness-unreachable-pattern.rs:66:13 | LL | / Some( LL | | 0 | 0) => {} | |______________________^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:69:15 + --> $DIR/exhaustiveness-unreachable-pattern.rs:72:15 | LL | | 0 | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:71:15 + --> $DIR/exhaustiveness-unreachable-pattern.rs:74:15 | LL | | 0] => {} | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:79:10 + --> $DIR/exhaustiveness-unreachable-pattern.rs:82:10 | LL | [1 | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:91:10 + --> $DIR/exhaustiveness-unreachable-pattern.rs:94:10 | LL | [true | ^^^^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:98:36 + --> $DIR/exhaustiveness-unreachable-pattern.rs:101:36 | LL | (true | false, None | Some(true | ^^^^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:114:14 + --> $DIR/exhaustiveness-unreachable-pattern.rs:117:14 | LL | Some(0 | ^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:133:19 + --> $DIR/exhaustiveness-unreachable-pattern.rs:136:19 | LL | | false) => {} | ^^^^^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:141:15 + --> $DIR/exhaustiveness-unreachable-pattern.rs:144:15 | LL | | true) => {} | ^^^^ error: unreachable pattern - --> $DIR/exhaustiveness-unreachable-pattern.rs:147:15 + --> $DIR/exhaustiveness-unreachable-pattern.rs:150:15 | LL | | true, | ^^^^ -error: aborting due to 24 previous errors +error: aborting due to 25 previous errors From 307a278d5c7afec2e329f5143022a352191a082d Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Fri, 1 Jan 2021 22:14:35 +0000 Subject: [PATCH 7/9] Identify subpatterns by the path to them instead of spans --- .../src/thir/pattern/check_match.rs | 2 +- .../src/thir/pattern/usefulness.rs | 341 ++++++++++++------ .../exhaustiveness-unreachable-pattern.rs | 4 +- .../exhaustiveness-unreachable-pattern.stderr | 13 +- .../issue-80501-or-pat-and-macro.rs | 6 +- .../issue-80501-or-pat-and-macro.stderr | 18 - 6 files changed, 252 insertions(+), 132 deletions(-) delete mode 100644 src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.stderr diff --git a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs b/compiler/rustc_mir_build/src/thir/pattern/check_match.rs index 47456f469f104..6ec602ff59b9c 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/check_match.rs @@ -434,7 +434,7 @@ fn report_arm_reachability<'p, 'tcx>( Reachable(unreachables) if unreachables.is_empty() => {} // The arm is reachable, but contains unreachable subpatterns (from or-patterns). Reachable(unreachables) => { - let mut unreachables: Vec<_> = unreachables.iter().collect(); + let mut unreachables = unreachables.clone(); // Emit lints in the order in which they occur in the file. unreachables.sort_unstable(); for span in unreachables { diff --git a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs index d31b5f104e5a9..3c3eb801f94ea 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs @@ -288,6 +288,7 @@ use super::{Pat, PatKind}; use super::{PatternFoldable, PatternFolder}; use rustc_data_structures::captures::Captures; +use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::sync::OnceCell; use rustc_arena::TypedArena; @@ -618,82 +619,236 @@ impl<'p, 'tcx> FromIterator> for Matrix<'p, 'tcx> { } } -/// Represents a set of `Span`s closed under the containment relation. That is, if a `Span` is -/// contained in the set then all `Span`s contained in it are also implicitly contained in the set. -/// In particular this means that when intersecting two sets, taking the intersection of some span -/// and one of its subspans returns the subspan, whereas a simple `HashSet` would have returned an -/// empty intersection. -/// It is assumed that two spans don't overlap without one being contained in the other; in other -/// words, that the inclusion structure forms a tree and not a DAG. -/// Intersection is not very efficient. It compares everything pairwise. If needed it could be made -/// faster by sorting the `Span`s and merging cleverly. -#[derive(Debug, Clone, Default)] -pub(crate) struct SpanSet { - /// The minimal set of `Span`s required to represent the whole set. If A and B are `Span`s in - /// the `SpanSet`, and A is a descendant of B, then only B will be in `root_spans`. - /// Invariant: the spans are disjoint. - root_spans: Vec, +/// Given a pattern or a pattern-stack, this struct captures a set of its subpattern branches. We +/// use that to track unreachable sub-patterns arising from or-patterns. In the absence of +/// or-patterns this will always be either `Empty` or `Full`. +/// We support a limited set of operations, so not all possible sets of subpatterns can be +/// represented. That's ok, we only want the ones that make sense to capture unreachable +/// subpatterns. +/// What we're trying to do is illustrated by this: +/// ``` +/// match (true, true) { +/// (true, true) => {} +/// (true | false, true | false) => {} +/// } +/// ``` +/// When we try the alternatives of the first or-pattern, the last `true` is unreachable in the +/// first alternative but no the other. So we don't want to report it as unreachable. Therefore we +/// intersect sets of unreachable patterns coming from different alternatives in order to figure +/// out which subpatterns are overall unreachable. +#[derive(Debug, Clone)] +enum SubPatSet<'p, 'tcx> { + /// The set containing the full pattern. + Full, + /// The empty set. + Empty, + /// If the pattern is a pattern with a constructor or a pattern-stack, we store a set for each + /// of its subpatterns. Missing entries in the map are implicitly empty. + Seq { subpats: FxHashMap> }, + /// If the pattern is an or-pattern, we store a set for each of its alternatives. Missing + /// entries in the map are implicitly full. Note: we always flatten nested or-patterns. + Alt { + subpats: FxHashMap>, + /// Counts the total number of alternatives in the pattern + alt_count: usize, + /// We keep the pattern around to retrieve spans. + pat: &'p Pat<'tcx>, + }, } -impl SpanSet { - /// Creates an empty set. - fn new() -> Self { - Self::default() - } - - /// Tests whether the set is empty. - pub(crate) fn is_empty(&self) -> bool { - self.root_spans.is_empty() +impl<'p, 'tcx> SubPatSet<'p, 'tcx> { + fn empty() -> Self { + SubPatSet::Empty } - - /// Iterate over the disjoint list of spans at the roots of this set. - pub(crate) fn iter<'a>(&'a self) -> impl Iterator + Captures<'a> { - self.root_spans.iter().copied() + fn full() -> Self { + SubPatSet::Full } - /// Tests whether the set contains a given Span. - fn contains(&self, span: Span) -> bool { - self.iter().any(|root_span| root_span.contains(span)) + fn is_full(&self) -> bool { + match self { + SubPatSet::Full => true, + SubPatSet::Empty => false, + // If any subpattern in a sequence is unreachable, the whole pattern is unreachable. + SubPatSet::Seq { subpats } => subpats.values().any(|set| set.is_full()), + SubPatSet::Alt { subpats, .. } => subpats.values().all(|set| set.is_full()), + } } - /// Add a span to the set if we know the span has no intersection in this set. - fn push_nonintersecting(&mut self, new_span: Span) { - self.root_spans.push(new_span); + fn is_empty(&self) -> bool { + match self { + SubPatSet::Full => false, + SubPatSet::Empty => true, + SubPatSet::Seq { subpats } => subpats.values().all(|sub_set| sub_set.is_empty()), + SubPatSet::Alt { subpats, alt_count, .. } => { + subpats.len() == *alt_count && subpats.values().all(|set| set.is_empty()) + } + } } - fn intersection_mut(&mut self, other: &Self) { - if self.is_empty() || other.is_empty() { - *self = Self::new(); + /// Intersect `self` with `other`, mutating `self`. + fn intersect(&mut self, other: Self) { + use SubPatSet::*; + // Intersecting with empty stays empty; intersecting with full changes nothing. + if self.is_empty() || other.is_full() { + return; + } else if self.is_full() { + *self = other; + return; + } else if other.is_empty() { + *self = Empty; return; } - // Those that were in `self` but not contained in `other` - let mut leftover = SpanSet::new(); - // We keep the elements in `self` that are also in `other`. - self.root_spans.retain(|span| { - let retain = other.contains(*span); - if !retain { - leftover.root_spans.push(*span); + + match (&mut *self, other) { + (Seq { subpats: s_set }, Seq { subpats: mut o_set }) => { + s_set.retain(|i, s_sub_set| { + // Missing entries count as empty. + let o_sub_set = o_set.remove(&i).unwrap_or(Empty); + s_sub_set.intersect(o_sub_set); + // We drop empty entries. + !s_sub_set.is_empty() + }); + // Everything left in `o_set` is missing from `s_set`, i.e. counts as empty. Since + // intersecting with empty returns empty, we can drop those entries. } - retain - }); - // We keep the elements in `other` that are also in the original `self`. You might think - // this is not needed because `self` already contains the intersection. But those aren't - // just sets of things. If `self = [a]`, `other = [b]` and `a` contains `b`, then `b` - // belongs in the intersection but we didn't catch it in the filtering above. We look at - // `leftover` instead of the full original `self` to avoid duplicates. - for span in other.iter() { - if leftover.contains(span) { - self.root_spans.push(span); + (Alt { subpats: s_set, .. }, Alt { subpats: mut o_set, .. }) => { + s_set.retain(|i, s_sub_set| { + // Missing entries count as full. + let o_sub_set = o_set.remove(&i).unwrap_or(Full); + s_sub_set.intersect(o_sub_set); + // We drop full entries. + !s_sub_set.is_full() + }); + // Everything left in `o_set` is missing from `s_set`, i.e. counts as full. Since + // intersecting with full changes nothing, we can take those entries as is. + s_set.extend(o_set); } + _ => bug!(), + } + + if self.is_empty() { + *self = Empty; } } + + /// Returns a list of the spans of the unreachable subpatterns. If `self` is full we return + /// `None`. + fn to_spans(&self) -> Option> { + /// Panics if `set.is_full()`. + fn fill_spans(set: &SubPatSet<'_, '_>, spans: &mut Vec) { + match set { + SubPatSet::Full => bug!(), + SubPatSet::Empty => {} + SubPatSet::Seq { subpats } => { + for (_, sub_set) in subpats { + fill_spans(sub_set, spans); + } + } + SubPatSet::Alt { subpats, pat, alt_count, .. } => { + let expanded = pat.expand_or_pat(); + for i in 0..*alt_count { + let sub_set = subpats.get(&i).unwrap_or(&SubPatSet::Full); + if sub_set.is_full() { + spans.push(expanded[i].span); + } else { + fill_spans(sub_set, spans); + } + } + } + } + } + + if self.is_full() { + return None; + } + if self.is_empty() { + return Some(Vec::new()); + } + let mut spans = Vec::new(); + fill_spans(self, &mut spans); + Some(spans) + } + + /// When `self` refers to a patstack that was obtained from specialization, after running + /// `unspecialize` it will refer to the original patstack before specialization. + fn unspecialize(self, arity: usize) -> Self { + use SubPatSet::*; + match self { + Full => Full, + Empty => Empty, + Seq { subpats } => { + // We gather the first `arity` subpatterns together and shift the remaining ones. + let mut new_subpats = FxHashMap::default(); + let mut new_subpats_first_col = FxHashMap::default(); + for (i, sub_set) in subpats { + if i < arity { + // The first `arity` indices are now part of the pattern in the first + // column. + new_subpats_first_col.insert(i, sub_set); + } else { + // Indices after `arity` are simply shifted + new_subpats.insert(i - arity + 1, sub_set); + } + } + if !new_subpats_first_col.is_empty() { + new_subpats.insert(0, Seq { subpats: new_subpats_first_col }); + } + Seq { subpats: new_subpats } + } + Alt { .. } => bug!(), // `self` is a patstack + } + } + + /// When `self` refers to a patstack that was obtained from splitting an or-pattern, after + /// running `unspecialize` it will refer to the original patstack before splitting. + /// + /// This case is subtle. Consider: + /// ``` + /// match Some(true) { + /// Some(true) => {} + /// None | Some(true | false) => {} + /// } + /// ``` + /// Imagine we naively preserved the sets of unreachable subpatterns. Here `None` would return + /// the empty set and `Some(true | false)` would return the set containing `true`. Intersecting + /// those two would return the empty set, so we'd miss that the last `true` is unreachable. + /// To fix that, when specializing a given alternative of an or-pattern, we consider all other + /// alternatives as unreachable. That way, intersecting the results will not unduly discard + /// unreachable subpatterns coming from the other alternatives. This is what this function does + /// (remember that missing entries in the `Alt` case count as full; in other words alternatives + /// other than `alt_id` count as unreachable). + fn unsplit_or_pat(mut self, alt_id: usize, alt_count: usize, pat: &'p Pat<'tcx>) -> Self { + use SubPatSet::*; + if self.is_full() { + return Full; + } + + let set_first_col = match &mut self { + Empty => Empty, + Seq { subpats } => subpats.remove(&0).unwrap_or(Empty), + Full => unreachable!(), + Alt { .. } => bug!(), // `self` is a patstack + }; + let mut subpats_first_col = FxHashMap::default(); + subpats_first_col.insert(alt_id, set_first_col); + let set_first_col = Alt { subpats: subpats_first_col, pat, alt_count }; + + let mut subpats = match self { + Empty => FxHashMap::default(), + Seq { subpats } => subpats, + Full => unreachable!(), + Alt { .. } => bug!(), // `self` is a patstack + }; + subpats.insert(0, set_first_col); + Seq { subpats } + } } #[derive(Clone, Debug)] -enum Usefulness<'tcx> { +enum Usefulness<'p, 'tcx> { /// Potentially carries a set of sub-branches that have been found to be unreachable. Used /// only in the presence of or-patterns, otherwise it stays empty. - NoWitnesses(SpanSet), + NoWitnesses(SubPatSet<'p, 'tcx>), /// When not carrying witnesses, indicates that the whole pattern is unreachable. NoWitnessesFull, /// Carries a list of witnesses of non-exhaustiveness. Non-empty. @@ -702,11 +857,11 @@ enum Usefulness<'tcx> { WithWitnessesEmpty, } -impl<'tcx> Usefulness<'tcx> { +impl<'p, 'tcx> Usefulness<'p, 'tcx> { fn new_useful(preference: WitnessPreference) -> Self { match preference { ConstructWitness => WithWitnesses(vec![Witness(vec![])]), - LeaveOutWitness => NoWitnesses(Default::default()), + LeaveOutWitness => NoWitnesses(SubPatSet::empty()), } } fn new_not_useful(preference: WitnessPreference) -> Self { @@ -718,33 +873,13 @@ impl<'tcx> Usefulness<'tcx> { /// Combine usefulnesses from two branches. This is an associative operation. fn extend(&mut self, other: Self) { - // If we have detected some unreachable sub-branches, we only want to keep them when they - // were unreachable in _all_ branches. Eg. in the following, the last `true` is unreachable - // in the second branch of the first or-pattern, but not otherwise. Therefore we don't want - // to lint that it is unreachable. - // ``` - // match (true, true) { - // (true, true) => {} - // (false | true, false | true) => {} - // } - // ``` - // Here however we _do_ want to lint that the last `false` is unreachable. In order to - // handle that correctly, each branch of an or-pattern marks the other branches as - // unreachable (see `unsplit_or_pat`). That way, intersecting the results will correctly - // identify unreachable sub-patterns. - // ``` - // match None { - // Some(false) => {} - // None | Some(true | false) => {} - // } - // ``` match (&mut *self, other) { (WithWitnesses(s), WithWitnesses(o)) => s.extend(o), (WithWitnessesEmpty, WithWitnesses(o)) => *self = WithWitnesses(o), (WithWitnesses(_), WithWitnessesEmpty) => {} (WithWitnessesEmpty, WithWitnessesEmpty) => {} - (NoWitnesses(s), NoWitnesses(o)) => s.intersection_mut(&o), + (NoWitnesses(s), NoWitnesses(o)) => s.intersect(o), (NoWitnessesFull, NoWitnesses(o)) => *self = NoWitnesses(o), (NoWitnesses(_), NoWitnessesFull) => {} (NoWitnessesFull, NoWitnessesFull) => {} @@ -761,8 +896,8 @@ impl<'tcx> Usefulness<'tcx> { let mut ret = Self::new_not_useful(pref); for u in usefulnesses { ret.extend(u); - if let NoWitnesses(spans) = &ret { - if spans.is_empty() { + if let NoWitnesses(subpats) = &ret { + if subpats.is_empty() { // Once we reach the empty set, more intersections won't change the result. return ret; } @@ -773,30 +908,19 @@ impl<'tcx> Usefulness<'tcx> { /// After calculating the usefulness for a branch of an or-pattern, call this to make this /// usefulness mergeable with those from the other branches. - fn unsplit_or_pat(self, this_span: Span, or_pat_spans: &[Span]) -> Self { - match self { - NoWitnesses(mut spans) => { - // We register the spans of the other branches of this or-pattern as being - // unreachable from this one. This ensures that intersecting together the sets of - // spans returns what we want. - // Until we optimize `SpanSet` however, intersecting this entails a number of - // comparisons quadratic in the number of branches. - for &span in or_pat_spans { - if span != this_span { - spans.push_nonintersecting(span); - } - } - NoWitnesses(spans) - } - NoWitnessesFull => NoWitnessesFull, + fn unsplit_or_pat(self, alt_id: usize, alt_count: usize, pat: &'p Pat<'tcx>) -> Self { + let subpats = match self { + NoWitnesses(subpats) => subpats, + NoWitnessesFull => SubPatSet::full(), WithWitnesses(_) | WithWitnessesEmpty => bug!(), - } + }; + NoWitnesses(subpats.unsplit_or_pat(alt_id, alt_count, pat)) } /// After calculating usefulness after a specialization, call this to recontruct a usefulness /// that makes sense for the matrix pre-specialization. This new usefulness can then be merged /// with the results of specializing with the other constructors. - fn apply_constructor<'p>( + fn apply_constructor( self, pcx: PatCtxt<'_, 'p, 'tcx>, matrix: &Matrix<'p, 'tcx>, // used to compute missing ctors @@ -836,7 +960,9 @@ impl<'tcx> Usefulness<'tcx> { }; WithWitnesses(new_witnesses) } - x => x, + NoWitnesses(subpats) => NoWitnesses(subpats.unspecialize(ctor_wild_subpatterns.len())), + NoWitnessesFull => NoWitnessesFull, + WithWitnessesEmpty => WithWitnessesEmpty, } } } @@ -953,7 +1079,7 @@ fn is_useful<'p, 'tcx>( hir_id: HirId, is_under_guard: bool, is_top_level: bool, -) -> Usefulness<'tcx> { +) -> Usefulness<'p, 'tcx> { debug!("matrix,v={:?}{:?}", matrix, v); let Matrix { patterns: rows, .. } = matrix; @@ -981,13 +1107,13 @@ fn is_useful<'p, 'tcx>( // If the first pattern is an or-pattern, expand it. let ret = if v.head().is_or_pat() { debug!("expanding or-pattern"); + let v_head = v.head(); let vs: Vec<_> = v.expand_or_pat().collect(); - let subspans: Vec<_> = vs.iter().map(|v| v.head().span).collect(); + let alt_count = vs.len(); // We expand the or pattern, trying each of its branches in turn and keeping careful track // of possible unreachable sub-branches. let mut matrix = matrix.clone(); - let usefulnesses = vs.into_iter().map(|v| { - let v_span = v.head().span; + let usefulnesses = vs.into_iter().enumerate().map(|(i, v)| { let usefulness = is_useful(cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false); // If pattern has a guard don't add it to the matrix. @@ -996,7 +1122,7 @@ fn is_useful<'p, 'tcx>( // branches like `Some(_) | Some(0)`. matrix.push(v); } - usefulness.unsplit_or_pat(v_span, &subspans) + usefulness.unsplit_or_pat(i, alt_count, v_head) }); Usefulness::merge(witness_preference, usefulnesses) } else { @@ -1045,7 +1171,7 @@ crate struct MatchArm<'p, 'tcx> { crate enum Reachability { /// Potentially carries a set of sub-branches that have been found to be unreachable. Used only /// in the presence of or-patterns, otherwise it stays empty. - Reachable(SpanSet), + Reachable(Vec), Unreachable, } @@ -1081,7 +1207,8 @@ crate fn compute_match_usefulness<'p, 'tcx>( matrix.push(v); } let reachability = match usefulness { - NoWitnesses(spans) => Reachability::Reachable(spans), + NoWitnesses(subpats) if subpats.is_full() => Reachability::Unreachable, + NoWitnesses(subpats) => Reachability::Reachable(subpats.to_spans().unwrap()), NoWitnessesFull => Reachability::Unreachable, WithWitnesses(..) | WithWitnessesEmpty => bug!(), }; diff --git a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs index 9718396ed4822..bdb7a1ec92b7f 100644 --- a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs +++ b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.rs @@ -103,8 +103,8 @@ fn main() { } macro_rules! t_or_f { () => { - (true // FIXME: should be unreachable - | false) + (true //~ ERROR unreachable + | false) }; } match (true, None) { diff --git a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr index 901a71885eb86..51991fc603967 100644 --- a/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr +++ b/src/test/ui/or-patterns/exhaustiveness-unreachable-pattern.stderr @@ -131,6 +131,17 @@ error: unreachable pattern LL | (true | false, None | Some(true | ^^^^ +error: unreachable pattern + --> $DIR/exhaustiveness-unreachable-pattern.rs:106:14 + | +LL | (true + | ^^^^ +... +LL | (true | false, None | Some(t_or_f!())) => {} + | --------- in this macro invocation + | + = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) + error: unreachable pattern --> $DIR/exhaustiveness-unreachable-pattern.rs:117:14 | @@ -155,5 +166,5 @@ error: unreachable pattern LL | | true, | ^^^^ -error: aborting due to 25 previous errors +error: aborting due to 26 previous errors diff --git a/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.rs b/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.rs index 483edcebd6254..aac7d7d5385a4 100644 --- a/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.rs +++ b/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.rs @@ -1,5 +1,5 @@ +// check-pass #![deny(unreachable_patterns)] -//~^ NOTE: lint level is defined here pub enum TypeCtor { Slice, Array, @@ -9,13 +9,13 @@ pub struct ApplicationTy(TypeCtor); macro_rules! ty_app { ($ctor:pat) => { - ApplicationTy($ctor) //~ ERROR unreachable pattern + ApplicationTy($ctor) }; } fn _foo(ty: ApplicationTy) { match ty { - ty_app!(TypeCtor::Array) | ty_app!(TypeCtor::Slice) => {} //~ NOTE: in this expansion + ty_app!(TypeCtor::Array) | ty_app!(TypeCtor::Slice) => {} } // same as above, with the macro expanded diff --git a/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.stderr b/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.stderr deleted file mode 100644 index 9d12b4a20e953..0000000000000 --- a/src/test/ui/pattern/usefulness/issue-80501-or-pat-and-macro.stderr +++ /dev/null @@ -1,18 +0,0 @@ -error: unreachable pattern - --> $DIR/issue-80501-or-pat-and-macro.rs:12:9 - | -LL | ApplicationTy($ctor) - | ^^^^^^^^^^^^^^^^^^^^ -... -LL | ty_app!(TypeCtor::Array) | ty_app!(TypeCtor::Slice) => {} - | ------------------------ in this macro invocation - | -note: the lint level is defined here - --> $DIR/issue-80501-or-pat-and-macro.rs:1:9 - | -LL | #![deny(unreachable_patterns)] - | ^^^^^^^^^^^^^^^^^^^^ - = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) - -error: aborting due to previous error - From 37e7dd22a990eaffa56da89b4fa988ef5822e59f Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Fri, 1 Jan 2021 22:14:54 +0000 Subject: [PATCH 8/9] Specialized `Usefulness` variants are redundant --- .../src/thir/pattern/usefulness.rs | 58 ++++++------------- 1 file changed, 19 insertions(+), 39 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs index 3c3eb801f94ea..15f22e03501ae 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs @@ -846,15 +846,14 @@ impl<'p, 'tcx> SubPatSet<'p, 'tcx> { #[derive(Clone, Debug)] enum Usefulness<'p, 'tcx> { - /// Potentially carries a set of sub-branches that have been found to be unreachable. Used - /// only in the presence of or-patterns, otherwise it stays empty. + /// Carries a set of subpatterns that have been found to be unreachable. If full, this + /// indicates the whole pattern is unreachable. If not, this indicates that the pattern is + /// reachable but has some unreachable sub-patterns (due to or-patterns). In the absence of + /// or-patterns, this is either `Empty` or `Full`. NoWitnesses(SubPatSet<'p, 'tcx>), - /// When not carrying witnesses, indicates that the whole pattern is unreachable. - NoWitnessesFull, - /// Carries a list of witnesses of non-exhaustiveness. Non-empty. + /// Carries a list of witnesses of non-exhaustiveness. If empty, indicates that the whole + /// pattern is unreachable. WithWitnesses(Vec>), - /// When carrying witnesses, indicates that the whole pattern is unreachable. - WithWitnessesEmpty, } impl<'p, 'tcx> Usefulness<'p, 'tcx> { @@ -866,27 +865,19 @@ impl<'p, 'tcx> Usefulness<'p, 'tcx> { } fn new_not_useful(preference: WitnessPreference) -> Self { match preference { - ConstructWitness => WithWitnessesEmpty, - LeaveOutWitness => NoWitnessesFull, + ConstructWitness => WithWitnesses(vec![]), + LeaveOutWitness => NoWitnesses(SubPatSet::full()), } } /// Combine usefulnesses from two branches. This is an associative operation. fn extend(&mut self, other: Self) { match (&mut *self, other) { + (WithWitnesses(_), WithWitnesses(o)) if o.is_empty() => {} + (WithWitnesses(s), WithWitnesses(o)) if s.is_empty() => *self = WithWitnesses(o), (WithWitnesses(s), WithWitnesses(o)) => s.extend(o), - (WithWitnessesEmpty, WithWitnesses(o)) => *self = WithWitnesses(o), - (WithWitnesses(_), WithWitnessesEmpty) => {} - (WithWitnessesEmpty, WithWitnessesEmpty) => {} - (NoWitnesses(s), NoWitnesses(o)) => s.intersect(o), - (NoWitnessesFull, NoWitnesses(o)) => *self = NoWitnesses(o), - (NoWitnesses(_), NoWitnessesFull) => {} - (NoWitnessesFull, NoWitnessesFull) => {} - - _ => { - unreachable!() - } + _ => unreachable!(), } } @@ -909,12 +900,10 @@ impl<'p, 'tcx> Usefulness<'p, 'tcx> { /// After calculating the usefulness for a branch of an or-pattern, call this to make this /// usefulness mergeable with those from the other branches. fn unsplit_or_pat(self, alt_id: usize, alt_count: usize, pat: &'p Pat<'tcx>) -> Self { - let subpats = match self { - NoWitnesses(subpats) => subpats, - NoWitnessesFull => SubPatSet::full(), - WithWitnesses(_) | WithWitnessesEmpty => bug!(), - }; - NoWitnesses(subpats.unsplit_or_pat(alt_id, alt_count, pat)) + match self { + NoWitnesses(subpats) => NoWitnesses(subpats.unsplit_or_pat(alt_id, alt_count, pat)), + WithWitnesses(_) => bug!(), + } } /// After calculating usefulness after a specialization, call this to recontruct a usefulness @@ -928,6 +917,7 @@ impl<'p, 'tcx> Usefulness<'p, 'tcx> { ctor_wild_subpatterns: &Fields<'p, 'tcx>, ) -> Self { match self { + WithWitnesses(witnesses) if witnesses.is_empty() => WithWitnesses(witnesses), WithWitnesses(witnesses) => { let new_witnesses = if matches!(ctor, Constructor::Missing) { let mut split_wildcard = SplitWildcard::new(pcx); @@ -961,8 +951,6 @@ impl<'p, 'tcx> Usefulness<'p, 'tcx> { WithWitnesses(new_witnesses) } NoWitnesses(subpats) => NoWitnesses(subpats.unspecialize(ctor_wild_subpatterns.len())), - NoWitnessesFull => NoWitnessesFull, - WithWitnessesEmpty => WithWitnessesEmpty, } } } @@ -1209,8 +1197,7 @@ crate fn compute_match_usefulness<'p, 'tcx>( let reachability = match usefulness { NoWitnesses(subpats) if subpats.is_full() => Reachability::Unreachable, NoWitnesses(subpats) => Reachability::Reachable(subpats.to_spans().unwrap()), - NoWitnessesFull => Reachability::Unreachable, - WithWitnesses(..) | WithWitnessesEmpty => bug!(), + WithWitnesses(..) => bug!(), }; (arm, reachability) }) @@ -1220,15 +1207,8 @@ crate fn compute_match_usefulness<'p, 'tcx>( let v = PatStack::from_pattern(wild_pattern); let usefulness = is_useful(cx, &matrix, &v, ConstructWitness, scrut_hir_id, false, true); let non_exhaustiveness_witnesses = match usefulness { - WithWitnessesEmpty => vec![], // Wildcard pattern isn't useful, so the match is exhaustive. - WithWitnesses(pats) => { - if pats.is_empty() { - bug!("Exhaustiveness check returned no witnesses") - } else { - pats.into_iter().map(|w| w.single_pattern()).collect() - } - } - NoWitnesses(_) | NoWitnessesFull => bug!(), + WithWitnesses(pats) => pats.into_iter().map(|w| w.single_pattern()).collect(), + NoWitnesses(_) => bug!(), }; UsefulnessReport { arm_usefulness, non_exhaustiveness_witnesses } } From ae6fcab733007b4d59b5b2aac1825bf1f275b0b2 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 1 Feb 2021 19:22:05 +0000 Subject: [PATCH 9/9] Make `SubPatSet` clearer by flipping its meaning --- .../src/thir/pattern/usefulness.rs | 213 ++++++++++-------- 1 file changed, 119 insertions(+), 94 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs index 15f22e03501ae..f3f21b903ea08 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs @@ -619,34 +619,45 @@ impl<'p, 'tcx> FromIterator> for Matrix<'p, 'tcx> { } } -/// Given a pattern or a pattern-stack, this struct captures a set of its subpattern branches. We -/// use that to track unreachable sub-patterns arising from or-patterns. In the absence of -/// or-patterns this will always be either `Empty` or `Full`. -/// We support a limited set of operations, so not all possible sets of subpatterns can be -/// represented. That's ok, we only want the ones that make sense to capture unreachable -/// subpatterns. -/// What we're trying to do is illustrated by this: +/// Given a pattern or a pattern-stack, this struct captures a set of its subpatterns. We use that +/// to track reachable sub-patterns arising from or-patterns. In the absence of or-patterns this +/// will always be either `Empty` (the whole pattern is unreachable) or `Full` (the whole pattern +/// is reachable). When there are or-patterns, some subpatterns may be reachable while others +/// aren't. In this case the whole pattern still counts as reachable, but we will lint the +/// unreachable subpatterns. +/// +/// This supports a limited set of operations, so not all possible sets of subpatterns can be +/// represented. That's ok, we only want the ones that make sense for our usage. +/// +/// What we're doing is illustrated by this: /// ``` -/// match (true, true) { -/// (true, true) => {} -/// (true | false, true | false) => {} +/// match (true, 0) { +/// (true, 0) => {} +/// (_, 1) => {} +/// (true | false, 0 | 1) => {} /// } /// ``` -/// When we try the alternatives of the first or-pattern, the last `true` is unreachable in the -/// first alternative but no the other. So we don't want to report it as unreachable. Therefore we -/// intersect sets of unreachable patterns coming from different alternatives in order to figure -/// out which subpatterns are overall unreachable. +/// When we try the alternatives of the `true | false` or-pattern, the last `0` is reachable in the +/// `false` alternative but not the `true`. So overall it is reachable. By contrast, the last `1` +/// is not reachable in either alternative, so we want to signal this to the user. +/// Therefore we take the union of sets of reachable patterns coming from different alternatives in +/// order to figure out which subpatterns are overall reachable. +/// +/// Invariant: we try to construct the smallest representation we can. In particular if +/// `self.is_empty()` we ensure that `self` is `Empty`, and same with `Full`. This is not important +/// for correctness currently. #[derive(Debug, Clone)] enum SubPatSet<'p, 'tcx> { + /// The empty set. This means the pattern is unreachable. + Empty, /// The set containing the full pattern. Full, - /// The empty set. - Empty, /// If the pattern is a pattern with a constructor or a pattern-stack, we store a set for each - /// of its subpatterns. Missing entries in the map are implicitly empty. + /// of its subpatterns. Missing entries in the map are implicitly full, because that's the + /// common case. Seq { subpats: FxHashMap> }, /// If the pattern is an or-pattern, we store a set for each of its alternatives. Missing - /// entries in the map are implicitly full. Note: we always flatten nested or-patterns. + /// entries in the map are implicitly empty. Note: we always flatten nested or-patterns. Alt { subpats: FxHashMap>, /// Counts the total number of alternatives in the pattern @@ -657,88 +668,91 @@ enum SubPatSet<'p, 'tcx> { } impl<'p, 'tcx> SubPatSet<'p, 'tcx> { - fn empty() -> Self { - SubPatSet::Empty - } fn full() -> Self { SubPatSet::Full } + fn empty() -> Self { + SubPatSet::Empty + } - fn is_full(&self) -> bool { + fn is_empty(&self) -> bool { match self { - SubPatSet::Full => true, - SubPatSet::Empty => false, + SubPatSet::Empty => true, + SubPatSet::Full => false, // If any subpattern in a sequence is unreachable, the whole pattern is unreachable. - SubPatSet::Seq { subpats } => subpats.values().any(|set| set.is_full()), - SubPatSet::Alt { subpats, .. } => subpats.values().all(|set| set.is_full()), + SubPatSet::Seq { subpats } => subpats.values().any(|set| set.is_empty()), + // An or-pattern is reachable if any of its alternatives is. + SubPatSet::Alt { subpats, .. } => subpats.values().all(|set| set.is_empty()), } } - fn is_empty(&self) -> bool { + fn is_full(&self) -> bool { match self { - SubPatSet::Full => false, - SubPatSet::Empty => true, - SubPatSet::Seq { subpats } => subpats.values().all(|sub_set| sub_set.is_empty()), + SubPatSet::Empty => false, + SubPatSet::Full => true, + // The whole pattern is reachable only when all its alternatives are. + SubPatSet::Seq { subpats } => subpats.values().all(|sub_set| sub_set.is_full()), + // The whole or-pattern is reachable only when all its alternatives are. SubPatSet::Alt { subpats, alt_count, .. } => { - subpats.len() == *alt_count && subpats.values().all(|set| set.is_empty()) + subpats.len() == *alt_count && subpats.values().all(|set| set.is_full()) } } } - /// Intersect `self` with `other`, mutating `self`. - fn intersect(&mut self, other: Self) { + /// Union `self` with `other`, mutating `self`. + fn union(&mut self, other: Self) { use SubPatSet::*; - // Intersecting with empty stays empty; intersecting with full changes nothing. - if self.is_empty() || other.is_full() { + // Union with full stays full; union with empty changes nothing. + if self.is_full() || other.is_empty() { return; - } else if self.is_full() { + } else if self.is_empty() { *self = other; return; - } else if other.is_empty() { - *self = Empty; + } else if other.is_full() { + *self = Full; return; } match (&mut *self, other) { (Seq { subpats: s_set }, Seq { subpats: mut o_set }) => { - s_set.retain(|i, s_sub_set| { - // Missing entries count as empty. - let o_sub_set = o_set.remove(&i).unwrap_or(Empty); - s_sub_set.intersect(o_sub_set); - // We drop empty entries. - !s_sub_set.is_empty() - }); - // Everything left in `o_set` is missing from `s_set`, i.e. counts as empty. Since - // intersecting with empty returns empty, we can drop those entries. - } - (Alt { subpats: s_set, .. }, Alt { subpats: mut o_set, .. }) => { s_set.retain(|i, s_sub_set| { // Missing entries count as full. let o_sub_set = o_set.remove(&i).unwrap_or(Full); - s_sub_set.intersect(o_sub_set); + s_sub_set.union(o_sub_set); // We drop full entries. !s_sub_set.is_full() }); // Everything left in `o_set` is missing from `s_set`, i.e. counts as full. Since - // intersecting with full changes nothing, we can take those entries as is. + // unioning with full returns full, we can drop those entries. + } + (Alt { subpats: s_set, .. }, Alt { subpats: mut o_set, .. }) => { + s_set.retain(|i, s_sub_set| { + // Missing entries count as empty. + let o_sub_set = o_set.remove(&i).unwrap_or(Empty); + s_sub_set.union(o_sub_set); + // We drop empty entries. + !s_sub_set.is_empty() + }); + // Everything left in `o_set` is missing from `s_set`, i.e. counts as empty. Since + // unioning with empty changes nothing, we can take those entries as is. s_set.extend(o_set); } _ => bug!(), } - if self.is_empty() { - *self = Empty; + if self.is_full() { + *self = Full; } } - /// Returns a list of the spans of the unreachable subpatterns. If `self` is full we return - /// `None`. - fn to_spans(&self) -> Option> { - /// Panics if `set.is_full()`. + /// Returns a list of the spans of the unreachable subpatterns. If `self` is empty (i.e. the + /// whole pattern is unreachable) we return `None`. + fn list_unreachable_spans(&self) -> Option> { + /// Panics if `set.is_empty()`. fn fill_spans(set: &SubPatSet<'_, '_>, spans: &mut Vec) { match set { - SubPatSet::Full => bug!(), - SubPatSet::Empty => {} + SubPatSet::Empty => bug!(), + SubPatSet::Full => {} SubPatSet::Seq { subpats } => { for (_, sub_set) in subpats { fill_spans(sub_set, spans); @@ -747,8 +761,9 @@ impl<'p, 'tcx> SubPatSet<'p, 'tcx> { SubPatSet::Alt { subpats, pat, alt_count, .. } => { let expanded = pat.expand_or_pat(); for i in 0..*alt_count { - let sub_set = subpats.get(&i).unwrap_or(&SubPatSet::Full); - if sub_set.is_full() { + let sub_set = subpats.get(&i).unwrap_or(&SubPatSet::Empty); + if sub_set.is_empty() { + // Found a unreachable subpattern. spans.push(expanded[i].span); } else { fill_spans(sub_set, spans); @@ -758,10 +773,11 @@ impl<'p, 'tcx> SubPatSet<'p, 'tcx> { } } - if self.is_full() { + if self.is_empty() { return None; } - if self.is_empty() { + if self.is_full() { + // No subpatterns are unreachable. return Some(Vec::new()); } let mut spans = Vec::new(); @@ -790,6 +806,7 @@ impl<'p, 'tcx> SubPatSet<'p, 'tcx> { new_subpats.insert(i - arity + 1, sub_set); } } + // If `new_subpats_first_col` has no entries it counts as full, so we can omit it. if !new_subpats_first_col.is_empty() { new_subpats.insert(0, Seq { subpats: new_subpats_first_col }); } @@ -802,31 +819,28 @@ impl<'p, 'tcx> SubPatSet<'p, 'tcx> { /// When `self` refers to a patstack that was obtained from splitting an or-pattern, after /// running `unspecialize` it will refer to the original patstack before splitting. /// - /// This case is subtle. Consider: + /// For example: /// ``` /// match Some(true) { /// Some(true) => {} /// None | Some(true | false) => {} /// } /// ``` - /// Imagine we naively preserved the sets of unreachable subpatterns. Here `None` would return - /// the empty set and `Some(true | false)` would return the set containing `true`. Intersecting - /// those two would return the empty set, so we'd miss that the last `true` is unreachable. - /// To fix that, when specializing a given alternative of an or-pattern, we consider all other - /// alternatives as unreachable. That way, intersecting the results will not unduly discard - /// unreachable subpatterns coming from the other alternatives. This is what this function does - /// (remember that missing entries in the `Alt` case count as full; in other words alternatives - /// other than `alt_id` count as unreachable). + /// Here `None` would return the full set and `Some(true | false)` would return the set + /// containing `false`. After `unsplit_or_pat`, we want the set to contain `None` and `false`. + /// This is what this function does. fn unsplit_or_pat(mut self, alt_id: usize, alt_count: usize, pat: &'p Pat<'tcx>) -> Self { use SubPatSet::*; - if self.is_full() { - return Full; + if self.is_empty() { + return Empty; } + // Subpatterns coming from inside the or-pattern alternative itself, e.g. in `None | Some(0 + // | 1)`. let set_first_col = match &mut self { - Empty => Empty, - Seq { subpats } => subpats.remove(&0).unwrap_or(Empty), - Full => unreachable!(), + Full => Full, + Seq { subpats } => subpats.remove(&0).unwrap_or(Full), + Empty => unreachable!(), Alt { .. } => bug!(), // `self` is a patstack }; let mut subpats_first_col = FxHashMap::default(); @@ -834,9 +848,9 @@ impl<'p, 'tcx> SubPatSet<'p, 'tcx> { let set_first_col = Alt { subpats: subpats_first_col, pat, alt_count }; let mut subpats = match self { - Empty => FxHashMap::default(), + Full => FxHashMap::default(), Seq { subpats } => subpats, - Full => unreachable!(), + Empty => unreachable!(), Alt { .. } => bug!(), // `self` is a patstack }; subpats.insert(0, set_first_col); @@ -844,12 +858,19 @@ impl<'p, 'tcx> SubPatSet<'p, 'tcx> { } } +/// This carries the results of computing usefulness, as described at the top of the file. When +/// checking usefulness of a match branch, we use the `NoWitnesses` variant, which also keeps track +/// of potential unreachable sub-patterns (in the presence of or-patterns). When checking +/// exhaustiveness of a whole match, we use the `WithWitnesses` variant, which carries a list of +/// witnesses of non-exhaustiveness when there are any. +/// Which variant to use is dictated by `WitnessPreference`. #[derive(Clone, Debug)] enum Usefulness<'p, 'tcx> { - /// Carries a set of subpatterns that have been found to be unreachable. If full, this - /// indicates the whole pattern is unreachable. If not, this indicates that the pattern is - /// reachable but has some unreachable sub-patterns (due to or-patterns). In the absence of - /// or-patterns, this is either `Empty` or `Full`. + /// Carries a set of subpatterns that have been found to be reachable. If empty, this indicates + /// the whole pattern is unreachable. If not, this indicates that the pattern is reachable but + /// that some sub-patterns may be unreachable (due to or-patterns). In the absence of + /// or-patterns this will always be either `Empty` (the whole pattern is unreachable) or `Full` + /// (the whole pattern is reachable). NoWitnesses(SubPatSet<'p, 'tcx>), /// Carries a list of witnesses of non-exhaustiveness. If empty, indicates that the whole /// pattern is unreachable. @@ -860,13 +881,13 @@ impl<'p, 'tcx> Usefulness<'p, 'tcx> { fn new_useful(preference: WitnessPreference) -> Self { match preference { ConstructWitness => WithWitnesses(vec![Witness(vec![])]), - LeaveOutWitness => NoWitnesses(SubPatSet::empty()), + LeaveOutWitness => NoWitnesses(SubPatSet::full()), } } fn new_not_useful(preference: WitnessPreference) -> Self { match preference { ConstructWitness => WithWitnesses(vec![]), - LeaveOutWitness => NoWitnesses(SubPatSet::full()), + LeaveOutWitness => NoWitnesses(SubPatSet::empty()), } } @@ -876,7 +897,7 @@ impl<'p, 'tcx> Usefulness<'p, 'tcx> { (WithWitnesses(_), WithWitnesses(o)) if o.is_empty() => {} (WithWitnesses(s), WithWitnesses(o)) if s.is_empty() => *self = WithWitnesses(o), (WithWitnesses(s), WithWitnesses(o)) => s.extend(o), - (NoWitnesses(s), NoWitnesses(o)) => s.intersect(o), + (NoWitnesses(s), NoWitnesses(o)) => s.union(o), _ => unreachable!(), } } @@ -888,8 +909,8 @@ impl<'p, 'tcx> Usefulness<'p, 'tcx> { for u in usefulnesses { ret.extend(u); if let NoWitnesses(subpats) = &ret { - if subpats.is_empty() { - // Once we reach the empty set, more intersections won't change the result. + if subpats.is_full() { + // Once we reach the full set, more unions won't change the result. return ret; } } @@ -1098,8 +1119,7 @@ fn is_useful<'p, 'tcx>( let v_head = v.head(); let vs: Vec<_> = v.expand_or_pat().collect(); let alt_count = vs.len(); - // We expand the or pattern, trying each of its branches in turn and keeping careful track - // of possible unreachable sub-branches. + // We try each or-pattern branch in turn. let mut matrix = matrix.clone(); let usefulnesses = vs.into_iter().enumerate().map(|(i, v)| { let usefulness = @@ -1155,11 +1175,14 @@ crate struct MatchArm<'p, 'tcx> { crate has_guard: bool, } +/// Indicates whether or not a given arm is reachable. #[derive(Clone, Debug)] crate enum Reachability { - /// Potentially carries a set of sub-branches that have been found to be unreachable. Used only - /// in the presence of or-patterns, otherwise it stays empty. + /// The arm is reachable. This additionally carries a set of or-pattern branches that have been + /// found to be unreachable despite the overall arm being reachable. Used only in the presence + /// of or-patterns, otherwise it stays empty. Reachable(Vec), + /// The arm is unreachable. Unreachable, } @@ -1195,8 +1218,10 @@ crate fn compute_match_usefulness<'p, 'tcx>( matrix.push(v); } let reachability = match usefulness { - NoWitnesses(subpats) if subpats.is_full() => Reachability::Unreachable, - NoWitnesses(subpats) => Reachability::Reachable(subpats.to_spans().unwrap()), + NoWitnesses(subpats) if subpats.is_empty() => Reachability::Unreachable, + NoWitnesses(subpats) => { + Reachability::Reachable(subpats.list_unreachable_spans().unwrap()) + } WithWitnesses(..) => bug!(), }; (arm, reachability)