diff --git a/clippy_lints/src/formatting.rs b/clippy_lints/src/formatting.rs index 48ee38348..cc3801423 100644 --- a/clippy_lints/src/formatting.rs +++ b/clippy_lints/src/formatting.rs @@ -31,8 +31,8 @@ declare_clippy_lint! { "suspicious formatting of `*=`, `-=` or `!=`" } -/// **What it does:** Checks for formatting of `else if`. It lints if the `else` -/// and `if` are not on the same line or the `else` seems to be missing. +/// **What it does:** Checks for formatting of `else`. It lints if the `else` +/// is followed immediately by a newline or the `else` seems to be missing. /// /// **Why is this bad?** This is probably some refactoring remnant, even if the /// code is correct, it might look confusing. @@ -42,19 +42,29 @@ declare_clippy_lint! { /// **Example:** /// ```rust,ignore /// if foo { +/// } { // looks like an `else` is missing here +/// } +/// +/// if foo { /// } if bar { // looks like an `else` is missing here /// } /// /// if foo { /// } else /// +/// { // this is the `else` block of the previous `if`, but should it be? +/// } +/// +/// if foo { +/// } else +/// /// if bar { // this is the `else` block of the previous `if`, but should it be? /// } /// ``` declare_clippy_lint! { pub SUSPICIOUS_ELSE_FORMATTING, style, - "suspicious formatting of `else if`" + "suspicious formatting of `else`" } /// **What it does:** Checks for possible missing comma in an array. It lints if @@ -96,7 +106,7 @@ impl EarlyLintPass for Formatting { match (&w[0].node, &w[1].node) { (&ast::StmtKind::Expr(ref first), &ast::StmtKind::Expr(ref second)) | (&ast::StmtKind::Expr(ref first), &ast::StmtKind::Semi(ref second)) => { - check_consecutive_ifs(cx, first, second); + check_missing_else(cx, first, second); }, _ => (), } @@ -105,7 +115,7 @@ impl EarlyLintPass for Formatting { fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) { check_assign(cx, expr); - check_else_if(cx, expr); + check_else(cx, expr); check_array(cx, expr); } } @@ -139,10 +149,18 @@ fn check_assign(cx: &EarlyContext<'_>, expr: &ast::Expr) { } } -/// Implementation of the `SUSPICIOUS_ELSE_FORMATTING` lint for weird `else if`. -fn check_else_if(cx: &EarlyContext<'_>, expr: &ast::Expr) { +/// Implementation of the `SUSPICIOUS_ELSE_FORMATTING` lint for weird `else`. +fn check_else(cx: &EarlyContext<'_>, expr: &ast::Expr) { if let Some((then, &Some(ref else_))) = unsugar_if(expr) { - if unsugar_if(else_).is_some() && !differing_macro_contexts(then.span, else_.span) && !in_macro(then.span) { + if (is_block(else_) || unsugar_if(else_).is_some()) + && !differing_macro_contexts(then.span, else_.span) + && !in_macro(then.span) + { + // workaround for rust-lang/rust#43081 + if expr.span.lo().0 == 0 && expr.span.hi().0 == 0 { + return; + } + // this will be a span from the closing ‘}’ of the “then” block (excluding) to // the // “if” of the “else if” block (excluding) @@ -154,14 +172,19 @@ fn check_else_if(cx: &EarlyContext<'_>, expr: &ast::Expr) { let else_pos = else_snippet.find("else").expect("there must be a `else` here"); if else_snippet[else_pos..].contains('\n') { + let else_desc = if unsugar_if(else_).is_some() { "if" } else { "{..}" }; + span_note_and_lint( cx, SUSPICIOUS_ELSE_FORMATTING, else_span, - "this is an `else if` but the formatting might hide it", + &format!("this is an `else {}` but the formatting might hide it", else_desc), else_span, - "to remove this lint, remove the `else` or remove the new line between `else` \ - and `if`", + &format!( + "to remove this lint, remove the `else` or remove the new line between \ + `else` and `{}`", + else_desc, + ), ); } } @@ -200,32 +223,47 @@ fn check_array(cx: &EarlyContext<'_>, expr: &ast::Expr) { } } -/// Implementation of the `SUSPICIOUS_ELSE_FORMATTING` lint for consecutive ifs. -fn check_consecutive_ifs(cx: &EarlyContext<'_>, first: &ast::Expr, second: &ast::Expr) { +fn check_missing_else(cx: &EarlyContext<'_>, first: &ast::Expr, second: &ast::Expr) { if !differing_macro_contexts(first.span, second.span) && !in_macro(first.span) && unsugar_if(first).is_some() - && unsugar_if(second).is_some() + && (is_block(second) || unsugar_if(second).is_some()) { // where the else would be let else_span = first.span.between(second.span); if let Some(else_snippet) = snippet_opt(cx, else_span) { if !else_snippet.contains('\n') { + let (looks_like, next_thing) = if unsugar_if(second).is_some() { + ("an `else if`", "the second `if`") + } else { + ("an `else {..}`", "the next block") + }; + span_note_and_lint( cx, SUSPICIOUS_ELSE_FORMATTING, else_span, - "this looks like an `else if` but the `else` is missing", + &format!("this looks like {} but the `else` is missing", looks_like), else_span, - "to remove this lint, add the missing `else` or add a new line before the second \ - `if`", + &format!( + "to remove this lint, add the missing `else` or add a new line before {}", + next_thing, + ), ); } } } } +fn is_block(expr: &ast::Expr) -> bool { + if let ast::ExprKind::Block(..) = expr.node { + true + } else { + false + } +} + /// Match `if` or `if let` expressions and return the `then` and `else` block. fn unsugar_if(expr: &ast::Expr) -> Option<(&P, &Option>)> { match expr.node { diff --git a/tests/ui/formatting.rs b/tests/ui/formatting.rs index 3bea98acf..b74f778b1 100644 --- a/tests/ui/formatting.rs +++ b/tests/ui/formatting.rs @@ -16,7 +16,11 @@ fn foo() -> bool { true } fn main() { - // weird `else if` formatting: + // weird `else` formatting: + if foo() { + } { + } + if foo() { } if foo() { } @@ -41,6 +45,17 @@ fn main() { let _ = 0; }; + if foo() { + } else + { + } + + if foo() { + } + else + { + } + if foo() { } else if foo() { // the span of the above error should continue here @@ -53,6 +68,20 @@ fn main() { } // those are ok: + if foo() { + } + { + } + + if foo() { + } else { + } + + if foo() { + } + else { + } + if foo() { } if foo() { diff --git a/tests/ui/formatting.stderr b/tests/ui/formatting.stderr index 7399a0d45..9f00a51bc 100644 --- a/tests/ui/formatting.stderr +++ b/tests/ui/formatting.stderr @@ -1,90 +1,119 @@ -error: this looks like an `else if` but the `else` is missing +error: this looks like an `else {..}` but the `else` is missing --> $DIR/formatting.rs:21:6 | -21 | } if foo() { +21 | } { | ^ | = note: `-D clippy::suspicious-else-formatting` implied by `-D warnings` + = note: to remove this lint, add the missing `else` or add a new line before the next block + +error: this looks like an `else if` but the `else` is missing + --> $DIR/formatting.rs:25:6 + | +25 | } if foo() { + | ^ + | = note: to remove this lint, add the missing `else` or add a new line before the second `if` error: this looks like an `else if` but the `else` is missing - --> $DIR/formatting.rs:28:10 + --> $DIR/formatting.rs:32:10 | -28 | } if foo() { +32 | } if foo() { | ^ | = note: to remove this lint, add the missing `else` or add a new line before the second `if` error: this looks like an `else if` but the `else` is missing - --> $DIR/formatting.rs:36:10 + --> $DIR/formatting.rs:40:10 | -36 | } if foo() { +40 | } if foo() { | ^ | = note: to remove this lint, add the missing `else` or add a new line before the second `if` +error: this is an `else {..}` but the formatting might hide it + --> $DIR/formatting.rs:49:6 + | +49 | } else + | ______^ +50 | | { + | |____^ + | + = note: to remove this lint, remove the `else` or remove the new line between `else` and `{..}` + +error: this is an `else {..}` but the formatting might hide it + --> $DIR/formatting.rs:54:6 + | +54 | } + | ______^ +55 | | else +56 | | { + | |____^ + | + = note: to remove this lint, remove the `else` or remove the new line between `else` and `{..}` + error: this is an `else if` but the formatting might hide it - --> $DIR/formatting.rs:45:6 + --> $DIR/formatting.rs:60:6 | -45 | } else +60 | } else | ______^ -46 | | if foo() { // the span of the above error should continue here +61 | | if foo() { // the span of the above error should continue here | |____^ | = note: to remove this lint, remove the `else` or remove the new line between `else` and `if` error: this is an `else if` but the formatting might hide it - --> $DIR/formatting.rs:50:6 + --> $DIR/formatting.rs:65:6 | -50 | } +65 | } | ______^ -51 | | else -52 | | if foo() { // the span of the above error should continue here +66 | | else +67 | | if foo() { // the span of the above error should continue here | |____^ | = note: to remove this lint, remove the `else` or remove the new line between `else` and `if` error: this looks like you are trying to use `.. -= ..`, but you really are doing `.. = (- ..)` - --> $DIR/formatting.rs:77:6 - | -77 | a =- 35; - | ^^^^ - | - = note: `-D clippy::suspicious-assignment-formatting` implied by `-D warnings` - = note: to remove this lint, use either `-=` or `= -` + --> $DIR/formatting.rs:106:6 + | +106 | a =- 35; + | ^^^^ + | + = note: `-D clippy::suspicious-assignment-formatting` implied by `-D warnings` + = note: to remove this lint, use either `-=` or `= -` error: this looks like you are trying to use `.. *= ..`, but you really are doing `.. = (* ..)` - --> $DIR/formatting.rs:78:6 - | -78 | a =* &191; - | ^^^^ - | - = note: to remove this lint, use either `*=` or `= *` + --> $DIR/formatting.rs:107:6 + | +107 | a =* &191; + | ^^^^ + | + = note: to remove this lint, use either `*=` or `= *` error: this looks like you are trying to use `.. != ..`, but you really are doing `.. = (! ..)` - --> $DIR/formatting.rs:81:6 - | -81 | b =! false; - | ^^^^ - | - = note: to remove this lint, use either `!=` or `= !` + --> $DIR/formatting.rs:110:6 + | +110 | b =! false; + | ^^^^ + | + = note: to remove this lint, use either `!=` or `= !` error: possibly missing a comma here - --> $DIR/formatting.rs:90:19 - | -90 | -1, -2, -3 // <= no comma here - | ^ - | - = note: `-D clippy::possible-missing-comma` implied by `-D warnings` - = note: to remove this lint, add a comma or write the expr in a single line + --> $DIR/formatting.rs:119:19 + | +119 | -1, -2, -3 // <= no comma here + | ^ + | + = note: `-D clippy::possible-missing-comma` implied by `-D warnings` + = note: to remove this lint, add a comma or write the expr in a single line error: possibly missing a comma here - --> $DIR/formatting.rs:94:19 - | -94 | -1, -2, -3 // <= no comma here - | ^ - | - = note: to remove this lint, add a comma or write the expr in a single line + --> $DIR/formatting.rs:123:19 + | +123 | -1, -2, -3 // <= no comma here + | ^ + | + = note: to remove this lint, add a comma or write the expr in a single line -error: aborting due to 10 previous errors +error: aborting due to 13 previous errors