From 53a6304c2a431ae97c3b584066804c6acd667541 Mon Sep 17 00:00:00 2001 From: Jakub Adam Wieczorek Date: Fri, 9 Aug 2019 09:43:26 +0000 Subject: [PATCH 1/2] Suggest using a qualified path in patterns with inconsistent bindings A program like the following one: ```rust enum E { A, B, C } fn f(x: E) -> bool { match x { A | B => false, C => true } } ``` is rejected by the compiler due to `E` variant paths not being in scope. In this case `A`, `B` are resolved as pattern bindings and consequently the pattern is considered invalid as the inner or-patterns do not bind to the same set of identifiers. This is expected but the compiler errors that follow could be surprising or confusing to some users. This commit adds a help note explaining that if the user desired to match against variants or consts, they should use a qualified path. The note is restricted to cases where the identifier starts with an upper-case sequence so as to reduce the false negatives. Since this happens during resolution, there's no clean way to check what the patterns match against. The syntactic criterium, however, is in line with the convention that's assumed by the `non-camel-case-types` lint. --- src/librustc_resolve/diagnostics.rs | 19 ++-- src/librustc_resolve/late.rs | 70 +++++++-------- src/librustc_resolve/lib.rs | 1 + .../ui/resolve/resolve-inconsistent-names.rs | 31 ++++++- .../resolve/resolve-inconsistent-names.stderr | 87 ++++++++++++++++++- 5 files changed, 159 insertions(+), 49 deletions(-) diff --git a/src/librustc_resolve/diagnostics.rs b/src/librustc_resolve/diagnostics.rs index 01a4a3c4bb244..b3f6252e4aabd 100644 --- a/src/librustc_resolve/diagnostics.rs +++ b/src/librustc_resolve/diagnostics.rs @@ -20,7 +20,7 @@ use syntax_pos::{BytePos, Span, MultiSpan}; use crate::resolve_imports::{ImportDirective, ImportDirectiveSubclass, ImportResolver}; use crate::{path_names_to_string, KNOWN_TOOLS}; -use crate::{CrateLint, LegacyScope, Module, ModuleOrUniformRoot}; +use crate::{BindingError, CrateLint, LegacyScope, Module, ModuleOrUniformRoot}; use crate::{PathResult, ParentScope, ResolutionError, Resolver, Scope, ScopeSet, Segment}; type Res = def::Res; @@ -207,21 +207,30 @@ impl<'a> Resolver<'a> { err } ResolutionError::VariableNotBoundInPattern(binding_error) => { - let target_sp = binding_error.target.iter().cloned().collect::>(); + let BindingError { name, target, origin, could_be_variant } = binding_error; + + let target_sp = target.iter().cloned().collect::>(); + let origin_sp = origin.iter().cloned().collect::>(); + let msp = MultiSpan::from_spans(target_sp.clone()); - let msg = format!("variable `{}` is not bound in all patterns", binding_error.name); + let msg = format!("variable `{}` is not bound in all patterns", name); let mut err = self.session.struct_span_err_with_code( msp, &msg, DiagnosticId::Error("E0408".into()), ); for sp in target_sp { - err.span_label(sp, format!("pattern doesn't bind `{}`", binding_error.name)); + err.span_label(sp, format!("pattern doesn't bind `{}`", name)); } - let origin_sp = binding_error.origin.iter().cloned(); for sp in origin_sp { err.span_label(sp, "variable not in all patterns"); } + if *could_be_variant { + let help_msg = format!( + "if you meant to match on a variant or a const, consider \ + making the path in the pattern qualified: `?::{}`", name); + err.span_help(span, &help_msg); + } err } ResolutionError::VariableBoundWithDifferentMode(variable_name, diff --git a/src/librustc_resolve/late.rs b/src/librustc_resolve/late.rs index 7cb11195ee02b..13bae25a69af0 100644 --- a/src/librustc_resolve/late.rs +++ b/src/librustc_resolve/late.rs @@ -1136,64 +1136,56 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { // Checks that all of the arms in an or-pattern have exactly the // same set of bindings, with the same binding modes for each. fn check_consistent_bindings(&mut self, pats: &[P]) { - if pats.is_empty() { + if pats.len() <= 1 { return; } let mut missing_vars = FxHashMap::default(); let mut inconsistent_vars = FxHashMap::default(); - for (i, p) in pats.iter().enumerate() { + for p in pats.iter() { let map_i = self.binding_mode_map(&p); - - for (j, q) in pats.iter().enumerate() { - if i == j { + for q in pats.iter() { + if p.id == q.id { continue; } let map_j = self.binding_mode_map(&q); - for (&key, &binding_i) in &map_i { - if map_j.is_empty() { // Account for missing bindings when - let binding_error = missing_vars // `map_j` has none. - .entry(key.name) - .or_insert(BindingError { - name: key.name, - origin: BTreeSet::new(), - target: BTreeSet::new(), - }); - binding_error.origin.insert(binding_i.span); - binding_error.target.insert(q.span); - } - for (&key_j, &binding_j) in &map_j { - match map_i.get(&key_j) { - None => { // missing binding - let binding_error = missing_vars + for (&key_j, &binding_j) in map_j.iter() { + 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: BTreeSet::new(), + target: BTreeSet::new(), + could_be_variant: + key_j.name.as_str().starts_with(char::is_uppercase) + }); + 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_j.name) - .or_insert(BindingError { - name: key_j.name, - origin: BTreeSet::new(), - target: BTreeSet::new(), - }); - 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)); - } + .or_insert((binding_j.span, binding_i.span)); } } } } } } - let mut missing_vars = missing_vars.iter().collect::>(); + + let mut missing_vars = missing_vars.iter_mut().collect::>(); missing_vars.sort(); - for (_, v) in missing_vars { + for (name, mut v) in missing_vars { + if inconsistent_vars.contains_key(name) { + v.could_be_variant = false; + } self.r.report_error( - *v.origin.iter().next().unwrap(), ResolutionError::VariableNotBoundInPattern(v) - ); + *v.origin.iter().next().unwrap(), + ResolutionError::VariableNotBoundInPattern(v)); } let mut inconsistent_vars = inconsistent_vars.iter().collect::>(); inconsistent_vars.sort(); diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index e786e1020026a..1cacc6b7d606d 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -135,6 +135,7 @@ struct BindingError { name: Name, origin: BTreeSet, target: BTreeSet, + could_be_variant: bool } impl PartialOrd for BindingError { diff --git a/src/test/ui/resolve/resolve-inconsistent-names.rs b/src/test/ui/resolve/resolve-inconsistent-names.rs index 59baa57d91b3d..2fb803c4b2ad4 100644 --- a/src/test/ui/resolve/resolve-inconsistent-names.rs +++ b/src/test/ui/resolve/resolve-inconsistent-names.rs @@ -1,7 +1,36 @@ +#![allow(non_camel_case_types)] + +enum E { A, B, c } + +mod m { + const CONST1: usize = 10; + const Const2: usize = 20; +} + fn main() { let y = 1; match y { a | b => {} //~ ERROR variable `a` is not bound in all patterns - //~^ ERROR variable `b` is not bound in all patterns + //~| ERROR variable `b` is not bound in all patterns + } + + let x = (E::A, E::B); + match x { + (A, B) | (ref B, c) | (c, A) => () + //~^ ERROR variable `A` is not bound in all patterns + //~| ERROR variable `B` is not bound in all patterns + //~| ERROR variable `B` is bound in inconsistent ways + //~| ERROR mismatched types + //~| ERROR variable `c` is not bound in all patterns + //~| HELP consider making the path in the pattern qualified: `?::A` + } + + let z = (10, 20); + match z { + (CONST1, _) | (_, Const2) => () + //~^ ERROR variable `CONST1` is not bound in all patterns + //~| HELP consider making the path in the pattern qualified: `?::CONST1` + //~| ERROR variable `Const2` is not bound in all patterns + //~| HELP consider making the path in the pattern qualified: `?::Const2` } } diff --git a/src/test/ui/resolve/resolve-inconsistent-names.stderr b/src/test/ui/resolve/resolve-inconsistent-names.stderr index c75718149fa4e..5a6ae31411ee2 100644 --- a/src/test/ui/resolve/resolve-inconsistent-names.stderr +++ b/src/test/ui/resolve/resolve-inconsistent-names.stderr @@ -1,5 +1,5 @@ error[E0408]: variable `a` is not bound in all patterns - --> $DIR/resolve-inconsistent-names.rs:4:12 + --> $DIR/resolve-inconsistent-names.rs:13:12 | LL | a | b => {} | - ^ pattern doesn't bind `a` @@ -7,13 +7,92 @@ LL | a | b => {} | variable not in all patterns error[E0408]: variable `b` is not bound in all patterns - --> $DIR/resolve-inconsistent-names.rs:4:8 + --> $DIR/resolve-inconsistent-names.rs:13:8 | LL | a | b => {} | ^ - variable not in all patterns | | | pattern doesn't bind `b` -error: aborting due to 2 previous errors +error[E0408]: variable `A` is not bound in all patterns + --> $DIR/resolve-inconsistent-names.rs:19:18 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | - ^^^^^^^^^^ - variable not in all patterns + | | | + | | pattern doesn't bind `A` + | variable not in all patterns + | +help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::A` + --> $DIR/resolve-inconsistent-names.rs:19:10 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | ^ + +error[E0408]: variable `B` is not bound in all patterns + --> $DIR/resolve-inconsistent-names.rs:19:31 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | - - ^^^^^^ pattern doesn't bind `B` + | | | + | | variable not in all patterns + | variable not in all patterns + +error[E0408]: variable `c` is not bound in all patterns + --> $DIR/resolve-inconsistent-names.rs:19:9 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | ^^^^^^ - - variable not in all patterns + | | | + | | variable not in all patterns + | pattern doesn't bind `c` + +error[E0409]: variable `B` is bound in inconsistent ways within the same match arm + --> $DIR/resolve-inconsistent-names.rs:19:23 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | - ^ bound in different ways + | | + | first binding + +error[E0408]: variable `CONST1` is not bound in all patterns + --> $DIR/resolve-inconsistent-names.rs:30:23 + | +LL | (CONST1, _) | (_, Const2) => () + | ------ ^^^^^^^^^^^ pattern doesn't bind `CONST1` + | | + | variable not in all patterns + | +help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::CONST1` + --> $DIR/resolve-inconsistent-names.rs:30:10 + | +LL | (CONST1, _) | (_, Const2) => () + | ^^^^^^ + +error[E0408]: variable `Const2` is not bound in all patterns + --> $DIR/resolve-inconsistent-names.rs:30:9 + | +LL | (CONST1, _) | (_, Const2) => () + | ^^^^^^^^^^^ ------ variable not in all patterns + | | + | pattern doesn't bind `Const2` + | +help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::Const2` + --> $DIR/resolve-inconsistent-names.rs:30:27 + | +LL | (CONST1, _) | (_, Const2) => () + | ^^^^^^ + +error[E0308]: mismatched types + --> $DIR/resolve-inconsistent-names.rs:19:19 + | +LL | (A, B) | (ref B, c) | (c, A) => () + | ^^^^^ expected enum `E`, found &E + | + = note: expected type `E` + found type `&E` + +error: aborting due to 9 previous errors -For more information about this error, try `rustc --explain E0408`. +Some errors have detailed explanations: E0308, E0408, E0409. +For more information about an error, try `rustc --explain E0308`. From 30db4ebdc225521853889b50cc5164646bbe66ed Mon Sep 17 00:00:00 2001 From: Jakub Adam Wieczorek Date: Fri, 9 Aug 2019 20:21:45 +0000 Subject: [PATCH 2/2] Apply suggestions from code review Co-Authored-By: Mazdak Farrokhzad --- src/librustc_resolve/diagnostics.rs | 14 +++--- src/librustc_resolve/late.rs | 48 +++++++++---------- src/librustc_resolve/lib.rs | 2 +- .../resolve/resolve-inconsistent-names.stderr | 6 +-- 4 files changed, 35 insertions(+), 35 deletions(-) diff --git a/src/librustc_resolve/diagnostics.rs b/src/librustc_resolve/diagnostics.rs index b3f6252e4aabd..9827ce6ef20f8 100644 --- a/src/librustc_resolve/diagnostics.rs +++ b/src/librustc_resolve/diagnostics.rs @@ -207,10 +207,10 @@ impl<'a> Resolver<'a> { err } ResolutionError::VariableNotBoundInPattern(binding_error) => { - let BindingError { name, target, origin, could_be_variant } = binding_error; + let BindingError { name, target, origin, could_be_path } = binding_error; - let target_sp = target.iter().cloned().collect::>(); - let origin_sp = origin.iter().cloned().collect::>(); + let target_sp = target.iter().copied().collect::>(); + let origin_sp = origin.iter().copied().collect::>(); let msp = MultiSpan::from_spans(target_sp.clone()); let msg = format!("variable `{}` is not bound in all patterns", name); @@ -225,10 +225,12 @@ impl<'a> Resolver<'a> { for sp in origin_sp { err.span_label(sp, "variable not in all patterns"); } - if *could_be_variant { + if *could_be_path { let help_msg = format!( - "if you meant to match on a variant or a const, consider \ - making the path in the pattern qualified: `?::{}`", name); + "if you meant to match on a variant or a `const` item, consider \ + making the path in the pattern qualified: `?::{}`", + name, + ); err.span_help(span, &help_msg); } err diff --git a/src/librustc_resolve/late.rs b/src/librustc_resolve/late.rs index 13bae25a69af0..358eaae11e712 100644 --- a/src/librustc_resolve/late.rs +++ b/src/librustc_resolve/late.rs @@ -1136,40 +1136,35 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { // Checks that all of the arms in an or-pattern have exactly the // same set of bindings, with the same binding modes for each. fn check_consistent_bindings(&mut self, pats: &[P]) { - if pats.len() <= 1 { - return; - } - let mut missing_vars = FxHashMap::default(); let mut inconsistent_vars = FxHashMap::default(); - for p in pats.iter() { - let map_i = self.binding_mode_map(&p); - for q in pats.iter() { - if p.id == q.id { - continue; - } - let map_j = self.binding_mode_map(&q); - for (&key_j, &binding_j) in map_j.iter() { - match map_i.get(&key_j) { + for pat_outer in pats.iter() { + let map_outer = self.binding_mode_map(&pat_outer); + + for pat_inner in pats.iter().filter(|pat| pat.id != pat_outer.id) { + let map_inner = self.binding_mode_map(&pat_inner); + + for (&key_inner, &binding_inner) in map_inner.iter() { + match map_outer.get(&key_inner) { None => { // missing binding let binding_error = missing_vars - .entry(key_j.name) + .entry(key_inner.name) .or_insert(BindingError { - name: key_j.name, + name: key_inner.name, origin: BTreeSet::new(), target: BTreeSet::new(), - could_be_variant: - key_j.name.as_str().starts_with(char::is_uppercase) + could_be_path: + key_inner.name.as_str().starts_with(char::is_uppercase) }); - binding_error.origin.insert(binding_j.span); - binding_error.target.insert(p.span); + binding_error.origin.insert(binding_inner.span); + binding_error.target.insert(pat_outer.span); } - Some(binding_i) => { // check consistent binding - if binding_i.binding_mode != binding_j.binding_mode { + Some(binding_outer) => { // check consistent binding + if binding_outer.binding_mode != binding_inner.binding_mode { inconsistent_vars - .entry(key_j.name) - .or_insert((binding_j.span, binding_i.span)); + .entry(key_inner.name) + .or_insert((binding_inner.span, binding_outer.span)); } } } @@ -1181,12 +1176,13 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { missing_vars.sort(); for (name, mut v) in missing_vars { if inconsistent_vars.contains_key(name) { - v.could_be_variant = false; + v.could_be_path = false; } self.r.report_error( *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 { @@ -1214,7 +1210,9 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> { self.resolve_pattern(pat, source, &mut bindings_list); } // This has to happen *after* we determine which pat_idents are variants - self.check_consistent_bindings(pats); + if pats.len() > 1 { + self.check_consistent_bindings(pats); + } } fn resolve_block(&mut self, block: &Block) { diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 1cacc6b7d606d..98782dfbc7a5f 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -135,7 +135,7 @@ struct BindingError { name: Name, origin: BTreeSet, target: BTreeSet, - could_be_variant: bool + could_be_path: bool } impl PartialOrd for BindingError { diff --git a/src/test/ui/resolve/resolve-inconsistent-names.stderr b/src/test/ui/resolve/resolve-inconsistent-names.stderr index 5a6ae31411ee2..f02867a0024b5 100644 --- a/src/test/ui/resolve/resolve-inconsistent-names.stderr +++ b/src/test/ui/resolve/resolve-inconsistent-names.stderr @@ -23,7 +23,7 @@ LL | (A, B) | (ref B, c) | (c, A) => () | | pattern doesn't bind `A` | variable not in all patterns | -help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::A` +help: if you meant to match on a variant or a `const` item, consider making the path in the pattern qualified: `?::A` --> $DIR/resolve-inconsistent-names.rs:19:10 | LL | (A, B) | (ref B, c) | (c, A) => () @@ -63,7 +63,7 @@ LL | (CONST1, _) | (_, Const2) => () | | | variable not in all patterns | -help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::CONST1` +help: if you meant to match on a variant or a `const` item, consider making the path in the pattern qualified: `?::CONST1` --> $DIR/resolve-inconsistent-names.rs:30:10 | LL | (CONST1, _) | (_, Const2) => () @@ -77,7 +77,7 @@ LL | (CONST1, _) | (_, Const2) => () | | | pattern doesn't bind `Const2` | -help: if you meant to match on a variant or a const, consider making the path in the pattern qualified: `?::Const2` +help: if you meant to match on a variant or a `const` item, consider making the path in the pattern qualified: `?::Const2` --> $DIR/resolve-inconsistent-names.rs:30:27 | LL | (CONST1, _) | (_, Const2) => ()