diff --git a/src/consts.rs b/src/consts.rs index ddc8560c9..4469853dd 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -17,7 +17,7 @@ use syntax::ast::{UintTy, FloatTy, StrStyle}; use syntax::ast::Sign::{self, Plus, Minus}; -#[derive(PartialEq, Eq, Debug, Copy, Clone)] +#[derive(PartialEq, Eq, Debug, Copy, Clone, Hash)] pub enum FloatWidth { Fw32, Fw64, @@ -34,7 +34,7 @@ impl From for FloatWidth { } /// a Lit_-like enum to fold constant `Expr`s into -#[derive(Eq, Debug, Clone)] +#[derive(Eq, Debug, Clone, Hash)] pub enum Constant { /// a String "abc" Str(String, StrStyle), diff --git a/src/copies.rs b/src/copies.rs index b1ea8f0c3..1899b47ac 100644 --- a/src/copies.rs +++ b/src/copies.rs @@ -1,6 +1,8 @@ use rustc::lint::*; use rustc_front::hir::*; -use utils::SpanlessEq; +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}; /// **What it does:** This lint checks for consecutive `ifs` with the same condition. This lint is @@ -46,8 +48,16 @@ impl LintPass for CopyAndPaste { impl LateLintPass for CopyAndPaste { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { if !in_macro(cx, expr.span) { + // skip ifs directly in else, it will be checked in the parent if + if let Some(&Expr{node: ExprIf(_, _, Some(ref else_expr)), ..}) = get_parent_expr(cx, expr) { + if else_expr.id == expr.id { + return; + } + } + + let (conds, blocks) = if_sequence(expr); lint_same_then_else(cx, expr); - lint_same_cond(cx, expr); + lint_same_cond(cx, &conds); } } } @@ -64,32 +74,22 @@ fn lint_same_then_else(cx: &LateContext, expr: &Expr) { } /// Implementation of `IFS_SAME_COND`. -fn lint_same_cond(cx: &LateContext, expr: &Expr) { - // skip ifs directly in else, it will be checked in the parent if - if let Some(&Expr{node: ExprIf(_, _, Some(ref else_expr)), ..}) = get_parent_expr(cx, expr) { - if else_expr.id == expr.id { - return; - } - } - - let conds = condition_sequence(expr); - - for (n, i) in conds.iter().enumerate() { - for j in conds.iter().skip(n+1) { - if SpanlessEq::new(cx).ignore_fn().eq_expr(i, j) { - 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"); - } - } +fn lint_same_cond(cx: &LateContext, conds: &[&Expr]) { + if let Some((i, j)) = search_same(cx, conds) { + 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"); } } -/// Return the list of condition expressions in a sequence of `if/else`. -/// Eg. would return `[a, b]` for the expression `if a {..} else if b {..}`. -fn condition_sequence(mut expr: &Expr) -> Vec<&Expr> { - let mut result = vec![]; +/// Return the list of condition expressions and the list of blocks in a sequence of `if/else`. +/// Eg. would return `([a, b], [c, d, e])` for the expression +/// `if a { c } else if b { d } else { e }`. +fn if_sequence(mut expr: &Expr) -> (Vec<&Expr>, Vec<&Block>) { + let mut conds = vec![]; + let mut blocks = vec![]; - while let ExprIf(ref cond, _, ref else_expr) = expr.node { - result.push(&**cond); + while let ExprIf(ref cond, ref then_block, ref else_expr) = expr.node { + conds.push(&**cond); + blocks.push(&**then_block); if let Some(ref else_expr) = *else_expr { expr = else_expr; @@ -99,5 +99,48 @@ fn condition_sequence(mut expr: &Expr) -> Vec<&Expr> { } } - result + // final `else {..}` + if !blocks.is_empty() { + if let ExprBlock(ref block) = expr.node { + blocks.push(&**block); + } + } + + (conds, blocks) +} + +fn search_same<'a>(cx: &LateContext, exprs: &[&'a Expr]) -> Option<(&'a Expr, &'a Expr)> { + // 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]) { + Some((&exprs[0], &exprs[1])) + } + else { + None + } + } + + 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) { + Entry::Occupied(o) => { + for o in o.get() { + if SpanlessEq::new(cx).ignore_fn().eq_expr(o, expr) { + return Some((o, expr)) + } + } + } + Entry::Vacant(v) => { v.insert(vec![expr]); } + } + } + + None } diff --git a/src/utils/hir.rs b/src/utils/hir.rs index 457f11a0d..bd2aebfd0 100644 --- a/src/utils/hir.rs +++ b/src/utils/hir.rs @@ -1,6 +1,8 @@ use consts::constant; use rustc::lint::*; use rustc_front::hir::*; +use std::hash::{Hash, Hasher, SipHasher}; +use syntax::ast::Name; use syntax::ptr::P; /// Type used to check whether two ast are the same. This is different from the operator @@ -242,3 +244,288 @@ fn over(left: &[X], right: &[X], mut eq_fn: F) -> bool { left.len() == right.len() && left.iter().zip(right).all(|(x, y)| eq_fn(x, y)) } + + +pub struct SpanlessHash<'a, 'tcx: 'a> { + /// Context used to evaluate constant expressions. + cx: &'a LateContext<'a, 'tcx>, + s: SipHasher, +} + +impl<'a, 'tcx: 'a> SpanlessHash<'a, 'tcx> { + pub fn new(cx: &'a LateContext<'a, 'tcx>) -> Self { + SpanlessHash { cx: cx, s: SipHasher::new() } + } + + pub fn finish(&self) -> u64 { + self.s.finish() + } + + pub fn hash_block(&mut self, b: &Block) { + for s in &b.stmts { + self.hash_stmt(s); + } + + if let Some(ref e) = b.expr { + self.hash_expr(e); + } + + b.rules.hash(&mut self.s); + } + + pub fn hash_expr(&mut self, e: &Expr) { + if let Some(e) = constant(self.cx, e) { + return e.hash(&mut self.s); + } + + match e.node { + ExprAddrOf(m, ref e) => { + let c: fn(_, _) -> _ = ExprAddrOf; + c.hash(&mut self.s); + m.hash(&mut self.s); + self.hash_expr(e); + } + ExprAgain(i) => { + let c: fn(_) -> _ = ExprAgain; + c.hash(&mut self.s); + if let Some(i) = i { + self.hash_name(&i.node.name); + } + } + ExprAssign(ref l, ref r) => { + let c: fn(_, _) -> _ = ExprAssign; + c.hash(&mut self.s); + self.hash_expr(l); + self.hash_expr(r); + } + ExprAssignOp(ref o, ref l, ref r) => { + let c: fn(_, _, _) -> _ = ExprAssignOp; + c.hash(&mut self.s); + o.hash(&mut self.s); + self.hash_expr(l); + self.hash_expr(r); + } + ExprBlock(ref b) => { + let c: fn(_) -> _ = ExprBlock; + c.hash(&mut self.s); + self.hash_block(b); + } + ExprBinary(op, ref l, ref r) => { + let c: fn(_, _, _) -> _ = ExprBinary; + c.hash(&mut self.s); + op.node.hash(&mut self.s); + self.hash_expr(l); + self.hash_expr(r); + } + ExprBreak(i) => { + let c: fn(_) -> _ = ExprBreak; + c.hash(&mut self.s); + if let Some(i) = i { + self.hash_name(&i.node.name); + } + } + ExprBox(ref e) => { + let c: fn(_) -> _ = ExprBox; + c.hash(&mut self.s); + self.hash_expr(e); + } + ExprCall(ref fun, ref args) => { + let c: fn(_, _) -> _ = ExprCall; + c.hash(&mut self.s); + self.hash_expr(fun); + self.hash_exprs(args); + } + ExprCast(ref e, ref _ty) => { + let c: fn(_, _) -> _ = ExprCast; + c.hash(&mut self.s); + self.hash_expr(e); + // TODO: _ty + } + ExprClosure(cap, _, ref b) => { + let c: fn(_, _, _) -> _ = ExprClosure; + c.hash(&mut self.s); + cap.hash(&mut self.s); + self.hash_block(b); + } + ExprField(ref e, ref f) => { + let c: fn(_, _) -> _ = ExprField; + c.hash(&mut self.s); + self.hash_expr(e); + self.hash_name(&f.node); + } + ExprIndex(ref a, ref i) => { + let c: fn(_, _) -> _ = ExprIndex; + c.hash(&mut self.s); + self.hash_expr(a); + self.hash_expr(i); + } + ExprInlineAsm(_) => { + let c: fn(_) -> _ = ExprInlineAsm; + c.hash(&mut self.s); + } + ExprIf(ref cond, ref t, ref e) => { + let c: fn(_, _, _) -> _ = ExprIf; + c.hash(&mut self.s); + self.hash_expr(cond); + self.hash_block(t); + if let Some(ref e) = *e { + self.hash_expr(e); + } + } + ExprLit(ref l) => { + let c: fn(_) -> _ = ExprLit; + c.hash(&mut self.s); + l.hash(&mut self.s); + }, + ExprLoop(ref b, ref i) => { + let c: fn(_, _) -> _ = ExprLoop; + c.hash(&mut self.s); + self.hash_block(b); + if let Some(i) = *i { + self.hash_name(&i.name); + } + } + ExprMatch(ref e, ref arms, ref s) => { + let c: fn(_, _, _) -> _ = ExprMatch; + c.hash(&mut self.s); + self.hash_expr(e); + + for arm in arms { + // TODO: arm.pat? + if let Some(ref e) = arm.guard { + self.hash_expr(e); + } + self.hash_expr(&arm.body); + } + + s.hash(&mut self.s); + } + ExprMethodCall(ref name, ref _tys, ref args) => { + let c: fn(_, _, _) -> _ = ExprMethodCall; + c.hash(&mut self.s); + self.hash_name(&name.node); + self.hash_exprs(args); + } + ExprRange(ref b, ref e) => { + let c: fn(_, _) -> _ = ExprRange; + c.hash(&mut self.s); + if let Some(ref b) = *b { + self.hash_expr(b); + } + if let Some(ref e) = *e { + self.hash_expr(e); + } + } + ExprRepeat(ref e, ref l) => { + let c: fn(_, _) -> _ = ExprRepeat; + c.hash(&mut self.s); + self.hash_expr(e); + self.hash_expr(l); + } + ExprRet(ref e) => { + let c: fn(_) -> _ = ExprRet; + c.hash(&mut self.s); + if let Some(ref e) = *e { + self.hash_expr(e); + } + } + ExprPath(ref _qself, ref subpath) => { + let c: fn(_, _) -> _ = ExprPath; + c.hash(&mut self.s); + self.hash_path(subpath); + } + ExprStruct(ref path, ref fields, ref expr) => { + let c: fn(_, _, _) -> _ = ExprStruct; + c.hash(&mut self.s); + + self.hash_path(path); + + for f in fields { + self.hash_name(&f.name.node); + self.hash_expr(&f.expr); + } + + if let Some(ref e) = *expr { + self.hash_expr(e); + } + } + ExprTup(ref tup) => { + let c: fn(_) -> _ = ExprTup; + c.hash(&mut self.s); + self.hash_exprs(tup); + }, + ExprTupField(ref le, li) => { + let c: fn(_, _) -> _ = ExprTupField; + c.hash(&mut self.s); + + self.hash_expr(le); + li.node.hash(&mut self.s); + } + ExprType(_, _) => { + let c: fn(_, _) -> _ = ExprType; + c.hash(&mut self.s); + // what’s an ExprType anyway? + } + ExprUnary(lop, ref le) => { + let c: fn(_, _) -> _ = ExprUnary; + c.hash(&mut self.s); + + lop.hash(&mut self.s); + self.hash_expr(le); + } + ExprVec(ref v) => { + let c: fn(_) -> _ = ExprVec; + c.hash(&mut self.s); + + self.hash_exprs(v); + }, + ExprWhile(ref cond, ref b, l) => { + let c: fn(_, _, _) -> _ = ExprWhile; + c.hash(&mut self.s); + + self.hash_expr(cond); + self.hash_block(b); + if let Some(l) = l { + self.hash_name(&l.name); + } + } + } + } + + pub fn hash_exprs(&mut self, e: &[P]) { + for e in e { + self.hash_expr(e); + } + } + + pub fn hash_name(&mut self, n: &Name) { + n.as_str().hash(&mut self.s); + } + + pub fn hash_path(&mut self, p: &Path) { + p.global.hash(&mut self.s); + for p in &p.segments { + self.hash_name(&p.identifier.name); + } + } + + pub fn hash_stmt(&mut self, b: &Stmt) { + match b.node { + StmtDecl(ref _decl, _) => { + let c: fn(_, _) -> _ = StmtDecl; + c.hash(&mut self.s); + // TODO: decl + } + StmtExpr(ref expr, _) => { + let c: fn(_, _) -> _ = StmtExpr; + c.hash(&mut self.s); + self.hash_expr(expr); + } + StmtSemi(ref expr, _) => { + let c: fn(_, _) -> _ = StmtSemi; + c.hash(&mut self.s); + self.hash_expr(expr); + } + } + } +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index d666ee36b..f5a028219 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -16,7 +16,7 @@ use syntax::errors::DiagnosticBuilder; use syntax::ptr::P; mod hir; -pub use self::hir::SpanlessEq; +pub use self::hir::{SpanlessEq, SpanlessHash}; pub type MethodArgs = HirVec>; // module DefPaths for certain structs/enums we check for diff --git a/tests/compile-fail/copies.rs b/tests/compile-fail/copies.rs index d6e666f29..9ae900439 100755 --- a/tests/compile-fail/copies.rs +++ b/tests/compile-fail/copies.rs @@ -105,6 +105,12 @@ fn if_same_then_else() -> &'static str { fn ifs_same_cond() { let a = 0; + let b = false; + + if b { + } + else if b { //~ERROR this if has the same condition as a previous if + } if a == 1 { }