Skip to content

[Experiment] Split exhaustiveness logic into its own crate #89570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
dc6c89a
Filter unstable and doc hidden variants in exhaustive check
DevinR528 Sep 18, 2021
068325b
Add test cases for unstable variants
DevinR528 Sep 20, 2021
6f86964
Diagnostic output for unstable variant tests
DevinR528 Sep 20, 2021
3dd5ad2
Add test cases for doc hidden variants
DevinR528 Sep 20, 2021
0d58694
Diagnostic output for doc hidden variants
DevinR528 Sep 20, 2021
311333b
Remove io::ErrorKind and update reachable-patterns diagnostic output
DevinR528 Sep 20, 2021
9927c1f
Fix behavior of doc hidden filtering
DevinR528 Sep 20, 2021
b02590d
Cleanup check for unstable attributes on variants
DevinR528 Oct 5, 2021
5d5b381
Move unstable patterns test to patterns/usefulness
DevinR528 Oct 5, 2021
34d9075
Add a test
Nadrieril Oct 1, 2021
0f1dcc6
Fix a comment
Nadrieril Sep 29, 2021
033aeb8
`PatternFolder` is no longer used
Nadrieril Oct 2, 2021
e8c10f5
Add a constructor for box patterns
Nadrieril Sep 29, 2021
25f0710
Add a constructor for ref patterns
Nadrieril Sep 29, 2021
13c418f
Add a constructor for tuple patterns
Nadrieril Sep 29, 2021
2cb2a54
Reorder the logic of `SplitWildcard::new`
Nadrieril Sep 29, 2021
3ca5461
Add explicit constructors for bool values
Nadrieril Sep 29, 2021
4c286e2
Evaluate consts earlier
Nadrieril Sep 30, 2021
604b31b
Don't lint from within the algorithm; collect instead
Nadrieril Oct 1, 2021
1684c64
Always report unreachable subpatterns
Nadrieril Oct 1, 2021
784d29a
Move conversion to/from `thir::Pat` out of deconstruct_pat
Nadrieril Oct 2, 2021
03eb4be
Move abi-related computations out of deconstruct_pat
Nadrieril Oct 2, 2021
474e763
Reuse `size` since we have it
Nadrieril Oct 2, 2021
c4f840d
Move `tcx`-dependent code out of deconstruct_pat
Nadrieril Oct 2, 2021
f1e56c6
Remove all matching on `Ty` from deconstruct_pat
Nadrieril Oct 3, 2021
ec2324b
Thread everything rustc-dependent via a trait
Nadrieril Oct 3, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 124 additions & 55 deletions compiler/rustc_mir_build/src/thir/pattern/check_match.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use super::deconstruct_pat::{Constructor, DeconstructedPat};
use super::usefulness::{
compute_match_usefulness, MatchArm, MatchCheckCtxt, Reachability, UsefulnessReport,
compute_match_usefulness, MatchArm, Reachability, UsefulnessCtxt, UsefulnessReport,
};
use super::{PatCtxt, PatternError};
use super::{deconstructed_to_pat, pat_to_deconstructed, MatchCheckCtxt, PatCtxt, PatternError};

use rustc_arena::TypedArena;
use rustc_ast::Mutability;
Expand All @@ -14,7 +14,8 @@ use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
use rustc_hir::{HirId, Pat};
use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt};
use rustc_session::lint::builtin::{
BINDINGS_WITH_VARIANT_NAME, IRREFUTABLE_LET_PATTERNS, UNREACHABLE_PATTERNS,
BINDINGS_WITH_VARIANT_NAME, IRREFUTABLE_LET_PATTERNS, NON_EXHAUSTIVE_OMITTED_PATTERNS,
OVERLAPPING_RANGE_ENDPOINTS, UNREACHABLE_PATTERNS,
};
use rustc_session::Session;
use rustc_span::{DesugaringKind, ExpnKind, Span};
Expand Down Expand Up @@ -50,7 +51,7 @@ struct MatchVisitor<'a, 'p, 'tcx> {
tcx: TyCtxt<'tcx>,
typeck_results: &'a ty::TypeckResults<'tcx>,
param_env: ty::ParamEnv<'tcx>,
pattern_arena: &'p TypedArena<DeconstructedPat<'p, 'tcx>>,
pattern_arena: &'p TypedArena<DeconstructedPat<'p, MatchCheckCtxt<'tcx>>>,
}

impl<'tcx> Visitor<'tcx> for MatchVisitor<'_, '_, 'tcx> {
Expand Down Expand Up @@ -127,28 +128,28 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> {

fn lower_pattern(
&self,
cx: &mut MatchCheckCtxt<'p, 'tcx>,
cx: &UsefulnessCtxt<'p, MatchCheckCtxt<'tcx>>,
pat: &'tcx hir::Pat<'tcx>,
have_errors: &mut bool,
) -> &'p DeconstructedPat<'p, 'tcx> {
) -> &'p DeconstructedPat<'p, MatchCheckCtxt<'tcx>> {
let mut patcx = PatCtxt::new(self.tcx, self.param_env, self.typeck_results);
patcx.include_lint_checks();
let pattern = patcx.lower_pattern(pat);
let pattern: &_ = cx.pattern_arena.alloc(DeconstructedPat::from_pat(cx, &pattern));
let pattern: &_ = cx.pattern_arena.alloc(pat_to_deconstructed(cx, &pattern));
if !patcx.errors.is_empty() {
*have_errors = true;
patcx.report_inlining_errors();
}
pattern
}

fn new_cx(&self, hir_id: HirId) -> MatchCheckCtxt<'p, 'tcx> {
MatchCheckCtxt {
fn new_cx(&self, hir_id: HirId) -> UsefulnessCtxt<'p, MatchCheckCtxt<'tcx>> {
let cx = MatchCheckCtxt {
tcx: self.tcx,
param_env: self.param_env,
module: self.tcx.parent_module(hir_id).to_def_id(),
pattern_arena: &self.pattern_arena,
}
};
UsefulnessCtxt { cx, pattern_arena: &self.pattern_arena }
}

fn check_let(&mut self, pat: &'tcx hir::Pat<'tcx>, expr: &hir::Expr<'_>, span: Span) {
Expand All @@ -164,15 +165,16 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> {
arms: &'tcx [hir::Arm<'tcx>],
source: hir::MatchSource,
) {
let mut cx = self.new_cx(scrut.hir_id);
let ucx = self.new_cx(scrut.hir_id);
let cx = &ucx.cx;

for arm in arms {
// Check the arm for some things unrelated to exhaustiveness.
self.check_patterns(&arm.pat, Refutable);
if let Some(hir::Guard::IfLet(ref pat, _)) = arm.guard {
self.check_patterns(pat, Refutable);
let tpat = self.lower_pattern(&mut cx, pat, &mut false);
check_let_reachability(&mut cx, pat.hir_id, tpat, tpat.span());
let tpat = self.lower_pattern(&ucx, pat, &mut false);
check_let_reachability(&ucx, pat.hir_id, tpat, tpat.span());
}
}

Expand All @@ -181,7 +183,7 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> {
let arms: Vec<_> = arms
.iter()
.map(|hir::Arm { pat, guard, .. }| MatchArm {
pat: self.lower_pattern(&mut cx, pat, &mut have_errors),
pat: self.lower_pattern(&ucx, pat, &mut have_errors),
hir_id: pat.hir_id,
has_guard: guard.is_some(),
})
Expand All @@ -193,11 +195,12 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> {
}

let scrut_ty = self.typeck_results.expr_ty_adjusted(scrut);
let report = compute_match_usefulness(&cx, &arms, scrut.hir_id, scrut_ty);
let report = compute_match_usefulness(&ucx, &arms, scrut.hir_id, scrut_ty);

match source {
hir::MatchSource::ForLoopDesugar | hir::MatchSource::Normal => {
report_arm_reachability(&cx, &report)
report_exhaustiveness_lints(&cx, &report);
report_arm_reachability(&cx, &report);
}
// Unreachable patterns in try and await expressions occur when one of
// the arms are an uninhabited type. Which is OK.
Expand All @@ -213,12 +216,14 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> {
}

fn check_irrefutable(&self, pat: &'tcx Pat<'tcx>, origin: &str, sp: Option<Span>) {
let mut cx = self.new_cx(pat.hir_id);
let ucx = self.new_cx(pat.hir_id);
let cx = &ucx.cx;

let pattern = self.lower_pattern(&mut cx, pat, &mut false);
let pattern = self.lower_pattern(&ucx, pat, &mut false);
let pattern_ty = pattern.ty();
let arms = vec![MatchArm { pat: pattern, hir_id: pat.hir_id, has_guard: false }];
let report = compute_match_usefulness(&cx, &arms, pat.hir_id, pattern_ty);
let report = compute_match_usefulness(&ucx, &arms, pat.hir_id, pattern_ty);
report_exhaustiveness_lints(&cx, &report);

// 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 @@ -356,11 +361,11 @@ fn check_for_bindings_named_same_as_variants(
}

/// Checks for common cases of "catchall" patterns that may not be intended as such.
fn pat_is_catchall(pat: &DeconstructedPat<'_, '_>) -> bool {
fn pat_is_catchall(pat: &DeconstructedPat<'_, MatchCheckCtxt<'_>>) -> bool {
use Constructor::*;
match pat.ctor() {
Wildcard => true,
Single => pat.iter_fields().all(|pat| pat_is_catchall(pat)),
Single | Tuple(_) | Ref(_) | BoxPat(_) => pat.iter_fields().all(|pat| pat_is_catchall(pat)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bugfix?

This seems like it might be a source of performance impact.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is because I split the Single case into more specific subcases so that I could abstract it all in a trait. I suspected this could be the source of perf degradation but my local measurements don't show much impact from this.
The problem of course is that my local measurements don't match what rust-timer reports here so you could be right. When I get back to this I'll measure intermediate commits with rust-timer.

_ => false,
}
}
Expand Down Expand Up @@ -437,17 +442,17 @@ fn irrefutable_let_pattern(tcx: TyCtxt<'_>, id: HirId, span: Span) {
}

fn check_let_reachability<'p, 'tcx>(
cx: &mut MatchCheckCtxt<'p, 'tcx>,
ucx: &UsefulnessCtxt<'p, MatchCheckCtxt<'tcx>>,
pat_id: HirId,
pat: &'p DeconstructedPat<'p, 'tcx>,
pat: &'p DeconstructedPat<'p, MatchCheckCtxt<'tcx>>,
span: Span,
) {
let cx = &ucx.cx;
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(&ucx, &arms, pat_id, pat.ty());
report_exhaustiveness_lints(&cx, &report);

// 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
// `is_uninhabited` check.
report_arm_reachability(&cx, &report);

if report.non_exhaustiveness_witnesses.is_empty() {
Expand All @@ -458,37 +463,98 @@ fn check_let_reachability<'p, 'tcx>(

/// Report unreachable arms, if any.
fn report_arm_reachability<'p, 'tcx>(
cx: &MatchCheckCtxt<'p, 'tcx>,
report: &UsefulnessReport<'p, 'tcx>,
cx: &MatchCheckCtxt<'tcx>,
report: &UsefulnessReport<'p, MatchCheckCtxt<'tcx>>,
) {
use Reachability::*;
let mut catchall = None;
for (arm, is_useful) in report.arm_usefulness.iter() {
match is_useful {
Unreachable => unreachable_pattern(cx.tcx, arm.pat.span(), arm.hir_id, catchall),
Reachable(unreachables) if unreachables.is_empty() => {}
// The arm is reachable, but contains unreachable subpatterns (from or-patterns).
Reachable(unreachables) => {
let mut unreachables = unreachables.clone();
// Emit lints in the order in which they occur in the file.
unreachables.sort_unstable();
for span in unreachables {
unreachable_pattern(cx.tcx, span, arm.hir_id, None);
}
for (arm, reachability) in report.arm_usefulness.iter() {
match reachability {
Reachability::Unreachable => {
unreachable_pattern(cx.tcx, arm.pat.span(), arm.hir_id, catchall)
}
Reachability::Reachable => {}
}
if !arm.has_guard && catchall.is_none() && pat_is_catchall(arm.pat) {
catchall = Some(arm.pat.span());
}
}
}

/// Report various things detected during exhaustiveness checking.
fn report_exhaustiveness_lints<'p, 'tcx>(
cx: &MatchCheckCtxt<'tcx>,
report: &UsefulnessReport<'p, MatchCheckCtxt<'tcx>>,
) {
if !report.unreachable_subpatterns.is_empty() {
let mut unreachables = report.unreachable_subpatterns.clone();
// Emit lints in the order in which they occur in the file.
unreachables.sort_unstable();
for (span, hir_id) in unreachables {
unreachable_pattern(cx.tcx, span, hir_id, None);
}
}
for (ty, span, hir_id, patterns) in &report.non_exhaustive_omitted_patterns {
lint_non_exhaustive_omitted_patterns(cx, ty, *span, *hir_id, patterns.as_slice());
}
for (span, hir_id, overlaps) in &report.overlapping_range_endpoints {
lint_overlapping_range_endpoints(cx, *span, *hir_id, overlaps.as_slice());
}
}

/// Report when some variants of a `non_exhaustive` enum failed to be listed explicitly.
///
/// NB: The partner lint for structs lives in `compiler/rustc_typeck/src/check/pat.rs`.
pub(super) fn lint_non_exhaustive_omitted_patterns<'p, 'tcx>(
cx: &MatchCheckCtxt<'tcx>,
scrut_ty: Ty<'tcx>,
span: Span,
hir_id: HirId,
witnesses: &[DeconstructedPat<'p, MatchCheckCtxt<'tcx>>],
) {
let joined_patterns = joined_uncovered_patterns(cx, &witnesses);
cx.tcx.struct_span_lint_hir(NON_EXHAUSTIVE_OMITTED_PATTERNS, hir_id, span, |build| {
let mut lint = build.build("some variants are not matched explicitly");
lint.span_label(span, pattern_not_covered_label(witnesses, &joined_patterns));
lint.help(
"ensure that all variants are matched explicitly by adding the suggested match arms",
);
lint.note(&format!(
"the matched value is of type `{}` and the `non_exhaustive_omitted_patterns` attribute was found",
scrut_ty,
));
lint.emit();
});
}

/// Lint on likely incorrect range patterns (#63987)
pub(super) fn lint_overlapping_range_endpoints<'p, 'tcx>(
cx: &MatchCheckCtxt<'tcx>,
span: Span,
hir_id: HirId,
overlaps: &[DeconstructedPat<'p, MatchCheckCtxt<'tcx>>],
) {
if !overlaps.is_empty() {
cx.tcx.struct_span_lint_hir(OVERLAPPING_RANGE_ENDPOINTS, hir_id, span, |lint| {
let mut err = lint.build("multiple patterns overlap on their endpoints");
for pat in overlaps {
err.span_label(
pat.span(),
&format!("this range overlaps on `{}`...", deconstructed_to_pat(cx, pat)),
);
}
err.span_label(span, "... with this range");
err.note("you likely meant to write mutually exclusive ranges");
err.emit();
});
}
}

/// Report that a match is not exhaustive.
fn non_exhaustive_match<'p, 'tcx>(
cx: &MatchCheckCtxt<'p, 'tcx>,
cx: &MatchCheckCtxt<'tcx>,
scrut_ty: Ty<'tcx>,
sp: Span,
witnesses: Vec<DeconstructedPat<'p, 'tcx>>,
witnesses: Vec<DeconstructedPat<'p, MatchCheckCtxt<'tcx>>>,
is_empty_match: bool,
) {
let non_empty_enum = match scrut_ty.kind() {
Expand Down Expand Up @@ -556,17 +622,17 @@ fn non_exhaustive_match<'p, 'tcx>(
}

crate fn joined_uncovered_patterns<'p, 'tcx>(
cx: &MatchCheckCtxt<'p, 'tcx>,
witnesses: &[DeconstructedPat<'p, 'tcx>],
cx: &MatchCheckCtxt<'tcx>,
witnesses: &[DeconstructedPat<'p, MatchCheckCtxt<'tcx>>],
) -> String {
const LIMIT: usize = 3;
let pat_to_str = |pat: &DeconstructedPat<'p, 'tcx>| pat.to_pat(cx).to_string();
let pat_to_str = |pat| deconstructed_to_pat(cx, pat).to_string();
match witnesses {
[] => bug!(),
[witness] => format!("`{}`", witness.to_pat(cx)),
[witness] => format!("`{}`", deconstructed_to_pat(cx, witness)),
[head @ .., tail] if head.len() < LIMIT => {
let head: Vec<_> = head.iter().map(pat_to_str).collect();
format!("`{}` and `{}`", head.join("`, `"), tail.to_pat(cx))
format!("`{}` and `{}`", head.join("`, `"), deconstructed_to_pat(cx, tail))
}
_ => {
let (head, tail) = witnesses.split_at(LIMIT);
Expand All @@ -577,18 +643,18 @@ crate fn joined_uncovered_patterns<'p, 'tcx>(
}

crate fn pattern_not_covered_label(
witnesses: &[DeconstructedPat<'_, '_>],
witnesses: &[DeconstructedPat<'_, MatchCheckCtxt<'_>>],
joined_patterns: &str,
) -> String {
format!("pattern{} {} not covered", rustc_errors::pluralize!(witnesses.len()), joined_patterns)
}

/// Point at the definition of non-covered `enum` variants.
fn adt_defined_here<'p, 'tcx>(
cx: &MatchCheckCtxt<'p, 'tcx>,
cx: &MatchCheckCtxt<'tcx>,
err: &mut DiagnosticBuilder<'_>,
ty: Ty<'tcx>,
witnesses: &[DeconstructedPat<'p, 'tcx>],
witnesses: &[DeconstructedPat<'p, MatchCheckCtxt<'tcx>>],
) {
let ty = ty.peel_refs();
if let ty::Adt(def, _) = ty.kind() {
Expand All @@ -605,10 +671,13 @@ fn adt_defined_here<'p, 'tcx>(
}

fn maybe_point_at_variant<'a, 'p: 'a, 'tcx: 'a>(
cx: &MatchCheckCtxt<'p, 'tcx>,
cx: &MatchCheckCtxt<'tcx>,
def: &AdtDef,
patterns: impl Iterator<Item = &'a DeconstructedPat<'p, 'tcx>>,
) -> Vec<Span> {
patterns: impl Iterator<Item = &'a DeconstructedPat<'p, MatchCheckCtxt<'tcx>>>,
) -> Vec<Span>
where
MatchCheckCtxt<'tcx>: 'p,
{
use Constructor::*;
let mut covered = vec![];
for pattern in patterns {
Expand Down
Loading