From 375b8168e48f7af89c754a41581f3a7102fae066 Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 13 Jan 2016 18:32:05 +0100 Subject: [PATCH 1/2] Remove useless curly braces in else { if .. } --- src/bit_mask.rs | 19 +++++++------------ src/consts.rs | 8 +++----- src/minmax.rs | 8 +++----- src/shadow.rs | 41 ++++++++++++++++++++--------------------- 4 files changed, 33 insertions(+), 43 deletions(-) diff --git a/src/bit_mask.rs b/src/bit_mask.rs index c2fd37420663..133212daf147 100644 --- a/src/bit_mask.rs +++ b/src/bit_mask.rs @@ -148,11 +148,10 @@ fn check_bit_mask(cx: &LateContext, bit_op: BinOp_, cmp_op: BinOp_, mask_value: mask_value, cmp_value)); } - } else { - if mask_value == 0 { - span_lint(cx, BAD_BIT_MASK, *span, "&-masking with zero"); - } + } else if mask_value == 0 { + span_lint(cx, BAD_BIT_MASK, *span, "&-masking with zero"); } + } BiBitOr => { if mask_value | cmp_value != cmp_value { @@ -177,10 +176,8 @@ fn check_bit_mask(cx: &LateContext, bit_op: BinOp_, cmp_op: BinOp_, mask_value: &format!("incompatible bit mask: `_ & {}` will always be lower than `{}`", mask_value, cmp_value)); - } else { - if mask_value == 0 { - span_lint(cx, BAD_BIT_MASK, *span, "&-masking with zero"); - } + } else if mask_value == 0 { + span_lint(cx, BAD_BIT_MASK, *span, "&-masking with zero"); } } BiBitOr => { @@ -209,10 +206,8 @@ fn check_bit_mask(cx: &LateContext, bit_op: BinOp_, cmp_op: BinOp_, mask_value: &format!("incompatible bit mask: `_ & {}` will never be higher than `{}`", mask_value, cmp_value)); - } else { - if mask_value == 0 { - span_lint(cx, BAD_BIT_MASK, *span, "&-masking with zero"); - } + } else if mask_value == 0 { + span_lint(cx, BAD_BIT_MASK, *span, "&-masking with zero"); } } BiBitOr => { diff --git a/src/consts.rs b/src/consts.rs index 171ba6f27f05..68320705fb6b 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -536,12 +536,10 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> { Plus }) .and_then(|ty| l64.checked_add(r64).map(|v| ConstantInt(v, ty))) + } else if ln { + add_neg_int(r64, rty, l64, lty) } else { - if ln { - add_neg_int(r64, rty, l64, lty) - } else { - add_neg_int(l64, lty, r64, rty) - } + add_neg_int(l64, lty, r64, rty) } } // TODO: float (would need bignum library?) diff --git a/src/minmax.rs b/src/minmax.rs index 2cce36f2a9c9..e72f2392054e 100644 --- a/src/minmax.rs +++ b/src/minmax.rs @@ -59,12 +59,10 @@ fn min_max<'a>(cx: &LateContext, expr: &'a Expr) -> Option<(MinMax, Constant, &' if match_def_path(cx, def_id, &["core", "cmp", "min"]) { fetch_const(args, Min) + } else if match_def_path(cx, def_id, &["core", "cmp", "max"]) { + fetch_const(args, Max) } else { - if match_def_path(cx, def_id, &["core", "cmp", "max"]) { - fetch_const(args, Max) - } else { - None - } + None } } else { None diff --git a/src/shadow.rs b/src/shadow.rs index 2d3e423eacb9..bcf3ce7116ac 100644 --- a/src/shadow.rs +++ b/src/shadow.rs @@ -204,29 +204,28 @@ fn lint_shadow(cx: &LateContext, name: Name, span: Span, lspan: Span, init: & snippet(cx, lspan, "_"), snippet(cx, expr.span, ".."))); note_orig(cx, db, SHADOW_SAME, prev_span); + } else if contains_self(name, expr) { + let db = span_note_and_lint(cx, + SHADOW_REUSE, + lspan, + &format!("{} is shadowed by {} which reuses the original value", + snippet(cx, lspan, "_"), + snippet(cx, expr.span, "..")), + expr.span, + "initialization happens here"); + note_orig(cx, db, SHADOW_REUSE, prev_span); } else { - if contains_self(name, expr) { - let db = span_note_and_lint(cx, - SHADOW_REUSE, - lspan, - &format!("{} is shadowed by {} which reuses the original value", - snippet(cx, lspan, "_"), - snippet(cx, expr.span, "..")), - expr.span, - "initialization happens here"); - note_orig(cx, db, SHADOW_REUSE, prev_span); - } else { - let db = span_note_and_lint(cx, - SHADOW_UNRELATED, - lspan, - &format!("{} is shadowed by {}", - snippet(cx, lspan, "_"), - snippet(cx, expr.span, "..")), - expr.span, - "initialization happens here"); - note_orig(cx, db, SHADOW_UNRELATED, prev_span); - } + let db = span_note_and_lint(cx, + SHADOW_UNRELATED, + lspan, + &format!("{} is shadowed by {}", + snippet(cx, lspan, "_"), + snippet(cx, expr.span, "..")), + expr.span, + "initialization happens here"); + note_orig(cx, db, SHADOW_UNRELATED, prev_span); } + } else { let db = span_lint(cx, SHADOW_UNRELATED, From c2444c604388f96a6949bb39bcf3730c6ef78c0f Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 13 Jan 2016 18:32:55 +0100 Subject: [PATCH 2/2] Lint about `else { if .. }` with useless braces --- README.md | 2 +- src/collapsible_if.rs | 71 +++++++++++++++++++--------- tests/compile-fail/collapsible_if.rs | 24 ++++++++++ 3 files changed, 73 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 631c1d7f1d2b..33bca7f871ff 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ name [cast_sign_loss](https://github.com/Manishearth/rust-clippy/wiki#cast_sign_loss) | allow | casts from signed types to unsigned types, e.g `x as u32` where `x: i32` [cmp_nan](https://github.com/Manishearth/rust-clippy/wiki#cmp_nan) | deny | comparisons to NAN (which will always return false, which is probably not intended) [cmp_owned](https://github.com/Manishearth/rust-clippy/wiki#cmp_owned) | warn | creating owned instances for comparing with others, e.g. `x == "foo".to_string()` -[collapsible_if](https://github.com/Manishearth/rust-clippy/wiki#collapsible_if) | warn | two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` can be written as `if x && y { foo() }` +[collapsible_if](https://github.com/Manishearth/rust-clippy/wiki#collapsible_if) | warn | two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` can be written as `if x && y { foo() }` and an `else { if .. } expression can be collapsed to `else if` [cyclomatic_complexity](https://github.com/Manishearth/rust-clippy/wiki#cyclomatic_complexity) | warn | finds functions that should be split up into multiple functions [deprecated_semver](https://github.com/Manishearth/rust-clippy/wiki#deprecated_semver) | warn | `Warn` on `#[deprecated(since = "x")]` where x is not semver [duplicate_underscore_argument](https://github.com/Manishearth/rust-clippy/wiki#duplicate_underscore_argument) | warn | Function arguments having names which only differ by an underscore diff --git a/src/collapsible_if.rs b/src/collapsible_if.rs index 4a17d3a4608d..fb1d7f696d16 100644 --- a/src/collapsible_if.rs +++ b/src/collapsible_if.rs @@ -16,9 +16,11 @@ use rustc::lint::*; use rustc_front::hir::*; use syntax::codemap::Spanned; -use utils::{in_macro, span_help_and_lint, snippet, snippet_block}; +use utils::{in_macro, snippet, snippet_block, span_lint_and_then}; -/// **What it does:** This lint checks for nested `if`-statements which can be collapsed by `&&`-combining their conditions. It is `Warn` by default. +/// **What it does:** This lint checks for nested `if`-statements which can be collapsed by +/// `&&`-combining their conditions and for `else { if .. } expressions that can be collapsed to +/// `else if ..`. It is `Warn` by default. /// /// **Why is this bad?** Each `if`-statement adds one level of nesting, which makes code look more complex than it really is. /// @@ -29,7 +31,8 @@ declare_lint! { pub COLLAPSIBLE_IF, Warn, "two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` \ - can be written as `if x && y { foo() }`" + can be written as `if x && y { foo() }` and an `else { if .. } expression can be collapsed to \ + `else if`" } #[derive(Copy,Clone)] @@ -50,20 +53,44 @@ impl LateLintPass for CollapsibleIf { } fn check_if(cx: &LateContext, e: &Expr) { - if let ExprIf(ref check, ref then, None) = e.node { - if let Some(&Expr{ node: ExprIf(ref check_inner, ref content, None), span: sp, ..}) = - single_stmt_of_block(then) { - if e.span.expn_id != sp.expn_id { - return; + if let ExprIf(ref check, ref then, ref else_) = e.node { + match *else_ { + Some(ref else_) => { + if_let_chain! {[ + let ExprBlock(ref block) = else_.node, + block.stmts.is_empty(), + block.rules == BlockCheckMode::DefaultBlock, + let Some(ref else_) = block.expr, + let ExprIf(_, _, _) = else_.node + ], { + span_lint_and_then(cx, + COLLAPSIBLE_IF, + block.span, + "this `else { if .. }` block can be collapsed", |db| { + db.span_suggestion(block.span, "try", + format!("else {}", + snippet_block(cx, else_.span, ".."))); + }); + }} + } + None => { + if let Some(&Expr{ node: ExprIf(ref check_inner, ref content, None), span: sp, ..}) = + single_stmt_of_block(then) { + if e.span.expn_id != sp.expn_id { + return; + } + span_lint_and_then(cx, + COLLAPSIBLE_IF, + e.span, + "this if statement can be collapsed", |db| { + db.span_suggestion(e.span, "try", + format!("if {} && {} {}", + check_to_string(cx, check), + check_to_string(cx, check_inner), + snippet_block(cx, content.span, ".."))); + }); + } } - span_help_and_lint(cx, - COLLAPSIBLE_IF, - e.span, - "this if statement can be collapsed", - &format!("try\nif {} && {} {}", - check_to_string(cx, check), - check_to_string(cx, check_inner), - snippet_block(cx, content.span, ".."))); } } } @@ -90,16 +117,14 @@ fn single_stmt_of_block(block: &Block) -> Option<&Expr> { } else { None } - } else { - if block.stmts.is_empty() { - if let Some(ref p) = block.expr { - Some(p) - } else { - None - } + } else if block.stmts.is_empty() { + if let Some(ref p) = block.expr { + Some(p) } else { None } + } else { + None } } diff --git a/tests/compile-fail/collapsible_if.rs b/tests/compile-fail/collapsible_if.rs index cc63e895f1c1..85eac28dc387 100644 --- a/tests/compile-fail/collapsible_if.rs +++ b/tests/compile-fail/collapsible_if.rs @@ -17,6 +17,30 @@ fn main() { } } + // Collaspe `else { if .. }` to `else if ..` + if x == "hello" { + print!("Hello "); + } else { //~ERROR: this `else { if .. }` + //~| HELP try + //~| SUGGESTION else if y == "world" + if y == "world" { + println!("world!") + } + } + + if x == "hello" { + print!("Hello "); + } else { //~ERROR this `else { if .. }` + //~| HELP try + //~| SUGGESTION else if y == "world" + if y == "world" { + println!("world") + } + else { + println!("!") + } + } + // Works because any if with an else statement cannot be collapsed. if x == "hello" { if y == "world" {