diff --git a/CHANGELOG.md b/CHANGELOG.md index b72387f0d718..4d6aff398b66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/README.md b/README.md index 3f3ed135e0ce..de01a2533238 100644 --- 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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1099738d7995..661e6415bb3f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -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, diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 9cb557ba6460..d9e5daf889fa 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -10,7 +10,7 @@ use std::collections::Bound; 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` @@ -121,6 +121,26 @@ declare_lint! { "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 @@ impl LintPass for MatchPass { MATCH_REF_PATS, MATCH_BOOL, SINGLE_MATCH_ELSE, - MATCH_OVERLAPPING_ARM) + MATCH_OVERLAPPING_ARM, + MATCH_WILD_ERR_ARM) } } @@ -143,6 +164,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MatchPass { 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 { diff --git a/tests/ui/matches.rs b/tests/ui/matches.rs index 00faed26818a..277f96be6862 100644 --- a/tests/ui/matches.rs +++ b/tests/ui/matches.rs @@ -283,5 +283,43 @@ fn overlapping() { } } +fn match_wild_err_arm() { + let x: Result = 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() { } diff --git a/tests/ui/matches.stderr b/tests/ui/matches.stderr index bc254cb0bcb3..438c2e12c29f 100644 --- a/tests/ui/matches.stderr +++ b/tests/ui/matches.stderr @@ -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