diff --git a/src/copies.rs b/src/copies.rs index 1899b47accd..5f7e7a2a643 100644 --- a/src/copies.rs +++ b/src/copies.rs @@ -3,7 +3,7 @@ use rustc_front::hir::*; use std::collections::HashMap; use std::collections::hash_map::Entry; use utils::{SpanlessEq, SpanlessHash}; -use utils::{get_parent_expr, in_macro, span_lint, span_note_and_lint}; +use utils::{get_parent_expr, in_macro, span_note_and_lint}; /// **What it does:** This lint checks for consecutive `ifs` with the same condition. This lint is /// `Warn` by default. @@ -56,26 +56,40 @@ impl LateLintPass for CopyAndPaste { } let (conds, blocks) = if_sequence(expr); - lint_same_then_else(cx, expr); + lint_same_then_else(cx, &blocks); lint_same_cond(cx, &conds); } } } /// Implementation of `IF_SAME_THEN_ELSE`. -fn lint_same_then_else(cx: &LateContext, expr: &Expr) { - if let ExprIf(_, ref then_block, Some(ref else_expr)) = expr.node { - if let ExprBlock(ref else_block) = else_expr.node { - if SpanlessEq::new(cx).eq_block(&then_block, &else_block) { - span_lint(cx, IF_SAME_THEN_ELSE, expr.span, "this if has the same then and else blocks"); - } - } +fn lint_same_then_else(cx: &LateContext, blocks: &[&Block]) { + let hash = |block| -> u64 { + let mut h = SpanlessHash::new(cx); + h.hash_block(block); + h.finish() + }; + let eq = |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"); } } /// Implementation of `IFS_SAME_COND`. fn lint_same_cond(cx: &LateContext, conds: &[&Expr]) { - if let Some((i, j)) = search_same(cx, conds) { + let hash = |expr| -> u64 { + let mut h = SpanlessHash::new(cx); + h.hash_expr(expr); + h.finish() + }; + let eq = |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"); } } @@ -109,13 +123,17 @@ fn if_sequence(mut expr: &Expr) -> (Vec<&Expr>, Vec<&Block>) { (conds, blocks) } -fn search_same<'a>(cx: &LateContext, exprs: &[&'a Expr]) -> Option<(&'a Expr, &'a Expr)> { +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 { // common cases if exprs.len() < 2 { return None; } else if exprs.len() == 2 { - return if SpanlessEq::new(cx).ignore_fn().eq_expr(&exprs[0], &exprs[1]) { + return if eq(&exprs[0], &exprs[1]) { Some((&exprs[0], &exprs[1])) } else { @@ -126,14 +144,10 @@ fn search_same<'a>(cx: &LateContext, exprs: &[&'a Expr]) -> Option<(&'a Expr, &' let mut map : HashMap<_, Vec<&'a _>> = HashMap::with_capacity(exprs.len()); for &expr in exprs { - let mut h = SpanlessHash::new(cx); - h.hash_expr(expr); - let h = h.finish(); - - match map.entry(h) { + match map.entry(hash(expr)) { Entry::Occupied(o) => { for o in o.get() { - if SpanlessEq::new(cx).ignore_fn().eq_expr(o, expr) { + if eq(o, expr) { return Some((o, expr)) } } diff --git a/src/utils/hir.rs b/src/utils/hir.rs index bd2aebfd013..c982510b3c3 100644 --- a/src/utils/hir.rs +++ b/src/utils/hir.rs @@ -246,6 +246,10 @@ fn over(left: &[X], right: &[X], mut eq_fn: F) -> bool } +/// Type used to hash an ast element. This is different from the `Hash` trait on ast types as this +/// trait would consider IDs and spans. +/// +/// All expressions kind are hashed, but some might have a weaker hash. pub struct SpanlessHash<'a, 'tcx: 'a> { /// Context used to evaluate constant expressions. cx: &'a LateContext<'a, 'tcx>, diff --git a/tests/compile-fail/copies.rs b/tests/compile-fail/copies.rs index 9ae90043948..f465141248a 100755 --- a/tests/compile-fail/copies.rs +++ b/tests/compile-fail/copies.rs @@ -5,16 +5,15 @@ #![allow(let_and_return)] #![allow(needless_return)] #![allow(unused_variables)] -#![deny(if_same_then_else)] -#![deny(ifs_same_cond)] fn foo() -> bool { unimplemented!() } +#[deny(if_same_then_else)] fn if_same_then_else() -> &'static str { - if true { //~ERROR this if has the same then and else blocks + if true { foo(); } - else { + else { //~ERROR this if has identical blocks foo(); } @@ -26,11 +25,11 @@ fn if_same_then_else() -> &'static str { foo(); } - let _ = if true { //~ERROR this if has the same then and else blocks + let _ = if true { foo(); 42 } - else { + else { //~ERROR this if has identical blocks foo(); 42 }; @@ -39,14 +38,14 @@ fn if_same_then_else() -> &'static str { foo(); } - let _ = if true { //~ERROR this if has the same then and else blocks + let _ = if true { 42 } - else { + else { //~ERROR this if has identical blocks 42 }; - if true { //~ERROR this if has the same then and else blocks + if true { let bar = if true { 42 } @@ -57,7 +56,7 @@ fn if_same_then_else() -> &'static str { while foo() { break; } bar + 1; } - else { + else { //~ERROR this if has identical blocks let bar = if true { 42 } @@ -69,7 +68,7 @@ fn if_same_then_else() -> &'static str { bar + 1; } - if true { //~ERROR this if has the same then and else blocks + if true { match 42 { 42 => (), a if a > 0 => (), @@ -77,7 +76,10 @@ fn if_same_then_else() -> &'static str { _ => (), } } - else { + else if false { + foo(); + } + else if foo() { //~ERROR this if has identical blocks match 42 { 42 => (), a if a > 0 => (), @@ -86,23 +88,36 @@ fn if_same_then_else() -> &'static str { } } - if true { //~ERROR this if has the same then and else blocks + if true { if let Some(a) = Some(42) {} } - else { + else { //~ERROR this if has identical blocks if let Some(a) = Some(42) {} } - if true { //~ERROR this if has the same then and else blocks + if true { + if let Some(a) = Some(42) {} + } + else { + if let Some(a) = Some(43) {} + } + + if true { let foo = ""; return &foo[0..]; } - else { + else if false { + let foo = "bar"; + return &foo[0..]; + } + else { //~ERROR this if has identical blocks let foo = ""; return &foo[0..]; } } +#[deny(ifs_same_cond)] +#[allow(if_same_then_else)] // all empty blocks fn ifs_same_cond() { let a = 0; let b = false;