Try to explain MATCH_SAME_ARMS better

Author: mcarton

clippy_lints/src/copies.rs

@@ -6,7 +6,7 @@ use std::collections::hash_map::Entry;
 use syntax::parse::token::InternedString;
 use syntax::util::small_vector::SmallVector;
 use utils::{SpanlessEq, SpanlessHash};
-use utils::{get_parent_expr, in_macro, span_note_and_lint};
+use utils::{get_parent_expr, in_macro, span_lint_and_then, span_note_and_lint, snippet};
 
 /// **What it does:** This lint checks for consecutive `ifs` with the same condition. This lint is
 /// `Warn` by default.
@@ -52,6 +52,23 @@ declare_lint! {
 ///     Baz => bar(), // <= oops
 /// }
 /// ```
+///
+/// This should probably be
+/// ```rust,ignore
+/// match foo {
+///     Bar => bar(),
+///     Quz => quz(),
+///     Baz => baz(), // <= fixed
+/// }
+/// ```
+///
+/// or if the original code was not a typo:
+/// ```rust,ignore
+/// match foo {
+///     Bar | Baz => bar(), // <= shows the intent better
+///     Quz => quz(),
+/// }
+/// ```
 declare_lint! {
     pub MATCH_SAME_ARMS,
     Warn,
@@ -143,12 +160,25 @@ fn lint_match_arms(cx: &LateContext, expr: &Expr) {
 
     if let ExprMatch(_, ref arms, MatchSource::Normal) = expr.node {
         if let Some((i, j)) = search_same(arms, hash, eq) {
-            span_note_and_lint(cx,
+            span_lint_and_then(cx,
                                MATCH_SAME_ARMS,
                                j.body.span,
                                "this `match` has identical arm bodies",
-                               i.body.span,
-                               "same as this");
+                               |db| {
+                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…
+
+                if i.pats.len() == 1 && j.pats.len() == 1 {
+                    let lhs = snippet(cx, i.pats[0].span, "<pat1>");
+                    let rhs = snippet(cx, j.pats[0].span, "<pat2>");
+                    db.span_note(i.body.span, &format!("consider refactoring into `{} | {}`", lhs, rhs));
+                }
+            });
         }
     }
 }

tests/compile-fail/copies.rs

@@ -21,6 +21,7 @@ struct Foo {
 #[deny(match_same_arms)]
 fn if_same_then_else() -> Result<&'static str, ()> {
     if true {
+        //~^NOTE same as this
         Foo { bar: 42 };
         0..10;
         ..;
@@ -62,6 +63,7 @@ fn if_same_then_else() -> Result<&'static str, ()> {
     }
 
     let _ = if true {
+        //~^NOTE same as this
         foo();
         42
     }
@@ -75,13 +77,15 @@ fn if_same_then_else() -> Result<&'static str, ()> {
     }
 
     let _ = if true {
+        //~^NOTE same as this
         42
     }
     else { //~ERROR this `if` has identical blocks
         42
     };
 
     if true {
+        //~^NOTE same as this
         let bar = if true {
             42
         }
@@ -105,6 +109,7 @@ fn if_same_then_else() -> Result<&'static str, ()> {
     }
 
     if true {
+        //~^NOTE same as this
         let _ = match 42 {
             42 => 1,
             a if a > 0 => 2,
@@ -125,13 +130,15 @@ fn if_same_then_else() -> Result<&'static str, ()> {
     }
 
     if true {
+        //~^NOTE same as this
         if let Some(a) = Some(42) {}
     }
     else { //~ERROR this `if` has identical blocks
         if let Some(a) = Some(42) {}
     }
 
     if true {
+        //~^NOTE same as this
         if let (1, .., 3) = (1, 2, 3) {}
     }
     else { //~ERROR this `if` has identical blocks
@@ -168,12 +175,16 @@ fn if_same_then_else() -> Result<&'static str, ()> {
 
     let _ = match 42 {
         42 => foo(),
+        //~^NOTE same as this
+        //~|NOTE `42 | 51`
         51 => foo(), //~ERROR this `match` has identical arm bodies
         _ => true,
     };
 
     let _ = match Some(42) {
         Some(_) => 24,
+        //~^NOTE same as this
+        //~|NOTE `Some(_) | None`
         None => 24, //~ERROR this `match` has identical arm bodies
     };
 
@@ -196,18 +207,24 @@ fn if_same_then_else() -> Result<&'static str, ()> {
 
     match (Some(42), Some(42)) {
         (Some(a), None) => bar(a),
+        //~^NOTE same as this
+        //~|NOTE `(Some(a), None) | (None, Some(a))`
         (None, Some(a)) => bar(a), //~ERROR this `match` has identical arm bodies
         _ => (),
     }
 
     match (Some(42), Some(42)) {
         (Some(a), ..) => bar(a),
+        //~^NOTE same as this
+        //~|NOTE `(Some(a), ..) | (.., Some(a))`
         (.., Some(a)) => bar(a), //~ERROR this `match` has identical arm bodies
         _ => (),
     }
 
     match (1, 2, 3) {
         (1, .., 3) => 42,
+        //~^NOTE same as this
+        //~|NOTE `(1, .., 3) | (.., 3)`
         (.., 3) => 42, //~ERROR this `match` has identical arm bodies
         _ => 0,
     };
@@ -219,13 +236,15 @@ fn if_same_then_else() -> Result<&'static str, ()> {
     }
 
     if true {
+        //~^NOTE same as this
         try!(Ok("foo"));
     }
     else { //~ERROR this `if` has identical blocks
         try!(Ok("foo"));
     }
 
     if true {
+        //~^NOTE same as this
         let foo = "";
         return Ok(&foo[0..]);
     }
@@ -246,16 +265,19 @@ fn ifs_same_cond() {
     let b = false;
 
     if b {
+        //~^NOTE same as this
     }
     else if b { //~ERROR this `if` has the same condition as a previous if
     }
 
     if a == 1 {
+        //~^NOTE same as this
     }
     else if a == 1 { //~ERROR this `if` has the same condition as a previous if
     }
 
     if 2*a == 1 {
+        //~^NOTE same as this
     }
     else if 2*a == 2 {
     }