mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-11-23 21:23:56 +00:00
Auto merge of #3341 - HMPerson1:possibly_missing_else, r=phansch
Teach `suspicious_else_formatting` about `if .. {..} {..}`
We essentially treat bare blocks `{..}` identically to `if .. {..}`, except for different lint messages.
Fixes #3044
This commit is contained in:
bors
commit
d9cc71fc42
3 changed files with 160 additions and 64 deletions
|
|
@ -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<ast::Block>, &Option<P<ast::Expr>>)> {
|
||||
match expr.node {
|
||||
|
|
|
|||
|
|
@ -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() {
|
||||
|
|
|
|||
|
|
@ -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 second `if`
|
||||
= 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:28:10
|
||||
--> $DIR/formatting.rs:25:6
|
||||
|
|
||||
28 | } if foo() {
|
||||
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:36:10
|
||||
--> $DIR/formatting.rs:32:10
|
||||
|
|
||||
36 | } 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:40:10
|
||||
|
|
||||
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
|
||||
--> $DIR/formatting.rs:106:6
|
||||
|
|
||||
77 | a =- 35;
|
||||
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
|
||||
--> $DIR/formatting.rs:107:6
|
||||
|
|
||||
78 | a =* &191;
|
||||
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
|
||||
--> $DIR/formatting.rs:110:6
|
||||
|
|
||||
81 | b =! false;
|
||||
110 | b =! false;
|
||||
| ^^^^
|
||||
|
|
||||
= note: to remove this lint, use either `!=` or `= !`
|
||||
|
||||
error: possibly missing a comma here
|
||||
--> $DIR/formatting.rs:90:19
|
||||
--> $DIR/formatting.rs:119:19
|
||||
|
|
||||
90 | -1, -2, -3 // <= no comma here
|
||||
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
|
||||
--> $DIR/formatting.rs:123:19
|
||||
|
|
||||
94 | -1, -2, -3 // <= no comma here
|
||||
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
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue