From 5804a306868aee5ff0a3d7829db7924978317f0e Mon Sep 17 00:00:00 2001 From: Jakub Bukaj Date: Thu, 20 Nov 2014 16:57:36 +0100 Subject: [PATCH] Warn on pattern bindings that have the same name as a variant ...of the type being matched. This change will result in a better diagnostic for code like the following: ```rust enum Enum { Foo, Bar } fn f(x: Enum) { match x { Foo => (), Bar => () } } ``` which would currently simply fail with an unreachable pattern error on the 2nd arm. The user is advised to either use a qualified path in the patterns or import the variants explicitly into the scope. --- src/librustc/diagnostics.rs | 3 +- src/librustc/middle/check_match.rs | 84 +++++++++++++------ src/test/compile-fail/issue-12116.rs | 3 +- src/test/compile-fail/issue-14221.rs | 2 + .../compile-fail/lint-uppercase-variables.rs | 3 +- src/test/run-pass/issue-19100.rs | 34 ++++++++ 6 files changed, 100 insertions(+), 29 deletions(-) create mode 100644 src/test/run-pass/issue-19100.rs diff --git a/src/librustc/diagnostics.rs b/src/librustc/diagnostics.rs index afbb18faa0b..28504210a3f 100644 --- a/src/librustc/diagnostics.rs +++ b/src/librustc/diagnostics.rs @@ -145,5 +145,6 @@ register_diagnostics!( E0166, E0167, E0168, - E0169 + E0169, + E0170 ) diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index bcfc003480f..ed119081f78 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -153,19 +153,14 @@ fn check_expr(cx: &mut MatchCheckCtxt, ex: &ast::Expr) { visit::walk_expr(cx, ex); match ex.node { ast::ExprMatch(ref scrut, ref arms, source) => { - // First, check legality of move bindings. for arm in arms.iter() { + // First, check legality of move bindings. check_legality_of_move_bindings(cx, arm.guard.is_some(), arm.pats.as_slice()); - for pat in arm.pats.iter() { - check_legality_of_bindings_in_at_patterns(cx, &**pat); - } - } - // Second, if there is a guard on each arm, make sure it isn't - // assigning or borrowing anything mutably. - for arm in arms.iter() { + // Second, if there is a guard on each arm, make sure it isn't + // assigning or borrowing anything mutably. match arm.guard { Some(ref guard) => check_for_mutation_in_guard(cx, &**guard), None => {} @@ -179,13 +174,23 @@ fn check_expr(cx: &mut MatchCheckCtxt, ex: &ast::Expr) { }).collect(), arm.guard.as_ref().map(|e| &**e)) }).collect::>, Option<&ast::Expr>)>>(); + // Bail out early if inlining failed. if static_inliner.failed { return; } - // Third, check if there are any references to NaN that we should warn about. - for &(ref pats, _) in inlined_arms.iter() { - check_for_static_nan(cx, pats.as_slice()); + for pat in inlined_arms + .iter() + .flat_map(|&(ref pats, _)| pats.iter()) { + // Third, check legality of move bindings. + check_legality_of_bindings_in_at_patterns(cx, &**pat); + + // Fourth, check if there are any references to NaN that we should warn about. + check_for_static_nan(cx, &**pat); + + // Fifth, check if for any of the patterns that match an enumerated type + // are bindings with the same name as one of the variants of said type. + check_for_bindings_named_the_same_as_variants(cx, &**pat); } // Fourth, check for unreachable arms. @@ -239,21 +244,49 @@ fn is_expr_const_nan(tcx: &ty::ctxt, expr: &ast::Expr) -> bool { } } -// Check that we do not match against a static NaN (#6804) -fn check_for_static_nan(cx: &MatchCheckCtxt, pats: &[P]) { - for pat in pats.iter() { - walk_pat(&**pat, |p| { - match p.node { - ast::PatLit(ref expr) if is_expr_const_nan(cx.tcx, &**expr) => { - span_warn!(cx.tcx.sess, p.span, E0003, - "unmatchable NaN in pattern, \ - use the is_nan method in a guard instead"); +fn check_for_bindings_named_the_same_as_variants(cx: &MatchCheckCtxt, pat: &Pat) { + walk_pat(pat, |p| { + match p.node { + ast::PatIdent(ast::BindByValue(ast::MutImmutable), ident, None) => { + let pat_ty = ty::pat_ty(cx.tcx, p); + if let ty::ty_enum(def_id, _) = pat_ty.sty { + let def = cx.tcx.def_map.borrow().get(&p.id).cloned(); + if let Some(DefLocal(_)) = def { + if ty::enum_variants(cx.tcx, def_id).iter().any(|variant| + token::get_name(variant.name) == token::get_name(ident.node.name) + && variant.args.len() == 0 + ) { + span_warn!(cx.tcx.sess, p.span, E0170, + "pattern binding `{}` is named the same as one \ + of the variants of the type `{}`", + token::get_ident(ident.node).get(), ty_to_string(cx.tcx, pat_ty)); + span_help!(cx.tcx.sess, p.span, + "if you meant to match on a variant, \ + consider making the path in the pattern qualified: `{}::{}`", + ty_to_string(cx.tcx, pat_ty), token::get_ident(ident.node).get()); + } + } } - _ => () } - true - }); - } + _ => () + } + true + }); +} + +// Check that we do not match against a static NaN (#6804) +fn check_for_static_nan(cx: &MatchCheckCtxt, pat: &Pat) { + walk_pat(pat, |p| { + match p.node { + ast::PatLit(ref expr) if is_expr_const_nan(cx.tcx, &**expr) => { + span_warn!(cx.tcx.sess, p.span, E0003, + "unmatchable NaN in pattern, \ + use the is_nan method in a guard instead"); + } + _ => () + } + true + }); } // Check for unreachable patterns @@ -414,8 +447,7 @@ fn construct_witness(cx: &MatchCheckCtxt, ctor: &Constructor, &Variant(vid) => (vid, ty::enum_variant_with_id(cx.tcx, cid, vid).arg_names.is_some()), _ => - (cid, ty::lookup_struct_fields(cx.tcx, cid).iter() - .any(|field| field.name != token::special_idents::unnamed_field.name)) + (cid, !ty::is_tuple_struct(cx.tcx, cid)) }; if is_structure { let fields = ty::lookup_struct_fields(cx.tcx, vid); diff --git a/src/test/compile-fail/issue-12116.rs b/src/test/compile-fail/issue-12116.rs index 59e29516a74..47907ac2be9 100644 --- a/src/test/compile-fail/issue-12116.rs +++ b/src/test/compile-fail/issue-12116.rs @@ -17,7 +17,8 @@ fn tail(source_list: &IntList) -> IntList { match source_list { &IntList::Cons(val, box ref next_list) => tail(next_list), &IntList::Cons(val, box Nil) => IntList::Cons(val, box Nil), - //~^ ERROR: unreachable pattern +//~^ ERROR unreachable pattern +//~^^ WARN pattern binding `Nil` is named the same as one of the variants of the type `IntList` _ => panic!() } } diff --git a/src/test/compile-fail/issue-14221.rs b/src/test/compile-fail/issue-14221.rs index c58012dc84c..e79be99a346 100644 --- a/src/test/compile-fail/issue-14221.rs +++ b/src/test/compile-fail/issue-14221.rs @@ -17,7 +17,9 @@ pub mod b { pub fn key(e: ::E) -> &'static str { match e { A => "A", +//~^ WARN pattern binding `A` is named the same as one of the variants of the type `E` B => "B", //~ ERROR: unreachable pattern +//~^ WARN pattern binding `B` is named the same as one of the variants of the type `E` } } } diff --git a/src/test/compile-fail/lint-uppercase-variables.rs b/src/test/compile-fail/lint-uppercase-variables.rs index 2f840ee0761..eb5c475e7ef 100644 --- a/src/test/compile-fail/lint-uppercase-variables.rs +++ b/src/test/compile-fail/lint-uppercase-variables.rs @@ -33,7 +33,8 @@ fn main() { match f.read(&mut buff) { Ok(cnt) => println!("read this many bytes: {}", cnt), Err(IoError{ kind: EndOfFile, .. }) => println!("Got end of file: {}", EndOfFile.to_string()), - //~^ ERROR variable `EndOfFile` should have a snake case name such as `end_of_file` +//~^ ERROR variable `EndOfFile` should have a snake case name such as `end_of_file` +//~^^ WARN `EndOfFile` is named the same as one of the variants of the type `std::io::IoErrorKind` } test(1); diff --git a/src/test/run-pass/issue-19100.rs b/src/test/run-pass/issue-19100.rs new file mode 100644 index 00000000000..cee5c808f99 --- /dev/null +++ b/src/test/run-pass/issue-19100.rs @@ -0,0 +1,34 @@ +// Copyright 2014 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 Foo { + Bar, + Baz +} + +impl Foo { + fn foo(&self) { + match self { + & +Bar if true +//~^ WARN pattern binding `Bar` is named the same as one of the variants of the type `Foo` +//~^^ HELP to match on a variant, consider making the path in the pattern qualified: `Foo::Bar` +=> println!("bar"), + & +Baz if false +//~^ WARN pattern binding `Baz` is named the same as one of the variants of the type `Foo` +//~^^ HELP to match on a variant, consider making the path in the pattern qualified: `Foo::Baz` +=> println!("baz"), +_ => () + } + } +} + +fn main() {}