From fe6f2a22ba48f8187ed69cd3d479d7a0dfa3b432 Mon Sep 17 00:00:00 2001 From: mcarton Date: Sat, 30 Jan 2016 18:03:53 +0100 Subject: [PATCH 1/6] Lint about consecutive ifs with same condition --- README.md | 1 + src/copies.rs | 68 ++++++++++++++++++++++++++++++++++++ src/lib.rs | 3 ++ src/utils.rs | 24 ++++++++++++- tests/compile-fail/copies.rs | 31 ++++++++++++++++ 5 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 src/copies.rs create mode 100755 tests/compile-fail/copies.rs diff --git a/README.md b/README.md index 20aaa03c51c..93921f01f6e 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,7 @@ name [for_loop_over_option](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option) | warn | for-looping over an `Option`, which is more clearly expressed as an `if let` [for_loop_over_result](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result) | warn | for-looping over a `Result`, which is more clearly expressed as an `if let` [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op) | warn | using identity operations, e.g. `x + 0` or `y / 1` +[ifs_same_cond](https://github.com/Manishearth/rust-clippy/wiki#ifs_same_cond) | warn | consecutive `ifs` with the same condition [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask) | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2` [inline_always](https://github.com/Manishearth/rust-clippy/wiki#inline_always) | warn | `#[inline(always)]` is a bad idea in most cases [invalid_regex](https://github.com/Manishearth/rust-clippy/wiki#invalid_regex) | deny | finds invalid regular expressions in `Regex::new(_)` invocations diff --git a/src/copies.rs b/src/copies.rs new file mode 100644 index 00000000000..33461a29670 --- /dev/null +++ b/src/copies.rs @@ -0,0 +1,68 @@ +use rustc::lint::*; +use rustc_front::hir::*; +use utils::{get_parent_expr, is_exp_equal, span_lint}; + +/// **What it does:** This lint checks for consecutive `ifs` with the same condition. This lint is +/// `Warn` by default. +/// +/// **Why is this bad?** This is probably a copy & paste error. +/// +/// **Known problems:** Hopefully none. +/// +/// **Example:** `if a == b { .. } else if a == b { .. }` +declare_lint! { + pub IFS_SAME_COND, + Warn, + "consecutive `ifs` with the same condition" +} + +#[derive(Copy, Clone, Debug)] +pub struct CopyAndPaste; + +impl LintPass for CopyAndPaste { + fn get_lints(&self) -> LintArray { + lint_array![ + IFS_SAME_COND + ] + } +} + +impl LateLintPass for CopyAndPaste { + fn check_expr(&mut self, 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 is_exp_equal(cx, i, j) { + span_lint(cx, IFS_SAME_COND, j.span, "this if as the same condition as a previous if"); + } + } + } + } +} + +/// Return the list of conditions expression 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![]; + + while let ExprIf(ref cond, _, ref else_expr) = expr.node { + result.push(&**cond); + + if let Some(ref else_expr) = *else_expr { + expr = else_expr; + } + else { + break; + } + } + + result +} diff --git a/src/lib.rs b/src/lib.rs index fa98beb8d74..6b2a43db795 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -37,6 +37,7 @@ use rustc_plugin::Registry; #[macro_use] pub mod utils; +pub mod copies; pub mod consts; pub mod types; pub mod misc; @@ -157,6 +158,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_late_lint_pass(box drop_ref::DropRefPass); reg.register_late_lint_pass(box types::AbsurdUnsignedComparisons); reg.register_late_lint_pass(box regex::RegexPass); + reg.register_late_lint_pass(box copies::CopyAndPaste); reg.register_lint_group("clippy_pedantic", vec![ enum_glob_use::ENUM_GLOB_USE, @@ -190,6 +192,7 @@ pub fn plugin_registrar(reg: &mut Registry) { block_in_if_condition::BLOCK_IN_IF_CONDITION_EXPR, block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT, collapsible_if::COLLAPSIBLE_IF, + copies::IFS_SAME_COND, cyclomatic_complexity::CYCLOMATIC_COMPLEXITY, derive::DERIVE_HASH_NOT_EQ, derive::EXPL_IMPL_CLONE_ON_COPY, diff --git a/src/utils.rs b/src/utils.rs index 74c1de6976f..72c6fe94ce2 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -596,16 +596,38 @@ pub fn is_exp_equal(cx: &LateContext, left: &Expr, right: &Expr) -> bool { } } match (&left.node, &right.node) { + (&ExprAddrOf(ref lmut, ref le), &ExprAddrOf(ref rmut, ref re)) => { + lmut == rmut && is_exp_equal(cx, le, re) + } + (&ExprBinary(lop, ref ll, ref lr), &ExprBinary(rop, ref rl, ref rr)) => { + lop.node == rop.node && is_exp_equal(cx, ll, rl) && is_exp_equal(cx, lr, rr) + } + (&ExprCall(ref lfun, ref largs), &ExprCall(ref rfun, ref rargs)) => { + is_exp_equal(cx, lfun, rfun) && is_exps_equal(cx, largs, rargs) + } + (&ExprCast(ref lx, ref lt), &ExprCast(ref rx, ref rt)) => is_exp_equal(cx, lx, rx) && 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) } + (&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, + (&ExprMethodCall(ref lname, ref ltys, ref largs), &ExprMethodCall(ref rname, ref rtys, ref rargs)) => { + // TODO: tys + lname.node == rname.node && ltys.is_empty() && rtys.is_empty() && is_exps_equal(cx, largs, rargs) + } (&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), + (&ExprTupField(ref le, li), &ExprTupField(ref re, ri)) => { + li.node == ri.node && is_exp_equal(cx, le, re) + } + (&ExprUnary(lop, ref le), &ExprUnary(rop, ref re)) => { + lop == rop && is_exp_equal(cx, le, re) + } (&ExprVec(ref l), &ExprVec(ref r)) => is_exps_equal(cx, l, r), - (&ExprCast(ref lx, ref lt), &ExprCast(ref rx, ref rt)) => is_exp_equal(cx, lx, rx) && is_cast_ty_equal(lt, rt), _ => false, } } diff --git a/tests/compile-fail/copies.rs b/tests/compile-fail/copies.rs new file mode 100755 index 00000000000..a29fd392c5e --- /dev/null +++ b/tests/compile-fail/copies.rs @@ -0,0 +1,31 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![deny(clippy)] + +fn foo() -> bool { unimplemented!() } + +fn main() { + let a = 0; + + if a == 1 { + } + else if a == 1 { //~ERROR this if as the same condition as a previous if + } + + if 2*a == 1 { + } + else if 2*a == 2 { + } + else if 2*a == 1 { //~ERROR this if as the same condition as a previous if + } + else if a == 1 { + } + + // Ok, maybe `foo` isn’t pure and this actually makes sense. But you should probably refactor + // this to make the intention clearer anyway. + if foo() { + } + else if foo() { //~ERROR this if as the same condition as a previous if + } +} From d862495d191dc432e92015a724780477f743152e Mon Sep 17 00:00:00 2001 From: mcarton Date: Sat, 30 Jan 2016 19:16:49 +0100 Subject: [PATCH 2/6] Lint ifs with the same then and else blocks --- README.md | 1 + src/copies.rs | 72 +++++++++++++++++++++++------ src/lib.rs | 1 + src/utils.rs | 10 +++- tests/compile-fail/copies.rs | 48 +++++++++++++++++-- tests/compile-fail/needless_bool.rs | 1 + 6 files changed, 115 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 93921f01f6e..01999cce70b 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,7 @@ name [for_loop_over_option](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option) | warn | for-looping over an `Option`, which is more clearly expressed as an `if let` [for_loop_over_result](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result) | warn | for-looping over a `Result`, which is more clearly expressed as an `if let` [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op) | warn | using identity operations, e.g. `x + 0` or `y / 1` +[if_same_then_else](https://github.com/Manishearth/rust-clippy/wiki#if_same_then_else) | warn | if with the same *then* and *else* blocks [ifs_same_cond](https://github.com/Manishearth/rust-clippy/wiki#ifs_same_cond) | warn | consecutive `ifs` with the same condition [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask) | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2` [inline_always](https://github.com/Manishearth/rust-clippy/wiki#inline_always) | warn | `#[inline(always)]` is a bad idea in most cases diff --git a/src/copies.rs b/src/copies.rs index 33461a29670..f8fe09c3133 100644 --- a/src/copies.rs +++ b/src/copies.rs @@ -1,6 +1,6 @@ use rustc::lint::*; use rustc_front::hir::*; -use utils::{get_parent_expr, is_exp_equal, span_lint}; +use utils::{get_parent_expr, in_macro, is_exp_equal, is_stmt_equal, over, 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. @@ -16,33 +16,79 @@ declare_lint! { "consecutive `ifs` with the same condition" } +/// **What it does:** This lint checks for `if/else` with the same body as the *then* part and the +/// *else* part. This lint is `Warn` by default. +/// +/// **Why is this bad?** This is probably a copy & paste error. +/// +/// **Known problems:** Hopefully none. +/// +/// **Example:** `if .. { 42 } else { 42 }` +declare_lint! { + pub IF_SAME_THEN_ELSE, + Warn, + "if with the same *then* and *else* blocks" +} + #[derive(Copy, Clone, Debug)] pub struct CopyAndPaste; impl LintPass for CopyAndPaste { fn get_lints(&self) -> LintArray { lint_array![ - IFS_SAME_COND + IFS_SAME_COND, + IF_SAME_THEN_ELSE ] } } impl LateLintPass for CopyAndPaste { fn check_expr(&mut self, 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; - } + if !in_macro(cx, expr.span) { + lint_same_then_else(cx, expr); + lint_same_cond(cx, expr); } + } +} - let conds = condition_sequence(expr); - - for (n, i) in conds.iter().enumerate() { - for j in conds.iter().skip(n+1) { - if is_exp_equal(cx, i, j) { - span_lint(cx, IFS_SAME_COND, j.span, "this if as the same condition as a previous if"); +/// 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 { + let must_lint = if let ExprBlock(ref else_block) = else_expr.node { + over(&then_block.stmts, &else_block.stmts, |l, r| is_stmt_equal(cx, l, r)) && + match (&then_block.expr, &else_block.expr) { + (&Some(ref then_expr), &Some(ref else_expr)) => { + is_exp_equal(cx, &then_expr, &else_expr) + } + (&None, &None) => true, + _ => false, } + } + else { + false + }; + + if must_lint { + span_lint(cx, IF_SAME_THEN_ELSE, expr.span, "this if has the same then and else blocks"); + } + } +} + +/// 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 is_exp_equal(cx, 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/lib.rs b/src/lib.rs index 6b2a43db795..35ec0e13c3c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -192,6 +192,7 @@ pub fn plugin_registrar(reg: &mut Registry) { block_in_if_condition::BLOCK_IN_IF_CONDITION_EXPR, block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT, collapsible_if::COLLAPSIBLE_IF, + copies::IF_SAME_THEN_ELSE, copies::IFS_SAME_COND, cyclomatic_complexity::CYCLOMATIC_COMPLEXITY, derive::DERIVE_HASH_NOT_EQ, diff --git a/src/utils.rs b/src/utils.rs index 72c6fe94ce2..13fd849993f 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -589,6 +589,14 @@ fn parse_attrs(sess: &Session, attrs: &[ast::Attribute], name: &' } } +pub fn is_stmt_equal(cx: &LateContext, left: &Stmt, right: &Stmt) -> bool { + match (&left.node, &right.node) { + (&StmtExpr(ref l, _), &StmtExpr(ref r, _)) => is_exp_equal(cx, l, r), + (&StmtSemi(ref l, _), &StmtSemi(ref r, _)) => is_exp_equal(cx, l, r), + _ => false, + } +} + pub fn is_exp_equal(cx: &LateContext, left: &Expr, right: &Expr) -> bool { if let (Some(l), Some(r)) = (constant(cx, left), constant(cx, right)) { if l == r { @@ -649,7 +657,7 @@ fn is_qself_equal(left: &QSelf, right: &QSelf) -> bool { left.ty.node == right.ty.node && left.position == right.position } -fn over(left: &[X], right: &[X], mut eq_fn: F) -> bool +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)) diff --git a/tests/compile-fail/copies.rs b/tests/compile-fail/copies.rs index a29fd392c5e..94c5c620f34 100755 --- a/tests/compile-fail/copies.rs +++ b/tests/compile-fail/copies.rs @@ -1,23 +1,61 @@ #![feature(plugin)] #![plugin(clippy)] +#![allow(dead_code)] #![deny(clippy)] fn foo() -> bool { unimplemented!() } -fn main() { +fn if_same_then_else() { + if true { //~ERROR this if has the same then and else blocks + foo(); + } + else { + foo(); + } + + if true { + foo(); + foo(); + } + else { + foo(); + } + + let _ = if true { //~ERROR this if has the same then and else blocks + foo(); + 42 + } + else { + foo(); + 42 + }; + + if true { + foo(); + } + + let _ = if true { //~ERROR this if has the same then and else blocks + 42 + } + else { + 42 + }; +} + +fn ifs_same_cond() { let a = 0; if a == 1 { } - else if a == 1 { //~ERROR this if as 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 as 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 { } @@ -26,6 +64,8 @@ fn main() { // this to make the intention clearer anyway. if foo() { } - else if foo() { //~ERROR this if as the same condition as a previous if + else if foo() { //~ERROR this if has the same condition as a previous if } } + +fn main() {} diff --git a/tests/compile-fail/needless_bool.rs b/tests/compile-fail/needless_bool.rs index 39fdf6353fd..c2ad24bc4ee 100644 --- a/tests/compile-fail/needless_bool.rs +++ b/tests/compile-fail/needless_bool.rs @@ -1,6 +1,7 @@ #![feature(plugin)] #![plugin(clippy)] +#[allow(if_same_then_else)] #[deny(needless_bool)] fn main() { let x = true; From 8e22d08129dc242cebcdb25b824fa4ffb57d4f7a Mon Sep 17 00:00:00 2001 From: mcarton Date: Sat, 30 Jan 2016 20:10:14 +0100 Subject: [PATCH 3/6] Improve is_exp_equal --- src/copies.rs | 13 +-- src/entry.rs | 2 +- src/eq_op.rs | 2 +- src/strings.rs | 4 +- src/utils.rs | 164 +++++++++++++++++++++++++++++++---- tests/compile-fail/copies.rs | 76 ++++++++++++++-- tests/compile-fail/eq_op.rs | 5 ++ 7 files changed, 227 insertions(+), 39 deletions(-) diff --git a/src/copies.rs b/src/copies.rs index f8fe09c3133..38f1be92d30 100644 --- a/src/copies.rs +++ b/src/copies.rs @@ -1,6 +1,6 @@ use rustc::lint::*; use rustc_front::hir::*; -use utils::{get_parent_expr, in_macro, is_exp_equal, is_stmt_equal, over, span_lint, span_note_and_lint}; +use utils::{get_parent_expr, in_macro, is_block_equal, is_exp_equal, 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,14 +55,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 { let must_lint = if let ExprBlock(ref else_block) = else_expr.node { - over(&then_block.stmts, &else_block.stmts, |l, r| is_stmt_equal(cx, l, r)) && - match (&then_block.expr, &else_block.expr) { - (&Some(ref then_expr), &Some(ref else_expr)) => { - is_exp_equal(cx, &then_expr, &else_expr) - } - (&None, &None) => true, - _ => false, - } + is_block_equal(cx, &then_block, &else_block, false) } else { false @@ -87,7 +80,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) { + if is_exp_equal(cx, i, j, true) { 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 64d6fa7be38..d5bb086fc21 100644 --- a/src/entry.rs +++ b/src/entry.rs @@ -89,7 +89,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]) + is_exp_equal(cx, key, ¶ms[1], false) ], { let help = if sole_expr { format!("{}.entry({}).or_insert({})", diff --git a/src/eq_op.rs b/src/eq_op.rs index 49037cd9ae7..06e4fdc6cb7 100644 --- a/src/eq_op.rs +++ b/src/eq_op.rs @@ -29,7 +29,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_cmp_or_bit(op) && is_exp_equal(cx, left, right) { + if is_cmp_or_bit(op) && is_exp_equal(cx, left, right, true) { span_lint(cx, EQ_OP, e.span, diff --git a/src/strings.rs b/src/strings.rs index f1a1341460e..b78db7f4b77 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -84,7 +84,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) { + if is_exp_equal(cx, target, left, false) { return; } } @@ -113,7 +113,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), + ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) => is_exp_equal(cx, target, left, false), 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.rs b/src/utils.rs index 13fd849993f..a8890f31cb0 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -589,59 +589,183 @@ fn parse_attrs(sess: &Session, attrs: &[ast::Attribute], name: &' } } -pub fn is_stmt_equal(cx: &LateContext, left: &Stmt, right: &Stmt) -> bool { +/// 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) { - (&StmtExpr(ref l, _), &StmtExpr(ref r, _)) => is_exp_equal(cx, l, r), - (&StmtSemi(ref l, _), &StmtSemi(ref r, _)) => is_exp_equal(cx, l, r), + (&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, } } -pub fn is_exp_equal(cx: &LateContext, left: &Expr, right: &Expr) -> bool { +/// 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) + 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) && is_exp_equal(cx, lr, 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)) => { - is_exp_equal(cx, lfun, rfun) && is_exps_equal(cx, largs, 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) } - (&ExprCast(ref lx, ref lt), &ExprCast(ref rx, ref rt)) => is_exp_equal(cx, lx, rx) && 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) + 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) && is_exp_equal(cx, li, 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)) } (&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 - lname.node == rname.node && ltys.is_empty() && rtys.is_empty() && is_exps_equal(cx, largs, rargs) + !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), + (&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) + 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) + 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()) } - (&ExprVec(ref l), &ExprVec(ref r)) => is_exps_equal(cx, l, r), _ => false, } } -fn is_exps_equal(cx: &LateContext, left: &[P], right: &[P]) -> bool { - over(left, right, |l, r| is_exp_equal(cx, l, r)) +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 { @@ -650,20 +774,22 @@ fn is_path_equal(left: &Path, right: &Path) -> bool { left.global == right.global && over(&left.segments, &right.segments, - |l, r| l.identifier.name == r.identifier.name && l.parameters == r.parameters) + |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)) } -fn both(l: &Option, r: &Option, mut eq_fn: F) -> bool +/// 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))) diff --git a/tests/compile-fail/copies.rs b/tests/compile-fail/copies.rs index 94c5c620f34..0f57b619ccc 100755 --- a/tests/compile-fail/copies.rs +++ b/tests/compile-fail/copies.rs @@ -2,11 +2,15 @@ #![plugin(clippy)] #![allow(dead_code)] -#![deny(clippy)] +#![allow(let_and_return)] +#![allow(needless_return)] +#![allow(unused_variables)] +#![deny(if_same_then_else)] +#![deny(ifs_same_cond)] fn foo() -> bool { unimplemented!() } -fn if_same_then_else() { +fn if_same_then_else() -> &'static str { if true { //~ERROR this if has the same then and else blocks foo(); } @@ -41,6 +45,62 @@ fn if_same_then_else() { else { 42 }; + + if true { //~ERROR this if has the same then and else blocks + let bar = if true { + 42 + } + else { + 43 + }; + + while foo() { break; } + bar + 1; + } + else { + let bar = if true { + 42 + } + else { + 43 + }; + + while foo() { break; } + bar + 1; + } + + if true { //~ERROR this if has the same then and else blocks + match 42 { + 42 => (), + a if a > 0 => (), + 10...15 => (), + _ => (), + } + } + else { + match 42 { + 42 => (), + a if a > 0 => (), + 10...15 => (), + _ => (), + } + } + + if true { //~ERROR this if has the same then and else blocks + if let Some(a) = Some(42) {} + } + else { + if let Some(a) = Some(42) {} + } + + if true { //~ERROR this if has the same then and else blocks + let foo = ""; + return &foo[0..]; + } + else { + let foo = ""; + return &foo[0..]; + } } fn ifs_same_cond() { @@ -60,11 +120,15 @@ fn ifs_same_cond() { else if a == 1 { } - // Ok, maybe `foo` isn’t pure and this actually makes sense. But you should probably refactor - // this to make the intention clearer anyway. - if foo() { + let mut v = vec![1]; + if v.pop() == None { // ok, functions } - else if foo() { //~ERROR this if has the same condition as a previous if + else if v.pop() == None { + } + + if v.len() == 42 { // ok, functions + } + else if v.len() == 42 { } } diff --git a/tests/compile-fail/eq_op.rs b/tests/compile-fail/eq_op.rs index fc59c2739a2..fe74d182da1 100644 --- a/tests/compile-fail/eq_op.rs +++ b/tests/compile-fail/eq_op.rs @@ -32,4 +32,9 @@ fn main() { // const folding 1 + 1 == 2; //~ERROR equal expressions 1 - 1 == 0; //~ERROR equal expressions + + let mut a = vec![1]; + a == a; //~ERROR equal expressions + 2*a.len() == 2*a.len(); // ok, functions + a.pop() == a.pop(); // ok, functions } From cd7a9132001e0a6de7ee2f7420c63bdf98ca6eff Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 3 Feb 2016 20:42:05 +0100 Subject: [PATCH 4/6] Add `-` and `/` to EQ_OP --- src/eq_op.rs | 14 +++++++++----- tests/compile-fail/eq_op.rs | 14 ++++++++++---- tests/compile-fail/identity_op.rs | 1 + tests/compile-fail/zero_div_zero.rs | 4 ++++ 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/eq_op.rs b/src/eq_op.rs index 06e4fdc6cb7..aecd0693ff1 100644 --- a/src/eq_op.rs +++ b/src/eq_op.rs @@ -4,9 +4,11 @@ use rustc_front::util as ast_util; use utils::{is_exp_equal, span_lint}; -/// **What it does:** This lint checks for equal operands to comparisons and bitwise binary operators (`&`, `|` and `^`). +/// **What it does:** This lint checks for equal operands to comparison, logical and bitwise, +/// difference and division binary operators (`==`, `>`, etc., `&&`, `||`, `&`, `|`, `^`, `-` and +/// `/`). /// -/// **Why is this bad?** This is usually just a typo. +/// **Why is this bad?** This is usually just a typo or a copy and paste error. /// /// **Known problems:** False negatives: We had some false positives regarding calls (notably [racer](https://github.com/phildawes/racer) had one instance of `x.pop() && x.pop()`), so we removed matching any function or method calls. We may introduce a whitelist of known pure functions in the future. /// @@ -29,19 +31,21 @@ 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_cmp_or_bit(op) && is_exp_equal(cx, left, right, true) { + if is_valid_operator(op) && is_exp_equal(cx, left, right, true) { span_lint(cx, EQ_OP, e.span, - &format!("equal expressions as operands to {}", ast_util::binop_to_string(op.node))); + &format!("equal expressions as operands to `{}`", ast_util::binop_to_string(op.node))); } } } } -fn is_cmp_or_bit(op: &BinOp) -> bool { +fn is_valid_operator(op: &BinOp) -> bool { match op.node { + BiSub | + BiDiv | BiEq | BiLt | BiLe | diff --git a/tests/compile-fail/eq_op.rs b/tests/compile-fail/eq_op.rs index fe74d182da1..7be5ef11ce6 100644 --- a/tests/compile-fail/eq_op.rs +++ b/tests/compile-fail/eq_op.rs @@ -19,9 +19,9 @@ fn main() { // unary and binary operators (-(2) < -(2)); //~ERROR equal expressions ((1 + 1) & (1 + 1) == (1 + 1) & (1 + 1)); - //~^ ERROR equal expressions - //~^^ ERROR equal expressions - //~^^^ ERROR equal expressions + //~^ ERROR equal expressions as operands to `==` + //~^^ ERROR equal expressions as operands to `&` + //~^^^ ERROR equal expressions as operands to `&` (1 * 2) + (3 * 4) == 1 * 2 + 3 * 4; //~ERROR equal expressions // various other things @@ -31,7 +31,13 @@ fn main() { // const folding 1 + 1 == 2; //~ERROR equal expressions - 1 - 1 == 0; //~ERROR equal expressions + 1 - 1 == 0; //~ERROR equal expressions as operands to `==` + //~^ ERROR equal expressions as operands to `-` + + 1 - 1; //~ERROR equal expressions + 1 / 1; //~ERROR equal expressions + true && true; //~ERROR equal expressions + true || true; //~ERROR equal expressions let mut a = vec![1]; a == a; //~ERROR equal expressions diff --git a/tests/compile-fail/identity_op.rs b/tests/compile-fail/identity_op.rs index 54551852d5e..c1141e0b460 100644 --- a/tests/compile-fail/identity_op.rs +++ b/tests/compile-fail/identity_op.rs @@ -5,6 +5,7 @@ const ONE : i64 = 1; const NEG_ONE : i64 = -1; const ZERO : i64 = 0; +#[allow(eq_op)] #[deny(identity_op)] fn main() { let x = 0; diff --git a/tests/compile-fail/zero_div_zero.rs b/tests/compile-fail/zero_div_zero.rs index 8c40923d3ed..c422e83873b 100644 --- a/tests/compile-fail/zero_div_zero.rs +++ b/tests/compile-fail/zero_div_zero.rs @@ -5,9 +5,13 @@ #[deny(zero_divided_by_zero)] fn main() { let nan = 0.0 / 0.0; //~ERROR constant division of 0.0 with 0.0 will always result in NaN + //~^ equal expressions as operands to `/` let f64_nan = 0.0 / 0.0f64; //~ERROR constant division of 0.0 with 0.0 will always result in NaN + //~^ equal expressions as operands to `/` let other_f64_nan = 0.0f64 / 0.0; //~ERROR constant division of 0.0 with 0.0 will always result in NaN + //~^ equal expressions as operands to `/` let one_more_f64_nan = 0.0f64/0.0f64; //~ERROR constant division of 0.0 with 0.0 will always result in NaN + //~^ equal expressions as operands to `/` let zero = 0.0; let other_zero = 0.0; let other_nan = zero / other_zero; // fine - this lint doesn't propegate constants. From 344698377f04124a5df01e498e55c3e27eac294d Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 5 Feb 2016 19:43:21 +0100 Subject: [PATCH 5/6] Fix typo --- README.md | 2 +- src/copies.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 01999cce70b..d134afe8a8e 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 114 lints included in this crate: +There are 116 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ diff --git a/src/copies.rs b/src/copies.rs index 38f1be92d30..84324dffc85 100644 --- a/src/copies.rs +++ b/src/copies.rs @@ -87,7 +87,7 @@ fn lint_same_cond(cx: &LateContext, expr: &Expr) { } } -/// Return the list of conditions expression in a sequence of `if/else`. +/// 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![]; From a9e1b1fba05ce94a65f511e2c07bd086c1b0f00f Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 7 Feb 2016 14:40:45 +0100 Subject: [PATCH 6/6] Small cleanup --- src/copies.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/copies.rs b/src/copies.rs index 84324dffc85..525c7b7a6fd 100644 --- a/src/copies.rs +++ b/src/copies.rs @@ -54,15 +54,10 @@ impl LateLintPass for CopyAndPaste { /// 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 { - let must_lint = if let ExprBlock(ref else_block) = else_expr.node { - is_block_equal(cx, &then_block, &else_block, false) - } - else { - false - }; - - if must_lint { - span_lint(cx, IF_SAME_THEN_ELSE, expr.span, "this if has the same then and else blocks"); + if let ExprBlock(ref else_block) = else_expr.node { + if is_block_equal(cx, &then_block, &else_block, false) { + span_lint(cx, IF_SAME_THEN_ELSE, expr.span, "this if has the same then and else blocks"); + } } } }