Lint on `Err(_)` arm of a match

This commit is contained in:
Bood Qian 2017-02-11 14:57:50 +08:00
parent 37a0e52a1e
commit a2d752807a
6 changed files with 129 additions and 4 deletions

View 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

View File

@ -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

View 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,

View File

@ -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 {

View 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() {
}

View 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