Skip to content

Commit

Permalink
Lint on panic! only
Browse files Browse the repository at this point in the history
  • Loading branch information
Bood Qian committed Feb 11, 2017
1 parent a2d7528 commit 64d2f8a
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 27 deletions.
32 changes: 15 additions & 17 deletions clippy_lints/src/matches.rs
Expand Up @@ -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.
///
Expand All @@ -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)]
Expand Down Expand Up @@ -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
}
}
Expand Down
24 changes: 17 additions & 7 deletions tests/ui/matches.rs
Expand Up @@ -289,31 +289,41 @@ fn match_wild_err_arm() {
match x {
Ok(3) => println!("ok"),
Ok(_) => println!("ok"),
Err(_) => println!("err")
Err(_) => panic!("err")
}

match x {
Ok(3) => println!("ok"),
Ok(_) => println!("ok"),
Err(_) => {
println!("err");
unreachable!()
}
Err(_) => {panic!()}
}

// allowed when using with unreachable as the only statement/expression
match x {
Ok(3) => println!("ok"),
Ok(_) => println!("ok"),
Err(_) => unreachable!()
Err(_) => {panic!();}
}

// allowed when not with `panic!` block
match x {
Ok(3) => println!("ok"),
Ok(_) => println!("ok"),
Err(_) => println!("err")
}

// 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"),
Expand Down
15 changes: 12 additions & 3 deletions tests/ui/matches.stderr
Expand Up @@ -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)]
Expand All @@ -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

0 comments on commit 64d2f8a

Please sign in to comment.