Skip to content
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.

Sign up for GitHub

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

Point at enum definition when match patterns are not exhaustive #58380

Merged
merged 3 commits into from
Mar 4, 2019
Merged

Point at enum definition when match patterns are not exhaustive #58380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0081ef2
Point at enum definition when match patterns are not exhaustive
estebank Feb 10, 2019
509bb0a
Use anonymous explicit lifetimes
estebank Feb 11, 2019
d651281
Reword error message
estebank Mar 3, 2019
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
1 change: 0 additions & 1 deletion src/librustc_mir/hair/pattern/_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,6 @@ impl<'tcx> Witness<'tcx> {
self.apply_constructor(cx, ctor, ty)
}


/// Constructs a partial witness for a pattern given a list of
/// patterns expanded by the specialization step.
///
Expand Down
144 changes: 116 additions & 28 deletions src/librustc_mir/hair/pattern/check_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use rustc::middle::expr_use_visitor as euv;
use rustc::middle::mem_categorization::cmt_;
use rustc::middle::region;
use rustc::session::Session;
use rustc::ty::{self, Ty, TyCtxt};
use rustc::ty::{self, Ty, TyCtxt, TyKind};
use rustc::ty::subst::{InternalSubsts, SubstsRef};
use rustc::lint;
use rustc_errors::{Applicability, DiagnosticBuilder};
Expand Down Expand Up @@ -204,25 +204,51 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> {
// is uninhabited.
let pat_ty = self.tables.node_type(scrut.hir_id);
let module = self.tcx.hir().get_module_parent_by_hir_id(scrut.hir_id);
let mut def_span = None;
let mut missing_variants = vec![];
Copy link
Member

Choose a reason for hiding this comment

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

I usually write empty vectors as Vec::new() (thinking, "no need to the macro sugar when there aren't any elements to initialize"), but rustfmt doesn't seem to have an opinion. (I guess you could argue that it's technically a semantic change.)

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.sty {
ty::Never => true,
ty::Adt(def, _) => def.variants.is_empty(),
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();
}
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: type `{}` \
is non-empty",
pat_ty));
span_help!(&mut err, scrut.span,
"ensure that all possible cases are being handled, \
possibly by adding wildcards or more match arms");
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
Expand Down Expand Up @@ -264,7 +290,7 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> {
};

let pattern_string = witness[0].single_pattern().to_string();
let mut diag = struct_span_err!(
let mut err = struct_span_err!(
self.tcx.sess, pat.span, E0005,
"refutable pattern in {}: `{}` not covered",
origin, pattern_string
Expand All @@ -277,8 +303,13 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> {
}
_ => format!("pattern `{}` not covered", pattern_string),
};
diag.span_label(pat.span, label_msg);
diag.emit();
err.span_label(pat.span, label_msg);
if let ty::Adt(def, _) = pattern_ty.sty {
if let Some(sp) = self.tcx.hir().span_if_local(def.did){
err.span_label(sp, format!("`{}` defined here", pattern_ty));
}
}
err.emit();
});
}
}
Expand Down Expand Up @@ -332,10 +363,11 @@ fn pat_is_catchall(pat: &Pat) -> bool {
}

// Check for unreachable patterns
fn check_arms<'a, 'tcx>(cx: &mut MatchCheckCtxt<'a, 'tcx>,
arms: &[(Vec<(&'a Pattern<'tcx>, &hir::Pat)>, Option<&hir::Expr>)],
source: hir::MatchSource)
{
fn check_arms<'a, 'tcx>(
cx: &mut MatchCheckCtxt<'a, 'tcx>,
arms: &[(Vec<(&'a Pattern<'tcx>, &hir::Pat)>, Option<&hir::Expr>)],
source: hir::MatchSource,
) {
let mut seen = Matrix::empty();
let mut catchall = None;
for (arm_index, &(ref pats, guard)) in arms.iter().enumerate() {
Expand Down Expand Up @@ -411,10 +443,12 @@ fn check_arms<'a, 'tcx>(cx: &mut MatchCheckCtxt<'a, 'tcx>,
}
}

fn check_exhaustive<'p, 'a: 'p, 'tcx: 'a>(cx: &mut MatchCheckCtxt<'a, 'tcx>,
scrut_ty: Ty<'tcx>,
sp: Span,
matrix: &Matrix<'p, 'tcx>) {
fn check_exhaustive<'p, 'a: 'p, 'tcx: 'a>(
cx: &mut MatchCheckCtxt<'a, 'tcx>,
scrut_ty: Ty<'tcx>,
sp: Span,
matrix: &Matrix<'p, 'tcx>,
) {
let wild_pattern = Pattern {
ty: scrut_ty,
span: DUMMY_SP,
Expand Down Expand Up @@ -448,11 +482,26 @@ fn check_exhaustive<'p, 'a: 'p, 'tcx: 'a>(cx: &mut MatchCheckCtxt<'a, 'tcx>,
1 => format!("pattern {} not covered", joined_patterns),
_ => format!("patterns {} not covered", joined_patterns),
};
create_e0004(cx.tcx.sess, sp,
format!("non-exhaustive patterns: {} not covered",
joined_patterns))
.span_label(sp, label_text)
.emit();
let mut err = create_e0004(cx.tcx.sess, sp, format!(
"non-exhaustive patterns: {} not covered",
joined_patterns,
));
err.span_label(sp, label_text);
// point at the definition of non-covered enum variants
if let ty::Adt(def, _) = scrut_ty.sty {
if let Some(sp) = cx.tcx.hir().span_if_local(def.did){
err.span_label(sp, format!("`{}` defined here", scrut_ty));
}
}
let patterns = witnesses.iter().map(|p| (**p).clone()).collect::<Vec<Pattern<'_>>>();
if patterns.len() < 4 {
for sp in maybe_point_at_variant(cx, &scrut_ty.sty, patterns.as_slice()) {
err.span_label(sp, "not covered");
}
}
err.help("ensure that all possible cases are being handled, \
possibly by adding wildcards or more match arms");
err.emit();
}
NotUseful => {
// This is good, wildcard pattern isn't reachable
Expand All @@ -461,10 +510,49 @@ fn check_exhaustive<'p, 'a: 'p, 'tcx: 'a>(cx: &mut MatchCheckCtxt<'a, 'tcx>,
}
}

fn maybe_point_at_variant(
cx: &mut MatchCheckCtxt<'a, 'tcx>,
sty: &TyKind<'tcx>,
patterns: &[Pattern<'_>],
) -> Vec<Span> {
let mut covered = vec![];
if let ty::Adt(def, _) = sty {
// Don't point at variants that have already been covered due to other patterns to avoid
// visual clutter
for pattern in patterns {
let pk: &PatternKind<'_> = &pattern.kind;
if let PatternKind::Variant { adt_def, variant_index, subpatterns, .. } = pk {
if adt_def.did == def.did {
let sp = def.variants[*variant_index].ident.span;
if covered.contains(&sp) {
continue;
}
covered.push(sp);
let subpatterns = subpatterns.iter()
.map(|field_pattern| field_pattern.pattern.clone())
.collect::<Vec<_>>();
covered.extend(
maybe_point_at_variant(cx, sty, subpatterns.as_slice()),
);
}
}
if let PatternKind::Leaf { subpatterns } = pk {
let subpatterns = subpatterns.iter()
.map(|field_pattern| field_pattern.pattern.clone())
.collect::<Vec<_>>();
covered.extend(maybe_point_at_variant(cx, sty, subpatterns.as_slice()));
}
}
}
covered
}

// Legality of move bindings checking
fn check_legality_of_move_bindings(cx: &MatchVisitor<'_, '_>,
has_guard: bool,
pats: &[P<Pat>]) {
fn check_legality_of_move_bindings(
cx: &MatchVisitor<'_, '_>,
has_guard: bool,
pats: &[P<Pat>],
) {
let mut by_ref_span = None;
for pat in pats {
pat.each_binding(|_, hir_id, span, _path| {
Expand Down
12 changes: 2 additions & 10 deletions src/test/ui/always-inhabited-union-ref.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,15 @@ error[E0004]: non-exhaustive patterns: type `&'static !` is non-empty
LL | match uninhab_ref() {
| ^^^^^^^^^^^^^
|
help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
--> $DIR/always-inhabited-union-ref.rs:23:11
|
LL | match uninhab_ref() {
| ^^^^^^^^^^^^^
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error[E0004]: non-exhaustive patterns: type `Foo` is non-empty
--> $DIR/always-inhabited-union-ref.rs:27:11
|
LL | match uninhab_union() {
| ^^^^^^^^^^^^^^^
|
help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
--> $DIR/always-inhabited-union-ref.rs:27:11
|
LL | match uninhab_union() {
| ^^^^^^^^^^^^^^^
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error: aborting due to 2 previous errors

Expand Down
22 changes: 22 additions & 0 deletions src/test/ui/check_match/issue-35609.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,48 +3,70 @@ error[E0004]: non-exhaustive patterns: `(B, _)`, `(C, _)`, `(D, _)` and 2 more n
|
LL | match (A, ()) { //~ ERROR non-exhaustive
| ^^^^^^^ patterns `(B, _)`, `(C, _)`, `(D, _)` and 2 more not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error[E0004]: non-exhaustive patterns: `(_, B)`, `(_, C)`, `(_, D)` and 2 more not covered
--> $DIR/issue-35609.rs:14:11
|
LL | match (A, A) { //~ ERROR non-exhaustive
| ^^^^^^ patterns `(_, B)`, `(_, C)`, `(_, D)` and 2 more not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error[E0004]: non-exhaustive patterns: `((B, _), _)`, `((C, _), _)`, `((D, _), _)` and 2 more not covered
--> $DIR/issue-35609.rs:18:11
|
LL | match ((A, ()), ()) { //~ ERROR non-exhaustive
| ^^^^^^^^^^^^^ patterns `((B, _), _)`, `((C, _), _)`, `((D, _), _)` and 2 more not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error[E0004]: non-exhaustive patterns: `((B, _), _)`, `((C, _), _)`, `((D, _), _)` and 2 more not covered
--> $DIR/issue-35609.rs:22:11
|
LL | match ((A, ()), A) { //~ ERROR non-exhaustive
| ^^^^^^^^^^^^ patterns `((B, _), _)`, `((C, _), _)`, `((D, _), _)` and 2 more not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error[E0004]: non-exhaustive patterns: `((B, _), _)`, `((C, _), _)`, `((D, _), _)` and 2 more not covered
--> $DIR/issue-35609.rs:26:11
|
LL | match ((A, ()), ()) { //~ ERROR non-exhaustive
| ^^^^^^^^^^^^^ patterns `((B, _), _)`, `((C, _), _)`, `((D, _), _)` and 2 more not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error[E0004]: non-exhaustive patterns: `S(B, _)`, `S(C, _)`, `S(D, _)` and 2 more not covered
--> $DIR/issue-35609.rs:31:11
|
LL | struct S(Enum, ());
| ------------------- `S` defined here
...
LL | match S(A, ()) { //~ ERROR non-exhaustive
| ^^^^^^^^ patterns `S(B, _)`, `S(C, _)`, `S(D, _)` and 2 more not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error[E0004]: non-exhaustive patterns: `Sd { x: B, .. }`, `Sd { x: C, .. }`, `Sd { x: D, .. }` and 2 more not covered
--> $DIR/issue-35609.rs:35:11
|
LL | struct Sd { x: Enum, y: () }
| ---------------------------- `Sd` defined here
...
LL | match (Sd { x: A, y: () }) { //~ ERROR non-exhaustive
| ^^^^^^^^^^^^^^^^^^^^ patterns `Sd { x: B, .. }`, `Sd { x: C, .. }`, `Sd { x: D, .. }` and 2 more not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error[E0004]: non-exhaustive patterns: `Some(B)`, `Some(C)`, `Some(D)` and 2 more not covered
--> $DIR/issue-35609.rs:39:11
|
LL | match Some(A) { //~ ERROR non-exhaustive
| ^^^^^^^ patterns `Some(B)`, `Some(C)`, `Some(D)` and 2 more not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error: aborting due to 8 previous errors

Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/consts/match_ice.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ error[E0004]: non-exhaustive patterns: `&S` not covered
|
LL | match C { //~ ERROR non-exhaustive
| ^ pattern `&S` not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error: aborting due to previous error

Expand Down
11 changes: 9 additions & 2 deletions src/test/ui/empty/empty-never-array.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
error[E0005]: refutable pattern in local binding: `T(_, _)` not covered
--> $DIR/empty-never-array.rs:10:9
|
LL | let Helper::U(u) = Helper::T(t, []);
| ^^^^^^^^^^^^ pattern `T(_, _)` not covered
LL | / enum Helper<T, U> {
LL | | T(T, [!; 0]),
LL | | #[allow(dead_code)]
LL | | U(U),
LL | | }
| |_- `Helper<T, U>` defined here
...
LL | let Helper::U(u) = Helper::T(t, []);
| ^^^^^^^^^^^^ pattern `T(_, _)` not covered

error: aborting due to previous error

Expand Down
8 changes: 2 additions & 6 deletions src/test/ui/error-codes/E0004-2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
error[E0004]: non-exhaustive patterns: type `std::option::Option<i32>` is non-empty
error[E0004]: non-exhaustive patterns: multiple patterns of type `std::option::Option<i32>` are not handled
--> $DIR/E0004-2.rs:4:11
|
LL | match x { } //~ ERROR E0004
| ^
|
help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
--> $DIR/E0004-2.rs:4:11
|
LL | match x { } //~ ERROR E0004
| ^
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error: aborting due to previous error

Expand Down
13 changes: 11 additions & 2 deletions src/test/ui/error-codes/E0004.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
error[E0004]: non-exhaustive patterns: `HastaLaVistaBaby` not covered
--> $DIR/E0004.rs:9:11
|
LL | match x { //~ ERROR E0004
| ^ pattern `HastaLaVistaBaby` not covered
LL | / enum Terminator {
LL | | HastaLaVistaBaby,
| | ---------------- not covered
LL | | TalkToMyHand,
LL | | }
| |_- `Terminator` defined here
...
LL | match x { //~ ERROR E0004
Copy link
Member

Choose a reason for hiding this comment

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

(possibly a bad idea, probably out of scope for this PR) I find it counterintuitive for the primary span of a "non-exaustive patterns" error to point to the x in match x, rather than the body of match arms. (That could get very visually cluttered, though, especially with the secondary definition-spans introduced here. Also, pointing at the discriminant does seem intuitive when it's a pattern rather than an opaque variable, as in the examples in src/test/ui/check_match/issue-35609.stderr.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have the same thoughts you do, but I lay on the side of using the smallest possible primary span, for VSCode's sake.

| ^ pattern `HastaLaVistaBaby` not covered
|
= help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

error: aborting due to previous error

Expand Down
Loading