Skip to content

Commit

Permalink
Auto merge of rust-lang#80104 - Nadrieril:usefulness-merging, r=varkor
Browse files Browse the repository at this point in the history 
Improve and fix diagnostics of exhaustiveness checking

Primarily, this fixes rust-lang#56379. This also fixes incorrect interactions between or-patterns and slice patterns that I discovered while working on rust-lang#56379. Those two examples show the incorrect diagnostics:

```rust
match &[][..] {
    [true] => {}
    [true // detected as unreachable but that's not true
        | false, ..] => {}
    _ => {}
}
match (true, None) {
    (true, Some(_)) => {}
    (false, Some(true)) => {}
    (true | false, None | Some(true // should be detected as unreachable
                               | false)) => {}
}
```

I did not measure any perf impact. However, I suspect that [`616ba9f`](rust-lang@616ba9f) should have a negative impact on large or-patterns. I'll see what the perf run says; I have optimization ideas up my sleeve if needed.

EDIT: I initially had a noticeable perf impact that I thought unavoidable. I then proceeded to avoid it x)

r? `@varkor`
`@rustbot` label +A-exhaustiveness-checking
  • Loading branch information
@bors
bors committed Dec 19, 2020
2 parents 1b6b06a + cefcadb commit 1f5bc17
Show file tree
Hide file tree
Showing 12 changed files with 286 additions and 139 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/thir/pattern/check_match.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ fn report_arm_reachability<'p, 'tcx>(
Useful(unreachables) if unreachables.is_empty() => {}
// The arm is reachable, but contains unreachable subpatterns (from or-patterns).
Useful(unreachables) => {
let mut unreachables: Vec<_> = unreachables.iter().flatten().copied().collect();
let mut unreachables: Vec<_> = unreachables.iter().collect();
// Emit lints in the order in which they occur in the file.
unreachables.sort_unstable();
for span in unreachables {
Expand Down
297 changes: 184 additions & 113 deletions compiler/rustc_mir_build/src/thir/pattern/usefulness.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,6 @@ use super::{Pat, PatKind};
use super::{PatternFoldable, PatternFolder};

use rustc_data_structures::captures::Captures;
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::sync::OnceCell;

use rustc_arena::TypedArena;
Expand Down Expand Up @@ -626,11 +625,82 @@ impl<'p, 'tcx> FromIterator<PatStack<'p, 'tcx>> 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<Span>,
}

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()
}

/// Iterate over the disjoint list of spans at the roots of this set.
pub(crate) fn iter<'a>(&'a self) -> impl Iterator<Item = Span> + Captures<'a> {
self.root_spans.iter().copied()
}

/// Tests whether the set contains a given Span.
fn contains(&self, span: Span) -> bool {
self.iter().any(|root_span| root_span.contains(span))
}

/// 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 intersection_mut(&mut self, other: &Self) {
if self.is_empty() || other.is_empty() {
*self = Self::new();
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);
}
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);
}
}
}
}

#[derive(Clone, Debug)]
crate enum Usefulness<'tcx> {
/// Carries, for each column in the matrix, 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(Vec<FxHashSet<Span>>),
/// 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),
/// Carries a list of witnesses of non-exhaustiveness.
UsefulWithWitness(Vec<Witness<'tcx>>),
NotUseful,
Expand All @@ -640,14 +710,97 @@ impl<'tcx> Usefulness<'tcx> {
fn new_useful(preference: WitnessPreference) -> Self {
match preference {
ConstructWitness => UsefulWithWitness(vec![Witness(vec![])]),
LeaveOutWitness => Useful(vec![]),
LeaveOutWitness => Useful(Default::default()),
}
}

fn is_useful(&self) -> bool {
!matches!(*self, NotUseful)
/// When trying several branches and each returns a `Usefulness`, we need to combine the
/// results together.
fn merge(usefulnesses: impl Iterator<Item = Self>) -> 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. 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.
// ```
// match None {
// Some(false) => {}
// None | Some(true | false) => {}
// }
// ```

// 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<SpanSet> = None;
// Witnesses of usefulness, if any.
let mut witnesses = Vec::new();

for u in usefulnesses {
match u {
Useful(spans) 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);
}
}
}

if !witnesses.is_empty() {
UsefulWithWitness(witnesses)
} else if let Some(unreachables) = unreachables {
Useful(unreachables)
} else {
NotUseful
}
}

/// 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 {
Useful(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);
}
}
Useful(spans)
}
x => x,
}
}

/// 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>(
self,
pcx: PatCtxt<'_, 'p, 'tcx>,
Expand Down Expand Up @@ -677,23 +830,6 @@ impl<'tcx> Usefulness<'tcx> {
};
UsefulWithWitness(new_witnesses)
}
Useful(mut unreachables) => {
if !unreachables.is_empty() {
// When we apply a constructor, there are `arity` columns of the matrix that
// corresponded to its arguments. All the unreachables found in these columns
// will, after `apply`, come from the first column. So we take the union of all
// the corresponding sets and put them in the first column.
// Note that `arity` may be 0, in which case we just push a new empty set.
let len = unreachables.len();
let arity = ctor_wild_subpatterns.len();
let mut unioned = FxHashSet::default();
for set in unreachables.drain((len - arity)..) {
unioned.extend(set)
}
unreachables.push(unioned);
}
Useful(unreachables)
}
x => x,
}
}
Expand Down Expand Up @@ -829,112 +965,47 @@ fn is_useful<'p, 'tcx>(

assert!(rows.iter().all(|r| r.len() == v.len()));

// 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 { cx, matrix, 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.
if let Some(vs) = v.expand_or_pat() {
let ret = if let Some(vs) = v.expand_or_pat() {
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.
//
// If two branches have detected some unreachable sub-branches, we need to be careful. If
// they were detected in columns that are not the current one, we want to keep only the
// sub-branches that 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) => {}
// }
// ```
// If however the sub-branches come from the current column, they come from the inside of
// the current or-pattern, and we want to keep them all. Eg. in the following, we _do_ want
// to lint that the last `false` is unreachable.
// ```
// match None {
// Some(false) => {}
// None | Some(true | false) => {}
// }
// ```

let mut matrix = matrix.clone();
// We keep track of sub-branches separately depending on whether they come from this column
// or from others.
let mut unreachables_this_column: FxHashSet<Span> = FxHashSet::default();
let mut unreachables_other_columns: Vec<FxHashSet<Span>> = Vec::default();
// Whether at least one branch is reachable.
let mut any_is_useful = false;

for v in vs {
let res = is_useful(cx, &matrix, &v, witness_preference, hir_id, is_under_guard, false);
match res {
Useful(unreachables) => {
if let Some((this_column, other_columns)) = unreachables.split_last() {
// We keep the union of unreachables found in the first column.
unreachables_this_column.extend(this_column);
// We keep the intersection of unreachables found in other columns.
if unreachables_other_columns.is_empty() {
unreachables_other_columns = other_columns.to_vec();
} else {
unreachables_other_columns = unreachables_other_columns
.into_iter()
.zip(other_columns)
.map(|(x, y)| x.intersection(&y).copied().collect())
.collect();
}
}
any_is_useful = true;
}
NotUseful => {
unreachables_this_column.insert(v.head().span);
}
UsefulWithWitness(_) => bug!(
"encountered or-pat in the expansion of `_` during exhaustiveness checking"
),
}

let usefulnesses = vs.into_iter().map(|v| {
let v_span = v.head().span;
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.
if !is_under_guard {
// We push the already-seen patterns into the matrix in order to detect redundant
// branches like `Some(_) | Some(0)`.
matrix.push(v);
}
}

return if any_is_useful {
let mut unreachables = if unreachables_other_columns.is_empty() {
let n_columns = v.len();
(0..n_columns - 1).map(|_| FxHashSet::default()).collect()
} else {
unreachables_other_columns
};
unreachables.push(unreachables_this_column);
Useful(unreachables)
} else {
NotUseful
};
}

// 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 { cx, matrix, ty, span: v.head().span, is_top_level };

debug!("is_useful_expand_first_col: ty={:#?}, expanding {:#?}", pcx.ty, v.head());

let ret = v
.head_ctor(cx)
.split(pcx, Some(hir_id))
.into_iter()
.map(|ctor| {
usefulness.unsplit_or_pat(v_span, &subspans)
});
Usefulness::merge(usefulnesses)
} else {
// We split the head constructor of `v`.
let ctors = v.head_ctor(cx).split(pcx, Some(hir_id));
// For each constructor, we compute whether there's a value that starts with it that would
// witness the usefulness of `v`.
let usefulnesses = ctors.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);
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)
})
.find(|result| result.is_useful())
.unwrap_or(NotUseful);
});
Usefulness::merge(usefulnesses)
};
debug!("is_useful::returns({:#?}, {:#?}) = {:?}", matrix, v, ret);
ret
}
Expand Down
Loading

0 comments on commit 1f5bc17

Please sign in to comment.