]> git.lizzy.rs Git - rust.git/commitdiff
Lint on `Err(_)` arm of a match
authorBood Qian <bood@glowing.com>
Sat, 11 Feb 2017 06:57:50 +0000 (14:57 +0800)
committerBood Qian <bood@glowing.com>
Sat, 11 Feb 2017 06:57:50 +0000 (14:57 +0800)
CHANGELOG.md
README.md
clippy_lints/src/lib.rs
clippy_lints/src/matches.rs
tests/ui/matches.rs
tests/ui/matches.stderr

index b72387f0d71883a7caef12c32edcff38756c3584..4d6aff398b6693026746a6ec167247ae6bda9d74 100644 (file)
@@ -362,6 +362,7 @@ All notable changes to this project will be documented in this file.
 [`match_overlapping_arm`]: https://github.com/Manishearth/rust-clippy/wiki#match_overlapping_arm
 [`match_ref_pats`]: https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats
 [`match_same_arms`]: https://github.com/Manishearth/rust-clippy/wiki#match_same_arms
+[`match_wild_err_arm`]: https://github.com/Manishearth/rust-clippy/wiki#match_wild_err_arm
 [`mem_forget`]: https://github.com/Manishearth/rust-clippy/wiki#mem_forget
 [`min_max`]: https://github.com/Manishearth/rust-clippy/wiki#min_max
 [`misrefactored_assign_op`]: https://github.com/Manishearth/rust-clippy/wiki#misrefactored_assign_op
index 3f3ed135e0cea2781a196c7e5ac06c37c9f78f38..de01a2533238173b44446d30a8be781d5f1ef487 100644 (file)
--- a/README.md
+++ b/README.md
@@ -180,7 +180,7 @@ transparently:
 
 ## Lints
 
-There are 186 lints included in this crate:
+There are 187 lints included in this crate:
 
 name                                                                                                                   | default | triggers on
 -----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -271,6 +271,7 @@ name
 [match_overlapping_arm](https://github.com/Manishearth/rust-clippy/wiki#match_overlapping_arm)                         | warn    | a match with overlapping arms
 [match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats)                                       | warn    | a match or `if let` with all arms prefixed with `&` instead of deref-ing the match expression
 [match_same_arms](https://github.com/Manishearth/rust-clippy/wiki#match_same_arms)                                     | warn    | `match` with identical arm bodies
+[match_wild_err_arm](https://github.com/Manishearth/rust-clippy/wiki#match_wild_err_arm)                               | warn    | a match with `Err(_)` arm
 [mem_forget](https://github.com/Manishearth/rust-clippy/wiki#mem_forget)                                               | allow   | `mem::forget` usage on `Drop` types, likely to cause memory leaks
 [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
 [misrefactored_assign_op](https://github.com/Manishearth/rust-clippy/wiki#misrefactored_assign_op)                     | warn    | having a variable on both sides of an assign op
index 1099738d7995077129ad4bfbc1ea08e895bfafa8..661e6415bb3f7a2e0f40bd6fb3988563d3d866b4 100644 (file)
@@ -412,6 +412,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         matches::MATCH_BOOL,
         matches::MATCH_OVERLAPPING_ARM,
         matches::MATCH_REF_PATS,
+        matches::MATCH_WILD_ERR_ARM,
         matches::SINGLE_MATCH,
         methods::CHARS_NEXT_CMP,
         methods::CLONE_DOUBLE_REF,
index 9cb557ba64600a5f3176a3792c1fc1379cde2fe0..d9e5daf889fabb76d72a5b40927b13eae80b3a5c 100644 (file)
@@ -10,7 +10,7 @@
 use syntax::ast::LitKind;
 use syntax::codemap::Span;
 use utils::paths;
-use utils::{match_type, snippet, span_note_and_lint, span_lint_and_then, in_external_macro, expr_block};
+use utils::{match_type, snippet, span_note_and_lint, span_lint_and_then, in_external_macro, expr_block, walk_ptrs_ty, is_expn_of};
 use utils::sugg::Sugg;
 
 /// **What it does:** Checks for matches with a single arm where an `if let`
     "a match with overlapping arms"
 }
 
+/// **What it does:** Checks for arm matches all errors with `Err(_)`.
+///
+/// **Why is this bad?** It is a bad practice to catch all errors the same way
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// let x : Result(i32, &str) = Ok(3);
+/// match x {
+///     Ok(_) => println!("ok"),
+///     Err(_) => println!("err"),
+/// }
+/// ```
+declare_lint! {
+    pub MATCH_WILD_ERR_ARM,
+    Warn,
+    "a match with `Err(_)` arm"
+}
+
 #[allow(missing_copy_implementations)]
 pub struct MatchPass;
 
@@ -130,7 +150,8 @@ fn get_lints(&self) -> LintArray {
                     MATCH_REF_PATS,
                     MATCH_BOOL,
                     SINGLE_MATCH_ELSE,
-                    MATCH_OVERLAPPING_ARM)
+                    MATCH_OVERLAPPING_ARM,
+                    MATCH_WILD_ERR_ARM)
     }
 }
 
@@ -143,6 +164,7 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
             check_single_match(cx, ex, arms, expr);
             check_match_bool(cx, ex, arms, expr);
             check_overlapping_arms(cx, ex, arms);
+            check_wild_err_arm(cx, ex, arms);
         }
         if let ExprMatch(ref ex, ref arms, source) = expr.node {
             check_match_ref_pats(cx, ex, arms, source, expr);
@@ -322,6 +344,45 @@ fn check_overlapping_arms(cx: &LateContext, ex: &Expr, arms: &[Arm]) {
     }
 }
 
+fn check_wild_err_arm(cx: &LateContext, ex: &Expr, arms: &[Arm]) {
+    let ex_ty = walk_ptrs_ty(cx.tables.expr_ty(ex));
+    if match_type(cx, ex_ty, &paths::RESULT) {
+        for arm in arms {
+            if let PatKind::TupleStruct(ref path, ref inner, _) = arm.pats[0].node {
+                let path_str = print::to_string(print::NO_ANN, |s| s.print_qpath(path, false));
+                if inner.iter().any(|pat| pat.node == PatKind::Wild) &&
+                    path_str == "Err" {
+                        // `Err(_)` arm found
+                        let mut need_lint = true;
+                        if let ExprBlock(ref block) = arm.body.node {
+                            if is_unreachable_block(cx, block) {
+                                need_lint = false;
+                            }
+                        }
+
+                        if need_lint {
+                            span_note_and_lint(cx,
+                                               MATCH_WILD_ERR_ARM,
+                                               arm.pats[0].span,
+                                               "Err(_) will match all errors, maybe not a good idea",
+                                               arm.pats[0].span,
+                                               "to remove this warning, match each error seperately or use unreachable macro");
+                        }
+                }
+            }
+        }
+    }
+}
+
+// If the block contains only a `unreachable!` macro (as expression or statement)
+fn is_unreachable_block(cx: &LateContext, block: &Block) -> bool {
+    match (&block.expr, block.stmts.len(), block.stmts.first()) {
+        (&Some(ref exp), 0, _) => is_expn_of(cx, exp.span, "unreachable").is_some(),
+        (&None, 1, Some(ref stmt)) => is_expn_of(cx, stmt.span, "unreachable").is_some(),
+        _ => false
+    }
+}
+
 fn check_match_ref_pats(cx: &LateContext, ex: &Expr, arms: &[Arm], source: MatchSource, expr: &Expr) {
     if has_only_ref_pats(arms) {
         if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node {
index 00faed26818ae62760a7a829ceee5fb6fe95d324..277f96be686244c928370ec82e8ba3ca93332f34 100644 (file)
@@ -283,5 +283,43 @@ fn overlapping() {
     }
 }
 
+fn match_wild_err_arm() {
+    let x: Result<i32, &str> = Ok(3);
+
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => println!("err")
+    }
+
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => {
+            println!("err");
+            unreachable!()
+        }
+    }
+
+    // allowed when using with unreachable as the only statement/expression
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => unreachable!()
+    }
+
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => {unreachable!()}
+    }
+
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => {unreachable!();}
+    }
+}
+
 fn main() {
 }
index bc254cb0bcb3dd06adafa0230ab4481bfe2d20e7..438c2e12c29f09658dee0ebe8dbb5eb4a7505dee 100644 (file)
@@ -388,5 +388,28 @@ note: overlaps with this
 275 |         0 ... 11 => println!("0 ... 11"),
     |         ^^^^^^^^
 
-error: aborting due to 23 previous errors
+error: Err(_) will match all errors, maybe not a good idea
+   --> $DIR/matches.rs:292:9
+    |
+292 |         Err(_) => println!("err")
+    |         ^^^^^^
+    |
+    = note: #[deny(match_wild_err_arm)] implied by #[deny(clippy)]
+note: lint level defined here
+   --> $DIR/matches.rs:5:9
+    |
+5   | #![deny(clippy)]
+    |         ^^^^^^
+    = note: to remove this warning, match each error seperately or use unreachable macro
+
+error: Err(_) will match all errors, maybe not a good idea
+   --> $DIR/matches.rs:298:9
+    |
+298 |         Err(_) => {
+    |         ^^^^^^
+    |
+    = note: #[deny(match_wild_err_arm)] implied by #[deny(clippy)]
+    = note: to remove this warning, match each error seperately or use unreachable macro
+
+error: aborting due to 25 previous errors