Add the MATCH_SAME_ARMS lint

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 119 lints included in this crate:
+There are 120 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -65,6 +65,7 @@ name
 [match_bool](https://github.com/Manishearth/rust-clippy/wiki#match_bool)                                       | warn    | a match on boolean expression; recommends `if..else` block instead
 [match_overlapping_arm](https://github.com/Manishearth/rust-clippy/wiki#match_overlapping_arm)                 | warn    | a match has overlapping arms
 [match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats)                               | warn    | a match or `if let` has all arms prefixed with `&`; the match expression can be dereferenced instead
+[match_same_arms](https://github.com/Manishearth/rust-clippy/wiki#match_same_arms)                             | warn    | `match` with identical arm bodies
 [min_max](https://github.com/Manishearth/rust-clippy/wiki#min_max)                                             | warn    | `min(_, max(_, _))` (or vice versa) with bounds clamping the result to a constant
 [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one)                                       | warn    | taking a number modulo 1, which always returns 0
 [mut_mut](https://github.com/Manishearth/rust-clippy/wiki#mut_mut)                                             | allow   | usage of double-mut refs, e.g. `&mut &mut ...` (either copy'n'paste error, or shows a fundamental misunderstanding of references)

src/copies.rs

@@ -1,7 +1,9 @@
 use rustc::lint::*;
+use rustc::middle::ty;
 use rustc_front::hir::*;
 use std::collections::HashMap;
 use std::collections::hash_map::Entry;
+use syntax::parse::token::InternedString;
 use utils::{SpanlessEq, SpanlessHash};
 use utils::{get_parent_expr, in_macro, span_note_and_lint};
 
@@ -33,6 +35,25 @@ declare_lint! {
     "if with the same *then* and *else* blocks"
 }
 
+/// **What it does:** This lint checks for `match` with identical arm bodies.
+///
+/// **Why is this bad?** This is probably a copy & paste error.
+///
+/// **Known problems:** Hopefully none.
+///
+/// **Example:**
+/// ```rust,ignore
+/// match foo {
+///     Bar => bar(),
+///     Quz => quz(),
+///     Baz => bar(), // <= oups
+/// ```
+declare_lint! {
+    pub MATCH_SAME_ARMS,
+    Warn,
+    "`match` with identical arm bodies"
+}
+
 #[derive(Copy, Clone, Debug)]
 pub struct CopyAndPaste;
 
@@ -40,7 +61,8 @@ impl LintPass for CopyAndPaste {
     fn get_lints(&self) -> LintArray {
         lint_array![
             IFS_SAME_COND,
-            IF_SAME_THEN_ELSE
+            IF_SAME_THEN_ELSE,
+            MATCH_SAME_ARMS
         ]
     }
 }
@@ -58,39 +80,63 @@ impl LateLintPass for CopyAndPaste {
             let (conds, blocks) = if_sequence(expr);
             lint_same_then_else(cx, &blocks);
             lint_same_cond(cx, &conds);
+            lint_match_arms(cx, expr);
         }
     }
 }
 
 /// Implementation of `IF_SAME_THEN_ELSE`.
 fn lint_same_then_else(cx: &LateContext, blocks: &[&Block]) {
-    let hash = |block| -> u64 {
+    let hash : &Fn(&&Block) -> u64 = &|block| -> u64 {
         let mut h = SpanlessHash::new(cx);
         h.hash_block(block);
         h.finish()
     };
-    let eq = |lhs, rhs| -> bool {
+
+    let eq : &Fn(&&Block, &&Block) -> bool = &|&lhs, &rhs| -> bool {
         SpanlessEq::new(cx).eq_block(lhs, rhs)
     };
 
     if let Some((i, j)) = search_same(blocks, hash, eq) {
-        span_note_and_lint(cx, IF_SAME_THEN_ELSE, j.span, "this if has identical blocks", i.span, "same as this");
+        span_note_and_lint(cx, IF_SAME_THEN_ELSE, j.span, "this `if` has identical blocks", i.span, "same as this");
     }
 }
 
 /// Implementation of `IFS_SAME_COND`.
 fn lint_same_cond(cx: &LateContext, conds: &[&Expr]) {
-    let hash = |expr| -> u64 {
+    let hash : &Fn(&&Expr) -> u64 = &|expr| -> u64 {
         let mut h = SpanlessHash::new(cx);
         h.hash_expr(expr);
         h.finish()
     };
-    let eq = |lhs, rhs| -> bool {
+
+    let eq : &Fn(&&Expr, &&Expr) -> bool = &|&lhs, &rhs| -> bool {
         SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, rhs)
     };
 
     if let Some((i, j)) = search_same(conds, hash, eq) {
-        span_note_and_lint(cx, IFS_SAME_COND, j.span, "this if has the same condition as a previous if", i.span, "same as this");
+        span_note_and_lint(cx, IFS_SAME_COND, j.span, "this `if` has the same condition as a previous if", i.span, "same as this");
+    }
+}
+
+/// Implementation if `MATCH_SAME_ARMS`.
+fn lint_match_arms(cx: &LateContext, expr: &Expr) {
+    let hash = |arm: &Arm| -> u64 {
+        let mut h = SpanlessHash::new(cx);
+        h.hash_expr(&arm.body);
+        h.finish()
+    };
+
+    let eq = |lhs: &Arm, rhs: &Arm| -> bool {
+        SpanlessEq::new(cx).eq_expr(&lhs.body, &rhs.body) &&
+            // all patterns should have the same bindings
+            bindings(cx, &lhs.pats[0]) == bindings(cx, &rhs.pats[0])
+    };
+
+    if let ExprMatch(_, ref arms, MatchSource::Normal) = expr.node {
+        if let Some((i, j)) = search_same(&**arms, hash, eq) {
+            span_note_and_lint(cx, MATCH_SAME_ARMS, j.body.span, "this `match` has identical arm bodies", i.body.span, "same as this");
+        }
     }
 }
 
@@ -123,11 +169,59 @@ fn if_sequence(mut expr: &Expr) -> (Vec<&Expr>, Vec<&Block>) {
     (conds, blocks)
 }
 
-fn search_same<'a, T, Hash, Eq>(exprs: &[&'a T],
+/// Return the list of bindings in a pattern.
-                                hash: Hash,
+fn bindings<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat) -> HashMap<InternedString, ty::Ty<'tcx>> {
-                                eq: Eq) -> Option<(&'a T, &'a T)>
+    fn bindings_impl<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat, map: &mut HashMap<InternedString, ty::Ty<'tcx>>) {
-where Hash: Fn(&'a T) -> u64,
+        match pat.node {
-      Eq: Fn(&'a T, &'a T) -> bool {
+            PatBox(ref pat) | PatRegion(ref pat, _) => bindings_impl(cx, pat, map),
+            PatEnum(_, Some(ref pats)) => {
+                for pat in pats {
+                    bindings_impl(cx, pat, map);
+                }
+            }
+            PatIdent(_, ref ident, ref as_pat) => {
+                if let Entry::Vacant(v) = map.entry(ident.node.name.as_str()) {
+                    v.insert(cx.tcx.pat_ty(pat));
+                }
+                if let Some(ref as_pat) = *as_pat {
+                    bindings_impl(cx, as_pat, map);
+                }
+            },
+            PatStruct(_, ref fields, _) => {
+                for pat in fields {
+                    bindings_impl(cx, &pat.node.pat, map);
+                }
+            }
+            PatTup(ref fields) => {
+                for pat in fields {
+                    bindings_impl(cx, pat, map);
+                }
+            }
+            PatVec(ref lhs, ref mid, ref rhs) => {
+                for pat in lhs {
+                    bindings_impl(cx, pat, map);
+                }
+                if let Some(ref mid) = *mid {
+                    bindings_impl(cx, mid, map);
+                }
+                for pat in rhs {
+                    bindings_impl(cx, pat, map);
+                }
+            }
+            PatEnum(..) | PatLit(..) | PatQPath(..) | PatRange(..) | PatWild => (),
+        }
+    }
+
+    let mut result = HashMap::new();
+    bindings_impl(cx, pat, &mut result);
+    result
+}
+
+fn search_same<T, Hash, Eq>(exprs: &[T],
+                            hash: Hash,
+                            eq: Eq) -> Option<(&T, &T)>
+where Hash: Fn(&T) -> u64,
+      Eq: Fn(&T, &T) -> bool {
     // common cases
     if exprs.len() < 2 {
         return None;
@@ -141,14 +235,14 @@ where Hash: Fn(&'a T) -> u64,
         }
     }
 
-    let mut map : HashMap<_, Vec<&'a _>> = HashMap::with_capacity(exprs.len());
+    let mut map : HashMap<_, Vec<&_>> = HashMap::with_capacity(exprs.len());
 
-    for &expr in exprs {
+    for expr in exprs {
         match map.entry(hash(expr)) {
             Entry::Occupied(o) => {
                 for o in o.get() {
-                    if eq(o, expr) {
+                    if eq(&o, expr) {
-                        return Some((o, expr))
+                        return Some((&o, expr))
                     }
                 }
             }

src/lib.rs

@@ -196,6 +196,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         collapsible_if::COLLAPSIBLE_IF,
         copies::IF_SAME_THEN_ELSE,
         copies::IFS_SAME_COND,
+        copies::MATCH_SAME_ARMS,
         cyclomatic_complexity::CYCLOMATIC_COMPLEXITY,
         derive::DERIVE_HASH_NOT_EQ,
         derive::EXPL_IMPL_CLONE_ON_COPY,

tests/compile-fail/copies.rs

@@ -5,15 +5,18 @@
 #![allow(let_and_return)]
 #![allow(needless_return)]
 #![allow(unused_variables)]
+#![allow(cyclomatic_complexity)]
 
+fn bar<T>(_: T) {}
 fn foo() -> bool { unimplemented!() }
 
 #[deny(if_same_then_else)]
+#[deny(match_same_arms)]
 fn if_same_then_else() -> &'static str {
     if true {
         foo();
     }
-    else { //~ERROR this if has identical blocks
+    else { //~ERROR this `if` has identical blocks
         foo();
     }
 
@@ -29,7 +32,7 @@ fn if_same_then_else() -> &'static str {
         foo();
         42
     }
-    else { //~ERROR this if has identical blocks
+    else { //~ERROR this `if` has identical blocks
         foo();
         42
     };
@@ -41,7 +44,7 @@ fn if_same_then_else() -> &'static str {
     let _ = if true {
         42
     }
-    else { //~ERROR this if has identical blocks
+    else { //~ERROR this `if` has identical blocks
         42
     };
 
@@ -56,7 +59,7 @@ fn if_same_then_else() -> &'static str {
         while foo() { break; }
         bar + 1;
     }
-    else { //~ERROR this if has identical blocks
+    else { //~ERROR this `if` has identical blocks
         let bar = if true {
             42
         }
@@ -69,29 +72,29 @@ fn if_same_then_else() -> &'static str {
     }
 
     if true {
-        match 42 {
+        let _ = match 42 {
-            42 => (),
+            42 => 1,
-            a if a > 0 => (),
+            a if a > 0 => 2,
-            10...15 => (),
+            10...15 => 3,
-            _ => (),
+            _ => 4,
-        }
+        };
     }
     else if false {
         foo();
     }
-    else if foo() { //~ERROR this if has identical blocks
+    else if foo() { //~ERROR this `if` has identical blocks
-        match 42 {
+        let _ = match 42 {
-            42 => (),
+            42 => 1,
-            a if a > 0 => (),
+            a if a > 0 => 2,
-            10...15 => (),
+            10...15 => 3,
-            _ => (),
+            _ => 4,
-        }
+        };
     }
 
     if true {
         if let Some(a) = Some(42) {}
     }
-    else { //~ERROR this if has identical blocks
+    else { //~ERROR this `if` has identical blocks
         if let Some(a) = Some(42) {}
     }
 
@@ -102,6 +105,30 @@ fn if_same_then_else() -> &'static str {
         if let Some(a) = Some(43) {}
     }
 
+    let _ = match 42 {
+        42 => foo(),
+        51 => foo(), //~ERROR this `match` has identical arm bodies
+        _ => true,
+    };
+
+    let _ = match Some(42) {
+        Some(42) => 24,
+        Some(a) => 24, // bindings are different
+        None => 0,
+    };
+
+    match (Some(42), Some(42)) {
+        (Some(a), None) => bar(a),
+        (None, Some(a)) => bar(a), //~ERROR this `match` has identical arm bodies
+        _ => (),
+    }
+
+    match (Some(42), Some("")) {
+        (Some(a), None) => bar(a),
+        (None, Some(a)) => bar(a), // bindings have different types
+        _ => (),
+    }
+
     if true {
         let foo = "";
         return &foo[0..];
@@ -110,7 +137,7 @@ fn if_same_then_else() -> &'static str {
         let foo = "bar";
         return &foo[0..];
     }
-    else { //~ERROR this if has identical blocks
+    else { //~ERROR this `if` has identical blocks
         let foo = "";
         return &foo[0..];
     }
@@ -124,19 +151,19 @@ fn ifs_same_cond() {
 
     if b {
     }
-    else if b { //~ERROR this if has the same condition as a previous if
+    else if b { //~ERROR this `if` has the same condition as a previous if
     }
 
     if a == 1 {
     }
-    else if a == 1 { //~ERROR this if has the same condition as a previous if
+    else if a == 1 { //~ERROR this `if` has the same condition as a previous if
     }
 
     if 2*a == 1 {
     }
     else if 2*a == 2 {
     }
-    else if 2*a == 1 { //~ERROR this if has the same condition as a previous if
+    else if 2*a == 1 { //~ERROR this `if` has the same condition as a previous if
     }
     else if a == 1 {
     }