diff --git a/README.md b/README.md index 55976104058..72ce27392a7 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,7 @@ len_zero | warn | checking `.len() == 0` or `.len() > 0` (or let_and_return | warn | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a function let_unit_value | warn | creating a let binding to a value of unit type, which usually can't be used afterwards linkedlist | warn | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a RingBuf +match_ref_pats | warn | a match has all arms prefixed with `&`; the match expression can be dereferenced instead modulo_one | warn | taking a number modulo 1, which always returns 0 mut_mut | warn | usage of double-mut refs, e.g. `&mut &mut ...` (either copy'n'paste error, or shows a fundamental misunderstanding of references) needless_bool | warn | if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }` diff --git a/src/approx_const.rs b/src/approx_const.rs index cfd646765c9..3e0ba4eb669 100644 --- a/src/approx_const.rs +++ b/src/approx_const.rs @@ -37,10 +37,10 @@ impl LintPass for ApproxConstant { } fn check_lit(cx: &Context, lit: &Lit, span: Span) { - match &lit.node { - &LitFloat(ref str, TyF32) => check_known_consts(cx, span, str, "f32"), - &LitFloat(ref str, TyF64) => check_known_consts(cx, span, str, "f64"), - &LitFloatUnsuffixed(ref str) => check_known_consts(cx, span, str, "f{32, 64}"), + match lit.node { + LitFloat(ref str, TyF32) => check_known_consts(cx, span, str, "f32"), + LitFloat(ref str, TyF64) => check_known_consts(cx, span, str, "f64"), + LitFloatUnsuffixed(ref str) => check_known_consts(cx, span, str, "f{32, 64}"), _ => () } } diff --git a/src/bit_mask.rs b/src/bit_mask.rs index 7789381da23..6537fcf4c1a 100644 --- a/src/bit_mask.rs +++ b/src/bit_mask.rs @@ -82,9 +82,9 @@ fn invert_cmp(cmp : BinOp_) -> BinOp_ { fn check_compare(cx: &Context, bit_op: &Expr, cmp_op: BinOp_, cmp_value: u64, span: &Span) { - match &bit_op.node { - &ExprParen(ref subexp) => check_compare(cx, subexp, cmp_op, cmp_value, span), - &ExprBinary(ref op, ref left, ref right) => { + match bit_op.node { + ExprParen(ref subexp) => check_compare(cx, subexp, cmp_op, cmp_value, span), + ExprBinary(ref op, ref left, ref right) => { if op.node != BiBitAnd && op.node != BiBitOr { return; } fetch_int_literal(cx, right).or_else(|| fetch_int_literal( cx, left)).map_or((), |mask| check_bit_mask(cx, op.node, @@ -182,13 +182,13 @@ fn check_ineffective_gt(cx: &Context, span: Span, m: u64, c: u64, op: &str) { } fn fetch_int_literal(cx: &Context, lit : &Expr) -> Option { - match &lit.node { - &ExprLit(ref lit_ptr) => { + match lit.node { + ExprLit(ref lit_ptr) => { if let &LitInt(value, _) = &lit_ptr.node { Option::Some(value) //TODO: Handle sign } else { Option::None } }, - &ExprPath(_, _) => { + ExprPath(_, _) => { // Important to let the borrow expire before the const lookup to avoid double // borrowing. let def_map = cx.tcx.def_map.borrow(); diff --git a/src/eta_reduction.rs b/src/eta_reduction.rs index 6712e787278..25e967b07e5 100644 --- a/src/eta_reduction.rs +++ b/src/eta_reduction.rs @@ -18,9 +18,9 @@ impl LintPass for EtaPass { } fn check_expr(&mut self, cx: &Context, expr: &Expr) { - match &expr.node { - &ExprCall(_, ref args) | - &ExprMethodCall(_, _, ref args) => { + match expr.node { + ExprCall(_, ref args) | + ExprMethodCall(_, _, ref args) => { for arg in args { check_closure(cx, &*arg) } diff --git a/src/len_zero.rs b/src/len_zero.rs index 073dcea582d..5eaa0256402 100644 --- a/src/len_zero.rs +++ b/src/len_zero.rs @@ -22,10 +22,10 @@ impl LintPass for LenZero { } fn check_item(&mut self, cx: &Context, item: &Item) { - match &item.node { - &ItemTrait(_, _, _, ref trait_items) => + match item.node { + ItemTrait(_, _, _, ref trait_items) => check_trait_items(cx, item, trait_items), - &ItemImpl(_, _, _, None, _, ref impl_items) => // only non-trait + ItemImpl(_, _, _, None, _, ref impl_items) => // only non-trait check_impl_items(cx, item, impl_items), _ => () } @@ -100,7 +100,7 @@ fn check_cmp(cx: &Context, span: Span, left: &Expr, right: &Expr, op: &str) { fn check_len_zero(cx: &Context, span: Span, method: &SpannedIdent, args: &[P], lit: &Lit, op: &str) { - if let &Spanned{node: LitInt(0, _), ..} = lit { + if let Spanned{node: LitInt(0, _), ..} = *lit { if method.node.name == "len" && args.len() == 1 && has_is_empty(cx, &*args[0]) { span_lint(cx, LEN_ZERO, span, &format!( diff --git a/src/lib.rs b/src/lib.rs index b0f46ae45ab..26af063c9a5 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -38,11 +38,11 @@ pub mod returns; pub mod lifetimes; pub mod loops; pub mod ranges; +pub mod matches; #[plugin_registrar] pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box types::TypePass as LintPassObject); - reg.register_lint_pass(box misc::MiscPass as LintPassObject); reg.register_lint_pass(box misc::TopLevelRefPass as LintPassObject); reg.register_lint_pass(box misc::CmpNan as LintPassObject); reg.register_lint_pass(box eq_op::EqOp as LintPassObject); @@ -71,6 +71,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box ranges::StepByZero as LintPassObject); reg.register_lint_pass(box types::CastPass as LintPassObject); reg.register_lint_pass(box types::TypeComplexityPass as LintPassObject); + reg.register_lint_pass(box matches::MatchPass as LintPassObject); reg.register_lint_group("clippy", vec![ approx_const::APPROX_CONSTANT, @@ -87,6 +88,8 @@ pub fn plugin_registrar(reg: &mut Registry) { loops::EXPLICIT_ITER_LOOP, loops::ITER_NEXT_LOOP, loops::NEEDLESS_RANGE_LOOP, + matches::MATCH_REF_PATS, + matches::SINGLE_MATCH, methods::OPTION_UNWRAP_USED, methods::RESULT_UNWRAP_USED, methods::STR_TO_STRING, @@ -96,7 +99,6 @@ pub fn plugin_registrar(reg: &mut Registry) { misc::FLOAT_CMP, misc::MODULO_ONE, misc::PRECEDENCE, - misc::SINGLE_MATCH, misc::TOPLEVEL_REF_ARG, mut_mut::MUT_MUT, needless_bool::NEEDLESS_BOOL, diff --git a/src/matches.rs b/src/matches.rs new file mode 100644 index 00000000000..002da07f50b --- /dev/null +++ b/src/matches.rs @@ -0,0 +1,86 @@ +use rustc::lint::*; +use syntax::ast; +use syntax::ast::*; +use std::borrow::Cow; + +use utils::{snippet, snippet_block, span_lint, span_help_and_lint}; + +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"); +declare_lint!(pub MATCH_REF_PATS, Warn, + "a match has all arms prefixed with `&`; the match expression can be \ + dereferenced instead"); + +#[allow(missing_copy_implementations)] +pub struct MatchPass; + +impl LintPass for MatchPass { + fn get_lints(&self) -> LintArray { + lint_array!(SINGLE_MATCH, MATCH_REF_PATS) + } + + fn check_expr(&mut self, cx: &Context, expr: &Expr) { + if let ExprMatch(ref ex, ref arms, ast::MatchSource::Normal) = expr.node { + // check preconditions for SINGLE_MATCH + // only two arms + if arms.len() == 2 && + // both of the arms have a single pattern and no guard + arms[0].pats.len() == 1 && arms[0].guard.is_none() && + arms[1].pats.len() == 1 && arms[1].guard.is_none() && + // and the second pattern is a `_` wildcard: this is not strictly necessary, + // since the exhaustiveness check will ensure the last one is a catch-all, + // but in some cases, an explicit match is preferred to catch situations + // when an enum is extended, so we don't consider these cases + arms[1].pats[0].node == PatWild(PatWildSingle) && + // finally, we don't want any content in the second arm (unit or empty block) + is_unit_expr(&*arms[1].body) + { + let body_code = snippet_block(cx, arms[0].body.span, ".."); + let body_code = if let ExprBlock(_) = arms[0].body.node { + body_code + } else { + Cow::Owned(format!("{{ {} }}", body_code)) + }; + span_help_and_lint(cx, SINGLE_MATCH, expr.span, + "you seem to be trying to use match for \ + destructuring a single pattern. Did you mean to \ + use `if let`?", + &*format!("try\nif let {} = {} {}", + snippet(cx, arms[0].pats[0].span, ".."), + snippet(cx, ex.span, ".."), + body_code) + ); + } + + // check preconditions for MATCH_REF_PATS + if has_only_ref_pats(arms) { + if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node { + span_lint(cx, MATCH_REF_PATS, expr.span, &format!( + "you don't need to add `&` to both the expression to match \ + and the patterns: use `match {} {{ ...`", snippet(cx, inner.span, ".."))); + } else { + span_lint(cx, MATCH_REF_PATS, expr.span, &format!( + "instead of prefixing all patterns with `&`, you can dereference the \ + expression to match: `match *{} {{ ...`", snippet(cx, ex.span, ".."))); + } + } + } + } +} + +fn is_unit_expr(expr: &Expr) -> bool { + match expr.node { + ExprTup(ref v) if v.is_empty() => true, + ExprBlock(ref b) if b.stmts.is_empty() && b.expr.is_none() => true, + _ => false, + } +} + +fn has_only_ref_pats(arms: &[Arm]) -> bool { + arms.iter().flat_map(|a| &a.pats).all(|p| match p.node { + PatRegion(..) => true, // &-patterns + PatWild(..) => true, // an "anything" wildcard is also fine + _ => false, + }) +} diff --git a/src/misc.rs b/src/misc.rs index aca849931bb..81b03db5e14 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -1,75 +1,14 @@ use rustc::lint::*; use syntax::ptr::P; -use syntax::ast; use syntax::ast::*; use syntax::ast_util::{is_comparison_binop, binop_to_string}; use syntax::codemap::{Span, Spanned}; use syntax::visit::FnKind; use rustc::middle::ty; -use std::borrow::Cow; -use utils::{match_path, snippet, snippet_block, span_lint, span_help_and_lint, walk_ptrs_ty}; +use utils::{match_path, snippet, span_lint, walk_ptrs_ty}; use consts::constant; -/// Handles uncategorized lints -/// Currently handles linting of if-let-able matches -#[allow(missing_copy_implementations)] -pub struct MiscPass; - - -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"); - -impl LintPass for MiscPass { - fn get_lints(&self) -> LintArray { - lint_array!(SINGLE_MATCH) - } - - fn check_expr(&mut self, cx: &Context, expr: &Expr) { - if let ExprMatch(ref ex, ref arms, ast::MatchSource::Normal) = expr.node { - // check preconditions: only two arms - if arms.len() == 2 && - // both of the arms have a single pattern and no guard - arms[0].pats.len() == 1 && arms[0].guard.is_none() && - arms[1].pats.len() == 1 && arms[1].guard.is_none() && - // and the second pattern is a `_` wildcard: this is not strictly necessary, - // since the exhaustiveness check will ensure the last one is a catch-all, - // but in some cases, an explicit match is preferred to catch situations - // when an enum is extended, so we don't consider these cases - arms[1].pats[0].node == PatWild(PatWildSingle) && - // finally, we don't want any content in the second arm (unit or empty block) - is_unit_expr(&*arms[1].body) - { - let body_code = snippet_block(cx, arms[0].body.span, ".."); - let body_code = if let ExprBlock(_) = arms[0].body.node { - body_code - } else { - Cow::Owned(format!("{{ {} }}", body_code)) - }; - span_help_and_lint(cx, SINGLE_MATCH, expr.span, - "you seem to be trying to use match for \ - destructuring a single pattern. Did you mean to \ - use `if let`?", - &*format!("try\nif let {} = {} {}", - snippet(cx, arms[0].pats[0].span, ".."), - snippet(cx, ex.span, ".."), - body_code) - ); - } - } - } -} - -fn is_unit_expr(expr: &Expr) -> bool { - match expr.node { - ExprTup(ref v) if v.is_empty() => true, - ExprBlock(ref b) if b.stmts.is_empty() && b.expr.is_none() => true, - _ => false, - } -} - - declare_lint!(pub TOPLEVEL_REF_ARG, Warn, "a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not \ `fn foo((ref x, ref y): (u8, u8))`)"); @@ -236,8 +175,8 @@ impl LintPass for CmpOwned { } fn check_to_owned(cx: &Context, expr: &Expr, other_span: Span) { - match &expr.node { - &ExprMethodCall(Spanned{node: ref ident, ..}, _, ref args) => { + match expr.node { + ExprMethodCall(Spanned{node: ref ident, ..}, _, ref args) => { let name = ident.name; if name == "to_string" || name == "to_owned" && is_str_arg(cx, args) { @@ -247,7 +186,7 @@ fn check_to_owned(cx: &Context, expr: &Expr, other_span: Span) { snippet(cx, other_span, ".."))) } }, - &ExprCall(ref path, _) => { + ExprCall(ref path, _) => { if let &ExprPath(None, ref path) = &path.node { if match_path(path, &["String", "from_str"]) || match_path(path, &["String", "from"]) { diff --git a/src/needless_bool.rs b/src/needless_bool.rs index 18d98f1f063..7671d63a35d 100644 --- a/src/needless_bool.rs +++ b/src/needless_bool.rs @@ -60,9 +60,9 @@ fn fetch_bool_block(block: &Block) -> Option { } fn fetch_bool_expr(expr: &Expr) -> Option { - match &expr.node { - &ExprBlock(ref block) => fetch_bool_block(block), - &ExprLit(ref lit_ptr) => if let &LitBool(value) = &lit_ptr.node { + match expr.node { + ExprBlock(ref block) => fetch_bool_block(block), + ExprLit(ref lit_ptr) => if let LitBool(value) = lit_ptr.node { Some(value) } else { None }, _ => None } diff --git a/src/strings.rs b/src/strings.rs index 64d18eeb26d..b24ea345244 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -65,13 +65,13 @@ fn is_string(cx: &Context, e: &Expr) -> bool { } fn is_add(cx: &Context, src: &Expr, target: &Expr) -> bool { - match &src.node { - &ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) => + match src.node { + ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) => is_exp_equal(cx, target, left), - &ExprBlock(ref block) => block.stmts.is_empty() && + ExprBlock(ref block) => block.stmts.is_empty() && block.expr.as_ref().map_or(false, |expr| is_add(cx, &*expr, target)), - &ExprParen(ref expr) => is_add(cx, &*expr, target), + ExprParen(ref expr) => is_add(cx, &*expr, target), _ => false } } diff --git a/src/types.rs b/src/types.rs index cd7a5dc3729..2b1aa515559 100644 --- a/src/types.rs +++ b/src/types.rs @@ -116,10 +116,10 @@ declare_lint!(pub CAST_POSSIBLE_TRUNCATION, Allow, /// Returns the size in bits of an integral type. /// Will return 0 if the type is not an int or uint variant fn int_ty_to_nbits(typ: &ty::TyS) -> usize { - let n = match &typ.sty { - &ty::TyInt(i) => 4 << (i as usize), - &ty::TyUint(u) => 4 << (u as usize), - _ => 0 + let n = match typ.sty { + ty::TyInt(i) => 4 << (i as usize), + ty::TyUint(u) => 4 << (u as usize), + _ => 0 }; // n == 4 is the usize/isize case if n == 4 { ::std::usize::BITS } else { n } @@ -139,16 +139,16 @@ impl LintPass for CastPass { match (cast_from.is_integral(), cast_to.is_integral()) { (true, false) => { let from_nbits = int_ty_to_nbits(cast_from); - let to_nbits : usize = match &cast_to.sty { - &ty::TyFloat(ast::TyF32) => 32, - &ty::TyFloat(ast::TyF64) => 64, + let to_nbits : usize = match cast_to.sty { + ty::TyFloat(ast::TyF32) => 32, + ty::TyFloat(ast::TyF64) => 64, _ => 0 }; if from_nbits != 0 { if from_nbits >= to_nbits { span_lint(cx, CAST_PRECISION_LOSS, expr.span, &format!("converting from {0} to {1}, which causes a loss of precision \ - ({0} is {2} bits wide, but {1}'s mantissa is only {3} bits wide)", + ({0} is {2} bits wide, but {1}'s mantissa is only {3} bits wide)", cast_from, cast_to, from_nbits, if to_nbits == 64 {52} else {23} )); } } diff --git a/tests/compile-fail/match_if_let.rs b/tests/compile-fail/matches.rs similarity index 50% rename from tests/compile-fail/match_if_let.rs rename to tests/compile-fail/matches.rs index bf2e7e43a52..3cc540992c9 100755 --- a/tests/compile-fail/match_if_let.rs +++ b/tests/compile-fail/matches.rs @@ -2,8 +2,9 @@ #![plugin(clippy)] #![deny(clippy)] +#![allow(unused)] -fn main(){ +fn single_match(){ let x = Some(1u8); match x { //~ ERROR you seem to be trying to use match //~^ HELP try @@ -36,3 +37,29 @@ fn main(){ _ => println!("nope"), } } + +fn ref_pats() { + let ref v = Some(0); + match v { //~ERROR instead of prefixing all patterns with `&` + &Some(v) => println!("{:?}", v), + &None => println!("none"), + } + match v { // this doesn't trigger, we have a different pattern + &Some(v) => println!("some"), + other => println!("other"), + } + let ref tup = (1, 2); + match tup { //~ERROR instead of prefixing all patterns with `&` + &(v, 1) => println!("{}", v), + _ => println!("none"), + } + // special case: using & both in expr and pats + let w = Some(0); + match &w { //~ERROR you don't need to add `&` to both + &Some(v) => println!("{:?}", v), + &None => println!("none"), + } +} + +fn main() { +}