From f8dbd32433bce7e63f48604423ba8d88e2bf47aa Mon Sep 17 00:00:00 2001 From: laurent Date: Wed, 29 Nov 2017 20:42:37 +0000 Subject: [PATCH 1/3] Add a couple small tests to the match-same-arm lint. --- clippy_lints/src/copies.rs | 12 ++++-------- tests/ui/matches.rs | 20 ++++++++++++++++++++ tests/ui/matches.stderr | 18 ++++++++++++++++++ 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 862272456ea..04874e60224 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -203,14 +203,10 @@ fn lint_match_arms(cx: &LateContext, expr: &Expr) { db.span_note(i.body.span, "same as this"); // Note: this does not use `span_suggestion` on purpose: there is no clean way - // to - // remove the other arm. Building a span and suggest to replace it to "" makes - // an - // even more confusing error message. Also in order not to make up a span for - // the - // whole pattern, the suggestion is only shown when there is only one pattern. - // The - // user should know about `|` if they are already using it… + // to remove the other arm. Building a span and suggest to replace it to "" + // makes an even more confusing error message. Also in order not to make up a + // span for the whole pattern, the suggestion is only shown when there is only + // one pattern. The user should know about `|` if they are already using it… if i.pats.len() == 1 && j.pats.len() == 1 { let lhs = snippet(cx, i.pats[0].span, ""); diff --git a/tests/ui/matches.rs b/tests/ui/matches.rs index f97038ca1f0..f15a57c4f85 100644 --- a/tests/ui/matches.rs +++ b/tests/ui/matches.rs @@ -277,6 +277,26 @@ fn match_wild_err_arm() { Ok(_) => println!("ok"), Err(_) => {unreachable!();} } + + // no warning because of the guard + match x { + Ok(x) if x*x == 64 => println!("ok"), + Ok(_) => println!("ok"), + Err(_) => println!("err") + } + + match (x, Some(1i32)) { + (Ok(x), Some(_)) => println!("ok {}", x), + (Ok(_), Some(x)) => println!("ok {}", x), + _ => println!("err") + } + + // no warning because of the different types for x + match (x, Some(1.0f64)) { + (Ok(x), Some(_)) => println!("ok {}", x), + (Ok(_), Some(x)) => println!("ok {}", x), + _ => println!("err") + } } fn main() { diff --git a/tests/ui/matches.stderr b/tests/ui/matches.stderr index bcb94bab26c..cc7c5a4fee2 100644 --- a/tests/ui/matches.stderr +++ b/tests/ui/matches.stderr @@ -390,3 +390,21 @@ note: consider refactoring into `Ok(3) | Ok(_)` | ^^^^^^^^^^^^^^ = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) +error: this `match` has identical arm bodies + --> $DIR/matches.rs:290:29 + | +290 | (Ok(_), Some(x)) => println!("ok {}", x), + | ^^^^^^^^^^^^^^^^^^^^ + | +note: same as this + --> $DIR/matches.rs:289:29 + | +289 | (Ok(x), Some(_)) => println!("ok {}", x), + | ^^^^^^^^^^^^^^^^^^^^ +note: consider refactoring into `(Ok(x), Some(_)) | (Ok(_), Some(x))` + --> $DIR/matches.rs:289:29 + | +289 | (Ok(x), Some(_)) => println!("ok {}", x), + | ^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + From 3eb642bcdd0ce547c4e361b1b8635c69aabb37ca Mon Sep 17 00:00:00 2001 From: laurent Date: Wed, 29 Nov 2017 20:52:49 +0000 Subject: [PATCH 2/3] Add another test. --- tests/ui/matches.rs | 8 ++++++++ tests/ui/matches.stderr | 30 ++++++++++++++++++++++++------ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/tests/ui/matches.rs b/tests/ui/matches.rs index f15a57c4f85..7da2858d6ec 100644 --- a/tests/ui/matches.rs +++ b/tests/ui/matches.rs @@ -285,6 +285,14 @@ fn match_wild_err_arm() { Err(_) => println!("err") } + // this is a current false positive, see #1996 + match x { + Ok(3) => println!("ok"), + Ok(x) if x*x == 64 => println!("ok 64"), + Ok(_) => println!("ok"), + Err(_) => println!("err") + } + match (x, Some(1i32)) { (Ok(x), Some(_)) => println!("ok {}", x), (Ok(_), Some(x)) => println!("ok {}", x), diff --git a/tests/ui/matches.stderr b/tests/ui/matches.stderr index cc7c5a4fee2..49c0e900d06 100644 --- a/tests/ui/matches.stderr +++ b/tests/ui/matches.stderr @@ -391,20 +391,38 @@ note: consider refactoring into `Ok(3) | Ok(_)` = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error: this `match` has identical arm bodies - --> $DIR/matches.rs:290:29 + --> $DIR/matches.rs:292:18 | -290 | (Ok(_), Some(x)) => println!("ok {}", x), +292 | Ok(_) => println!("ok"), + | ^^^^^^^^^^^^^^ + | +note: same as this + --> $DIR/matches.rs:290:18 + | +290 | Ok(3) => println!("ok"), + | ^^^^^^^^^^^^^^ +note: consider refactoring into `Ok(3) | Ok(_)` + --> $DIR/matches.rs:290:18 + | +290 | Ok(3) => println!("ok"), + | ^^^^^^^^^^^^^^ + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + +error: this `match` has identical arm bodies + --> $DIR/matches.rs:298:29 + | +298 | (Ok(_), Some(x)) => println!("ok {}", x), | ^^^^^^^^^^^^^^^^^^^^ | note: same as this - --> $DIR/matches.rs:289:29 + --> $DIR/matches.rs:297:29 | -289 | (Ok(x), Some(_)) => println!("ok {}", x), +297 | (Ok(x), Some(_)) => println!("ok {}", x), | ^^^^^^^^^^^^^^^^^^^^ note: consider refactoring into `(Ok(x), Some(_)) | (Ok(_), Some(x))` - --> $DIR/matches.rs:289:29 + --> $DIR/matches.rs:297:29 | -289 | (Ok(x), Some(_)) => println!("ok {}", x), +297 | (Ok(x), Some(_)) => println!("ok {}", x), | ^^^^^^^^^^^^^^^^^^^^ = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) From c3ae2ddeb36d59432e97c07f85ac841abf3483b6 Mon Sep 17 00:00:00 2001 From: laurent Date: Wed, 29 Nov 2017 21:42:58 +0000 Subject: [PATCH 3/3] Fix a bug in search_same + add a test case. --- clippy_lints/src/consts.rs | 3 +-- clippy_lints/src/copies.rs | 9 ++++++--- clippy_lints/src/doc.rs | 3 +-- tests/ui/matches.rs | 8 ++++++++ tests/ui/matches.stderr | 18 ++++++++++++++++++ 5 files changed, 34 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/consts.rs b/clippy_lints/src/consts.rs index 7e6f3c2acf1..69527ba6ff8 100644 --- a/clippy_lints/src/consts.rs +++ b/clippy_lints/src/consts.rs @@ -74,9 +74,8 @@ fn eq(&self, other: &Self) -> bool { } }, (&Constant::Bool(l), &Constant::Bool(r)) => l == r, - (&Constant::Vec(ref l), &Constant::Vec(ref r)) => l == r, + (&Constant::Vec(ref l), &Constant::Vec(ref r)) | (&Constant::Tuple(ref l), &Constant::Tuple(ref r)) => l == r, (&Constant::Repeat(ref lv, ref ls), &Constant::Repeat(ref rv, ref rs)) => ls == rs && lv == rv, - (&Constant::Tuple(ref l), &Constant::Tuple(ref r)) => l == r, _ => false, // TODO: Are there inter-type equalities? } } diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 04874e60224..5a693ce5524 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -325,10 +325,13 @@ fn search_same(exprs: &[T], hash: Hash, eq: Eq) -> Option<(&T, &T)> for expr in exprs { match map.entry(hash(expr)) { - Entry::Occupied(o) => for o in o.get() { - if eq(o, expr) { - return Some((o, expr)); + Entry::Occupied(mut o) => { + for o in o.get() { + if eq(o, expr) { + return Some((o, expr)); + } } + o.get_mut().push(expr); }, Entry::Vacant(v) => { v.insert(vec![expr]); diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index b6542b2ebca..ea8ecb91d0a 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -206,8 +206,7 @@ fn check_doc<'a, Events: Iterator)>>( End(Link(_, _)) => in_link = None, Start(_tag) | End(_tag) => (), // We don't care about other tags Html(_html) | InlineHtml(_html) => (), // HTML is weird, just ignore it - SoftBreak => (), - HardBreak => (), + SoftBreak | HardBreak => (), FootnoteReference(text) | Text(text) => { if Some(&text) == in_link.as_ref() { // Probably a link of the form `` diff --git a/tests/ui/matches.rs b/tests/ui/matches.rs index 7da2858d6ec..352749d48e1 100644 --- a/tests/ui/matches.rs +++ b/tests/ui/matches.rs @@ -305,6 +305,14 @@ fn match_wild_err_arm() { (Ok(_), Some(x)) => println!("ok {}", x), _ => println!("err") } + + // because of a bug, no warning was generated for this case before #2251 + match x { + Ok(_tmp) => println!("ok"), + Ok(3) => println!("ok"), + Ok(_) => println!("ok"), + Err(_) => {unreachable!();} + } } fn main() { diff --git a/tests/ui/matches.stderr b/tests/ui/matches.stderr index 49c0e900d06..beb3387d038 100644 --- a/tests/ui/matches.stderr +++ b/tests/ui/matches.stderr @@ -426,3 +426,21 @@ note: consider refactoring into `(Ok(x), Some(_)) | (Ok(_), Some(x))` | ^^^^^^^^^^^^^^^^^^^^ = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) +error: this `match` has identical arm bodies + --> $DIR/matches.rs:313:18 + | +313 | Ok(_) => println!("ok"), + | ^^^^^^^^^^^^^^ + | +note: same as this + --> $DIR/matches.rs:312:18 + | +312 | Ok(3) => println!("ok"), + | ^^^^^^^^^^^^^^ +note: consider refactoring into `Ok(3) | Ok(_)` + --> $DIR/matches.rs:312:18 + | +312 | Ok(3) => println!("ok"), + | ^^^^^^^^^^^^^^ + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) +