From e1c7914c2e1afe34073f01acb5638963029cb961 Mon Sep 17 00:00:00 2001 From: mcarton Date: Sat, 30 Jan 2016 18:03:53 +0100 Subject: [PATCH 01/10] Add missing ExprIndex to is_exp_equal --- src/utils.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/utils.rs b/src/utils.rs index 4c89b7f113d..9adbff90e66 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -718,6 +718,9 @@ pub fn is_exp_equal(cx: &LateContext, left: &Expr, right: &Expr, ignore_fn: bool is_block_equal(cx, lt, rt, ignore_fn) && both(le, re, |l, r| is_exp_equal(cx, l, r, ignore_fn)) } + (&ExprIndex(ref la, ref li), &ExprIndex(ref ra, ref ri)) => { + is_exp_equal(cx, la, ra) && is_exp_equal(cx, li, ri) + } (&ExprLit(ref l), &ExprLit(ref r)) => l.node == r.node, (&ExprMatch(ref le, ref la, ref ls), &ExprMatch(ref re, ref ra, ref rs)) => { ls == rs && From 91c16fc8e605ddb7f0b4f967bd7fbf62ccd6a292 Mon Sep 17 00:00:00 2001 From: mcarton Date: Sat, 6 Feb 2016 20:13:25 +0100 Subject: [PATCH 02/10] Refactor Expr comparisons --- src/copies.rs | 7 +- src/entry.rs | 5 +- src/eq_op.rs | 4 +- src/strings.rs | 7 +- src/utils/hir.rs | 239 +++++++++++++++++++++++++++++++++ src/{utils.rs => utils/mod.rs} | 227 +------------------------------ 6 files changed, 254 insertions(+), 235 deletions(-) create mode 100644 src/utils/hir.rs rename src/{utils.rs => utils/mod.rs} (68%) diff --git a/src/copies.rs b/src/copies.rs index 525c7b7a6fd..b1ea8f0c347 100644 --- a/src/copies.rs +++ b/src/copies.rs @@ -1,6 +1,7 @@ use rustc::lint::*; use rustc_front::hir::*; -use utils::{get_parent_expr, in_macro, is_block_equal, is_exp_equal, span_lint, span_note_and_lint}; +use utils::SpanlessEq; +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 /// `Warn` by default. @@ -55,7 +56,7 @@ impl LateLintPass for CopyAndPaste { 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 is_block_equal(cx, &then_block, &else_block, false) { + 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"); } } @@ -75,7 +76,7 @@ fn lint_same_cond(cx: &LateContext, expr: &Expr) { for (n, i) in conds.iter().enumerate() { for j in conds.iter().skip(n+1) { - if is_exp_equal(cx, i, j, true) { + 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"); } } diff --git a/src/entry.rs b/src/entry.rs index d5bb086fc21..c2f2e956e5e 100644 --- a/src/entry.rs +++ b/src/entry.rs @@ -1,8 +1,9 @@ use rustc::lint::*; use rustc_front::hir::*; use syntax::codemap::Span; -use utils::{get_item_name, is_exp_equal, match_type, snippet, span_lint_and_then, walk_ptrs_ty}; +use utils::SpanlessEq; use utils::{BTREEMAP_PATH, HASHMAP_PATH}; +use utils::{get_item_name, match_type, snippet, span_lint_and_then, walk_ptrs_ty}; /// **What it does:** This lint checks for uses of `contains_key` + `insert` on `HashMap` or /// `BTreeMap`. @@ -89,7 +90,7 @@ fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, key: &Expr, expr: params.len() == 3, name.node.as_str() == "insert", get_item_name(cx, map) == get_item_name(cx, &*params[0]), - is_exp_equal(cx, key, ¶ms[1], false) + SpanlessEq::new(cx).eq_expr(key, ¶ms[1]) ], { let help = if sole_expr { format!("{}.entry({}).or_insert({})", diff --git a/src/eq_op.rs b/src/eq_op.rs index aecd0693ff1..fc1cab2cd71 100644 --- a/src/eq_op.rs +++ b/src/eq_op.rs @@ -2,7 +2,7 @@ use rustc::lint::*; use rustc_front::hir::*; use rustc_front::util as ast_util; -use utils::{is_exp_equal, span_lint}; +use utils::{SpanlessEq, span_lint}; /// **What it does:** This lint checks for equal operands to comparison, logical and bitwise, /// difference and division binary operators (`==`, `>`, etc., `&&`, `||`, `&`, `|`, `^`, `-` and @@ -31,7 +31,7 @@ impl LintPass for EqOp { impl LateLintPass for EqOp { fn check_expr(&mut self, cx: &LateContext, e: &Expr) { if let ExprBinary(ref op, ref left, ref right) = e.node { - if is_valid_operator(op) && is_exp_equal(cx, left, right, true) { + if is_valid_operator(op) && SpanlessEq::new(cx).ignore_fn().eq_expr(left, right) { span_lint(cx, EQ_OP, e.span, diff --git a/src/strings.rs b/src/strings.rs index b78db7f4b77..40d137101a6 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -7,7 +7,8 @@ use rustc::lint::*; use rustc_front::hir::*; use syntax::codemap::Spanned; -use utils::{is_exp_equal, match_type, span_lint, walk_ptrs_ty, get_parent_expr}; +use utils::{match_type, span_lint, walk_ptrs_ty, get_parent_expr}; +use utils::SpanlessEq; use utils::STRING_PATH; /// **What it does:** This lint matches code of the form `x = x + y` (without `let`!). @@ -84,7 +85,7 @@ impl LateLintPass for StringAdd { if let Some(ref p) = parent { if let ExprAssign(ref target, _) = p.node { // avoid duplicate matches - if is_exp_equal(cx, target, left, false) { + if SpanlessEq::new(cx).eq_expr(target, left) { return; } } @@ -113,7 +114,7 @@ fn is_string(cx: &LateContext, e: &Expr) -> bool { fn is_add(cx: &LateContext, src: &Expr, target: &Expr) -> bool { match src.node { - ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) => is_exp_equal(cx, target, left, false), + ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) => SpanlessEq::new(cx).eq_expr(target, left), ExprBlock(ref block) => { block.stmts.is_empty() && block.expr.as_ref().map_or(false, |expr| is_add(cx, expr, target)) } diff --git a/src/utils/hir.rs b/src/utils/hir.rs new file mode 100644 index 00000000000..95356772e60 --- /dev/null +++ b/src/utils/hir.rs @@ -0,0 +1,239 @@ +use consts::constant; +use rustc::lint::*; +use rustc_front::hir::*; +use syntax::ptr::P; + +/// Type used to check whether two ast are the same. This is different from the operator +/// `==` on ast types as this operator would compare true equality with ID and span. +/// +/// Note that some expressions kinds are not considered but could be added. +pub struct SpanlessEq<'a, 'tcx: 'a> { + /// Context used to evaluate constant expressions. + cx: &'a LateContext<'a, 'tcx>, + /// If is true, never consider as equal expressions containing fonction calls. + ignore_fn: bool, +} + +impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { + pub fn new(cx: &'a LateContext<'a, 'tcx>) -> Self { + SpanlessEq { cx: cx, ignore_fn: false } + } + + pub fn ignore_fn(self) -> Self { + SpanlessEq { cx: self.cx, ignore_fn: true } + } + + /// Check whether two statements are the same. + pub fn eq_stmt(&self, left: &Stmt, right: &Stmt) -> bool { + match (&left.node, &right.node) { + (&StmtDecl(ref l, _), &StmtDecl(ref r, _)) => { + if let (&DeclLocal(ref l), &DeclLocal(ref r)) = (&l.node, &r.node) { + // TODO: tys + l.ty.is_none() && r.ty.is_none() && + both(&l.init, &r.init, |l, r| self.eq_expr(l, r)) + } + else { + false + } + } + (&StmtExpr(ref l, _), &StmtExpr(ref r, _)) => self.eq_expr(l, r), + (&StmtSemi(ref l, _), &StmtSemi(ref r, _)) => self.eq_expr(l, r), + _ => false, + } + } + + /// Check whether two blocks are the same. + pub fn eq_block(&self, left: &Block, right: &Block) -> bool { + over(&left.stmts, &right.stmts, |l, r| self.eq_stmt(l, r)) && + both(&left.expr, &right.expr, |l, r| self.eq_expr(l, r)) + } + + // ok, it’s a big function, but mostly one big match with simples cases + #[allow(cyclomatic_complexity)] + pub fn eq_expr(&self, left: &Expr, right: &Expr) -> bool { + if let (Some(l), Some(r)) = (constant(self.cx, left), constant(self.cx, right)) { + if l == r { + return true; + } + } + + match (&left.node, &right.node) { + (&ExprAddrOf(ref lmut, ref le), &ExprAddrOf(ref rmut, ref re)) => { + lmut == rmut && self.eq_expr(le, re) + } + (&ExprAgain(li), &ExprAgain(ri)) => { + both(&li, &ri, |l, r| l.node.name.as_str() == r.node.name.as_str()) + } + (&ExprAssign(ref ll, ref lr), &ExprAssign(ref rl, ref rr)) => { + self.eq_expr(ll, rl) && self.eq_expr(lr, rr) + } + (&ExprAssignOp(ref lo, ref ll, ref lr), &ExprAssignOp(ref ro, ref rl, ref rr)) => { + lo.node == ro.node && self.eq_expr(ll, rl) && self.eq_expr(lr, rr) + } + (&ExprBlock(ref l), &ExprBlock(ref r)) => { + self.eq_block(l, r) + } + (&ExprBinary(lop, ref ll, ref lr), &ExprBinary(rop, ref rl, ref rr)) => { + lop.node == rop.node && self.eq_expr(ll, rl) && self.eq_expr(lr, rr) + } + (&ExprBreak(li), &ExprBreak(ri)) => { + both(&li, &ri, |l, r| l.node.name.as_str() == r.node.name.as_str()) + } + (&ExprBox(ref l), &ExprBox(ref r)) => { + self.eq_expr(l, r) + } + (&ExprCall(ref lfun, ref largs), &ExprCall(ref rfun, ref rargs)) => { + !self.ignore_fn && + self.eq_expr(lfun, rfun) && + self.eq_exprs(largs, rargs) + } + (&ExprCast(ref lx, ref lt), &ExprCast(ref rx, ref rt)) => { + self.eq_expr(lx, rx) && self.eq_ty(lt, rt) + } + (&ExprField(ref lfexp, ref lfident), &ExprField(ref rfexp, ref rfident)) => { + lfident.node == rfident.node && self.eq_expr(lfexp, rfexp) + } + (&ExprIndex(ref la, ref li), &ExprIndex(ref ra, ref ri)) => { + self.eq_expr(la, ra) && self.eq_expr(li, ri) + } + (&ExprIf(ref lc, ref lt, ref le), &ExprIf(ref rc, ref rt, ref re)) => { + self.eq_expr(lc, rc) && + self.eq_block(lt, rt) && + both(le, re, |l, r| self.eq_expr(l, r)) + } + (&ExprLit(ref l), &ExprLit(ref r)) => l.node == r.node, + (&ExprMatch(ref le, ref la, ref ls), &ExprMatch(ref re, ref ra, ref rs)) => { + ls == rs && + self.eq_expr(le, re) && + over(la, ra, |l, r| { + self.eq_expr(&l.body, &r.body) && + both(&l.guard, &r.guard, |l, r| self.eq_expr(l, r)) && + over(&l.pats, &r.pats, |l, r| self.eq_pat(l, r)) + }) + } + (&ExprMethodCall(ref lname, ref ltys, ref largs), &ExprMethodCall(ref rname, ref rtys, ref rargs)) => { + // TODO: tys + !self.ignore_fn && + lname.node == rname.node && + ltys.is_empty() && + rtys.is_empty() && + self.eq_exprs(largs, rargs) + } + (&ExprRange(ref lb, ref le), &ExprRange(ref rb, ref re)) => { + both(lb, rb, |l, r| self.eq_expr(l, r)) && + both(le, re, |l, r| self.eq_expr(l, r)) + } + (&ExprRepeat(ref le, ref ll), &ExprRepeat(ref re, ref rl)) => { + self.eq_expr(le, re) && self.eq_expr(ll, rl) + } + (&ExprRet(ref l), &ExprRet(ref r)) => { + both(l, r, |l, r| self.eq_expr(l, r)) + } + (&ExprPath(ref lqself, ref lsubpath), &ExprPath(ref rqself, ref rsubpath)) => { + both(lqself, rqself, |l, r| self.eq_qself(l, r)) && self.eq_path(lsubpath, rsubpath) + } + (&ExprTup(ref ltup), &ExprTup(ref rtup)) => self.eq_exprs(ltup, rtup), + (&ExprTupField(ref le, li), &ExprTupField(ref re, ri)) => { + li.node == ri.node && self.eq_expr(le, re) + } + (&ExprUnary(lop, ref le), &ExprUnary(rop, ref re)) => { + lop == rop && self.eq_expr(le, re) + } + (&ExprVec(ref l), &ExprVec(ref r)) => self.eq_exprs(l, r), + (&ExprWhile(ref lc, ref lb, ref ll), &ExprWhile(ref rc, ref rb, ref rl)) => { + self.eq_expr(lc, rc) && + self.eq_block(lb, rb) && + both(ll, rl, |l, r| l.name.as_str() == r.name.as_str()) + } + _ => false, + } + } + + fn eq_exprs(&self, left: &[P], right: &[P]) -> bool { + over(left, right, |l, r| self.eq_expr(l, r)) + } + + /// Check whether two patterns are the same. + pub fn eq_pat(&self, left: &Pat, right: &Pat) -> bool { + match (&left.node, &right.node) { + (&PatBox(ref l), &PatBox(ref r)) => { + self.eq_pat(l, r) + } + (&PatEnum(ref lp, ref la), &PatEnum(ref rp, ref ra)) => { + self.eq_path(lp, rp) && + both(la, ra, |l, r| { + over(l, r, |l, r| self.eq_pat(l, r)) + }) + } + (&PatIdent(ref lb, ref li, ref lp), &PatIdent(ref rb, ref ri, ref rp)) => { + lb == rb && li.node.name.as_str() == ri.node.name.as_str() && + both(lp, rp, |l, r| self.eq_pat(l, r)) + } + (&PatLit(ref l), &PatLit(ref r)) => { + self.eq_expr(l, r) + } + (&PatQPath(ref ls, ref lp), &PatQPath(ref rs, ref rp)) => { + self.eq_qself(ls, rs) && self.eq_path(lp, rp) + } + (&PatTup(ref l), &PatTup(ref r)) => { + over(l, r, |l, r| self.eq_pat(l, r)) + } + (&PatRange(ref ls, ref le), &PatRange(ref rs, ref re)) => { + self.eq_expr(ls, rs) && + self.eq_expr(le, re) + } + (&PatRegion(ref le, ref lm), &PatRegion(ref re, ref rm)) => { + lm == rm && self.eq_pat(le, re) + } + (&PatVec(ref ls, ref li, ref le), &PatVec(ref rs, ref ri, ref re)) => { + over(ls, rs, |l, r| self.eq_pat(l, r)) && + over(le, re, |l, r| self.eq_pat(l, r)) && + both(li, ri, |l, r| self.eq_pat(l, r)) + } + (&PatWild, &PatWild) => true, + _ => false, + } + } + + fn eq_path(&self, left: &Path, right: &Path) -> bool { + // The == of idents doesn't work with different contexts, + // we have to be explicit about hygiene + left.global == right.global && + over(&left.segments, + &right.segments, + |l, r| l.identifier.name.as_str() == r.identifier.name.as_str() && l.parameters == r.parameters) + } + + fn eq_qself(&self, left: &QSelf, right: &QSelf) -> bool { + left.ty.node == right.ty.node && left.position == right.position + } + + fn eq_ty(&self, left: &Ty, right: &Ty) -> bool { + match (&left.node, &right.node) { + (&TyVec(ref lvec), &TyVec(ref rvec)) => self.eq_ty(lvec, rvec), + (&TyPtr(ref lmut), &TyPtr(ref rmut)) => lmut.mutbl == rmut.mutbl && self.eq_ty(&*lmut.ty, &*rmut.ty), + (&TyRptr(_, ref lrmut), &TyRptr(_, ref rrmut)) => { + lrmut.mutbl == rrmut.mutbl && self.eq_ty(&*lrmut.ty, &*rrmut.ty) + } + (&TyPath(ref lq, ref lpath), &TyPath(ref rq, ref rpath)) => { + both(lq, rq, |l, r| self.eq_qself(l, r)) && self.eq_path(lpath, rpath) + } + (&TyInfer, &TyInfer) => true, + _ => false, + } + } +} + +/// Check if the two `Option`s are both `None` or some equal values as per `eq_fn`. +fn both(l: &Option, r: &Option, mut eq_fn: F) -> bool + where F: FnMut(&X, &X) -> bool +{ + l.as_ref().map_or_else(|| r.is_none(), |x| r.as_ref().map_or(false, |y| eq_fn(x, y))) +} + +/// Check if two slices are equal as per `eq_fn`. +fn over(left: &[X], right: &[X], mut eq_fn: F) -> bool + where F: FnMut(&X, &X) -> bool +{ + left.len() == right.len() && left.iter().zip(right).all(|(x, y)| eq_fn(x, y)) +} diff --git a/src/utils.rs b/src/utils/mod.rs similarity index 68% rename from src/utils.rs rename to src/utils/mod.rs index 9adbff90e66..d666ee36bce 100644 --- a/src/utils.rs +++ b/src/utils/mod.rs @@ -1,4 +1,3 @@ -use consts::constant; use reexport::*; use rustc::front::map::Node; use rustc::lint::{LintContext, LateContext, Level, Lint}; @@ -16,6 +15,8 @@ use syntax::codemap::{ExpnInfo, Span, ExpnFormat}; use syntax::errors::DiagnosticBuilder; use syntax::ptr::P; +mod hir; +pub use self::hir::SpanlessEq; pub type MethodArgs = HirVec>; // module DefPaths for certain structs/enums we check for @@ -591,230 +592,6 @@ fn parse_attrs(sess: &Session, attrs: &[ast::Attribute], name: &' } } -/// Check whether two statements are the same. -/// See also `is_exp_equal`. -pub fn is_stmt_equal(cx: &LateContext, left: &Stmt, right: &Stmt, ignore_fn: bool) -> bool { - match (&left.node, &right.node) { - (&StmtDecl(ref l, _), &StmtDecl(ref r, _)) => { - if let (&DeclLocal(ref l), &DeclLocal(ref r)) = (&l.node, &r.node) { - // TODO: tys - l.ty.is_none() && r.ty.is_none() && - both(&l.init, &r.init, |l, r| is_exp_equal(cx, l, r, ignore_fn)) - } - else { - false - } - } - (&StmtExpr(ref l, _), &StmtExpr(ref r, _)) => is_exp_equal(cx, l, r, ignore_fn), - (&StmtSemi(ref l, _), &StmtSemi(ref r, _)) => is_exp_equal(cx, l, r, ignore_fn), - _ => false, - } -} - -/// Check whether two blocks are the same. -/// See also `is_exp_equal`. -pub fn is_block_equal(cx: &LateContext, left: &Block, right: &Block, ignore_fn: bool) -> bool { - over(&left.stmts, &right.stmts, |l, r| is_stmt_equal(cx, l, r, ignore_fn)) && - both(&left.expr, &right.expr, |l, r| is_exp_equal(cx, l, r, ignore_fn)) -} - -/// Check whether two pattern are the same. -/// See also `is_exp_equal`. -pub fn is_pat_equal(cx: &LateContext, left: &Pat, right: &Pat, ignore_fn: bool) -> bool { - match (&left.node, &right.node) { - (&PatBox(ref l), &PatBox(ref r)) => { - is_pat_equal(cx, l, r, ignore_fn) - } - (&PatEnum(ref lp, ref la), &PatEnum(ref rp, ref ra)) => { - is_path_equal(lp, rp) && - both(la, ra, |l, r| { - over(l, r, |l, r| is_pat_equal(cx, l, r, ignore_fn)) - }) - } - (&PatIdent(ref lb, ref li, ref lp), &PatIdent(ref rb, ref ri, ref rp)) => { - lb == rb && li.node.name.as_str() == ri.node.name.as_str() && - both(lp, rp, |l, r| is_pat_equal(cx, l, r, ignore_fn)) - } - (&PatLit(ref l), &PatLit(ref r)) => { - is_exp_equal(cx, l, r, ignore_fn) - } - (&PatQPath(ref ls, ref lp), &PatQPath(ref rs, ref rp)) => { - is_qself_equal(ls, rs) && is_path_equal(lp, rp) - } - (&PatTup(ref l), &PatTup(ref r)) => { - over(l, r, |l, r| is_pat_equal(cx, l, r, ignore_fn)) - } - (&PatRange(ref ls, ref le), &PatRange(ref rs, ref re)) => { - is_exp_equal(cx, ls, rs, ignore_fn) && - is_exp_equal(cx, le, re, ignore_fn) - } - (&PatRegion(ref le, ref lm), &PatRegion(ref re, ref rm)) => { - lm == rm && is_pat_equal(cx, le, re, ignore_fn) - } - (&PatVec(ref ls, ref li, ref le), &PatVec(ref rs, ref ri, ref re)) => { - over(ls, rs, |l, r| is_pat_equal(cx, l, r, ignore_fn)) && - over(le, re, |l, r| is_pat_equal(cx, l, r, ignore_fn)) && - both(li, ri, |l, r| is_pat_equal(cx, l, r, ignore_fn)) - } - (&PatWild, &PatWild) => true, - _ => false, - } -} - -/// Check whether two expressions are the same. This is different from the operator `==` on -/// expression as this operator would compare true equality with ID and span. -/// If `ignore_fn` is true, never consider as equal fonction calls. -/// -/// Note that some expression kinds are not considered but could be added. -#[allow(cyclomatic_complexity)] // ok, it’s a big function, but mostly one big match with simples cases -pub fn is_exp_equal(cx: &LateContext, left: &Expr, right: &Expr, ignore_fn: bool) -> bool { - if let (Some(l), Some(r)) = (constant(cx, left), constant(cx, right)) { - if l == r { - return true; - } - } - - match (&left.node, &right.node) { - (&ExprAddrOf(ref lmut, ref le), &ExprAddrOf(ref rmut, ref re)) => { - lmut == rmut && is_exp_equal(cx, le, re, ignore_fn) - } - (&ExprAgain(li), &ExprAgain(ri)) => { - both(&li, &ri, |l, r| l.node.name.as_str() == r.node.name.as_str()) - } - (&ExprAssign(ref ll, ref lr), &ExprAssign(ref rl, ref rr)) => { - is_exp_equal(cx, ll, rl, ignore_fn) && is_exp_equal(cx, lr, rr, ignore_fn) - } - (&ExprAssignOp(ref lo, ref ll, ref lr), &ExprAssignOp(ref ro, ref rl, ref rr)) => { - lo.node == ro.node && is_exp_equal(cx, ll, rl, ignore_fn) && is_exp_equal(cx, lr, rr, ignore_fn) - } - (&ExprBlock(ref l), &ExprBlock(ref r)) => { - is_block_equal(cx, l, r, ignore_fn) - } - (&ExprBinary(lop, ref ll, ref lr), &ExprBinary(rop, ref rl, ref rr)) => { - lop.node == rop.node && is_exp_equal(cx, ll, rl, ignore_fn) && is_exp_equal(cx, lr, rr, ignore_fn) - } - (&ExprBreak(li), &ExprBreak(ri)) => { - both(&li, &ri, |l, r| l.node.name.as_str() == r.node.name.as_str()) - } - (&ExprBox(ref l), &ExprBox(ref r)) => { - is_exp_equal(cx, l, r, ignore_fn) - } - (&ExprCall(ref lfun, ref largs), &ExprCall(ref rfun, ref rargs)) => { - !ignore_fn && - is_exp_equal(cx, lfun, rfun, ignore_fn) && - is_exps_equal(cx, largs, rargs, ignore_fn) - } - (&ExprCast(ref lx, ref lt), &ExprCast(ref rx, ref rt)) => { - is_exp_equal(cx, lx, rx, ignore_fn) && is_cast_ty_equal(lt, rt) - } - (&ExprField(ref lfexp, ref lfident), &ExprField(ref rfexp, ref rfident)) => { - lfident.node == rfident.node && is_exp_equal(cx, lfexp, rfexp, ignore_fn) - } - (&ExprIndex(ref la, ref li), &ExprIndex(ref ra, ref ri)) => { - is_exp_equal(cx, la, ra, ignore_fn) && is_exp_equal(cx, li, ri, ignore_fn) - } - (&ExprIf(ref lc, ref lt, ref le), &ExprIf(ref rc, ref rt, ref re)) => { - is_exp_equal(cx, lc, rc, ignore_fn) && - is_block_equal(cx, lt, rt, ignore_fn) && - both(le, re, |l, r| is_exp_equal(cx, l, r, ignore_fn)) - } - (&ExprIndex(ref la, ref li), &ExprIndex(ref ra, ref ri)) => { - is_exp_equal(cx, la, ra) && is_exp_equal(cx, li, ri) - } - (&ExprLit(ref l), &ExprLit(ref r)) => l.node == r.node, - (&ExprMatch(ref le, ref la, ref ls), &ExprMatch(ref re, ref ra, ref rs)) => { - ls == rs && - is_exp_equal(cx, le, re, ignore_fn) && - over(la, ra, |l, r| { - is_exp_equal(cx, &l.body, &r.body, ignore_fn) && - both(&l.guard, &r.guard, |l, r| is_exp_equal(cx, l, r, ignore_fn)) && - over(&l.pats, &r.pats, |l, r| is_pat_equal(cx, l, r, ignore_fn)) - }) - } - (&ExprMethodCall(ref lname, ref ltys, ref largs), &ExprMethodCall(ref rname, ref rtys, ref rargs)) => { - // TODO: tys - !ignore_fn && - lname.node == rname.node && - ltys.is_empty() && - rtys.is_empty() && - is_exps_equal(cx, largs, rargs, ignore_fn) - } - (&ExprRange(ref lb, ref le), &ExprRange(ref rb, ref re)) => { - both(lb, rb, |l, r| is_exp_equal(cx, l, r, ignore_fn)) && - both(le, re, |l, r| is_exp_equal(cx, l, r, ignore_fn)) - } - (&ExprRepeat(ref le, ref ll), &ExprRepeat(ref re, ref rl)) => { - is_exp_equal(cx, le, re, ignore_fn) && is_exp_equal(cx, ll, rl, ignore_fn) - } - (&ExprRet(ref l), &ExprRet(ref r)) => { - both(l, r, |l, r| is_exp_equal(cx, l, r, ignore_fn)) - } - (&ExprPath(ref lqself, ref lsubpath), &ExprPath(ref rqself, ref rsubpath)) => { - both(lqself, rqself, is_qself_equal) && is_path_equal(lsubpath, rsubpath) - } - (&ExprTup(ref ltup), &ExprTup(ref rtup)) => is_exps_equal(cx, ltup, rtup, ignore_fn), - (&ExprTupField(ref le, li), &ExprTupField(ref re, ri)) => { - li.node == ri.node && is_exp_equal(cx, le, re, ignore_fn) - } - (&ExprUnary(lop, ref le), &ExprUnary(rop, ref re)) => { - lop == rop && is_exp_equal(cx, le, re, ignore_fn) - } - (&ExprVec(ref l), &ExprVec(ref r)) => is_exps_equal(cx, l, r, ignore_fn), - (&ExprWhile(ref lc, ref lb, ref ll), &ExprWhile(ref rc, ref rb, ref rl)) => { - is_exp_equal(cx, lc, rc, ignore_fn) && - is_block_equal(cx, lb, rb, ignore_fn) && - both(ll, rl, |l, r| l.name.as_str() == r.name.as_str()) - } - _ => false, - } -} - -fn is_exps_equal(cx: &LateContext, left: &[P], right: &[P], ignore_fn: bool) -> bool { - over(left, right, |l, r| is_exp_equal(cx, l, r, ignore_fn)) -} - -fn is_path_equal(left: &Path, right: &Path) -> bool { - // The == of idents doesn't work with different contexts, - // we have to be explicit about hygiene - left.global == right.global && - over(&left.segments, - &right.segments, - |l, r| l.identifier.name.as_str() == r.identifier.name.as_str() && l.parameters == r.parameters) -} - -fn is_qself_equal(left: &QSelf, right: &QSelf) -> bool { - left.ty.node == right.ty.node && left.position == right.position -} - -/// Check if two slices are equal as per `eq_fn`. -pub fn over(left: &[X], right: &[X], mut eq_fn: F) -> bool - where F: FnMut(&X, &X) -> bool -{ - left.len() == right.len() && left.iter().zip(right).all(|(x, y)| eq_fn(x, y)) -} - -/// Check if the two `Option`s are both `None` or some equal values as per `eq_fn`. -pub fn both(l: &Option, r: &Option, mut eq_fn: F) -> bool - where F: FnMut(&X, &X) -> bool -{ - l.as_ref().map_or_else(|| r.is_none(), |x| r.as_ref().map_or(false, |y| eq_fn(x, y))) -} - -fn is_cast_ty_equal(left: &Ty, right: &Ty) -> bool { - match (&left.node, &right.node) { - (&TyVec(ref lvec), &TyVec(ref rvec)) => is_cast_ty_equal(lvec, rvec), - (&TyPtr(ref lmut), &TyPtr(ref rmut)) => lmut.mutbl == rmut.mutbl && is_cast_ty_equal(&*lmut.ty, &*rmut.ty), - (&TyRptr(_, ref lrmut), &TyRptr(_, ref rrmut)) => { - lrmut.mutbl == rrmut.mutbl && is_cast_ty_equal(&*lrmut.ty, &*rrmut.ty) - } - (&TyPath(ref lq, ref lpath), &TyPath(ref rq, ref rpath)) => { - both(lq, rq, is_qself_equal) && is_path_equal(lpath, rpath) - } - (&TyInfer, &TyInfer) => true, - _ => false, - } -} - /// Return the pre-expansion span if is this comes from an expansion of the macro `name`. pub fn is_expn_of(cx: &LateContext, mut span: Span, name: &str) -> Option { loop { From afee209d5a85c41f02145869ca360f23a69344d1 Mon Sep 17 00:00:00 2001 From: mcarton Date: Sat, 6 Feb 2016 22:41:12 +0100 Subject: [PATCH 03/10] Add missing ExprLoop to SpanlessEq --- src/utils/hir.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/utils/hir.rs b/src/utils/hir.rs index 95356772e60..457f11a0d26 100644 --- a/src/utils/hir.rs +++ b/src/utils/hir.rs @@ -58,7 +58,7 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { } match (&left.node, &right.node) { - (&ExprAddrOf(ref lmut, ref le), &ExprAddrOf(ref rmut, ref re)) => { + (&ExprAddrOf(lmut, ref le), &ExprAddrOf(rmut, ref re)) => { lmut == rmut && self.eq_expr(le, re) } (&ExprAgain(li), &ExprAgain(ri)) => { @@ -102,6 +102,11 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { both(le, re, |l, r| self.eq_expr(l, r)) } (&ExprLit(ref l), &ExprLit(ref r)) => l.node == r.node, + (&ExprLoop(ref lb, ref ll), &ExprLoop(ref rb, ref rl)) => { + self.eq_block(lb, rb) && + both(ll, rl, |l, r| l.name.as_str() == r.name.as_str()) + + } (&ExprMatch(ref le, ref la, ref ls), &ExprMatch(ref re, ref ra, ref rs)) => { ls == rs && self.eq_expr(le, re) && From 88beb351940e888932b30c555c273f18a0254aff Mon Sep 17 00:00:00 2001 From: mcarton Date: Tue, 9 Feb 2016 15:18:27 +0100 Subject: [PATCH 04/10] Implement Expr spanless-hashing --- src/consts.rs | 4 +- src/copies.rs | 93 +++++++++--- src/utils/hir.rs | 287 +++++++++++++++++++++++++++++++++++ src/utils/mod.rs | 2 +- tests/compile-fail/copies.rs | 6 + 5 files changed, 364 insertions(+), 28 deletions(-) diff --git a/src/consts.rs b/src/consts.rs index ddc8560c9b3..4469853ddf9 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 b1ea8f0c347..1899b47accd 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 457f11a0d26..bd2aebfd013 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 d666ee36bce..f5a028219ed 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 d6e666f299b..9ae90043948 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 { } From ee830ba55e12b9f360b41768078db805332dbea4 Mon Sep 17 00:00:00 2001 From: mcarton Date: Tue, 9 Feb 2016 16:45:47 +0100 Subject: [PATCH 05/10] Extend IF_SAME_THEN_ELSE to ifs sequences --- src/copies.rs | 50 +++++++++++++++++++++++------------- src/utils/hir.rs | 4 +++ tests/compile-fail/copies.rs | 47 +++++++++++++++++++++------------ 3 files changed, 67 insertions(+), 34 deletions(-) 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; From 5ddc615a40d1f33ab23899a4fe63856be9f96435 Mon Sep 17 00:00:00 2001 From: mcarton Date: Tue, 9 Feb 2016 17:10:50 +0100 Subject: [PATCH 06/10] Add missing types to eq_ty --- src/utils/hir.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/utils/hir.rs b/src/utils/hir.rs index c982510b3c3..1ccb411b775 100644 --- a/src/utils/hir.rs +++ b/src/utils/hir.rs @@ -218,6 +218,9 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { fn eq_ty(&self, left: &Ty, right: &Ty) -> bool { match (&left.node, &right.node) { (&TyVec(ref lvec), &TyVec(ref rvec)) => self.eq_ty(lvec, rvec), + (&TyFixedLengthVec(ref lt, ref ll), &TyFixedLengthVec(ref rt, ref rl)) => { + self.eq_ty(lt, rt) && self.eq_expr(ll, rl) + } (&TyPtr(ref lmut), &TyPtr(ref rmut)) => lmut.mutbl == rmut.mutbl && self.eq_ty(&*lmut.ty, &*rmut.ty), (&TyRptr(_, ref lrmut), &TyRptr(_, ref rrmut)) => { lrmut.mutbl == rrmut.mutbl && self.eq_ty(&*lrmut.ty, &*rrmut.ty) @@ -225,6 +228,7 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { (&TyPath(ref lq, ref lpath), &TyPath(ref rq, ref rpath)) => { both(lq, rq, |l, r| self.eq_qself(l, r)) && self.eq_path(lpath, rpath) } + (&TyTup(ref l), &TyTup(ref r)) => over(l, r, |l, r| self.eq_ty(l, r)), (&TyInfer, &TyInfer) => true, _ => false, } From cbbc667b1b77764949714285356f475159bd2491 Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 10 Feb 2016 00:38:53 +0100 Subject: [PATCH 07/10] Dogfood for future MATCH_SAME_ARMS lint --- src/len_zero.rs | 4 +--- src/methods.rs | 21 ++++++++++----------- src/misc.rs | 3 +-- src/utils/hir.rs | 4 ++-- tests/compile-fail/cyclomatic_complexity.rs | 16 ++++++++-------- tests/compile-fail/matches.rs | 10 +++++----- 6 files changed, 27 insertions(+), 31 deletions(-) diff --git a/src/len_zero.rs b/src/len_zero.rs index ea0c873cb54..125a7c0ae78 100644 --- a/src/len_zero.rs +++ b/src/len_zero.rs @@ -140,9 +140,7 @@ fn check_cmp(cx: &LateContext, span: Span, left: &Expr, right: &Expr, op: &str) } } match (&left.node, &right.node) { - (&ExprLit(ref lit), &ExprMethodCall(ref method, _, ref args)) => { - check_len_zero(cx, span, &method.node, args, lit, op) - } + (&ExprLit(ref lit), &ExprMethodCall(ref method, _, ref args)) | (&ExprMethodCall(ref method, _, ref args), &ExprLit(ref lit)) => { check_len_zero(cx, span, &method.node, args, lit, op) } diff --git a/src/methods.rs b/src/methods.rs index 40d975545e9..9263d657371 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -865,12 +865,11 @@ enum SelfKind { impl SelfKind { fn matches(&self, slf: &ExplicitSelf_, allow_value_for_ref: bool) -> bool { match (self, slf) { - (&SelfKind::Value, &SelfValue(_)) => true, - (&SelfKind::Ref, &SelfRegion(_, Mutability::MutImmutable, _)) => true, - (&SelfKind::RefMut, &SelfRegion(_, Mutability::MutMutable, _)) => true, - (&SelfKind::Ref, &SelfValue(_)) => allow_value_for_ref, - (&SelfKind::RefMut, &SelfValue(_)) => allow_value_for_ref, - (&SelfKind::No, &SelfStatic) => true, + (&SelfKind::Value, &SelfValue(_)) | + (&SelfKind::Ref, &SelfRegion(_, Mutability::MutImmutable, _)) | + (&SelfKind::RefMut, &SelfRegion(_, Mutability::MutMutable, _)) | + (&SelfKind::No, &SelfStatic) => true, + (&SelfKind::Ref, &SelfValue(_)) | (&SelfKind::RefMut, &SelfValue(_)) => allow_value_for_ref, (_, &SelfExplicit(ref ty, _)) => self.matches_explicit_type(ty, allow_value_for_ref), _ => false, } @@ -878,11 +877,11 @@ impl SelfKind { fn matches_explicit_type(&self, ty: &Ty, allow_value_for_ref: bool) -> bool { match (self, &ty.node) { - (&SelfKind::Value, &TyPath(..)) => true, - (&SelfKind::Ref, &TyRptr(_, MutTy { mutbl: Mutability::MutImmutable, .. })) => true, - (&SelfKind::RefMut, &TyRptr(_, MutTy { mutbl: Mutability::MutMutable, .. })) => true, - (&SelfKind::Ref, &TyPath(..)) => allow_value_for_ref, - (&SelfKind::RefMut, &TyPath(..)) => allow_value_for_ref, + (&SelfKind::Value, &TyPath(..)) | + (&SelfKind::Ref, &TyRptr(_, MutTy { mutbl: Mutability::MutImmutable, .. })) | + (&SelfKind::RefMut, &TyRptr(_, MutTy { mutbl: Mutability::MutMutable, .. })) => true, + (&SelfKind::Ref, &TyPath(..)) | + (&SelfKind::RefMut, &TyPath(..)) => allow_value_for_ref, _ => false, } } diff --git a/src/misc.rs b/src/misc.rs index f570c18b742..076e6e385c2 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -421,8 +421,7 @@ impl LateLintPass for UsedUnderscoreBinding { fn is_used(cx: &LateContext, expr: &Expr) -> bool { if let Some(ref parent) = get_parent_expr(cx, expr) { match parent.node { - ExprAssign(_, ref rhs) => **rhs == *expr, - ExprAssignOp(_, _, ref rhs) => **rhs == *expr, + ExprAssign(_, ref rhs) | ExprAssignOp(_, _, ref rhs) => **rhs == *expr, _ => is_used(cx, &parent), } } else { diff --git a/src/utils/hir.rs b/src/utils/hir.rs index 1ccb411b775..f8695956f09 100644 --- a/src/utils/hir.rs +++ b/src/utils/hir.rs @@ -38,8 +38,8 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { false } } - (&StmtExpr(ref l, _), &StmtExpr(ref r, _)) => self.eq_expr(l, r), - (&StmtSemi(ref l, _), &StmtSemi(ref r, _)) => self.eq_expr(l, r), + (&StmtExpr(ref l, _), &StmtExpr(ref r, _)) | + (&StmtSemi(ref l, _), &StmtSemi(ref r, _)) => self.eq_expr(l, r), _ => false, } } diff --git a/tests/compile-fail/cyclomatic_complexity.rs b/tests/compile-fail/cyclomatic_complexity.rs index 5bdca7f3629..3a4a83af5c6 100644 --- a/tests/compile-fail/cyclomatic_complexity.rs +++ b/tests/compile-fail/cyclomatic_complexity.rs @@ -138,15 +138,15 @@ fn bloo() { #[cyclomatic_complexity = "0"] fn baa() { //~ ERROR: the function has a cyclomatic complexity of 2 let x = || match 99 { - 0 => true, - 1 => false, - 2 => true, - 4 => true, - 6 => true, - 9 => true, - _ => false, + 0 => 0, + 1 => 1, + 2 => 2, + 4 => 4, + 6 => 6, + 9 => 9, + _ => 42, }; - if x() { + if x() == 42 { println!("x"); } else { println!("not x"); diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index 71cc7c59f8f..46d3ff8d5fb 100644 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -101,8 +101,8 @@ fn match_bool() { let test: bool = true; match test { //~ ERROR you seem to be trying to match on a boolean expression - true => (), - false => (), + true => 0, + false => 42, }; let option = 1; @@ -128,9 +128,9 @@ fn match_bool() { // Not linted match option { - 1 ... 10 => (), - 11 ... 20 => (), - _ => (), + 1 ... 10 => 1, + 11 ... 20 => 2, + _ => 3, }; } From f309dc3c0fae39b993f95058a619f8591e9935df Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 10 Feb 2016 01:22:53 +0100 Subject: [PATCH 08/10] 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 6bb7a2e230e..c2d3b16074c 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 5f7e7a2a643..aea17c3132d 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 775b9830750..675dbdd2dd7 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 f465141248a..623f9967bd4 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 { } From 68ecd06f4cf67a996af348a5ccc90d6a0f25589f Mon Sep 17 00:00:00 2001 From: mcarton Date: Thu, 11 Feb 2016 23:49:35 +0100 Subject: [PATCH 09/10] Small optimisation of most common cases --- src/copies.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/copies.rs b/src/copies.rs index aea17c3132d..e2defe8f364 100644 --- a/src/copies.rs +++ b/src/copies.rs @@ -4,6 +4,7 @@ use rustc_front::hir::*; use std::collections::HashMap; use std::collections::hash_map::Entry; use syntax::parse::token::InternedString; +use syntax::util::small_vector::SmallVector; use utils::{SpanlessEq, SpanlessHash}; use utils::{get_parent_expr, in_macro, span_note_and_lint}; @@ -78,8 +79,8 @@ impl LateLintPass for CopyAndPaste { } let (conds, blocks) = if_sequence(expr); - lint_same_then_else(cx, &blocks); - lint_same_cond(cx, &conds); + lint_same_then_else(cx, blocks.as_slice()); + lint_same_cond(cx, conds.as_slice()); lint_match_arms(cx, expr); } } @@ -143,9 +144,9 @@ fn lint_match_arms(cx: &LateContext, expr: &Expr) { /// 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![]; +fn if_sequence(mut expr: &Expr) -> (SmallVector<&Expr>, SmallVector<&Block>) { + let mut conds = SmallVector::zero(); + let mut blocks = SmallVector::zero(); while let ExprIf(ref cond, ref then_block, ref else_expr) = expr.node { conds.push(&**cond); From 07228a104109b00ad3ec553a33eb6b496fc97451 Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 12 Feb 2016 15:51:55 +0100 Subject: [PATCH 10/10] Fix `Hash` implementation for `Constant` --- src/consts.rs | 93 +++++++++++++++++++++++++++++++------------------ tests/consts.rs | 10 +++++- 2 files changed, 68 insertions(+), 35 deletions(-) diff --git a/src/consts.rs b/src/consts.rs index 4469853ddf9..416ec82799c 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -2,22 +2,21 @@ use rustc::lint::LateContext; use rustc::middle::const_eval::lookup_const_by_id; -use rustc::middle::def::PathResolution; -use rustc::middle::def::Def; +use rustc::middle::def::{Def, PathResolution}; use rustc_front::hir::*; -use syntax::ptr::P; -use std::cmp::PartialOrd; use std::cmp::Ordering::{self, Greater, Less, Equal}; -use std::rc::Rc; +use std::cmp::PartialOrd; +use std::hash::{Hash, Hasher}; +use std::mem; use std::ops::Deref; - -use syntax::ast::Lit_; -use syntax::ast::LitIntType; -use syntax::ast::{UintTy, FloatTy, StrStyle}; +use std::rc::Rc; +use syntax::ast::{LitIntType, Lit_}; use syntax::ast::Sign::{self, Plus, Minus}; +use syntax::ast::{UintTy, FloatTy, StrStyle}; +use syntax::ptr::P; -#[derive(PartialEq, Eq, Debug, Copy, Clone, Hash)] +#[derive(Debug, Copy, Clone)] pub enum FloatWidth { Fw32, Fw64, @@ -34,7 +33,7 @@ impl From for FloatWidth { } /// a Lit_-like enum to fold constant `Expr`s into -#[derive(Eq, Debug, Clone, Hash)] +#[derive(Debug, Clone)] pub enum Constant { /// a String "abc" Str(String, StrStyle), @@ -100,18 +99,12 @@ impl PartialEq for Constant { (&Constant::Int(lv, lty), &Constant::Int(rv, rty)) => { lv == rv && (is_negative(lty) & (lv != 0)) == (is_negative(rty) & (rv != 0)) } - (&Constant::Float(ref ls, lw), &Constant::Float(ref rs, rw)) => { - use self::FloatWidth::*; - if match (lw, rw) { - (FwAny, _) | (_, FwAny) | (Fw32, Fw32) | (Fw64, Fw64) => true, + (&Constant::Float(ref ls, _), &Constant::Float(ref rs, _)) => { + // we want `Fw32 == FwAny` and `FwAny == Fw64`, by transitivity we must have + // `Fw32 == Fw64` so don’t compare them + match (ls.parse::(), rs.parse::()) { + (Ok(l), Ok(r)) => l.eq(&r), _ => false, - } { - match (ls.parse::(), rs.parse::()) { - (Ok(l), Ok(r)) => l.eq(&r), - _ => false, - } - } else { - false } } (&Constant::Bool(l), &Constant::Bool(r)) => l == r, @@ -123,6 +116,46 @@ impl PartialEq for Constant { } } +impl Hash for Constant { + fn hash(&self, state: &mut H) where H: Hasher { + match *self { + Constant::Str(ref s, ref k) => { + s.hash(state); + k.hash(state); + } + Constant::Binary(ref b) => { + b.hash(state); + } + Constant::Byte(u) => { + u.hash(state); + } + Constant::Char(c) => { + c.hash(state); + } + Constant::Int(u, t) => { + u.hash(state); + t.hash(state); + } + Constant::Float(ref f, _) => { + // don’t use the width here because of PartialEq implementation + if let Ok(f) = f.parse::() { + unsafe { mem::transmute::(f) }.hash(state); + } + } + Constant::Bool(b) => { + b.hash(state); + } + Constant::Vec(ref v) | Constant::Tuple(ref v)=> { + v.hash(state); + } + Constant::Repeat(ref c, l) => { + c.hash(state); + l.hash(state); + } + } + } +} + impl PartialOrd for Constant { fn partial_cmp(&self, other: &Constant) -> Option { match (self, other) { @@ -143,18 +176,10 @@ impl PartialOrd for Constant { (false, true) => Greater, }) } - (&Constant::Float(ref ls, lw), &Constant::Float(ref rs, rw)) => { - use self::FloatWidth::*; - if match (lw, rw) { - (FwAny, _) | (_, FwAny) | (Fw32, Fw32) | (Fw64, Fw64) => true, - _ => false, - } { - match (ls.parse::(), rs.parse::()) { - (Ok(ref l), Ok(ref r)) => l.partial_cmp(r), - _ => None, - } - } else { - None + (&Constant::Float(ref ls, _), &Constant::Float(ref rs, _)) => { + match (ls.parse::(), rs.parse::()) { + (Ok(ref l), Ok(ref r)) => l.partial_cmp(r), + _ => None, } } (&Constant::Bool(ref l), &Constant::Bool(ref r)) => Some(l.cmp(r)), diff --git a/tests/consts.rs b/tests/consts.rs index 75082e646a0..ab8636b8131 100644 --- a/tests/consts.rs +++ b/tests/consts.rs @@ -17,7 +17,7 @@ use syntax::ast::LitIntType::*; use syntax::ast::StrStyle::*; use syntax::ast::Sign::*; -use clippy::consts::{constant_simple, Constant}; +use clippy::consts::{constant_simple, Constant, FloatWidth}; fn spanned(t: T) -> Spanned { Spanned{ node: t, span: COMMAND_LINE_SP } @@ -78,4 +78,12 @@ fn test_ops() { check(ONE, &binop(BiSub, litone.clone(), litzero.clone())); check(ONE, &binop(BiMul, litone.clone(), litone.clone())); check(ONE, &binop(BiDiv, litone.clone(), litone.clone())); + + let half_any = Constant::Float("0.5".into(), FloatWidth::FwAny); + let half32 = Constant::Float("0.5".into(), FloatWidth::Fw32); + let half64 = Constant::Float("0.5".into(), FloatWidth::Fw64); + + assert_eq!(half_any, half32); + assert_eq!(half_any, half64); + assert_eq!(half32, half64); // for transitivity }