diff --git a/README.md b/README.md index d424aca9f..3a957a7e4 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 128 lints included in this crate: +There are 129 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -52,6 +52,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_not_else](https://github.com/Manishearth/rust-clippy/wiki#if_not_else) | warn | finds if branches that could be swapped so no negation operation is necessary on the condition [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` diff --git a/src/block_in_if_condition.rs b/src/block_in_if_condition.rs index 6db77a5ce..7004910df 100644 --- a/src/block_in_if_condition.rs +++ b/src/block_in_if_condition.rs @@ -45,9 +45,7 @@ impl<'v> Visitor<'v> for ExVisitor<'v> { fn visit_expr(&mut self, expr: &'v Expr) { if let ExprClosure(_, _, ref block) = expr.node { let complex = { - if !block.stmts.is_empty() { - true - } else { + if block.stmts.is_empty() { if let Some(ref ex) = block.expr { match ex.node { ExprBlock(_) => true, @@ -56,6 +54,8 @@ impl<'v> Visitor<'v> for ExVisitor<'v> { } else { false } + } else { + true } }; if complex { diff --git a/src/consts.rs b/src/consts.rs index 64f45d72c..6dd5651e7 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -159,10 +159,10 @@ impl PartialOrd for Constant { fn partial_cmp(&self, other: &Constant) -> Option { match (self, other) { (&Constant::Str(ref ls, ref lsty), &Constant::Str(ref rs, ref rsty)) => { - if lsty != rsty { - None - } else { + if lsty == rsty { Some(ls.cmp(rs)) + } else { + None } } (&Constant::Byte(ref l), &Constant::Byte(ref r)) => Some(l.cmp(r)), diff --git a/src/enum_variants.rs b/src/enum_variants.rs index a95fca8c6..179ce24cf 100644 --- a/src/enum_variants.rs +++ b/src/enum_variants.rs @@ -89,12 +89,10 @@ impl EarlyLintPass for EnumVariantNames { let post_camel = camel_case_from(&post); post = &post[post_camel..]; } - let (what, value) = if !pre.is_empty() { - ("pre", pre) - } else if !post.is_empty() { - ("post", post) - } else { - return; + let (what, value) = match (pre.is_empty(), post.is_empty()) { + (true, true) => return, + (false, _) => ("pre", pre), + (true, false) => ("post", post), }; span_help_and_lint(cx, ENUM_VARIANT_NAMES, diff --git a/src/if_not_else.rs b/src/if_not_else.rs new file mode 100644 index 000000000..1a074a723 --- /dev/null +++ b/src/if_not_else.rs @@ -0,0 +1,53 @@ +//! lint on if branches that could be swapped so no `!` operation is necessary on the condition + +use rustc::lint::*; +use syntax::attr::*; +use syntax::ast::*; + +use utils::span_help_and_lint; + +/// **What it does:** Warns on the use of `!` or `!=` in an if condition with an else branch +/// +/// **Why is this bad?** Negations reduce the readability of statements +/// +/// **Known problems:** None +/// +/// **Example:** if !v.is_empty() { a() } else { b() } +declare_lint! { + pub IF_NOT_ELSE, Warn, + "finds if branches that could be swapped so no negation operation is necessary on the condition" +} + +pub struct IfNotElse; + +impl LintPass for IfNotElse { + fn get_lints(&self) -> LintArray { + lint_array!(IF_NOT_ELSE) + } +} + +impl EarlyLintPass for IfNotElse { + fn check_expr(&mut self, cx: &EarlyContext, item: &Expr) { + if let ExprKind::If(ref cond, _, Some(ref els)) = item.node { + if let ExprKind::Block(..) = els.node { + match cond.node { + ExprKind::Unary(UnOp::Not, _) => { + span_help_and_lint(cx, + IF_NOT_ELSE, + item.span, + "Unnecessary boolean `not` operation", + "remove the `!` and swap the blocks of the if/else"); + }, + ExprKind::Binary(ref kind, _, _) if kind.node == BinOpKind::Ne => { + span_help_and_lint(cx, + IF_NOT_ELSE, + item.span, + "Unnecessary `!=` operation", + "change to `==` and swap the blocks of the if/else"); + }, + _ => {}, + } + } + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 50ea765f8..7436dada0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -60,6 +60,7 @@ pub mod eta_reduction; pub mod format; pub mod formatting; pub mod identity_op; +pub mod if_not_else; pub mod items_after_statements; pub mod len_zero; pub mod lifetimes; @@ -171,6 +172,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_late_lint_pass(box format::FormatMacLint); reg.register_early_lint_pass(box formatting::Formatting); reg.register_late_lint_pass(box swap::Swap); + reg.register_early_lint_pass(box if_not_else::IfNotElse); reg.register_lint_group("clippy_pedantic", vec![ enum_glob_use::ENUM_GLOB_USE, @@ -222,6 +224,7 @@ pub fn plugin_registrar(reg: &mut Registry) { formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING, formatting::SUSPICIOUS_ELSE_FORMATTING, identity_op::IDENTITY_OP, + if_not_else::IF_NOT_ELSE, items_after_statements::ITEMS_AFTER_STATEMENTS, len_zero::LEN_WITHOUT_IS_EMPTY, len_zero::LEN_ZERO, diff --git a/src/loops.rs b/src/loops.rs index e6b28d20e..5e8623827 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -357,10 +357,10 @@ fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, ex }; let take: Cow<_> = if let Some(ref r) = *r { - if !is_len_call(&r, &indexed) { - format!(".take({})", snippet(cx, r.span, "..")).into() - } else { + if is_len_call(&r, &indexed) { "".into() + } else { + format!(".take({})", snippet(cx, r.span, "..")).into() } } else { "".into() diff --git a/src/matches.rs b/src/matches.rs index 35c0dbb39..d83288860 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -249,23 +249,21 @@ fn check_match_bool(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { }; if let Some((ref true_expr, ref false_expr)) = exprs { - if !is_unit_expr(true_expr) { - if !is_unit_expr(false_expr) { + match (is_unit_expr(true_expr), is_unit_expr(false_expr)) { + (false, false) => Some(format!("if {} {} else {}", snippet(cx, ex.span, "b"), expr_block(cx, true_expr, None, ".."), - expr_block(cx, false_expr, None, ".."))) - } else { + expr_block(cx, false_expr, None, ".."))), + (false, true) => Some(format!("if {} {}", snippet(cx, ex.span, "b"), - expr_block(cx, true_expr, None, ".."))) - } - } else if !is_unit_expr(false_expr) { - Some(format!("try\nif !{} {}", - snippet(cx, ex.span, "b"), - expr_block(cx, false_expr, None, ".."))) - } else { - None + expr_block(cx, true_expr, None, ".."))), + (true, false) => + Some(format!("try\nif !{} {}", + snippet(cx, ex.span, "b"), + expr_block(cx, false_expr, None, ".."))), + (true, true) => None, } } else { None diff --git a/tests/compile-fail/entry.rs b/tests/compile-fail/entry.rs index 7dc4054ec..e65ef503b 100644 --- a/tests/compile-fail/entry.rs +++ b/tests/compile-fail/entry.rs @@ -1,6 +1,6 @@ #![feature(plugin)] #![plugin(clippy)] -#![allow(unused)] +#![allow(unused, if_not_else)] #![deny(map_entry)] diff --git a/tests/compile-fail/if_not_else.rs b/tests/compile-fail/if_not_else.rs new file mode 100644 index 000000000..eb716e459 --- /dev/null +++ b/tests/compile-fail/if_not_else.rs @@ -0,0 +1,18 @@ +#![feature(plugin)] +#![plugin(clippy)] +#![deny(clippy)] + +fn bla() -> bool { unimplemented!() } + +fn main() { + if !bla() { //~ ERROR: Unnecessary boolean `not` operation + println!("Bugs"); + } else { + println!("Bunny"); + } + if 4 != 5 { //~ ERROR: Unnecessary `!=` operation + println!("Bugs"); + } else { + println!("Bunny"); + } +}