From 6100743842ace6db859edfeed9959ac941210c2b Mon Sep 17 00:00:00 2001 From: Vincent Esche Date: Tue, 19 Jan 2016 08:40:07 +0100 Subject: [PATCH 1/4] Improve error message for non-exhaustive patterns Changes error message from displaying first found missing constructor witness to showing up to 10, if necessary. Fixes issue #16884. --- src/librustc/middle/check_match.rs | 40 ++++++++++--------- .../non-exhaustive-pattern-witness.rs | 2 +- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index 8e5c5788201cc..2d882b4c97d88 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -368,31 +368,34 @@ fn raw_pat<'a>(p: &'a Pat) -> &'a Pat { fn check_exhaustive(cx: &MatchCheckCtxt, sp: Span, matrix: &Matrix, source: hir::MatchSource) { match is_useful(cx, matrix, &[DUMMY_WILD_PAT], ConstructWitness) { UsefulWithWitness(pats) => { - let witness = match &pats[..] { - [ref witness] => &**witness, - [] => DUMMY_WILD_PAT, - _ => unreachable!() + let witnesses = match &pats[..] { + [] => vec![DUMMY_WILD_PAT], + [p..] => { + p.iter().map(|w| &**w ).collect() + } }; match source { hir::MatchSource::ForLoopDesugar => { - // `witness` has the form `Some()`, peel off the `Some` - let witness = match witness.node { + // `witnesses[0]` has the form `Some()`, peel off the `Some` + let witness = match witnesses[0].node { hir::PatEnum(_, Some(ref pats)) => match &pats[..] { [ref pat] => &**pat, _ => unreachable!(), }, _ => unreachable!(), }; - span_err!(cx.tcx.sess, sp, E0297, "refutable pattern in `for` loop binding: \ `{}` not covered", pat_to_string(witness)); }, _ => { + let pattern_strings: Vec<_> = witnesses.iter().map(|w| { + pat_to_string(w) + }).take(10).collect(); span_err!(cx.tcx.sess, sp, E0004, "non-exhaustive patterns: `{}` not covered", - pat_to_string(witness) + pattern_strings.join("`, `") ); }, } @@ -594,14 +597,14 @@ impl<'tcx, 'container> ty::AdtDefData<'tcx, 'container> { } } -fn missing_constructor(cx: &MatchCheckCtxt, &Matrix(ref rows): &Matrix, - left_ty: Ty, max_slice_length: usize) -> Option { +fn missing_constructors(cx: &MatchCheckCtxt, &Matrix(ref rows): &Matrix, + left_ty: Ty, max_slice_length: usize) -> Vec { let used_constructors: Vec = rows.iter() .flat_map(|row| pat_constructors(cx, row[0], left_ty, max_slice_length)) .collect(); all_constructors(cx, left_ty, max_slice_length) .into_iter() - .find(|c| !used_constructors.contains(c)) + .filter(|c| !used_constructors.contains(c)).collect() } /// This determines the set of all possible constructors of a pattern matching @@ -680,8 +683,8 @@ fn is_useful(cx: &MatchCheckCtxt, let constructors = pat_constructors(cx, v[0], left_ty, max_slice_length); if constructors.is_empty() { - match missing_constructor(cx, matrix, left_ty, max_slice_length) { - None => { + match &missing_constructors(cx, matrix, left_ty, max_slice_length)[..] { + [] => { all_constructors(cx, left_ty, max_slice_length).into_iter().map(|c| { match is_useful_specialized(cx, matrix, v, c.clone(), left_ty, witness) { UsefulWithWitness(pats) => UsefulWithWitness({ @@ -701,7 +704,7 @@ fn is_useful(cx: &MatchCheckCtxt, }).find(|result| result != &NotUseful).unwrap_or(NotUseful) }, - Some(constructor) => { + [constructors..] => { let matrix = rows.iter().filter_map(|r| { if pat_is_binding_or_wild(&cx.tcx.def_map.borrow(), raw_pat(r[0])) { Some(r[1..].to_vec()) @@ -711,10 +714,11 @@ fn is_useful(cx: &MatchCheckCtxt, }).collect(); match is_useful(cx, &matrix, &v[1..], witness) { UsefulWithWitness(pats) => { - let arity = constructor_arity(cx, &constructor, left_ty); - let wild_pats = vec![DUMMY_WILD_PAT; arity]; - let enum_pat = construct_witness(cx, &constructor, wild_pats, left_ty); - let mut new_pats = vec![enum_pat]; + let mut new_pats: Vec<_> = constructors.into_iter().map(|constructor| { + let arity = constructor_arity(cx, &constructor, left_ty); + let wild_pats = vec![DUMMY_WILD_PAT; arity]; + construct_witness(cx, &constructor, wild_pats, left_ty) + }).collect(); new_pats.extend(pats); UsefulWithWitness(new_pats) }, diff --git a/src/test/compile-fail/non-exhaustive-pattern-witness.rs b/src/test/compile-fail/non-exhaustive-pattern-witness.rs index 146265bf0e150..62e61e8bf59f7 100644 --- a/src/test/compile-fail/non-exhaustive-pattern-witness.rs +++ b/src/test/compile-fail/non-exhaustive-pattern-witness.rs @@ -34,7 +34,7 @@ fn struct_with_a_nested_enum_and_vector() { fn enum_with_multiple_missing_variants() { match Color::Red { - //~^ ERROR non-exhaustive patterns: `Red` not covered + //~^ ERROR non-exhaustive patterns: `Red`, `Green` not covered Color::CustomRGBA { .. } => () } } From 48e83268930e2d21ff8894dc2eb65767d5b858fe Mon Sep 17 00:00:00 2001 From: Vincent Esche Date: Tue, 19 Jan 2016 12:13:40 +0100 Subject: [PATCH 2/4] Refined error message. More human-readable error message showing ellipsis for excessively long witness lists. --- src/librustc/middle/check_match.rs | 24 ++++++++---- .../non-exhaustive-pattern-witness.rs | 39 +++++++++++++++---- 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index 2d882b4c97d88..100e78d2f4564 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -368,11 +368,10 @@ fn raw_pat<'a>(p: &'a Pat) -> &'a Pat { fn check_exhaustive(cx: &MatchCheckCtxt, sp: Span, matrix: &Matrix, source: hir::MatchSource) { match is_useful(cx, matrix, &[DUMMY_WILD_PAT], ConstructWitness) { UsefulWithWitness(pats) => { - let witnesses = match &pats[..] { - [] => vec![DUMMY_WILD_PAT], - [p..] => { - p.iter().map(|w| &**w ).collect() - } + let witnesses = if pats.is_empty() { + vec![DUMMY_WILD_PAT] + } else { + pats.iter().map(|w| &**w ).collect() }; match source { hir::MatchSource::ForLoopDesugar => { @@ -392,10 +391,21 @@ fn check_exhaustive(cx: &MatchCheckCtxt, sp: Span, matrix: &Matrix, source: hir: _ => { let pattern_strings: Vec<_> = witnesses.iter().map(|w| { pat_to_string(w) - }).take(10).collect(); + }).collect(); + let (tail, head) = pattern_strings.split_last().unwrap(); + const HEAD_LIMIT: usize = 9; + let joined_patterns = match head.len() { + 0 => tail.clone(), + 1...HEAD_LIMIT => head.join("`, `") + "` and `" + tail, + _ => { + let head_iter = head.to_owned().into_iter(); + let truncated_head: Vec<_> = head_iter.take(HEAD_LIMIT).collect(); + truncated_head.join("`, `") + "`, … and `" + tail + } + }; span_err!(cx.tcx.sess, sp, E0004, "non-exhaustive patterns: `{}` not covered", - pattern_strings.join("`, `") + joined_patterns ); }, } diff --git a/src/test/compile-fail/non-exhaustive-pattern-witness.rs b/src/test/compile-fail/non-exhaustive-pattern-witness.rs index 62e61e8bf59f7..a84d42f1a7028 100644 --- a/src/test/compile-fail/non-exhaustive-pattern-witness.rs +++ b/src/test/compile-fail/non-exhaustive-pattern-witness.rs @@ -16,12 +16,6 @@ struct Foo { second: Option<[usize; 4]> } -enum Color { - Red, - Green, - CustomRGBA { a: bool, r: u8, g: u8, b: u8 } -} - fn struct_with_a_nested_enum_and_vector() { match (Foo { first: true, second: None }) { //~^ ERROR non-exhaustive patterns: `Foo { first: false, second: Some([_, _, _, _]) }` not covered @@ -32,13 +26,42 @@ fn struct_with_a_nested_enum_and_vector() { } } -fn enum_with_multiple_missing_variants() { +enum Color { + Red, + Green, + CustomRGBA { a: bool, r: u8, g: u8, b: u8 } +} + +fn enum_with_two_missing_variants() { match Color::Red { - //~^ ERROR non-exhaustive patterns: `Red`, `Green` not covered + //~^ ERROR non-exhaustive patterns: `Red` and `Green` not covered Color::CustomRGBA { .. } => () } } +enum Direction { + North, East, South, West +} + +fn enum_with_three_or_more_missing_variants() { + match Direction::North { + //~^ ERROR non-exhaustive patterns: `East`, `South` and `West` not covered + Direction::North => () + } +} + +enum ExcessiveEnum { + First, Second, Third, Fourth, Fifth, Sixth, Seventh, Eighth, Ninth, Tenth, Eleventh, Twelfth +} + +fn enum_with_excessive_missing_variants() { + match ExcessiveEnum::First { + //~^ ERROR `Sixth`, `Seventh`, `Eighth`, `Ninth`, `Tenth`, … and `Twelfth` not covered + + ExcessiveEnum::First => () + } +} + fn enum_struct_variant() { match Color::Red { //~^ ERROR non-exhaustive patterns: `CustomRGBA { a: true, .. }` not covered From 70692ce27953c91800549d6929b24e32b003c4f0 Mon Sep 17 00:00:00 2001 From: Vincent Esche Date: Thu, 21 Jan 2016 20:52:11 +0100 Subject: [PATCH 3/4] Refined error message to truncate at 3 and hint at number of hidden patterns for excessive cases. --- src/librustc/middle/check_match.rs | 20 ++++++++++--------- .../non-exhaustive-pattern-witness.rs | 11 +++++----- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index 100e78d2f4564..6ed576d209f2f 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -392,19 +392,21 @@ fn check_exhaustive(cx: &MatchCheckCtxt, sp: Span, matrix: &Matrix, source: hir: let pattern_strings: Vec<_> = witnesses.iter().map(|w| { pat_to_string(w) }).collect(); - let (tail, head) = pattern_strings.split_last().unwrap(); - const HEAD_LIMIT: usize = 9; - let joined_patterns = match head.len() { - 0 => tail.clone(), - 1...HEAD_LIMIT => head.join("`, `") + "` and `" + tail, + const LIMIT: usize = 3; + let joined_patterns = match pattern_strings.len() { + 0 => unreachable!(), + 1 => format!("`{}`", pattern_strings[0]), + 2...LIMIT => { + let (tail, head) = pattern_strings.split_last().unwrap(); + format!("`{}`", head.join("`, `") + "` and `" + tail) + }, _ => { - let head_iter = head.to_owned().into_iter(); - let truncated_head: Vec<_> = head_iter.take(HEAD_LIMIT).collect(); - truncated_head.join("`, `") + "`, … and `" + tail + let (head, tail) = pattern_strings.split_at(LIMIT); + format!("`{}` and {} more", head.join("`, `"), tail.len()) } }; span_err!(cx.tcx.sess, sp, E0004, - "non-exhaustive patterns: `{}` not covered", + "non-exhaustive patterns: {} not covered", joined_patterns ); }, diff --git a/src/test/compile-fail/non-exhaustive-pattern-witness.rs b/src/test/compile-fail/non-exhaustive-pattern-witness.rs index a84d42f1a7028..b986878f78396 100644 --- a/src/test/compile-fail/non-exhaustive-pattern-witness.rs +++ b/src/test/compile-fail/non-exhaustive-pattern-witness.rs @@ -32,10 +32,11 @@ enum Color { CustomRGBA { a: bool, r: u8, g: u8, b: u8 } } -fn enum_with_two_missing_variants() { +fn enum_with_single_missing_variant() { match Color::Red { - //~^ ERROR non-exhaustive patterns: `Red` and `Green` not covered - Color::CustomRGBA { .. } => () + //~^ ERROR non-exhaustive patterns: `Red` not covered + Color::CustomRGBA { .. } => (), + Color::Green => () } } @@ -43,7 +44,7 @@ enum Direction { North, East, South, West } -fn enum_with_three_or_more_missing_variants() { +fn enum_with_multiple_missing_variants() { match Direction::North { //~^ ERROR non-exhaustive patterns: `East`, `South` and `West` not covered Direction::North => () @@ -56,7 +57,7 @@ enum ExcessiveEnum { fn enum_with_excessive_missing_variants() { match ExcessiveEnum::First { - //~^ ERROR `Sixth`, `Seventh`, `Eighth`, `Ninth`, `Tenth`, … and `Twelfth` not covered + //~^ ERROR `Second`, `Third`, `Fourth` and 8 more not covered ExcessiveEnum::First => () } From 2c7a19a10df23f132dfa96bea49bc3a6a16cc231 Mon Sep 17 00:00:00 2001 From: Vincent Esche Date: Fri, 22 Jan 2016 17:13:17 +0100 Subject: [PATCH 4/4] Adjusted PR to better match project's coding style. --- src/librustc/middle/check_match.rs | 78 +++++++++++++++--------------- 1 file changed, 38 insertions(+), 40 deletions(-) diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index 6ed576d209f2f..192c512044be9 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -616,7 +616,8 @@ fn missing_constructors(cx: &MatchCheckCtxt, &Matrix(ref rows): &Matrix, .collect(); all_constructors(cx, left_ty, max_slice_length) .into_iter() - .filter(|c| !used_constructors.contains(c)).collect() + .filter(|c| !used_constructors.contains(c)) + .collect() } /// This determines the set of all possible constructors of a pattern matching @@ -695,47 +696,44 @@ fn is_useful(cx: &MatchCheckCtxt, let constructors = pat_constructors(cx, v[0], left_ty, max_slice_length); if constructors.is_empty() { - match &missing_constructors(cx, matrix, left_ty, max_slice_length)[..] { - [] => { - all_constructors(cx, left_ty, max_slice_length).into_iter().map(|c| { - match is_useful_specialized(cx, matrix, v, c.clone(), left_ty, witness) { - UsefulWithWitness(pats) => UsefulWithWitness({ - let arity = constructor_arity(cx, &c, left_ty); - let mut result = { - let pat_slice = &pats[..]; - let subpats: Vec<_> = (0..arity).map(|i| { - pat_slice.get(i).map_or(DUMMY_WILD_PAT, |p| &**p) - }).collect(); - vec![construct_witness(cx, &c, subpats, left_ty)] - }; - result.extend(pats.into_iter().skip(arity)); - result - }), - result => result - } - }).find(|result| result != &NotUseful).unwrap_or(NotUseful) - }, - - [constructors..] => { - let matrix = rows.iter().filter_map(|r| { - if pat_is_binding_or_wild(&cx.tcx.def_map.borrow(), raw_pat(r[0])) { - Some(r[1..].to_vec()) - } else { - None - } - }).collect(); - match is_useful(cx, &matrix, &v[1..], witness) { - UsefulWithWitness(pats) => { - let mut new_pats: Vec<_> = constructors.into_iter().map(|constructor| { - let arity = constructor_arity(cx, &constructor, left_ty); - let wild_pats = vec![DUMMY_WILD_PAT; arity]; - construct_witness(cx, &constructor, wild_pats, left_ty) - }).collect(); - new_pats.extend(pats); - UsefulWithWitness(new_pats) - }, + let constructors = missing_constructors(cx, matrix, left_ty, max_slice_length); + if constructors.is_empty() { + all_constructors(cx, left_ty, max_slice_length).into_iter().map(|c| { + match is_useful_specialized(cx, matrix, v, c.clone(), left_ty, witness) { + UsefulWithWitness(pats) => UsefulWithWitness({ + let arity = constructor_arity(cx, &c, left_ty); + let mut result = { + let pat_slice = &pats[..]; + let subpats: Vec<_> = (0..arity).map(|i| { + pat_slice.get(i).map_or(DUMMY_WILD_PAT, |p| &**p) + }).collect(); + vec![construct_witness(cx, &c, subpats, left_ty)] + }; + result.extend(pats.into_iter().skip(arity)); + result + }), result => result } + }).find(|result| result != &NotUseful).unwrap_or(NotUseful) + } else { + let matrix = rows.iter().filter_map(|r| { + if pat_is_binding_or_wild(&cx.tcx.def_map.borrow(), raw_pat(r[0])) { + Some(r[1..].to_vec()) + } else { + None + } + }).collect(); + match is_useful(cx, &matrix, &v[1..], witness) { + UsefulWithWitness(pats) => { + let mut new_pats: Vec<_> = constructors.into_iter().map(|constructor| { + let arity = constructor_arity(cx, &constructor, left_ty); + let wild_pats = vec![DUMMY_WILD_PAT; arity]; + construct_witness(cx, &constructor, wild_pats, left_ty) + }).collect(); + new_pats.extend(pats); + UsefulWithWitness(new_pats) + }, + result => result } } } else {