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;