From b0d02726af9bb83f8c8d90ea911ed2d6941840cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 9 Feb 2017 17:54:56 -0800 Subject: [PATCH 1/2] Clean up "pattern doesn't bind x" messages Group "missing variable bind" spans in `or` matches and clarify wording for the two possible cases: when a variable from the first pattern is not in any of the subsequent patterns, and when a variable in any of the other patterns is not in the first one. Before: ``` error[E0408]: variable `a` from pattern #1 is not bound in pattern #2 --> file.rs:10:23 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^^^^ pattern doesn't bind `a` error[E0408]: variable `b` from pattern #2 is not bound in pattern #1 --> file.rs:10:32 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^ pattern doesn't bind `b` error[E0408]: variable `a` from pattern #1 is not bound in pattern #3 --> file.rs:10:37 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `a` error[E0408]: variable `d` from pattern #1 is not bound in pattern #3 --> file.rs:10:37 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `d` error[E0408]: variable `c` from pattern #3 is not bound in pattern #1 --> file.rs:10:43 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^ pattern doesn't bind `c` error[E0408]: variable `d` from pattern #1 is not bound in pattern #4 --> file.rs:10:48 | 10 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } | ^^^^^^^^ pattern doesn't bind `d` error: aborting due to 6 previous errors ``` After: ``` error[E0408]: variable `a` is not bound in all patterns --> file.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { intln!("{:?}", a); } | - ^^^^^^^^^^^ ^^^^^^^^ - variable t in all patterns | | | | | | | pattern doesn't bind `a` | | pattern doesn't bind `a` | variable not in all patterns error[E0408]: variable `d` is not bound in all patterns --> file.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { intln!("{:?}", a); } | - - ^^^^^^^^ ^^^^^^^^ pattern esn't bind `d` | | | | | | | pattern doesn't bind `d` | | variable not in all patterns | variable not in all patterns error[E0408]: variable `b` is not bound in all patterns --> file.rs:20:37 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { intln!("{:?}", a); } | ^^^^^^^^^^^ - ^^^^^^^^ ^^^^^^^^ pattern esn't bind `b` | | | | | | | pattern doesn't bind `b` | | variable not in all patterns | pattern doesn't bind `b` error[E0408]: variable `c` is not bound in all patterns --> file.rs:20:48 | 20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { intln!("{:?}", a); } | ^^^^^^^^^^^ ^^^^^^^^^^^ - ^^^^^^^^ pattern esn't bind `c` | | | | | | | variable not in all tterns | | pattern doesn't bind `c` | pattern doesn't bind `c` error: aborting due to 4 previous errors ``` * Have only one presentation for binding consistency errors * Point to same binding in multiple patterns when possible * Check inconsistent bindings in all arms * Simplify wording of diagnostic message * Sort emition and spans of binding errors for deterministic output --- src/librustc_resolve/lib.rs | 140 +++++++++++++----- src/test/compile-fail/E0408.rs | 3 +- src/test/compile-fail/issue-2848.rs | 3 +- src/test/compile-fail/issue-2849.rs | 2 +- .../resolve-inconsistent-binding-mode.rs | 6 +- .../resolve-inconsistent-names.rs | 6 +- src/test/ui/mismatched_types/E0409.stderr | 2 +- src/test/ui/span/issue-39698.rs | 22 +++ src/test/ui/span/issue-39698.stderr | 42 ++++++ 9 files changed, 176 insertions(+), 50 deletions(-) create mode 100644 src/test/ui/span/issue-39698.rs create mode 100644 src/test/ui/span/issue-39698.stderr diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 0565db28ec5c9..68c45ded8c5ef 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -97,6 +97,31 @@ enum AssocSuggestion { AssocItem, } +#[derive(Eq)] +struct BindingError { + name: Name, + origin: FxHashSet, + target: FxHashSet, +} + +impl PartialOrd for BindingError { + fn partial_cmp(&self, other: &BindingError) -> Option { + Some(self.cmp(other)) + } +} + +impl PartialEq for BindingError { + fn eq(&self, other: &BindingError) -> bool { + self.name == other.name + } +} + +impl Ord for BindingError { + fn cmp(&self, other: &BindingError) -> cmp::Ordering { + self.name.cmp(&other.name) + } +} + enum ResolutionError<'a> { /// error E0401: can't use type parameters from outer function TypeParametersFromOuterFunction, @@ -110,10 +135,10 @@ enum ResolutionError<'a> { TypeNotMemberOfTrait(Name, &'a str), /// error E0438: const is not a member of trait ConstNotMemberOfTrait(Name, &'a str), - /// error E0408: variable `{}` from pattern #{} is not bound in pattern #{} - VariableNotBoundInPattern(Name, usize, usize), - /// error E0409: variable is bound with different mode in pattern #{} than in pattern #1 - VariableBoundWithDifferentMode(Name, usize, Span), + /// error E0408: variable `{}` is not bound in all patterns + VariableNotBoundInPattern(&'a BindingError), + /// error E0409: variable `{}` is bound in inconsistent ways within the same match arm + VariableBoundWithDifferentMode(Name, Span), /// error E0415: identifier is bound more than once in this parameter list IdentifierBoundMoreThanOnceInParameterList(&'a str), /// error E0416: identifier is bound more than once in the same pattern @@ -207,27 +232,30 @@ fn resolve_struct_error<'sess, 'a>(resolver: &'sess Resolver, err.span_label(span, &format!("not a member of trait `{}`", trait_)); err } - ResolutionError::VariableNotBoundInPattern(variable_name, from, to) => { - let mut err = struct_span_err!(resolver.session, - span, - E0408, - "variable `{}` from pattern #{} is not bound in pattern #{}", - variable_name, - from, - to); - err.span_label(span, &format!("pattern doesn't bind `{}`", variable_name)); + ResolutionError::VariableNotBoundInPattern(binding_error) => { + let mut target_sp = binding_error.target.iter().map(|x| *x).collect::>(); + target_sp.sort(); + let msp = MultiSpan::from_spans(target_sp.clone()); + let msg = format!("variable `{}` is not bound in all patterns", binding_error.name); + let mut err = resolver.session.struct_span_err_with_code(msp, &msg, "E0408"); + for sp in target_sp { + err.span_label(sp, &format!("pattern doesn't bind `{}`", binding_error.name)); + } + let mut origin_sp = binding_error.origin.iter().map(|x| *x).collect::>(); + origin_sp.sort(); + for sp in origin_sp { + err.span_label(sp, &"variable not in all patterns"); + } err } ResolutionError::VariableBoundWithDifferentMode(variable_name, - pattern_number, first_binding_span) => { let mut err = struct_span_err!(resolver.session, span, E0409, - "variable `{}` is bound with different mode in pattern #{} than in \ - pattern #1", - variable_name, - pattern_number); + "variable `{}` is bound in inconsistent \ + ways within the same match arm", + variable_name); err.span_label(span, &format!("bound in different ways")); err.span_label(first_binding_span, &format!("first binding")); err @@ -335,7 +363,7 @@ fn resolve_struct_error<'sess, 'a>(resolver: &'sess Resolver, } } -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] struct BindingInfo { span: Span, binding_mode: BindingMode, @@ -1904,36 +1932,66 @@ impl<'a> Resolver<'a> { if arm.pats.is_empty() { return; } - let map_0 = self.binding_mode_map(&arm.pats[0]); + + let mut missing_vars = FxHashMap(); + let mut inconsistent_vars = FxHashMap(); for (i, p) in arm.pats.iter().enumerate() { let map_i = self.binding_mode_map(&p); - for (&key, &binding_0) in &map_0 { - match map_i.get(&key) { - None => { - let error = ResolutionError::VariableNotBoundInPattern(key.name, 1, i + 1); - resolve_error(self, p.span, error); + for (j, q) in arm.pats.iter().enumerate() { + if i == j { + continue; + } + + let map_j = self.binding_mode_map(&q); + for (&key, &binding_i) in &map_i { + if map_j.len() == 0 { // Account for missing bindings when + let binding_error = missing_vars // map_j has none. + .entry(key.name) + .or_insert(BindingError { + name: key.name, + origin: FxHashSet(), + target: FxHashSet(), + }); + binding_error.origin.insert(binding_i.span); + binding_error.target.insert(q.span); } - Some(binding_i) => { - if binding_0.binding_mode != binding_i.binding_mode { - resolve_error(self, - binding_i.span, - ResolutionError::VariableBoundWithDifferentMode( - key.name, - i + 1, - binding_0.span)); + for (&key_j, &binding_j) in &map_j { + match map_i.get(&key_j) { + None => { // missing binding + let binding_error = missing_vars + .entry(key_j.name) + .or_insert(BindingError { + name: key_j.name, + origin: FxHashSet(), + target: FxHashSet(), + }); + binding_error.origin.insert(binding_j.span); + binding_error.target.insert(p.span); + } + Some(binding_i) => { // check consistent binding + if binding_i.binding_mode != binding_j.binding_mode { + inconsistent_vars + .entry(key.name) + .or_insert((binding_j.span, binding_i.span)); + } + } } } } } - - for (&key, &binding) in &map_i { - if !map_0.contains_key(&key) { - resolve_error(self, - binding.span, - ResolutionError::VariableNotBoundInPattern(key.name, i + 1, 1)); - } - } + } + let mut missing_vars = missing_vars.iter().collect::>(); + missing_vars.sort(); + for (_, v) in missing_vars { + resolve_error(self, + *v.origin.iter().next().unwrap(), + ResolutionError::VariableNotBoundInPattern(v)); + } + let mut inconsistent_vars = inconsistent_vars.iter().collect::>(); + inconsistent_vars.sort(); + for (name, v) in inconsistent_vars { + resolve_error(self, v.0, ResolutionError::VariableBoundWithDifferentMode(*name, v.1)); } } diff --git a/src/test/compile-fail/E0408.rs b/src/test/compile-fail/E0408.rs index d75f612482772..ce77a537e263d 100644 --- a/src/test/compile-fail/E0408.rs +++ b/src/test/compile-fail/E0408.rs @@ -12,7 +12,8 @@ fn main() { let x = Some(0); match x { - Some(y) | None => {} //~ ERROR variable `y` from pattern #1 is not bound in pattern #2 + Some(y) | None => {} //~ ERROR variable `y` is not bound in all patterns _ => () //~| NOTE pattern doesn't bind `y` + //~| NOTE variable not in all patterns } } diff --git a/src/test/compile-fail/issue-2848.rs b/src/test/compile-fail/issue-2848.rs index f5e0c545bb524..38bd7adefd91f 100644 --- a/src/test/compile-fail/issue-2848.rs +++ b/src/test/compile-fail/issue-2848.rs @@ -19,7 +19,8 @@ mod bar { fn main() { use bar::foo::{alpha, charlie}; match alpha { - alpha | beta => {} //~ ERROR variable `beta` from pattern #2 is not bound in pattern #1 + alpha | beta => {} //~ ERROR variable `beta` is not bound in all patterns charlie => {} //~| NOTE pattern doesn't bind `beta` + //~| NOTE variable not in all patterns } } diff --git a/src/test/compile-fail/issue-2849.rs b/src/test/compile-fail/issue-2849.rs index 48f4cac9711a8..203b28bd5e417 100644 --- a/src/test/compile-fail/issue-2849.rs +++ b/src/test/compile-fail/issue-2849.rs @@ -13,6 +13,6 @@ enum foo { alpha, beta(isize) } fn main() { match foo::alpha { foo::alpha | foo::beta(i) => {} - //~^ ERROR variable `i` from pattern #2 is not bound in pattern #1 + //~^ ERROR variable `i` is not bound in all patterns } } diff --git a/src/test/compile-fail/resolve-inconsistent-binding-mode.rs b/src/test/compile-fail/resolve-inconsistent-binding-mode.rs index 284c08ef09b28..63d33a9e5fa6f 100644 --- a/src/test/compile-fail/resolve-inconsistent-binding-mode.rs +++ b/src/test/compile-fail/resolve-inconsistent-binding-mode.rs @@ -15,7 +15,7 @@ enum opts { fn matcher1(x: opts) { match x { opts::a(ref i) | opts::b(i) => {} - //~^ ERROR variable `i` is bound with different mode in pattern #2 than in pattern #1 + //~^ ERROR variable `i` is bound in inconsistent ways within the same match arm //~^^ ERROR mismatched types opts::c(_) => {} } @@ -24,7 +24,7 @@ fn matcher1(x: opts) { fn matcher2(x: opts) { match x { opts::a(ref i) | opts::b(i) => {} - //~^ ERROR variable `i` is bound with different mode in pattern #2 than in pattern #1 + //~^ ERROR variable `i` is bound in inconsistent ways within the same match arm //~^^ ERROR mismatched types opts::c(_) => {} } @@ -33,7 +33,7 @@ fn matcher2(x: opts) { fn matcher4(x: opts) { match x { opts::a(ref mut i) | opts::b(ref i) => {} - //~^ ERROR variable `i` is bound with different mode in pattern #2 than in pattern #1 + //~^ ERROR variable `i` is bound in inconsistent ways within the same match arm //~^^ ERROR mismatched types opts::c(_) => {} } diff --git a/src/test/compile-fail/resolve-inconsistent-names.rs b/src/test/compile-fail/resolve-inconsistent-names.rs index 1e2541502ace8..7fee5aedb06ed 100644 --- a/src/test/compile-fail/resolve-inconsistent-names.rs +++ b/src/test/compile-fail/resolve-inconsistent-names.rs @@ -11,9 +11,11 @@ fn main() { let y = 1; match y { - a | b => {} //~ ERROR variable `a` from pattern #1 is not bound in pattern #2 - //~^ ERROR variable `b` from pattern #2 is not bound in pattern #1 + a | b => {} //~ ERROR variable `a` is not bound in all patterns + //~^ ERROR variable `b` is not bound in all patterns //~| NOTE pattern doesn't bind `a` //~| NOTE pattern doesn't bind `b` + //~| NOTE variable not in all patterns + //~| NOTE variable not in all patterns } } diff --git a/src/test/ui/mismatched_types/E0409.stderr b/src/test/ui/mismatched_types/E0409.stderr index 251e247fa28ba..45a42b1c271f8 100644 --- a/src/test/ui/mismatched_types/E0409.stderr +++ b/src/test/ui/mismatched_types/E0409.stderr @@ -1,4 +1,4 @@ -error[E0409]: variable `y` is bound with different mode in pattern #2 than in pattern #1 +error[E0409]: variable `y` is bound in inconsistent ways within the same match arm --> $DIR/E0409.rs:15:23 | 15 | (0, ref y) | (y, 0) => {} //~ ERROR E0409 diff --git a/src/test/ui/span/issue-39698.rs b/src/test/ui/span/issue-39698.rs new file mode 100644 index 0000000000000..17b3f1c5a885e --- /dev/null +++ b/src/test/ui/span/issue-39698.rs @@ -0,0 +1,22 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +enum T { + T1(i32, i32), + T2(i32, i32), + T3(i32), + T4(i32), +} + +fn main() { + match T::T1(123, 456) { + T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } + } +} diff --git a/src/test/ui/span/issue-39698.stderr b/src/test/ui/span/issue-39698.stderr new file mode 100644 index 0000000000000..97d802f839831 --- /dev/null +++ b/src/test/ui/span/issue-39698.stderr @@ -0,0 +1,42 @@ +error[E0408]: variable `a` is not bound in all patterns + --> $DIR/issue-39698.rs:20:23 + | +20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } + | - ^^^^^^^^^^^ ^^^^^^^^ - variable not in all patterns + | | | | + | | | pattern doesn't bind `a` + | | pattern doesn't bind `a` + | variable not in all patterns + +error[E0408]: variable `d` is not bound in all patterns + --> $DIR/issue-39698.rs:20:37 + | +20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } + | - - ^^^^^^^^ ^^^^^^^^ pattern doesn't bind `d` + | | | | + | | | pattern doesn't bind `d` + | | variable not in all patterns + | variable not in all patterns + +error[E0408]: variable `b` is not bound in all patterns + --> $DIR/issue-39698.rs:20:9 + | +20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } + | ^^^^^^^^^^^ - ^^^^^^^^ ^^^^^^^^ pattern doesn't bind `b` + | | | | + | | | pattern doesn't bind `b` + | | variable not in all patterns + | pattern doesn't bind `b` + +error[E0408]: variable `c` is not bound in all patterns + --> $DIR/issue-39698.rs:20:9 + | +20 | T::T1(a, d) | T::T2(d, b) | T::T3(c) | T::T4(a) => { println!("{:?}", a); } + | ^^^^^^^^^^^ ^^^^^^^^^^^ - ^^^^^^^^ pattern doesn't bind `c` + | | | | + | | | variable not in all patterns + | | pattern doesn't bind `c` + | pattern doesn't bind `c` + +error: aborting due to 4 previous errors + From 780bea5067858737ebee373405698f0e4f9141f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 5 Mar 2017 20:19:05 -0300 Subject: [PATCH 2/2] Use `BTreeSet` instead of `FxHashSet` --- src/librustc_resolve/lib.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 68c45ded8c5ef..a3b815df86da0 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -69,6 +69,7 @@ use errors::DiagnosticBuilder; use std::cell::{Cell, RefCell}; use std::cmp; +use std::collections::BTreeSet; use std::fmt; use std::mem::replace; use std::rc::Rc; @@ -100,8 +101,8 @@ enum AssocSuggestion { #[derive(Eq)] struct BindingError { name: Name, - origin: FxHashSet, - target: FxHashSet, + origin: BTreeSet, + target: BTreeSet, } impl PartialOrd for BindingError { @@ -233,16 +234,14 @@ fn resolve_struct_error<'sess, 'a>(resolver: &'sess Resolver, err } ResolutionError::VariableNotBoundInPattern(binding_error) => { - let mut target_sp = binding_error.target.iter().map(|x| *x).collect::>(); - target_sp.sort(); + let target_sp = binding_error.target.iter().map(|x| *x).collect::>(); let msp = MultiSpan::from_spans(target_sp.clone()); let msg = format!("variable `{}` is not bound in all patterns", binding_error.name); let mut err = resolver.session.struct_span_err_with_code(msp, &msg, "E0408"); for sp in target_sp { err.span_label(sp, &format!("pattern doesn't bind `{}`", binding_error.name)); } - let mut origin_sp = binding_error.origin.iter().map(|x| *x).collect::>(); - origin_sp.sort(); + let origin_sp = binding_error.origin.iter().map(|x| *x).collect::>(); for sp in origin_sp { err.span_label(sp, &"variable not in all patterns"); } @@ -1950,8 +1949,8 @@ impl<'a> Resolver<'a> { .entry(key.name) .or_insert(BindingError { name: key.name, - origin: FxHashSet(), - target: FxHashSet(), + origin: BTreeSet::new(), + target: BTreeSet::new(), }); binding_error.origin.insert(binding_i.span); binding_error.target.insert(q.span); @@ -1963,8 +1962,8 @@ impl<'a> Resolver<'a> { .entry(key_j.name) .or_insert(BindingError { name: key_j.name, - origin: FxHashSet(), - target: FxHashSet(), + origin: BTreeSet::new(), + target: BTreeSet::new(), }); binding_error.origin.insert(binding_j.span); binding_error.target.insert(p.span);