diff --git a/README.md b/README.md index 2673d81f808..fbd6f4ebfe6 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ name [chars_next_cmp](https://github.com/Manishearth/rust-clippy/wiki#chars_next_cmp) | warn | using `.chars().next()` to check if a string starts with a char [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 [derive_hash_not_eq](https://github.com/Manishearth/rust-clippy/wiki#derive_hash_not_eq) | warn | deriving `Hash` but implementing `PartialEq` explicitly diff --git a/src/bit_mask.rs b/src/bit_mask.rs index 6c6d277e25b..b0d3c3d3f78 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/collapsible_if.rs b/src/collapsible_if.rs index 4a17d3a4608..fb1d7f696d1 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/src/consts.rs b/src/consts.rs index 34f7e924de7..31094e5b6c6 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 2cce36f2a9c..e72f2392054 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 1c445f42b55..5bd392abb3c 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, diff --git a/tests/compile-fail/collapsible_if.rs b/tests/compile-fail/collapsible_if.rs index cc63e895f1c..85eac28dc38 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" {