Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Rollup merge of rust-lang#67026 - Nadrieril:improve-usefulness-empty,…
… r=varkor,Centril,estebank

Improve diagnostics and code for exhaustiveness of empty matches

There was a completely separate check and diagnostics for the case of an empty match. This led to slightly different error messages and duplicated code.
This improves code reuse and generally clarifies what happens for empty matches. This also clarifies the action of the `exhaustive_patterns` feature, and ensures that this feature doesn't change diagnostics in places it doesn't need to.
  • Loading branch information
Centril committed Dec 13, 2019
2 parents 3964a55 + fbd2cd0 commit df9e491
Show file tree
Hide file tree
Showing 20 changed files with 889 additions and 182 deletions.
119 changes: 69 additions & 50 deletions src/librustc_mir/hair/pattern/_match.rs
Expand Up @@ -238,7 +238,7 @@ use super::{FieldPat, Pat, PatKind, PatRange};
use rustc::hir::def_id::DefId;
use rustc::hir::{HirId, RangeEnd};
use rustc::ty::layout::{Integer, IntegerExt, Size, VariantIdx};
use rustc::ty::{self, Const, Ty, TyCtxt, TypeFoldable};
use rustc::ty::{self, Const, Ty, TyCtxt, TypeFoldable, VariantDef};

use rustc::lint;
use rustc::mir::interpret::{truncate, AllocId, ConstValue, Pointer, Scalar};
Expand Down Expand Up @@ -354,7 +354,7 @@ impl PatternFolder<'tcx> for LiteralExpander<'tcx> {
}

impl<'tcx> Pat<'tcx> {
fn is_wildcard(&self) -> bool {
pub(super) fn is_wildcard(&self) -> bool {
match *self.kind {
PatKind::Binding { subpattern: None, .. } | PatKind::Wild => true,
_ => false,
Expand Down Expand Up @@ -596,9 +596,21 @@ impl<'a, 'tcx> MatchCheckCtxt<'a, 'tcx> {
}
}

fn is_local(&self, ty: Ty<'tcx>) -> bool {
// Returns whether the given type is an enum from another crate declared `#[non_exhaustive]`.
pub fn is_foreign_non_exhaustive_enum(&self, ty: Ty<'tcx>) -> bool {
match ty.kind {
ty::Adt(adt_def, ..) => adt_def.did.is_local(),
ty::Adt(def, ..) => {
def.is_enum() && def.is_variant_list_non_exhaustive() && !def.did.is_local()
}
_ => false,
}
}

// Returns whether the given variant is from another crate and has its fields declared
// `#[non_exhaustive]`.
fn is_foreign_non_exhaustive_variant(&self, ty: Ty<'tcx>, variant: &VariantDef) -> bool {
match ty.kind {
ty::Adt(def, ..) => variant.is_field_list_non_exhaustive() && !def.did.is_local(),
_ => false,
}
}
Expand Down Expand Up @@ -758,6 +770,10 @@ impl<'tcx> Constructor<'tcx> {
// Returns the set of constructors covered by `self` but not by
// anything in `other_ctors`.
fn subtract_ctors(&self, other_ctors: &Vec<Constructor<'tcx>>) -> Vec<Constructor<'tcx>> {
if other_ctors.is_empty() {
return vec![self.clone()];
}

match self {
// Those constructors can only match themselves.
Single | Variant(_) | ConstantValue(..) | FloatRange(..) => {
Expand Down Expand Up @@ -858,8 +874,7 @@ impl<'tcx> Constructor<'tcx> {
vec![Pat::wildcard_from_ty(substs.type_at(0))]
} else {
let variant = &adt.variants[self.variant_index_for_adt(cx, adt)];
let is_non_exhaustive =
variant.is_field_list_non_exhaustive() && !cx.is_local(ty);
let is_non_exhaustive = cx.is_foreign_non_exhaustive_variant(ty, variant);
variant
.fields
.iter()
Expand Down Expand Up @@ -1205,6 +1220,8 @@ impl<'tcx> Witness<'tcx> {
///
/// We make sure to omit constructors that are statically impossible. E.g., for
/// `Option<!>`, we do not include `Some(_)` in the returned list of constructors.
/// Invariant: this returns an empty `Vec` if and only if the type is uninhabited (as determined by
/// `cx.is_uninhabited()`).
fn all_constructors<'a, 'tcx>(
cx: &mut MatchCheckCtxt<'a, 'tcx>,
pcx: PatCtxt<'tcx>,
Expand Down Expand Up @@ -1235,47 +1252,45 @@ fn all_constructors<'a, 'tcx>(
vec![Slice(Slice { array_len: None, kind })]
}
ty::Adt(def, substs) if def.is_enum() => {
let ctors: Vec<_> = def
.variants
.iter()
.filter(|v| {
!cx.tcx.features().exhaustive_patterns
|| !v
.uninhabited_from(cx.tcx, substs, def.adt_kind())
let ctors: Vec<_> = if cx.tcx.features().exhaustive_patterns {
// If `exhaustive_patterns` is enabled, we exclude variants known to be
// uninhabited.
def.variants
.iter()
.filter(|v| {
!v.uninhabited_from(cx.tcx, substs, def.adt_kind())
.contains(cx.tcx, cx.module)
})
.map(|v| Variant(v.def_id))
.collect();

// If our scrutinee is *privately* an empty enum, we must treat it as though it had an
// "unknown" constructor (in that case, all other patterns obviously can't be variants)
// to avoid exposing its emptyness. See the `match_privately_empty` test for details.
// FIXME: currently the only way I know of something can be a privately-empty enum is
// when the exhaustive_patterns feature flag is not present, so this is only needed for
// that case.
let is_privately_empty = ctors.is_empty() && !cx.is_uninhabited(pcx.ty);
// If the enum is declared as `#[non_exhaustive]`, we treat it as if it had an
// additionnal "unknown" constructor.
let is_declared_nonexhaustive =
def.is_variant_list_non_exhaustive() && !cx.is_local(pcx.ty);

if is_privately_empty || is_declared_nonexhaustive {
// There is no point in enumerating all possible variants, because the user can't
// actually match against them themselves. So we return only the fictitious
// constructor.
// E.g., in an example like:
// ```
// let err: io::ErrorKind = ...;
// match err {
// io::ErrorKind::NotFound => {},
// }
// ```
// we don't want to show every possible IO error, but instead have only `_` as the
// witness.
vec![NonExhaustive]
})
.map(|v| Variant(v.def_id))
.collect()
} else {
ctors
}
def.variants.iter().map(|v| Variant(v.def_id)).collect()
};

// If the enum is declared as `#[non_exhaustive]`, we treat it as if it had an
// additional "unknown" constructor.
// There is no point in enumerating all possible variants, because the user can't
// actually match against them all themselves. So we always return only the fictitious
// constructor.
// E.g., in an example like:
// ```
// let err: io::ErrorKind = ...;
// match err {
// io::ErrorKind::NotFound => {},
// }
// ```
// we don't want to show every possible IO error, but instead have only `_` as the
// witness.
let is_declared_nonexhaustive = cx.is_foreign_non_exhaustive_enum(pcx.ty);

// If `exhaustive_patterns` is disabled and our scrutinee is an empty enum, we treat it
// as though it had an "unknown" constructor to avoid exposing its emptyness. Note that
// an empty match will still be considered exhaustive because that case is handled
// separately in `check_match`.
let is_secretly_empty =
def.variants.is_empty() && !cx.tcx.features().exhaustive_patterns;

if is_secretly_empty || is_declared_nonexhaustive { vec![NonExhaustive] } else { ctors }
}
ty::Char => {
vec![
Expand Down Expand Up @@ -1605,6 +1620,7 @@ pub fn is_useful<'p, 'tcx>(
v: &PatStack<'p, 'tcx>,
witness_preference: WitnessPreference,
hir_id: HirId,
is_top_level: bool,
) -> Usefulness<'tcx, 'p> {
let &Matrix(ref rows) = matrix;
debug!("is_useful({:#?}, {:#?})", matrix, v);
Expand Down Expand Up @@ -1632,7 +1648,7 @@ pub fn is_useful<'p, 'tcx>(
let mut unreachable_pats = Vec::new();
let mut any_is_useful = false;
for v in vs {
let res = is_useful(cx, &matrix, &v, witness_preference, hir_id);
let res = is_useful(cx, &matrix, &v, witness_preference, hir_id, false);
match res {
Useful(pats) => {
any_is_useful = true;
Expand Down Expand Up @@ -1732,7 +1748,7 @@ pub fn is_useful<'p, 'tcx>(
} else {
let matrix = matrix.specialize_wildcard();
let v = v.to_tail();
let usefulness = is_useful(cx, &matrix, &v, witness_preference, hir_id);
let usefulness = is_useful(cx, &matrix, &v, witness_preference, hir_id, false);

// In this case, there's at least one "free"
// constructor that is only matched against by
Expand Down Expand Up @@ -1761,7 +1777,10 @@ pub fn is_useful<'p, 'tcx>(
// `(<direction-1>, <direction-2>, true)` - we are
// satisfied with `(_, _, true)`. In this case,
// `used_ctors` is empty.
if missing_ctors.all_ctors_are_missing() {
// The exception is: if we are at the top-level, for example in an empty match, we
// sometimes prefer reporting the list of constructors instead of just `_`.
let report_ctors_rather_than_wildcard = is_top_level && !IntRange::is_integral(pcx.ty);
if missing_ctors.all_ctors_are_missing() && !report_ctors_rather_than_wildcard {
// All constructors are unused. Add a wild pattern
// rather than each individual constructor.
usefulness.apply_wildcard(pcx.ty)
Expand Down Expand Up @@ -1793,7 +1812,7 @@ fn is_useful_specialized<'p, 'tcx>(
cx.pattern_arena.alloc_from_iter(ctor.wildcard_subpatterns(cx, lty));
let matrix = matrix.specialize_constructor(cx, &ctor, ctor_wild_subpatterns);
v.specialize_constructor(cx, &ctor, ctor_wild_subpatterns)
.map(|v| is_useful(cx, &matrix, &v, witness_preference, hir_id))
.map(|v| is_useful(cx, &matrix, &v, witness_preference, hir_id, false))
.map(|u| u.apply_constructor(cx, &ctor, lty))
.unwrap_or(NotUseful)
}
Expand Down Expand Up @@ -2308,7 +2327,7 @@ fn specialize_one_pattern<'p, 'tcx>(

PatKind::Variant { adt_def, variant_index, ref subpatterns, .. } => {
let ref variant = adt_def.variants[variant_index];
let is_non_exhaustive = variant.is_field_list_non_exhaustive() && !cx.is_local(pat.ty);
let is_non_exhaustive = cx.is_foreign_non_exhaustive_variant(pat.ty, variant);
Some(Variant(variant.def_id))
.filter(|variant_constructor| variant_constructor == constructor)
.map(|_| {
Expand Down
133 changes: 54 additions & 79 deletions src/librustc_mir/hair/pattern/check_match.rs
Expand Up @@ -148,8 +148,8 @@ impl<'tcx> MatchVisitor<'_, 'tcx> {
self.tables,
);
patcx.include_lint_checks();
let pattern: &_ =
cx.pattern_arena.alloc(expand_pattern(cx, patcx.lower_pattern(&arm.pat)));
let pattern = patcx.lower_pattern(&arm.pat);
let pattern: &_ = cx.pattern_arena.alloc(expand_pattern(cx, pattern));
if !patcx.errors.is_empty() {
patcx.report_inlining_errors(arm.pat.span);
have_errors = true;
Expand All @@ -166,73 +166,12 @@ impl<'tcx> MatchVisitor<'_, 'tcx> {
// Fourth, check for unreachable arms.
let matrix = check_arms(cx, &inlined_arms, source);

// Then, if the match has no arms, check whether the scrutinee
// is uninhabited.
let pat_ty = self.tables.node_type(scrut.hir_id);
let module = self.tcx.hir().get_module_parent(scrut.hir_id);
let mut def_span = None;
let mut missing_variants = vec![];
if inlined_arms.is_empty() {
let scrutinee_is_uninhabited = if self.tcx.features().exhaustive_patterns {
self.tcx.is_ty_uninhabited_from(module, pat_ty)
} else {
match pat_ty.kind {
ty::Never => true,
ty::Adt(def, _) => {
def_span = self.tcx.hir().span_if_local(def.did);
if def.variants.len() < 4 && !def.variants.is_empty() {
// keep around to point at the definition of non-covered variants
missing_variants =
def.variants.iter().map(|variant| variant.ident).collect();
}

let is_non_exhaustive_and_non_local =
def.is_variant_list_non_exhaustive() && !def.did.is_local();

!(is_non_exhaustive_and_non_local) && def.variants.is_empty()
}
_ => false,
}
};
if !scrutinee_is_uninhabited {
// We know the type is inhabited, so this must be wrong
let mut err = create_e0004(
self.tcx.sess,
scrut.span,
format!(
"non-exhaustive patterns: {}",
match missing_variants.len() {
0 => format!("type `{}` is non-empty", pat_ty),
1 => format!(
"pattern `{}` of type `{}` is not handled",
missing_variants[0].name, pat_ty,
),
_ => format!(
"multiple patterns of type `{}` are not handled",
pat_ty
),
}
),
);
err.help(
"ensure that all possible cases are being handled, \
possibly by adding wildcards or more match arms",
);
if let Some(sp) = def_span {
err.span_label(sp, format!("`{}` defined here", pat_ty));
}
// point at the definition of non-covered enum variants
for variant in &missing_variants {
err.span_label(variant.span, "variant not covered");
}
err.emit();
}
// If the type *is* uninhabited, it's vacuously exhaustive
return;
}

// Fifth, check if the match is exhaustive.
let scrut_ty = self.tables.node_type(scrut.hir_id);
check_exhaustive(cx, scrut_ty, scrut.span, &matrix, scrut.hir_id);
// Note: An empty match isn't the same as an empty matrix for diagnostics purposes,
// since an empty matrix can occur when there are arms, if those arms all have guards.
let is_empty_match = inlined_arms.is_empty();
check_exhaustive(cx, scrut_ty, scrut.span, &matrix, scrut.hir_id, is_empty_match);
})
}

Expand Down Expand Up @@ -390,7 +329,7 @@ fn check_arms<'p, 'tcx>(
for (arm_index, (pat, hir_pat, has_guard)) in arms.iter().enumerate() {
let v = PatStack::from_pattern(pat);

match is_useful(cx, &seen, &v, LeaveOutWitness, hir_pat.hir_id) {
match is_useful(cx, &seen, &v, LeaveOutWitness, hir_pat.hir_id, true) {
NotUseful => {
match source {
hir::MatchSource::IfDesugar { .. } | hir::MatchSource::WhileDesugar => bug!(),
Expand Down Expand Up @@ -478,7 +417,8 @@ fn check_not_useful<'p, 'tcx>(
hir_id: HirId,
) -> Result<(), Vec<super::Pat<'tcx>>> {
let wild_pattern = cx.pattern_arena.alloc(super::Pat::wildcard_from_ty(ty));
match is_useful(cx, matrix, &PatStack::from_pattern(wild_pattern), ConstructWitness, hir_id) {
let v = PatStack::from_pattern(wild_pattern);
match is_useful(cx, matrix, &v, ConstructWitness, hir_id, true) {
NotUseful => Ok(()), // This is good, wildcard pattern isn't reachable.
UsefulWithWitness(pats) => Err(if pats.is_empty() {
bug!("Exhaustiveness check returned no witnesses")
Expand All @@ -495,25 +435,60 @@ fn check_exhaustive<'p, 'tcx>(
sp: Span,
matrix: &Matrix<'p, 'tcx>,
hir_id: HirId,
is_empty_match: bool,
) {
// In the absence of the `exhaustive_patterns` feature, empty matches are not detected by
// `is_useful` to exhaustively match uninhabited types, so we manually check here.
if is_empty_match && !cx.tcx.features().exhaustive_patterns {
let scrutinee_is_visibly_uninhabited = match scrut_ty.kind {
ty::Never => true,
ty::Adt(def, _) => {
def.is_enum()
&& def.variants.is_empty()
&& !cx.is_foreign_non_exhaustive_enum(scrut_ty)
}
_ => false,
};
if scrutinee_is_visibly_uninhabited {
// If the type *is* uninhabited, an empty match is vacuously exhaustive.
return;
}
}

let witnesses = match check_not_useful(cx, scrut_ty, matrix, hir_id) {
Ok(_) => return,
Err(err) => err,
};

let joined_patterns = joined_uncovered_patterns(&witnesses);
let mut err = create_e0004(
cx.tcx.sess,
sp,
format!("non-exhaustive patterns: {} not covered", joined_patterns),
);
err.span_label(sp, pattern_not_covered_label(&witnesses, &joined_patterns));
let non_empty_enum = match scrut_ty.kind {
ty::Adt(def, _) => def.is_enum() && !def.variants.is_empty(),
_ => false,
};
// In the case of an empty match, replace the '`_` not covered' diagnostic with something more
// informative.
let mut err;
if is_empty_match && !non_empty_enum {
err = create_e0004(
cx.tcx.sess,
sp,
format!("non-exhaustive patterns: type `{}` is non-empty", scrut_ty),
);
} else {
let joined_patterns = joined_uncovered_patterns(&witnesses);
err = create_e0004(
cx.tcx.sess,
sp,
format!("non-exhaustive patterns: {} not covered", joined_patterns),
);
err.span_label(sp, pattern_not_covered_label(&witnesses, &joined_patterns));
};

adt_defined_here(cx, &mut err, scrut_ty, &witnesses);
err.help(
"ensure that all possible cases are being handled, \
possibly by adding wildcards or more match arms",
)
.emit();
);
err.emit();
}

fn joined_uncovered_patterns(witnesses: &[super::Pat<'_>]) -> String {
Expand Down

0 comments on commit df9e491

Please sign in to comment.