Skip to content

Commit

Permalink
Warn on pattern bindings that have the same name as a variant
Browse files Browse the repository at this point in the history
...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.
  • Loading branch information
Jakub Bukaj committed Nov 26, 2014
1 parent 6faff24 commit 5804a30
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 29 deletions.
3 changes: 2 additions & 1 deletion src/librustc/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,5 +145,6 @@ register_diagnostics!(
E0166,
E0167,
E0168,
E0169
E0169,
E0170
)
84 changes: 58 additions & 26 deletions src/librustc/middle/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {}
Expand All @@ -179,13 +174,23 @@ fn check_expr(cx: &mut MatchCheckCtxt, ex: &ast::Expr) {
}).collect(), arm.guard.as_ref().map(|e| &**e))
}).collect::<Vec<(Vec<P<Pat>>, 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.
Expand Down Expand Up @@ -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<Pat>]) {
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
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/test/compile-fail/issue-12116.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!()
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/issue-14221.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/test/compile-fail/lint-uppercase-variables.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
34 changes: 34 additions & 0 deletions src/test/run-pass/issue-19100.rs
Original file line number Diff line number Diff line change
@@ -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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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() {}

0 comments on commit 5804a30

Please sign in to comment.