Extend IF_SAME_THEN_ELSE to ifs sequences

Author: mcarton

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))
                     }
                 }

src/utils/hir.rs

@@ -246,6 +246,10 @@ fn over<X, F>(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>,

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,15 +68,18 @@ 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 => (),
             10...15 => (),
             _ => (),
         }
     }
-    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 { //~ERROR this if has identical blocks
+        if let Some(a) = Some(42) {}
+    }
+
+    if true {
         if let Some(a) = Some(42) {}
     }
     else {
-        if let Some(a) = Some(42) {}
+        if let Some(a) = Some(43) {}
     }
 
-    if true { //~ERROR this if has the same then and else blocks
+    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;