From d6c0435c81c594c9fb3d563d0306ac24c25cc2ca Mon Sep 17 00:00:00 2001 From: Oliver 'ker' Schneider Date: Sun, 24 Jan 2016 12:24:16 +0100 Subject: [PATCH 1/2] lint on single match expressions with a value in the else path --- README.md | 3 +- src/lib.rs | 1 + src/matches.rs | 76 ++++++++++++++++++++++++++++------- tests/compile-fail/matches.rs | 22 +++++++++- 4 files changed, 85 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index ec1dd7f6dbb..36037036147 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 104 lints included in this crate: +There are 105 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -89,6 +89,7 @@ name [shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | allow | The name is re-bound without even using the original value [should_implement_trait](https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait) | warn | defining a method that should be implementing a std trait [single_match](https://github.com/Manishearth/rust-clippy/wiki#single_match) | warn | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead +[single_match_else](https://github.com/Manishearth/rust-clippy/wiki#single_match_else) | allow | a match statement with a two arms where the second arm's pattern is a wildcard; recommends `if let` instead [str_to_string](https://github.com/Manishearth/rust-clippy/wiki#str_to_string) | warn | using `to_string()` on a str, which should be `to_owned()` [string_add](https://github.com/Manishearth/rust-clippy/wiki#string_add) | allow | using `x + ..` where x is a `String`; suggests using `push_str()` instead [string_add_assign](https://github.com/Manishearth/rust-clippy/wiki#string_add_assign) | allow | using `x = x + ..` where x is a `String`; suggests using `push_str()` instead diff --git a/src/lib.rs b/src/lib.rs index 82bd2d39a12..6d8ea99ebcc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -149,6 +149,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_group("clippy_pedantic", vec![ + matches::SINGLE_MATCH_ELSE, methods::OPTION_UNWRAP_USED, methods::RESULT_UNWRAP_USED, methods::WRONG_PUB_SELF_CONVENTION, diff --git a/src/matches.rs b/src/matches.rs index 0c18d35fa12..c866a48d223 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -28,6 +28,23 @@ declare_lint!(pub SINGLE_MATCH, Warn, "a match statement with a single nontrivial arm (i.e, where the other arm \ is `_ => {}`) is used; recommends `if let` instead"); +/// **What it does:** This lint checks for matches with a two arms where an `if let` will usually suffice. It is `Allow` by default. +/// +/// **Why is this bad?** Just readability – `if let` nests less than a `match`. +/// +/// **Known problems:** Personal style preferences may differ +/// +/// **Example:** +/// ``` +/// match x { +/// Some(ref foo) -> bar(foo), +/// _ => bar(other_ref), +/// } +/// ``` +declare_lint!(pub SINGLE_MATCH_ELSE, Allow, + "a match statement with a two arms where the second arm's pattern is a wildcard; \ + recommends `if let` instead"); + /// **What it does:** This lint checks for matches where all arms match a reference, suggesting to remove the reference and deref the matched expression instead. It also checks for `if let &foo = bar` blocks. It is `Warn` by default. /// /// **Why is this bad?** It just makes the code less readable. That reference destructuring adds nothing to the code. @@ -89,7 +106,7 @@ pub struct MatchPass; impl LintPass for MatchPass { fn get_lints(&self) -> LintArray { - lint_array!(SINGLE_MATCH, MATCH_REF_PATS, MATCH_BOOL) + lint_array!(SINGLE_MATCH, MATCH_REF_PATS, MATCH_BOOL, SINGLE_MATCH_ELSE) } } @@ -112,34 +129,49 @@ impl LateLintPass for MatchPass { fn check_single_match(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { if arms.len() == 2 && arms[0].pats.len() == 1 && arms[0].guard.is_none() && - arms[1].pats.len() == 1 && arms[1].guard.is_none() && - is_unit_expr(&arms[1].body) { + arms[1].pats.len() == 1 && arms[1].guard.is_none() { + let els = if is_unit_expr(&arms[1].body) { + None + } else if let ExprBlock(_) = arms[1].body.node { + // matches with blocks that contain statements are prettier as `if let + else` + Some(&*arms[1].body) + } else { + // allow match arms with just expressions + return; + }; let ty = cx.tcx.expr_ty(ex); if ty.sty != ty::TyBool || cx.current_level(MATCH_BOOL) == Allow { - check_single_match_single_pattern(cx, ex, arms, expr); - check_single_match_opt_like(cx, ex, arms, expr, ty); + check_single_match_single_pattern(cx, ex, arms, expr, els); + check_single_match_opt_like(cx, ex, arms, expr, ty, els); } } } -fn check_single_match_single_pattern(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { +fn check_single_match_single_pattern(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr, els: Option<&Expr>) { if arms[1].pats[0].node == PatWild { + let lint = if els.is_some() { + SINGLE_MATCH_ELSE + } else { + SINGLE_MATCH + }; + let els_str = els.map_or(String::new(), |els| format!(" else {}", expr_block(cx, els, None, ".."))); span_lint_and_then(cx, - SINGLE_MATCH, + lint, expr.span, "you seem to be trying to use match for destructuring a single pattern. \ Consider using `if let`", |db| { db.span_suggestion(expr.span, "try this", - format!("if let {} = {} {}", + format!("if let {} = {} {}{}", snippet(cx, arms[0].pats[0].span, ".."), snippet(cx, ex.span, ".."), - expr_block(cx, &arms[0].body, None, ".."))); + expr_block(cx, &arms[0].body, None, ".."), + els_str)); }); } } -fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr, ty: ty::Ty) { - // list of candidate Enums we know will never get any more membre +fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr, ty: ty::Ty, els: Option<&Expr>) { + // list of candidate Enums we know will never get any more members let candidates = &[ (&COW_PATH, "Borrowed"), (&COW_PATH, "Cow::Borrowed"), @@ -151,23 +183,37 @@ fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: ]; let path = match arms[1].pats[0].node { - PatEnum(ref path, _) => path.to_string(), + PatEnum(ref path, Some(ref inner)) => { + // contains any non wildcard patterns? e.g. Err(err) + if inner.iter().any(|pat| if let PatWild = pat.node { false } else { true }) { + return; + } + path.to_string() + }, + PatEnum(ref path, None) => path.to_string(), PatIdent(BindByValue(MutImmutable), ident, None) => ident.node.to_string(), _ => return }; for &(ty_path, pat_path) in candidates { if &path == pat_path && match_type(cx, ty, ty_path) { + let lint = if els.is_some() { + SINGLE_MATCH_ELSE + } else { + SINGLE_MATCH + }; + let els_str = els.map_or(String::new(), |els| format!(" else {}", expr_block(cx, els, None, ".."))); span_lint_and_then(cx, - SINGLE_MATCH, + lint, expr.span, "you seem to be trying to use match for destructuring a single pattern. \ Consider using `if let`", |db| { db.span_suggestion(expr.span, "try this", - format!("if let {} = {} {}", + format!("if let {} = {} {}{}", snippet(cx, arms[0].pats[0].span, ".."), snippet(cx, ex.span, ".."), - expr_block(cx, &arms[0].body, None, ".."))); + expr_block(cx, &arms[0].body, None, ".."), + els_str)); }); } } diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index c58e62419c6..71cc7c59f8f 100644 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -3,12 +3,32 @@ #![plugin(clippy)] #![deny(clippy)] #![allow(unused)] +#![deny(single_match_else)] use std::borrow::Cow; enum Foo { Bar, Baz(u8) } use Foo::*; +enum ExprNode { + ExprAddrOf, + Butterflies, + Unicorns, +} + +static NODE: ExprNode = ExprNode::Unicorns; + +fn unwrap_addr() -> Option<&'static ExprNode> { + match ExprNode::Butterflies { //~ ERROR you seem to be trying to use match + //~^ HELP try + ExprNode::ExprAddrOf => Some(&NODE), + _ => { + let x = 5; + None + }, + } +} + fn single_match(){ let x = Some(1u8); @@ -33,7 +53,7 @@ fn single_match(){ _ => () } - // Not linted (content in the else) + // Not linted (no block with statements in the single arm) match z { (2...3, 7...9) => println!("{:?}", z), _ => println!("nope"), From 07ace32ac91875be65c40a8957eb0982c027bd16 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 1 Feb 2016 11:28:39 +0100 Subject: [PATCH 2/2] fallout --- src/collapsible_if.rs | 65 ++++++++++++++++++++----------------------- src/mut_reference.rs | 28 +++++++------------ src/utils.rs | 20 ++++--------- 3 files changed, 45 insertions(+), 68 deletions(-) diff --git a/src/collapsible_if.rs b/src/collapsible_if.rs index dd89b22a40f..4b3c4174df7 100644 --- a/src/collapsible_if.rs +++ b/src/collapsible_if.rs @@ -54,43 +54,38 @@ impl LateLintPass for CollapsibleIf { fn check_if(cx: &LateContext, e: &Expr) { if let ExprIf(ref check, ref then, ref else_) = e.node { - match *else_ { - Some(ref else_) => { - if_let_chain! {[ - let ExprBlock(ref block) = else_.node, - block.stmts.is_empty(), - block.rules == BlockCheckMode::DefaultBlock, - let Some(ref else_) = block.expr, - let ExprIf(_, _, _) = else_.node - ], { - span_lint_and_then(cx, - COLLAPSIBLE_IF, - block.span, - "this `else { if .. }` block can be collapsed", |db| { - db.span_suggestion(block.span, "try", - format!("else {}", - snippet_block(cx, else_.span, ".."))); - }); - }} - } - None => { - if let Some(&Expr{ node: ExprIf(ref check_inner, ref content, None), span: sp, ..}) = + if let Some(ref else_) = *else_ { + if_let_chain! {[ + let ExprBlock(ref block) = else_.node, + block.stmts.is_empty(), + block.rules == BlockCheckMode::DefaultBlock, + let Some(ref else_) = block.expr, + let ExprIf(_, _, _) = else_.node + ], { + span_lint_and_then(cx, + COLLAPSIBLE_IF, + block.span, + "this `else { if .. }` block can be collapsed", |db| { + db.span_suggestion(block.span, "try", + format!("else {}", + snippet_block(cx, else_.span, ".."))); + }); + }} + } else if let Some(&Expr{ node: ExprIf(ref check_inner, ref content, None), span: sp, ..}) = single_stmt_of_block(then) { - if e.span.expn_id != sp.expn_id { - return; - } - span_lint_and_then(cx, - COLLAPSIBLE_IF, - e.span, - "this if statement can be collapsed", |db| { - db.span_suggestion(e.span, "try", - format!("if {} && {} {}", - check_to_string(cx, check), - check_to_string(cx, check_inner), - snippet_block(cx, content.span, ".."))); - }); - } + if e.span.expn_id != sp.expn_id { + return; } + span_lint_and_then(cx, + COLLAPSIBLE_IF, + e.span, + "this if statement can be collapsed", |db| { + db.span_suggestion(e.span, "try", + format!("if {} && {} {}", + check_to_string(cx, check), + check_to_string(cx, check_inner), + snippet_block(cx, content.span, ".."))); + }); } } } diff --git a/src/mut_reference.rs b/src/mut_reference.rs index d92e449e7f2..15e8d310cd6 100644 --- a/src/mut_reference.rs +++ b/src/mut_reference.rs @@ -33,27 +33,19 @@ impl LateLintPass for UnnecessaryMutPassed { let borrowed_table = cx.tcx.tables.borrow(); match e.node { ExprCall(ref fn_expr, ref arguments) => { - match borrowed_table.node_types.get(&fn_expr.id) { - Some(function_type) => { - if let ExprPath(_, ref path) = fn_expr.node { - check_arguments(cx, &arguments, function_type, &format!("{}", path)); - } - } - None => unreachable!(), // A function with unknown type is called. - // If this happened the compiler would have aborted the - // compilation long ago. - }; - - + let function_type = borrowed_table.node_types + .get(&fn_expr.id) + .expect("A function with an unknown type is called. \ + If this happened, the compiler would have \ + aborted the compilation long ago"); + if let ExprPath(_, ref path) = fn_expr.node { + check_arguments(cx, &arguments, function_type, &format!("{}", path)); + } } ExprMethodCall(ref name, _, ref arguments) => { let method_call = MethodCall::expr(e.id); - match borrowed_table.method_map.get(&method_call) { - Some(method_type) => { - check_arguments(cx, &arguments, method_type.ty, &format!("{}", name.node.as_str())) - } - None => unreachable!(), // Just like above, this should never happen. - }; + let method_type = borrowed_table.method_map.get(&method_call).expect("This should never happen."); + check_arguments(cx, &arguments, method_type.ty, &format!("{}", name.node.as_str())) } _ => {} } diff --git a/src/utils.rs b/src/utils.rs index b5355919496..ff0c59ab290 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -657,20 +657,10 @@ pub fn is_expn_of(cx: &LateContext, mut span: Span, name: &str) -> Option (ei.callee.name(), ei.call_site) }) }); - - return match span_name_span { - Some((mac_name, new_span)) => { - if mac_name.as_str() == name { - Some(new_span) - } - else { - span = new_span; - continue; - } - } - None => { - None - } - }; + match span_name_span { + Some((mac_name, new_span)) if mac_name.as_str() == name => return Some(new_span), + None => return None, + Some((_, new_span)) => span = new_span, + } } }