Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn on pattern bindings that have the same name as a variant #19115

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
34 changes: 34 additions & 0 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -948,4 +948,38 @@ mod test {
assert!(test_items.next().is_some());
assert!(test_items.next().is_none());
}

#[test]
fn test_can_print_warnings() {
{
let matches = getopts(&[
"-Awarnings".to_string()
], optgroups().as_slice()).unwrap();
let registry = diagnostics::registry::Registry::new(&[]);
let sessopts = build_session_options(&matches);
let sess = build_session(sessopts, None, registry);
assert!(!sess.can_print_warnings);
}

{
let matches = getopts(&[
"-Awarnings".to_string(),
"-Dwarnings".to_string()
], optgroups().as_slice()).unwrap();
let registry = diagnostics::registry::Registry::new(&[]);
let sessopts = build_session_options(&matches);
let sess = build_session(sessopts, None, registry);
assert!(sess.can_print_warnings);
}

{
let matches = getopts(&[
"-Adead_code".to_string()
], optgroups().as_slice()).unwrap();
let registry = diagnostics::registry::Registry::new(&[]);
let sessopts = build_session_options(&matches);
let sess = build_session(sessopts, None, registry);
assert!(sess.can_print_warnings);
}
}
}
22 changes: 19 additions & 3 deletions src/librustc/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ pub struct Session {
/// The maximum recursion limit for potentially infinitely recursive
/// operations such as auto-dereference and monomorphization.
pub recursion_limit: Cell<uint>,

pub can_print_warnings: bool
}

impl Session {
Expand Down Expand Up @@ -82,13 +84,19 @@ impl Session {
self.diagnostic().handler().abort_if_errors()
}
pub fn span_warn(&self, sp: Span, msg: &str) {
self.diagnostic().span_warn(sp, msg)
if self.can_print_warnings {
self.diagnostic().span_warn(sp, msg)
}
}
pub fn span_warn_with_code(&self, sp: Span, msg: &str, code: &str) {
self.diagnostic().span_warn_with_code(sp, msg, code)
if self.can_print_warnings {
self.diagnostic().span_warn_with_code(sp, msg, code)
}
}
pub fn warn(&self, msg: &str) {
self.diagnostic().handler().warn(msg)
if self.can_print_warnings {
self.diagnostic().handler().warn(msg)
}
}
pub fn opt_span_warn(&self, opt_sp: Option<Span>, msg: &str) {
match opt_sp {
Expand Down Expand Up @@ -247,6 +255,13 @@ pub fn build_session_(sopts: config::Options,
}
);

let can_print_warnings = sopts.lint_opts
.iter()
.filter(|&&(ref key, _)| key.as_slice() == "warnings")
.map(|&(_, ref level)| *level != lint::Allow)
.last()
.unwrap_or(true);

let sess = Session {
target: target_cfg,
opts: sopts,
Expand All @@ -265,6 +280,7 @@ pub fn build_session_(sopts: config::Options,
crate_metadata: RefCell::new(Vec::new()),
features: RefCell::new(feature_gate::Features::new()),
recursion_limit: Cell::new(64),
can_print_warnings: can_print_warnings
};

sess.lint_store.borrow_mut().register_builtin(Some(&sess));
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() {}