Skip to content

Commit

Permalink
Implement lint as a separate columns-wise pass
Browse files Browse the repository at this point in the history
  • Loading branch information
@Nadrieril
Nadrieril committed May 16, 2023
1 parent 38e7d42 commit d5edcb1
Show file tree
Hide file tree
Showing 10 changed files with 334 additions and 200 deletions.
21 changes: 11 additions & 10 deletions compiler/rustc_lint_defs/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3798,8 +3798,10 @@ declare_lint! {
}

declare_lint! {
/// The `non_exhaustive_omitted_patterns` lint detects when a wildcard (`_` or `..`) in a
/// pattern for a `#[non_exhaustive]` struct or enum is reachable.
/// The `non_exhaustive_omitted_patterns` lint detects when some variants or fields of a
/// `#[non_exhaustive]` struct or enum are not mentioned explicitly in a pattern. This allows
/// downstream crates to be warned when new variants or fields are added to the upstream struct
/// or enum.
///
/// ### Example
///
Expand Down Expand Up @@ -3827,8 +3829,8 @@ declare_lint! {
/// warning: reachable patterns not covered of non exhaustive enum
/// --> $DIR/reachable-patterns.rs:70:9
/// |
/// LL | _ => {}
/// | ^ pattern `B` not covered
/// LL | match Bar::A {
/// | ^ pattern `Bar::B` not covered
/// |
/// note: the lint level is defined here
/// --> $DIR/reachable-patterns.rs:69:16
Expand All @@ -3841,12 +3843,11 @@ declare_lint! {
///
/// ### Explanation
///
/// Structs and enums tagged with `#[non_exhaustive]` force the user to add a
/// (potentially redundant) wildcard when pattern-matching, to allow for future
/// addition of fields or variants. The `non_exhaustive_omitted_patterns` lint
/// detects when such a wildcard happens to actually catch some fields/variants.
/// In other words, when the match without the wildcard would not be exhaustive.
/// This lets the user be informed if new fields/variants were added.
/// Structs and enums tagged with `#[non_exhaustive]` force the user to add a (potentially
/// redundant) wildcard when pattern-matching, to allow for future addition of fields or
/// variants. The `non_exhaustive_omitted_patterns` lint detects when such a wildcard happens to
/// actually catch some fields/variants. This lets the user be informed if new fields/variants
/// were added.
pub NON_EXHAUSTIVE_OMITTED_PATTERNS,
Allow,
"detect when patterns of types marked `non_exhaustive` are missed",
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_mir_build/src/thir/pattern/check_match.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> {

let scrut = &self.thir[scrut];
let scrut_ty = scrut.ty;
let report = compute_match_usefulness(&cx, &tarms, self.lint_level, scrut_ty);
let report = compute_match_usefulness(&cx, &tarms, self.lint_level, scrut_ty, scrut.span);

match source {
// Don't report arm reachability of desugared `match $iter.into_iter() { iter => .. }`
Expand Down Expand Up @@ -415,7 +415,8 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> {
let pattern = self.lower_pattern(&mut cx, pat);
let pattern_ty = pattern.ty();
let arm = MatchArm { pat: pattern, hir_id: self.lint_level, has_guard: false };
let report = compute_match_usefulness(&cx, &[arm], self.lint_level, pattern_ty);
let report =
compute_match_usefulness(&cx, &[arm], self.lint_level, pattern_ty, pattern.span());

// Note: we ignore whether the pattern is unreachable (i.e. whether the type is empty). We
// only care about exhaustiveness here.
Expand Down Expand Up @@ -583,7 +584,7 @@ fn is_let_irrefutable<'p, 'tcx>(
pat: &'p DeconstructedPat<'p, 'tcx>,
) -> bool {
let arms = [MatchArm { pat, hir_id: pat_id, has_guard: false }];
let report = compute_match_usefulness(&cx, &arms, pat_id, pat.ty());
let report = compute_match_usefulness(&cx, &arms, pat_id, pat.ty(), pat.span());

// Report if the pattern is unreachable, which can only occur when the type is uninhabited.
// This also reports unreachable sub-patterns though, so we can't just replace it with an
Expand Down
45 changes: 24 additions & 21 deletions compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -643,9 +643,8 @@ pub(super) enum Constructor<'tcx> {
/// for those types for which we cannot list constructors explicitly, like `f64` and `str`.
NonExhaustive,
/// Stands for constructors that are not seen in the matrix, as explained in the documentation
/// for [`SplitWildcard`]. The carried `bool` is used for the `non_exhaustive_omitted_patterns`
/// lint.
Missing { nonexhaustive_enum_missing_real_variants: bool },
/// for [`SplitWildcard`].
Missing,
/// Wildcard pattern.
Wildcard,
/// Or-pattern.
Expand Down Expand Up @@ -1053,6 +1052,16 @@ impl<'tcx> SplitWildcard<'tcx> {
self.all_ctors.iter().filter(move |ctor| !ctor.is_covered_by_any(pcx, &self.matrix_ctors))
}

/// Iterate over the constructors for this type that are present in the matrix. This has the
/// effect of deduplicating present constructors.
/// WARNING: this omits special constructors like `Wildcard` and `Opaque`.
pub(super) fn iter_present<'a, 'p>(
&'a self,
pcx: &'a PatCtxt<'a, 'p, 'tcx>,
) -> impl Iterator<Item = &'a Constructor<'tcx>> + Captures<'p> {
self.all_ctors.iter().filter(move |ctor| ctor.is_covered_by_any(pcx, &self.matrix_ctors))
}

/// Return the set of constructors resulting from splitting the wildcard. As explained at the
/// top of the file, if any constructors are missing we can ignore the present ones.
fn into_ctors(self, pcx: &PatCtxt<'_, '_, 'tcx>) -> SmallVec<[Constructor<'tcx>; 1]> {
Expand Down Expand Up @@ -1086,15 +1095,7 @@ impl<'tcx> SplitWildcard<'tcx> {
// sometimes prefer reporting the list of constructors instead of just `_`.
let report_when_all_missing = pcx.is_top_level && !IntRange::is_integral(pcx.ty);
let ctor = if !self.matrix_ctors.is_empty() || report_when_all_missing {
if pcx.is_non_exhaustive {
Missing {
nonexhaustive_enum_missing_real_variants: self
.iter_missing(pcx)
.any(|c| !(c.is_non_exhaustive() || c.is_unstable_variant(pcx))),
}
} else {
Missing { nonexhaustive_enum_missing_real_variants: false }
}
Missing
} else {
Wildcard
};
Expand Down Expand Up @@ -1240,9 +1241,10 @@ impl<'p, 'tcx> Fields<'p, 'tcx> {

/// Values and patterns can be represented as a constructor applied to some fields. This represents
/// a pattern in this form.
/// This also keeps track of whether the pattern has been found reachable during analysis. For this
/// reason we should be careful not to clone patterns for which we care about that. Use
/// `clone_and_forget_reachability` if you're sure.
/// This also uses interior mutability to keep track of whether the pattern has been found reachable
/// during analysis. For this reason we should be careful not to clone patterns for which we care
/// about that.
#[derive(Clone)]
pub(crate) struct DeconstructedPat<'p, 'tcx> {
ctor: Constructor<'tcx>,
fields: Fields<'p, 'tcx>,
Expand Down Expand Up @@ -1273,12 +1275,6 @@ impl<'p, 'tcx> DeconstructedPat<'p, 'tcx> {
DeconstructedPat::new(ctor, fields, pcx.ty, pcx.span)
}

/// Clone this value. This method emphasizes that cloning loses reachability information and
/// should be done carefully.
pub(super) fn clone_and_forget_reachability(&self) -> Self {
DeconstructedPat::new(self.ctor.clone(), self.fields, self.ty, self.span)
}

pub(crate) fn from_pat(cx: &MatchCheckCtxt<'p, 'tcx>, pat: &Pat<'tcx>) -> Self {
let mkpat = |pat| DeconstructedPat::from_pat(cx, pat);
let ctor;
Expand Down Expand Up @@ -1522,6 +1518,13 @@ impl<'p, 'tcx> DeconstructedPat<'p, 'tcx> {
pub(super) fn is_or_pat(&self) -> bool {
matches!(self.ctor, Or)
}
pub(super) fn flatten_or_pat(&'p self) -> SmallVec<[&'p Self; 1]> {
if self.is_or_pat() {
self.iter_fields().flat_map(|p| p.flatten_or_pat()).collect()
} else {
smallvec![self]
}
}

pub(super) fn ctor(&self) -> &Constructor<'tcx> {
&self.ctor
Expand Down
183 changes: 123 additions & 60 deletions compiler/rustc_mir_build/src/thir/pattern/usefulness.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -642,14 +642,7 @@ impl<'p, 'tcx> Usefulness<'p, 'tcx> {
.into_iter()
.flat_map(|witness| {
new_patterns.iter().map(move |pat| {
Witness(
witness
.0
.iter()
.chain(once(pat))
.map(DeconstructedPat::clone_and_forget_reachability)
.collect(),
)
Witness(witness.0.iter().chain(once(pat)).cloned().collect())
})
})
.collect()
Expand Down Expand Up @@ -843,7 +836,6 @@ fn is_useful<'p, 'tcx>(
}
// We split the head constructor of `v`.
let split_ctors = v_ctor.split(pcx, matrix.heads().map(DeconstructedPat::ctor));
let is_non_exhaustive_and_wild = is_non_exhaustive && v_ctor.is_wildcard();
// For each constructor, we compute whether there's a value that starts with it that would
// witness the usefulness of `v`.
let start_matrix = &matrix;
Expand All @@ -864,56 +856,6 @@ fn is_useful<'p, 'tcx>(
)
});
let usefulness = usefulness.apply_constructor(pcx, start_matrix, &ctor);

// When all the conditions are met we have a match with a `non_exhaustive` enum
// that has the potential to trigger the `non_exhaustive_omitted_patterns` lint.
// To understand the workings checkout `Constructor::split` and `SplitWildcard::new/into_ctors`
if is_non_exhaustive_and_wild
// Only emit a lint on refutable patterns.
&& cx.refutable
// We check that the match has a wildcard pattern and that wildcard is useful,
// meaning there are variants that are covered by the wildcard. Without the check
// for `witness_preference` the lint would trigger on `if let NonExhaustiveEnum::A = foo {}`
&& usefulness.is_useful() && matches!(witness_preference, RealArm)
&& matches!(
&ctor,
Constructor::Missing { nonexhaustive_enum_missing_real_variants: true }
)
{
let patterns = {
let mut split_wildcard = SplitWildcard::new(pcx);
split_wildcard.split(pcx, matrix.heads().map(DeconstructedPat::ctor));
// 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(_)`.
split_wildcard
.iter_missing(pcx)
// Filter out the `NonExhaustive` because we want to list only real
// variants. Also remove any unstable feature gated variants.
// Because of how we computed `nonexhaustive_enum_missing_real_variants`,
// this will not return an empty `Vec`.
.filter(|c| !(c.is_non_exhaustive() || c.is_unstable_variant(pcx)))
.cloned()
.map(|missing_ctor| DeconstructedPat::wild_from_ctor(pcx, missing_ctor))
.collect::<Vec<_>>()
};

// Report that a match of a `non_exhaustive` enum marked with `non_exhaustive_omitted_patterns`
// is not exhaustive enough.
//
// NB: The partner lint for structs lives in `compiler/rustc_hir_analysis/src/check/pat.rs`.
cx.tcx.emit_spanned_lint(
NON_EXHAUSTIVE_OMITTED_PATTERNS,
lint_root,
pcx.span,
NonExhaustiveOmittedPattern {
scrut_ty: pcx.ty,
uncovered: Uncovered::new(pcx.span, pcx.cx, patterns),
},
);
}

ret.extend(usefulness);
}
}
Expand All @@ -925,6 +867,96 @@ fn is_useful<'p, 'tcx>(
ret
}

/// Traverse the patterns to collect any variants of a non_exhaustive enum that fail to be mentioned
/// in a given column. This traverses patterns column-by-column, where a column is the intuitive
/// notion of "subpatterns that inspect the same subvalue".
/// Despite similarities with `is_useful`, this traversal is different. Notably this is linear in the
/// depth of patterns, whereas `is_useful` is worst-case exponential (exhaustiveness is NP-complete).
fn collect_nonexhaustive_missing_variants<'p, 'tcx>(
cx: &MatchCheckCtxt<'p, 'tcx>,
column: &[&DeconstructedPat<'p, 'tcx>],
) -> Vec<DeconstructedPat<'p, 'tcx>> {
let mut witnesses = Vec::new();
// If the column is all wildcards, we don't want to collect anything and we can't recurse
// further.
let is_all_wildcards = column.iter().all(|p| p.ctor().is_wildcard());
if column.is_empty() || is_all_wildcards {
return witnesses;
}

let ty = column[0].ty();
let span = column[0].span();
let is_non_exhaustive = cx.is_foreign_non_exhaustive_enum(ty);
let pcx = &PatCtxt { cx, ty, span, is_top_level: false, is_non_exhaustive };

let mut split_wildcard = SplitWildcard::new(pcx);
split_wildcard.split(pcx, column.iter().map(|p| p.ctor()));

if is_non_exhaustive {
witnesses.extend(
split_wildcard
.iter_missing(pcx)
// Filter out the `NonExhaustive` ctor because we want to list only real variants.
// Also remove any unstable feature gated variants.
.filter(|c| !(c.is_non_exhaustive() || c.is_unstable_variant(pcx)))
.cloned()
.map(|missing_ctor| DeconstructedPat::wild_from_ctor(pcx, missing_ctor)),
)
}

// Recurse into the fields.
for ctor in split_wildcard.iter_present(pcx) {
let arity = ctor.arity(pcx);
if arity == 0 {
continue;
}

// We specialize the column by `ctor`. This gives us `arity`-many columns of patterns. These
// columns may not have the same length in the presence of or-patterns (this is why we can't
// reuse what `Matrix` does).
let mut specialized_columns: Vec<Vec<_>> = (0..arity).map(|_| Vec::new()).collect();
let relevant_patterns = column.iter().filter(|pat| ctor.is_covered_by(pcx, pat.ctor()));
for pat in relevant_patterns {
let specialized = pat.specialize(pcx, &ctor);
for (subpat, sub_column) in specialized.iter().zip(&mut specialized_columns) {
if subpat.is_or_pat() {
sub_column.extend(subpat.iter_fields())
} else {
sub_column.push(subpat)
}
}
}
assert!(
!specialized_columns[0].is_empty(),
"ctor {ctor:?} was listed as present but isn't"
);

for (i, col_i) in specialized_columns.iter().enumerate() {
// Compute witnesses for each column.
let wits_for_col_i = collect_nonexhaustive_missing_variants(cx, col_i.as_slice());
if wits_for_col_i.is_empty() {
continue;
}
// For each witness, we build a new pattern in the shape of `ctor(_, _, wit, _, _)`,
// adding enough wildcards to match `arity`.
let fields = Fields::wildcards(pcx, &ctor);
for wit in wits_for_col_i {
let fields_with_wit = Fields::from_iter(
cx,
fields
.iter_patterns()
.enumerate()
.map(|(j, wild)| if i == j { &wit } else { wild })
.cloned(),
);
let pat = DeconstructedPat::new(ctor.clone(), fields_with_wit, ty, DUMMY_SP);
witnesses.push(pat);
}
}
}
witnesses
}

/// The arm of a match expression.
#[derive(Clone, Copy, Debug)]
pub(crate) struct MatchArm<'p, 'tcx> {
Expand Down Expand Up @@ -965,6 +997,7 @@ pub(crate) fn compute_match_usefulness<'p, 'tcx>(
arms: &[MatchArm<'p, 'tcx>],
lint_root: HirId,
scrut_ty: Ty<'tcx>,
scrut_span: Span,
) -> UsefulnessReport<'p, 'tcx> {
let mut matrix = Matrix::empty();
let arm_usefulness: Vec<_> = arms
Expand All @@ -989,9 +1022,39 @@ pub(crate) fn compute_match_usefulness<'p, 'tcx>(
let wild_pattern = cx.pattern_arena.alloc(DeconstructedPat::wildcard(scrut_ty, DUMMY_SP));
let v = PatStack::from_pattern(wild_pattern);
let usefulness = is_useful(cx, &matrix, &v, FakeExtraWildcard, lint_root, false, true);
let non_exhaustiveness_witnesses = match usefulness {
let non_exhaustiveness_witnesses: Vec<_> = match usefulness {
WithWitnesses(pats) => pats.into_iter().map(|w| w.single_pattern()).collect(),
NoWitnesses { .. } => bug!(),
};

// Run the non_exhaustive_omitted_patterns lint. Only run on refutable patterns to avoid hittin
// `if let`s. Only run if the match is exhaustive otherwise the error is redundant.
if cx.refutable
&& non_exhaustiveness_witnesses.is_empty()
&& !matches!(
cx.tcx.lint_level_at_node(NON_EXHAUSTIVE_OMITTED_PATTERNS, lint_root).0,
rustc_session::lint::Level::Allow
)
{
let pat_column = arms.iter().flat_map(|arm| arm.pat.flatten_or_pat()).collect::<Vec<_>>();
let witnesses = collect_nonexhaustive_missing_variants(cx, &pat_column);

if !witnesses.is_empty() {
// Report that a match of a `non_exhaustive` enum marked with `non_exhaustive_omitted_patterns`
// is not exhaustive enough.
//
// NB: The partner lint for structs lives in `compiler/rustc_hir_analysis/src/check/pat.rs`.
cx.tcx.emit_spanned_lint(
NON_EXHAUSTIVE_OMITTED_PATTERNS,
lint_root,
scrut_span,
NonExhaustiveOmittedPattern {
scrut_ty,
uncovered: Uncovered::new(scrut_span, cx, witnesses),
},
);
}
}

UsefulnessReport { arm_usefulness, non_exhaustiveness_witnesses }
}
Loading

0 comments on commit d5edcb1

Please sign in to comment.