lint on single match expressions with a value in the else path

diff --git a/README.md b/README.md
index ec1dd7f6dbb3a7f93cf665565d71912e79197b59..360370361475fd9bec2105a12d7a3acf5e17bd0c 100644 (file)
--- a/README.md
+++ b/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 104 lints included in this crate:
+There are 105 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -89,6 +89,7 @@ name
 [shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated)                           | allow   | The name is re-bound without even using the original value
 [should_implement_trait](https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait)               | warn    | defining a method that should be implementing a std trait
 [single_match](https://github.com/Manishearth/rust-clippy/wiki#single_match)                                   | warn    | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead
+[single_match_else](https://github.com/Manishearth/rust-clippy/wiki#single_match_else)                         | allow   | a match statement with a two arms where the second arm's pattern is a wildcard; recommends `if let` instead
 [str_to_string](https://github.com/Manishearth/rust-clippy/wiki#str_to_string)                                 | warn    | using `to_string()` on a str, which should be `to_owned()`
 [string_add](https://github.com/Manishearth/rust-clippy/wiki#string_add)                                       | allow   | using `x + ..` where x is a `String`; suggests using `push_str()` instead
 [string_add_assign](https://github.com/Manishearth/rust-clippy/wiki#string_add_assign)                         | allow   | using `x = x + ..` where x is a `String`; suggests using `push_str()` instead

diff --git a/src/lib.rs b/src/lib.rs
index 82bd2d39a1294c469c33a87ce91d09d34b69ebbf..6d8ea99ebcccf7f6a86040f028d27b60d9b3e654 100644 (file)
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -149,6 +149,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
 
 
     reg.register_lint_group("clippy_pedantic", vec![
+        matches::SINGLE_MATCH_ELSE,
         methods::OPTION_UNWRAP_USED,
         methods::RESULT_UNWRAP_USED,
         methods::WRONG_PUB_SELF_CONVENTION,

diff --git a/src/matches.rs b/src/matches.rs
index 0c18d35fa128333198085ccba044559ebe721cd4..c866a48d223b2e83f605f000c9e9df867992be54 100644 (file)
--- a/src/matches.rs
+++ b/src/matches.rs
@@ -28,6 +28,23 @@
               "a match statement with a single nontrivial arm (i.e, where the other arm \
                is `_ => {}`) is used; recommends `if let` instead");
 
+/// **What it does:** This lint checks for matches with a two arms where an `if let` will usually suffice. It is `Allow` by default.
+///
+/// **Why is this bad?** Just readability – `if let` nests less than a `match`.
+///
+/// **Known problems:** Personal style preferences may differ
+///
+/// **Example:**
+/// ```
+/// match x {
+///     Some(ref foo) -> bar(foo),
+///     _ => bar(other_ref),
+/// }
+/// ```
+declare_lint!(pub SINGLE_MATCH_ELSE, Allow,
+             "a match statement with a two arms where the second arm's pattern is a wildcard; \
+              recommends `if let` instead");
+
 /// **What it does:** This lint checks for matches where all arms match a reference, suggesting to remove the reference and deref the matched expression instead. It also checks for `if let &foo = bar` blocks. It is `Warn` by default.
 ///
 /// **Why is this bad?** It just makes the code less readable. That reference destructuring adds nothing to the code.
@@ -89,7 +106,7 @@
 
 impl LintPass for MatchPass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(SINGLE_MATCH, MATCH_REF_PATS, MATCH_BOOL)
+        lint_array!(SINGLE_MATCH, MATCH_REF_PATS, MATCH_BOOL, SINGLE_MATCH_ELSE)
     }
 }
 
@@ -112,34 +129,49 @@ fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
 fn check_single_match(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) {
     if arms.len() == 2 &&
        arms[0].pats.len() == 1 && arms[0].guard.is_none() &&
-       arms[1].pats.len() == 1 && arms[1].guard.is_none() &&
-       is_unit_expr(&arms[1].body) {
+       arms[1].pats.len() == 1 && arms[1].guard.is_none() {
+           let els = if is_unit_expr(&arms[1].body) {
+               None
+           } else if let ExprBlock(_) = arms[1].body.node {
+               // matches with blocks that contain statements are prettier as `if let + else`
+               Some(&*arms[1].body)
+           } else {
+               // allow match arms with just expressions
+               return;
+           };
            let ty = cx.tcx.expr_ty(ex);
            if ty.sty != ty::TyBool || cx.current_level(MATCH_BOOL) == Allow {
-                check_single_match_single_pattern(cx, ex, arms, expr);
-                check_single_match_opt_like(cx, ex, arms, expr, ty);
+                check_single_match_single_pattern(cx, ex, arms, expr, els);
+                check_single_match_opt_like(cx, ex, arms, expr, ty, els);
            }
     }
 }
 
-fn check_single_match_single_pattern(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) {
+fn check_single_match_single_pattern(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr, els: Option<&Expr>) {
     if arms[1].pats[0].node == PatWild {
+        let lint = if els.is_some() {
+            SINGLE_MATCH_ELSE
+        } else {
+            SINGLE_MATCH
+        };
+        let els_str = els.map_or(String::new(), |els| format!(" else {}", expr_block(cx, els, None, "..")));
         span_lint_and_then(cx,
-                           SINGLE_MATCH,
+                           lint,
                            expr.span,
                            "you seem to be trying to use match for destructuring a single pattern. \
                            Consider using `if let`", |db| {
                 db.span_suggestion(expr.span, "try this",
-                                   format!("if let {} = {} {}",
+                                   format!("if let {} = {} {}{}",
                                            snippet(cx, arms[0].pats[0].span, ".."),
                                            snippet(cx, ex.span, ".."),
-                                           expr_block(cx, &arms[0].body, None, "..")));
+                                           expr_block(cx, &arms[0].body, None, ".."),
+                                           els_str));
             });
     }
 }
 
-fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr, ty: ty::Ty) {
-    // list of candidate Enums we know will never get any more membre
+fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr, ty: ty::Ty, els: Option<&Expr>) {
+    // list of candidate Enums we know will never get any more members
     let candidates = &[
         (&COW_PATH, "Borrowed"),
         (&COW_PATH, "Cow::Borrowed"),
@@ -151,23 +183,37 @@ fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr:
     ];
 
     let path = match arms[1].pats[0].node {
-        PatEnum(ref path, _) => path.to_string(),
+        PatEnum(ref path, Some(ref inner)) => {
+            // contains any non wildcard patterns? e.g. Err(err)
+            if inner.iter().any(|pat| if let PatWild = pat.node { false } else { true }) {
+                return;
+            }
+            path.to_string()
+        },
+        PatEnum(ref path, None) => path.to_string(),
         PatIdent(BindByValue(MutImmutable), ident, None) => ident.node.to_string(),
         _ => return
     };
 
     for &(ty_path, pat_path) in candidates {
         if &path == pat_path && match_type(cx, ty, ty_path) {
+            let lint = if els.is_some() {
+                SINGLE_MATCH_ELSE
+            } else {
+                SINGLE_MATCH
+            };
+            let els_str = els.map_or(String::new(), |els| format!(" else {}", expr_block(cx, els, None, "..")));
             span_lint_and_then(cx,
-                               SINGLE_MATCH,
+                               lint,
                                expr.span,
                                "you seem to be trying to use match for destructuring a single pattern. \
                                Consider using `if let`", |db| {
                 db.span_suggestion(expr.span, "try this",
-                                   format!("if let {} = {} {}",
+                                   format!("if let {} = {} {}{}",
                                            snippet(cx, arms[0].pats[0].span, ".."),
                                            snippet(cx, ex.span, ".."),
-                                           expr_block(cx, &arms[0].body, None, "..")));
+                                           expr_block(cx, &arms[0].body, None, ".."),
+                                           els_str));
             });
         }
     }

diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs
index c58e62419c6b7cd866dbf267efdd890af01db1f5..71cc7c59f8fdf93007c5e749a49d317043c3424e 100644 (file)
--- a/tests/compile-fail/matches.rs
+++ b/tests/compile-fail/matches.rs
@@ -3,12 +3,32 @@
 #![plugin(clippy)]
 #![deny(clippy)]
 #![allow(unused)]
+#![deny(single_match_else)]
 
 use std::borrow::Cow;
 
 enum Foo { Bar, Baz(u8) }
 use Foo::*;
 
+enum ExprNode {
+    ExprAddrOf,
+    Butterflies,
+    Unicorns,
+}
+
+static NODE: ExprNode = ExprNode::Unicorns;
+
+fn unwrap_addr() -> Option<&'static ExprNode> {
+    match ExprNode::Butterflies {   //~ ERROR you seem to be trying to use match
+                                    //~^ HELP try
+        ExprNode::ExprAddrOf => Some(&NODE),
+        _ => {
+            let x = 5;
+            None
+        },
+    }
+}
+
 fn single_match(){
     let x = Some(1u8);
 
@@ -33,7 +53,7 @@ fn single_match(){
         _ => ()
     }
 
-    // Not linted (content in the else)
+    // Not linted (no block with statements in the single arm)
     match z {
         (2...3, 7...9) => println!("{:?}", z),
         _ => println!("nope"),