From f309dc3c0fae39b993f95058a619f8591e9935df Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 10 Feb 2016 01:22:53 +0100 Subject: [PATCH] Add the MATCH_SAME_ARMS lint --- README.md | 3 +- src/copies.rs | 126 ++++++++++++++++++++++++++++++----- src/lib.rs | 1 + tests/compile-fail/copies.rs | 71 ++++++++++++++------ 4 files changed, 162 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 6bb7a2e23..c2d3b1607 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 119 lints included in this crate: +There are 120 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -65,6 +65,7 @@ name [match_bool](https://github.com/Manishearth/rust-clippy/wiki#match_bool) | warn | a match on boolean expression; recommends `if..else` block instead [match_overlapping_arm](https://github.com/Manishearth/rust-clippy/wiki#match_overlapping_arm) | warn | a match has overlapping arms [match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats) | warn | a match or `if let` has all arms prefixed with `&`; the match expression can be dereferenced instead +[match_same_arms](https://github.com/Manishearth/rust-clippy/wiki#match_same_arms) | warn | `match` with identical arm bodies [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 [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one) | warn | taking a number modulo 1, which always returns 0 [mut_mut](https://github.com/Manishearth/rust-clippy/wiki#mut_mut) | allow | usage of double-mut refs, e.g. `&mut &mut ...` (either copy'n'paste error, or shows a fundamental misunderstanding of references) diff --git a/src/copies.rs b/src/copies.rs index 5f7e7a2a6..aea17c313 100644 --- a/src/copies.rs +++ b/src/copies.rs @@ -1,7 +1,9 @@ use rustc::lint::*; +use rustc::middle::ty; use rustc_front::hir::*; use std::collections::HashMap; use std::collections::hash_map::Entry; +use syntax::parse::token::InternedString; use utils::{SpanlessEq, SpanlessHash}; use utils::{get_parent_expr, in_macro, span_note_and_lint}; @@ -33,6 +35,25 @@ declare_lint! { "if with the same *then* and *else* blocks" } +/// **What it does:** This lint checks for `match` with identical arm bodies. +/// +/// **Why is this bad?** This is probably a copy & paste error. +/// +/// **Known problems:** Hopefully none. +/// +/// **Example:** +/// ```rust,ignore +/// match foo { +/// Bar => bar(), +/// Quz => quz(), +/// Baz => bar(), // <= oups +/// ``` +declare_lint! { + pub MATCH_SAME_ARMS, + Warn, + "`match` with identical arm bodies" +} + #[derive(Copy, Clone, Debug)] pub struct CopyAndPaste; @@ -40,7 +61,8 @@ impl LintPass for CopyAndPaste { fn get_lints(&self) -> LintArray { lint_array![ IFS_SAME_COND, - IF_SAME_THEN_ELSE + IF_SAME_THEN_ELSE, + MATCH_SAME_ARMS ] } } @@ -58,39 +80,63 @@ impl LateLintPass for CopyAndPaste { let (conds, blocks) = if_sequence(expr); lint_same_then_else(cx, &blocks); lint_same_cond(cx, &conds); + lint_match_arms(cx, expr); } } } /// Implementation of `IF_SAME_THEN_ELSE`. fn lint_same_then_else(cx: &LateContext, blocks: &[&Block]) { - let hash = |block| -> u64 { + let hash : &Fn(&&Block) -> u64 = &|block| -> u64 { let mut h = SpanlessHash::new(cx); h.hash_block(block); h.finish() }; - let eq = |lhs, rhs| -> bool { + + let eq : &Fn(&&Block, &&Block) -> bool = &|&lhs, &rhs| -> bool { SpanlessEq::new(cx).eq_block(lhs, rhs) }; if let Some((i, j)) = search_same(blocks, hash, eq) { - span_note_and_lint(cx, IF_SAME_THEN_ELSE, j.span, "this if has identical blocks", i.span, "same as this"); + span_note_and_lint(cx, IF_SAME_THEN_ELSE, j.span, "this `if` has identical blocks", i.span, "same as this"); } } /// Implementation of `IFS_SAME_COND`. fn lint_same_cond(cx: &LateContext, conds: &[&Expr]) { - let hash = |expr| -> u64 { + let hash : &Fn(&&Expr) -> u64 = &|expr| -> u64 { let mut h = SpanlessHash::new(cx); h.hash_expr(expr); h.finish() }; - let eq = |lhs, rhs| -> bool { + + let eq : &Fn(&&Expr, &&Expr) -> bool = &|&lhs, &rhs| -> bool { SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, rhs) }; if let Some((i, j)) = search_same(conds, hash, eq) { - span_note_and_lint(cx, IFS_SAME_COND, j.span, "this if has the same condition as a previous if", i.span, "same as this"); + span_note_and_lint(cx, IFS_SAME_COND, j.span, "this `if` has the same condition as a previous if", i.span, "same as this"); + } +} + +/// Implementation if `MATCH_SAME_ARMS`. +fn lint_match_arms(cx: &LateContext, expr: &Expr) { + let hash = |arm: &Arm| -> u64 { + let mut h = SpanlessHash::new(cx); + h.hash_expr(&arm.body); + h.finish() + }; + + let eq = |lhs: &Arm, rhs: &Arm| -> bool { + SpanlessEq::new(cx).eq_expr(&lhs.body, &rhs.body) && + // all patterns should have the same bindings + bindings(cx, &lhs.pats[0]) == bindings(cx, &rhs.pats[0]) + }; + + if let ExprMatch(_, ref arms, MatchSource::Normal) = expr.node { + if let Some((i, j)) = search_same(&**arms, hash, eq) { + span_note_and_lint(cx, MATCH_SAME_ARMS, j.body.span, "this `match` has identical arm bodies", i.body.span, "same as this"); + } } } @@ -123,11 +169,59 @@ fn if_sequence(mut expr: &Expr) -> (Vec<&Expr>, Vec<&Block>) { (conds, blocks) } -fn search_same<'a, T, Hash, Eq>(exprs: &[&'a T], - hash: Hash, - eq: Eq) -> Option<(&'a T, &'a T)> -where Hash: Fn(&'a T) -> u64, - Eq: Fn(&'a T, &'a T) -> bool { +/// Return the list of bindings in a pattern. +fn bindings<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat) -> HashMap> { + fn bindings_impl<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat, map: &mut HashMap>) { + match pat.node { + PatBox(ref pat) | PatRegion(ref pat, _) => bindings_impl(cx, pat, map), + PatEnum(_, Some(ref pats)) => { + for pat in pats { + bindings_impl(cx, pat, map); + } + } + PatIdent(_, ref ident, ref as_pat) => { + if let Entry::Vacant(v) = map.entry(ident.node.name.as_str()) { + v.insert(cx.tcx.pat_ty(pat)); + } + if let Some(ref as_pat) = *as_pat { + bindings_impl(cx, as_pat, map); + } + }, + PatStruct(_, ref fields, _) => { + for pat in fields { + bindings_impl(cx, &pat.node.pat, map); + } + } + PatTup(ref fields) => { + for pat in fields { + bindings_impl(cx, pat, map); + } + } + PatVec(ref lhs, ref mid, ref rhs) => { + for pat in lhs { + bindings_impl(cx, pat, map); + } + if let Some(ref mid) = *mid { + bindings_impl(cx, mid, map); + } + for pat in rhs { + bindings_impl(cx, pat, map); + } + } + PatEnum(..) | PatLit(..) | PatQPath(..) | PatRange(..) | PatWild => (), + } + } + + let mut result = HashMap::new(); + bindings_impl(cx, pat, &mut result); + result +} + +fn search_same(exprs: &[T], + hash: Hash, + eq: Eq) -> Option<(&T, &T)> +where Hash: Fn(&T) -> u64, + Eq: Fn(&T, &T) -> bool { // common cases if exprs.len() < 2 { return None; @@ -141,14 +235,14 @@ where Hash: Fn(&'a T) -> u64, } } - let mut map : HashMap<_, Vec<&'a _>> = HashMap::with_capacity(exprs.len()); + let mut map : HashMap<_, Vec<&_>> = HashMap::with_capacity(exprs.len()); - for &expr in exprs { + for expr in exprs { match map.entry(hash(expr)) { Entry::Occupied(o) => { for o in o.get() { - if eq(o, expr) { - return Some((o, expr)) + if eq(&o, expr) { + return Some((&o, expr)) } } } diff --git a/src/lib.rs b/src/lib.rs index 775b98307..675dbdd2d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -196,6 +196,7 @@ pub fn plugin_registrar(reg: &mut Registry) { collapsible_if::COLLAPSIBLE_IF, copies::IF_SAME_THEN_ELSE, copies::IFS_SAME_COND, + copies::MATCH_SAME_ARMS, cyclomatic_complexity::CYCLOMATIC_COMPLEXITY, derive::DERIVE_HASH_NOT_EQ, derive::EXPL_IMPL_CLONE_ON_COPY, diff --git a/tests/compile-fail/copies.rs b/tests/compile-fail/copies.rs index f46514124..623f9967b 100755 --- a/tests/compile-fail/copies.rs +++ b/tests/compile-fail/copies.rs @@ -5,15 +5,18 @@ #![allow(let_and_return)] #![allow(needless_return)] #![allow(unused_variables)] +#![allow(cyclomatic_complexity)] +fn bar(_: T) {} fn foo() -> bool { unimplemented!() } #[deny(if_same_then_else)] +#[deny(match_same_arms)] fn if_same_then_else() -> &'static str { if true { foo(); } - else { //~ERROR this if has identical blocks + else { //~ERROR this `if` has identical blocks foo(); } @@ -29,7 +32,7 @@ fn if_same_then_else() -> &'static str { foo(); 42 } - else { //~ERROR this if has identical blocks + else { //~ERROR this `if` has identical blocks foo(); 42 }; @@ -41,7 +44,7 @@ fn if_same_then_else() -> &'static str { let _ = if true { 42 } - else { //~ERROR this if has identical blocks + else { //~ERROR this `if` has identical blocks 42 }; @@ -56,7 +59,7 @@ fn if_same_then_else() -> &'static str { while foo() { break; } bar + 1; } - else { //~ERROR this if has identical blocks + else { //~ERROR this `if` has identical blocks let bar = if true { 42 } @@ -69,29 +72,29 @@ fn if_same_then_else() -> &'static str { } if true { - match 42 { - 42 => (), - a if a > 0 => (), - 10...15 => (), - _ => (), - } + let _ = match 42 { + 42 => 1, + a if a > 0 => 2, + 10...15 => 3, + _ => 4, + }; } else if false { foo(); } - else if foo() { //~ERROR this if has identical blocks - match 42 { - 42 => (), - a if a > 0 => (), - 10...15 => (), - _ => (), - } + else if foo() { //~ERROR this `if` has identical blocks + let _ = match 42 { + 42 => 1, + a if a > 0 => 2, + 10...15 => 3, + _ => 4, + }; } if true { if let Some(a) = Some(42) {} } - else { //~ERROR this if has identical blocks + else { //~ERROR this `if` has identical blocks if let Some(a) = Some(42) {} } @@ -102,6 +105,30 @@ fn if_same_then_else() -> &'static str { if let Some(a) = Some(43) {} } + let _ = match 42 { + 42 => foo(), + 51 => foo(), //~ERROR this `match` has identical arm bodies + _ => true, + }; + + let _ = match Some(42) { + Some(42) => 24, + Some(a) => 24, // bindings are different + None => 0, + }; + + match (Some(42), Some(42)) { + (Some(a), None) => bar(a), + (None, Some(a)) => bar(a), //~ERROR this `match` has identical arm bodies + _ => (), + } + + match (Some(42), Some("")) { + (Some(a), None) => bar(a), + (None, Some(a)) => bar(a), // bindings have different types + _ => (), + } + if true { let foo = ""; return &foo[0..]; @@ -110,7 +137,7 @@ fn if_same_then_else() -> &'static str { let foo = "bar"; return &foo[0..]; } - else { //~ERROR this if has identical blocks + else { //~ERROR this `if` has identical blocks let foo = ""; return &foo[0..]; } @@ -124,19 +151,19 @@ fn ifs_same_cond() { if b { } - else if b { //~ERROR this if has the same condition as a previous if + else if b { //~ERROR this `if` has the same condition as a previous if } if a == 1 { } - else if a == 1 { //~ERROR this if has the same condition as a previous if + else if a == 1 { //~ERROR this `if` has the same condition as a previous if } if 2*a == 1 { } else if 2*a == 2 { } - else if 2*a == 1 { //~ERROR this if has the same condition as a previous if + else if 2*a == 1 { //~ERROR this `if` has the same condition as a previous if } else if a == 1 { }