Skip to content

Commit e256055

Browse files
ManishearthManishearth
committed
Merge pull request #725 from oli-obk/swap_if_arms
lint ! and != in if expressions with else branches
2 parents 35e00e2 + 3b7720f commit e256055

File tree

10 files changed

+100
-29
lines changed

10 files changed

+100
-29
lines changed

‎README.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
88
[Jump to usage instructions](#usage)
99

1010
##Lints
11-
There are 128 lints included in this crate:
11+
There are 129 lints included in this crate:
1212

1313
name | default | meaning
1414
---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -52,6 +52,7 @@ name
5252
[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`
5353
[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`
5454
[identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op) | warn | using identity operations, e.g. `x + 0` or `y / 1`
55+
[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
5556
[if_same_then_else](https://github.com/Manishearth/rust-clippy/wiki#if_same_then_else) | warn | if with the same *then* and *else* blocks
5657
[ifs_same_cond](https://github.com/Manishearth/rust-clippy/wiki#ifs_same_cond) | warn | consecutive `ifs` with the same condition
5758
[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`

‎src/block_in_if_condition.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,7 @@ impl<'v> Visitor<'v> for ExVisitor<'v> {
4545
fn visit_expr(&mut self, expr: &'v Expr) {
4646
if let ExprClosure(_, _, ref block) = expr.node {
4747
let complex = {
48-
if !block.stmts.is_empty() {
49-
true
50-
} else {
48+
if block.stmts.is_empty() {
5149
if let Some(ref ex) = block.expr {
5250
match ex.node {
5351
ExprBlock(_) => true,
@@ -56,6 +54,8 @@ impl<'v> Visitor<'v> for ExVisitor<'v> {
5654
} else {
5755
false
5856
}
57+
} else {
58+
true
5959
}
6060
};
6161
if complex {

‎src/consts.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -159,10 +159,10 @@ impl PartialOrd for Constant {
159159
fn partial_cmp(&self, other: &Constant) -> Option<Ordering> {
160160
match (self, other) {
161161
(&Constant::Str(ref ls, ref lsty), &Constant::Str(ref rs, ref rsty)) => {
162-
if lsty != rsty {
163-
None
164-
} else {
162+
if lsty == rsty {
165163
Some(ls.cmp(rs))
164+
} else {
165+
None
166166
}
167167
}
168168
(&Constant::Byte(ref l), &Constant::Byte(ref r)) => Some(l.cmp(r)),

‎src/enum_variants.rs‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -89,12 +89,10 @@ impl EarlyLintPass for EnumVariantNames {
8989
let post_camel = camel_case_from(&post);
9090
post = &post[post_camel..];
9191
}
92-
let (what, value) = if !pre.is_empty() {
93-
("pre", pre)
94-
} else if !post.is_empty() {
95-
("post", post)
96-
} else {
97-
return;
92+
let (what, value) = match (pre.is_empty(), post.is_empty()) {
93+
(true, true) => return,
94+
(false, _) => ("pre", pre),
95+
(true, false) => ("post", post),
9896
};
9997
span_help_and_lint(cx,
10098
ENUM_VARIANT_NAMES,

‎src/if_not_else.rs‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
//! lint on if branches that could be swapped so no `!` operation is necessary on the condition
2+
3+
use rustc::lint::*;
4+
use syntax::attr::*;
5+
use syntax::ast::*;
6+
7+
use utils::span_help_and_lint;
8+
9+
/// **What it does:** Warns on the use of `!` or `!=` in an if condition with an else branch
10+
///
11+
/// **Why is this bad?** Negations reduce the readability of statements
12+
///
13+
/// **Known problems:** None
14+
///
15+
/// **Example:** if !v.is_empty() { a() } else { b() }
16+
declare_lint! {
17+
pub IF_NOT_ELSE, Warn,
18+
"finds if branches that could be swapped so no negation operation is necessary on the condition"
19+
}
20+
21+
pub struct IfNotElse;
22+
23+
impl LintPass for IfNotElse {
24+
fn get_lints(&self) -> LintArray {
25+
lint_array!(IF_NOT_ELSE)
26+
}
27+
}
28+
29+
impl EarlyLintPass for IfNotElse {
30+
fn check_expr(&mut self, cx: &EarlyContext, item: &Expr) {
31+
if let ExprKind::If(ref cond, _, Some(ref els)) = item.node {
32+
if let ExprKind::Block(..) = els.node {
33+
match cond.node {
34+
ExprKind::Unary(UnOp::Not, _) => {
35+
span_help_and_lint(cx,
36+
IF_NOT_ELSE,
37+
item.span,
38+
"Unnecessary boolean `not` operation",
39+
"remove the `!` and swap the blocks of the if/else");
40+
},
41+
ExprKind::Binary(ref kind, _, _) if kind.node == BinOpKind::Ne => {
42+
span_help_and_lint(cx,
43+
IF_NOT_ELSE,
44+
item.span,
45+
"Unnecessary `!=` operation",
46+
"change to `==` and swap the blocks of the if/else");
47+
},
48+
_ => {},
49+
}
50+
}
51+
}
52+
}
53+
}

‎src/lib.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ pub mod eta_reduction;
6060
pub mod format;
6161
pub mod formatting;
6262
pub mod identity_op;
63+
pub mod if_not_else;
6364
pub mod items_after_statements;
6465
pub mod len_zero;
6566
pub mod lifetimes;
@@ -171,6 +172,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
171172
reg.register_late_lint_pass(box format::FormatMacLint);
172173
reg.register_early_lint_pass(box formatting::Formatting);
173174
reg.register_late_lint_pass(box swap::Swap);
175+
reg.register_early_lint_pass(box if_not_else::IfNotElse);
174176

175177
reg.register_lint_group("clippy_pedantic", vec![
176178
enum_glob_use::ENUM_GLOB_USE,
@@ -222,6 +224,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
222224
formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
223225
formatting::SUSPICIOUS_ELSE_FORMATTING,
224226
identity_op::IDENTITY_OP,
227+
if_not_else::IF_NOT_ELSE,
225228
items_after_statements::ITEMS_AFTER_STATEMENTS,
226229
len_zero::LEN_WITHOUT_IS_EMPTY,
227230
len_zero::LEN_ZERO,

‎src/loops.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -357,10 +357,10 @@ fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, ex
357357
};
358358

359359
let take: Cow<_> = if let Some(ref r) = *r {
360-
if !is_len_call(&r, &indexed) {
361-
format!(".take({})", snippet(cx, r.span, "..")).into()
362-
} else {
360+
if is_len_call(&r, &indexed) {
363361
"".into()
362+
} else {
363+
format!(".take({})", snippet(cx, r.span, "..")).into()
364364
}
365365
} else {
366366
"".into()

‎src/matches.rs‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -249,23 +249,21 @@ fn check_match_bool(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) {
249249
};
250250

251251
if let Some((ref true_expr, ref false_expr)) = exprs {
252-
if !is_unit_expr(true_expr) {
253-
if !is_unit_expr(false_expr) {
252+
match (is_unit_expr(true_expr), is_unit_expr(false_expr)) {
253+
(false, false) =>
254254
Some(format!("if {} {} else {}",
255255
snippet(cx, ex.span, "b"),
256256
expr_block(cx, true_expr, None, ".."),
257-
expr_block(cx, false_expr, None, "..")))
258-
} else {
257+
expr_block(cx, false_expr, None, ".."))),
258+
(false, true) =>
259259
Some(format!("if {} {}",
260260
snippet(cx, ex.span, "b"),
261-
expr_block(cx, true_expr, None, "..")))
262-
}
263-
} else if !is_unit_expr(false_expr) {
264-
Some(format!("try\nif !{} {}",
265-
snippet(cx, ex.span, "b"),
266-
expr_block(cx, false_expr, None, "..")))
267-
} else {
268-
None
261+
expr_block(cx, true_expr, None, ".."))),
262+
(true, false) =>
263+
Some(format!("try\nif !{} {}",
264+
snippet(cx, ex.span, "b"),
265+
expr_block(cx, false_expr, None, ".."))),
266+
(true, true) => None,
269267
}
270268
} else {
271269
None

‎tests/compile-fail/entry.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
#![feature(plugin)]
22
#![plugin(clippy)]
3-
#![allow(unused)]
3+
#![allow(unused, if_not_else)]
44

55
#![deny(map_entry)]
66

‎tests/compile-fail/if_not_else.rs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
#![feature(plugin)]
2+
#![plugin(clippy)]
3+
#![deny(clippy)]
4+
5+
fn bla() -> bool { unimplemented!() }
6+
7+
fn main() {
8+
if !bla() { //~ ERROR: Unnecessary boolean `not` operation
9+
println!("Bugs");
10+
} else {
11+
println!("Bunny");
12+
}
13+
if 4 != 5 { //~ ERROR: Unnecessary `!=` operation
14+
println!("Bugs");
15+
} else {
16+
println!("Bunny");
17+
}
18+
}

0 commit comments

Comments
 (0)