From a2d752807a017f70f28a70b7f16dc6a20cd566ee Mon Sep 17 00:00:00 2001 From: Bood Qian Date: Sat, 11 Feb 2017 14:57:50 +0800 Subject: [PATCH 1/6] Lint on `Err(_)` arm of a match --- CHANGELOG.md | 1 + README.md | 3 +- clippy_lints/src/lib.rs | 1 + clippy_lints/src/matches.rs | 65 +++++++++++++++++++++++++++++++++++-- tests/ui/matches.rs | 38 ++++++++++++++++++++++ tests/ui/matches.stderr | 25 +++++++++++++- 6 files changed, 129 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b72387f0d71..4d6aff398b6 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 3f3ed135e0c..de01a253323 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 1099738d799..661e6415bb3 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 9cb557ba646..d9e5daf889f 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 00faed26818..277f96be686 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 bc254cb0bcb..438c2e12c29 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 From 64d2f8af8e44423edc94f0ac3eb186c66966541b Mon Sep 17 00:00:00 2001 From: Bood Qian Date: Sat, 11 Feb 2017 21:42:42 +0800 Subject: [PATCH 2/6] Lint on `panic!` only --- clippy_lints/src/matches.rs | 32 +++++++++++++--------------- tests/ui/matches.rs | 42 +++++++++++++++++++++++-------------- tests/ui/matches.stderr | 15 ++++++++++--- 3 files changed, 53 insertions(+), 36 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index d9e5daf889f..57216ad8d41 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -121,9 +121,11 @@ declare_lint! { "a match with overlapping arms" } -/// **What it does:** Checks for arm matches all errors with `Err(_)`. +/// **What it does:** Checks for arm which matches all errors with `Err(_)` +/// and take drastic actions like `panic!`. /// -/// **Why is this bad?** It is a bad practice to catch all errors the same way +/// **Why is this bad?** It is generally a bad practice, just like +/// catching all exceptions in java with `catch(Exception)` /// /// **Known problems:** None. /// @@ -132,13 +134,13 @@ declare_lint! { /// let x : Result(i32, &str) = Ok(3); /// match x { /// Ok(_) => println!("ok"), -/// Err(_) => println!("err"), +/// Err(_) => panic!("err"), /// } /// ``` declare_lint! { pub MATCH_WILD_ERR_ARM, Warn, - "a match with `Err(_)` arm" + "a match with `Err(_)` arm and take drastic actions" } #[allow(missing_copy_implementations)] @@ -353,32 +355,28 @@ fn check_wild_err_arm(cx: &LateContext, ex: &Expr, arms: &[Arm]) { 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 { + if_let_chain! {[ + let ExprBlock(ref block) = arm.body.node, + is_panic_block(cx, block) + ], { 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 { +// If the block contains only a `panic!` macro (as expression or statement) +fn is_panic_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(), + (&Some(ref exp), 0, _) => is_expn_of(cx, exp.span, "panic").is_some() && is_expn_of(cx, exp.span, "unreachable").is_none(), + (&None, 1, Some(ref stmt)) => is_expn_of(cx, stmt.span, "panic").is_some() && is_expn_of(cx, stmt.span, "unreachable").is_none(), _ => false } } diff --git a/tests/ui/matches.rs b/tests/ui/matches.rs index 277f96be686..46a99293a35 100644 --- a/tests/ui/matches.rs +++ b/tests/ui/matches.rs @@ -286,34 +286,44 @@ fn overlapping() { fn match_wild_err_arm() { let x: Result = Ok(3); + match x { + Ok(3) => println!("ok"), + Ok(_) => println!("ok"), + Err(_) => panic!("err") + } + + match x { + Ok(3) => println!("ok"), + Ok(_) => println!("ok"), + Err(_) => {panic!()} + } + + match x { + Ok(3) => println!("ok"), + Ok(_) => println!("ok"), + Err(_) => {panic!();} + } + + // allowed when not with `panic!` block 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!() - } - + // allowed when used with `unreachable!` 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"), diff --git a/tests/ui/matches.stderr b/tests/ui/matches.stderr index 438c2e12c29..bc8584b8587 100644 --- a/tests/ui/matches.stderr +++ b/tests/ui/matches.stderr @@ -391,7 +391,7 @@ note: overlaps with this error: Err(_) will match all errors, maybe not a good idea --> $DIR/matches.rs:292:9 | -292 | Err(_) => println!("err") +292 | Err(_) => panic!("err") | ^^^^^^ | = note: #[deny(match_wild_err_arm)] implied by #[deny(clippy)] @@ -405,11 +405,20 @@ note: lint level defined here error: Err(_) will match all errors, maybe not a good idea --> $DIR/matches.rs:298:9 | -298 | Err(_) => { +298 | Err(_) => {panic!()} | ^^^^^^ | = 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 +error: Err(_) will match all errors, maybe not a good idea + --> $DIR/matches.rs:304:9 + | +304 | Err(_) => {panic!();} + | ^^^^^^ + | + = 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 26 previous errors From 12c53752b80a6bb1c0cab735f5e87a765de1ef26 Mon Sep 17 00:00:00 2001 From: Bood Qian Date: Sat, 11 Feb 2017 21:47:26 +0800 Subject: [PATCH 3/6] Apply rustfmt --- clippy_lints/src/matches.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 57216ad8d41..310e9a22753 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -10,7 +10,8 @@ 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, walk_ptrs_ty, is_expn_of}; +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` @@ -352,10 +353,9 @@ fn check_wild_err_arm(cx: &LateContext, ex: &Expr, arms: &[Arm]) { 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 - if_let_chain! {[ + if inner.iter().any(|pat| pat.node == PatKind::Wild) && path_str == "Err" { + // `Err(_)` arm found + if_let_chain! {[ let ExprBlock(ref block) = arm.body.node, is_panic_block(cx, block) ], { @@ -364,7 +364,8 @@ fn check_wild_err_arm(cx: &LateContext, ex: &Expr, arms: &[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"); + "to remove this warning, match each error seperately \ + or use unreachable macro"); }} } } @@ -375,9 +376,13 @@ fn check_wild_err_arm(cx: &LateContext, ex: &Expr, arms: &[Arm]) { // If the block contains only a `panic!` macro (as expression or statement) fn is_panic_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, "panic").is_some() && is_expn_of(cx, exp.span, "unreachable").is_none(), - (&None, 1, Some(ref stmt)) => is_expn_of(cx, stmt.span, "panic").is_some() && is_expn_of(cx, stmt.span, "unreachable").is_none(), - _ => false + (&Some(ref exp), 0, _) => { + is_expn_of(cx, exp.span, "panic").is_some() && is_expn_of(cx, exp.span, "unreachable").is_none() + }, + (&None, 1, Some(ref stmt)) => { + is_expn_of(cx, stmt.span, "panic").is_some() && is_expn_of(cx, stmt.span, "unreachable").is_none() + }, + _ => false, } } From 1c381ec642556ced608517df5518bbec7f535e14 Mon Sep 17 00:00:00 2001 From: Bood Qian Date: Sat, 11 Feb 2017 22:11:19 +0800 Subject: [PATCH 4/6] Move more into if_let_chain --- clippy_lints/src/matches.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 310e9a22753..02d089e1af4 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -353,21 +353,21 @@ fn check_wild_err_arm(cx: &LateContext, ex: &Expr, arms: &[Arm]) { 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 - if_let_chain! {[ - let ExprBlock(ref block) = arm.body.node, - is_panic_block(cx, block) - ], { - 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_let_chain! {[ + path_str == "Err", + inner.iter().any(|pat| pat.node == PatKind::Wild), + let ExprBlock(ref block) = arm.body.node, + is_panic_block(cx, block) + ], { + // `Err(_)` arm with `panic!` found + 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"); + }} } } } From 9824c997fe6ee50015e1701e94231331d815db7d Mon Sep 17 00:00:00 2001 From: Bood Qian Date: Sun, 12 Feb 2017 09:16:37 +0800 Subject: [PATCH 5/6] Remove unnecessary ref --- clippy_lints/src/matches.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 02d089e1af4..66b55c48455 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -379,7 +379,7 @@ fn is_panic_block(cx: &LateContext, block: &Block) -> bool { (&Some(ref exp), 0, _) => { is_expn_of(cx, exp.span, "panic").is_some() && is_expn_of(cx, exp.span, "unreachable").is_none() }, - (&None, 1, Some(ref stmt)) => { + (&None, 1, Some(stmt)) => { is_expn_of(cx, stmt.span, "panic").is_some() && is_expn_of(cx, stmt.span, "unreachable").is_none() }, _ => false, From 1177f3915c7c487ef968552e7cbe4c7bc2e5228a Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 13 Feb 2017 11:15:36 +0100 Subject: [PATCH 6/6] update readme --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index c87733f9dfa..3660937262a 100644 --- a/README.md +++ b/README.md @@ -180,7 +180,7 @@ transparently: ## Lints -There are 187 lints included in this crate: +There are 188 lints included in this crate: name | default | triggers on -----------------------------------------------------------------------------------------------------------------------|---------|---------------------------------------------------------------------------------------------------------------------------------- @@ -271,7 +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 +[match_wild_err_arm](https://github.com/Manishearth/rust-clippy/wiki#match_wild_err_arm) | warn | a match with `Err(_)` arm and take drastic actions [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