Fix needless_continue false positive
This commit is contained in:
Taiki Endo
parent
1cdac4a9c7
commit
1bc7da2fec
@ -175,19 +175,19 @@ fn check_expr(&mut self, ctx: &EarlyContext<'_>, expr: &ast::Expr) {
|
||||
/// - The expression is a `continue` node.
|
||||
/// - The expression node is a block with the first statement being a
|
||||
/// `continue`.
|
||||
fn needless_continue_in_else(else_expr: &ast::Expr) -> bool {
|
||||
fn needless_continue_in_else(else_expr: &ast::Expr, label: Option<&ast::Label>) -> bool {
|
||||
match else_expr.node {
|
||||
ast::ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block),
|
||||
ast::ExprKind::Continue(_) => true,
|
||||
ast::ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block, label),
|
||||
ast::ExprKind::Continue(l) => compare_labels(label, l.as_ref()),
|
||||
_ => false,
|
||||
}
|
||||
}
|
||||
|
||||
fn is_first_block_stmt_continue(block: &ast::Block) -> bool {
|
||||
fn is_first_block_stmt_continue(block: &ast::Block, label: Option<&ast::Label>) -> bool {
|
||||
block.stmts.get(0).map_or(false, |stmt| match stmt.node {
|
||||
ast::StmtKind::Semi(ref e) | ast::StmtKind::Expr(ref e) => {
|
||||
if let ast::ExprKind::Continue(_) = e.node {
|
||||
true
|
||||
if let ast::ExprKind::Continue(ref l) = e.node {
|
||||
compare_labels(label, l.as_ref())
|
||||
} else {
|
||||
false
|
||||
}
|
||||
@ -196,17 +196,29 @@ fn is_first_block_stmt_continue(block: &ast::Block) -> bool {
|
||||
})
|
||||
}
|
||||
|
||||
/// If the `continue` has a label, check it matches the label of the loop.
|
||||
fn compare_labels(loop_label: Option<&ast::Label>, continue_label: Option<&ast::Label>) -> bool {
|
||||
match (loop_label, continue_label) {
|
||||
// `loop { continue; }` or `'a loop { continue; }`
|
||||
(_, None) => true,
|
||||
// `loop { continue 'a; }`
|
||||
(None, _) => false,
|
||||
// `'a loop { continue 'a; }` or `'a loop { continue 'b; }`
|
||||
(Some(x), Some(y)) => x.ident == y.ident,
|
||||
}
|
||||
}
|
||||
|
||||
/// If `expr` is a loop expression (while/while let/for/loop), calls `func` with
|
||||
/// the AST object representing the loop block of `expr`.
|
||||
fn with_loop_block<F>(expr: &ast::Expr, mut func: F)
|
||||
where
|
||||
F: FnMut(&ast::Block),
|
||||
F: FnMut(&ast::Block, Option<&ast::Label>),
|
||||
{
|
||||
match expr.node {
|
||||
ast::ExprKind::While(_, ref loop_block, _)
|
||||
| ast::ExprKind::WhileLet(_, _, ref loop_block, _)
|
||||
| ast::ExprKind::ForLoop(_, _, ref loop_block, _)
|
||||
| ast::ExprKind::Loop(ref loop_block, _) => func(loop_block),
|
||||
ast::ExprKind::While(_, ref loop_block, ref label)
|
||||
| ast::ExprKind::WhileLet(_, _, ref loop_block, ref label)
|
||||
| ast::ExprKind::ForLoop(_, _, ref loop_block, ref label)
|
||||
| ast::ExprKind::Loop(ref loop_block, ref label) => func(loop_block, label.as_ref()),
|
||||
_ => {},
|
||||
}
|
||||
}
|
||||
@ -344,7 +356,7 @@ fn suggestion_snippet_for_continue_inside_else<'a>(
|
||||
}
|
||||
|
||||
fn check_and_warn<'a>(ctx: &EarlyContext<'_>, expr: &'a ast::Expr) {
|
||||
with_loop_block(expr, |loop_block| {
|
||||
with_loop_block(expr, |loop_block, label| {
|
||||
for (i, stmt) in loop_block.stmts.iter().enumerate() {
|
||||
with_if_expr(stmt, |if_expr, cond, then_block, else_expr| {
|
||||
let data = &LintData {
|
||||
@ -355,14 +367,14 @@ fn check_and_warn<'a>(ctx: &EarlyContext<'_>, expr: &'a ast::Expr) {
|
||||
else_expr,
|
||||
block_stmts: &loop_block.stmts,
|
||||
};
|
||||
if needless_continue_in_else(else_expr) {
|
||||
if needless_continue_in_else(else_expr, label) {
|
||||
emit_warning(
|
||||
ctx,
|
||||
data,
|
||||
DROP_ELSE_BLOCK_AND_MERGE_MSG,
|
||||
LintType::ContinueInsideElseBlock,
|
||||
);
|
||||
} else if is_first_block_stmt_continue(then_block) {
|
||||
} else if is_first_block_stmt_continue(then_block, label) {
|
||||
emit_warning(ctx, data, DROP_ELSE_BLOCK_MSG, LintType::ContinueInsideThenBlock);
|
||||
}
|
||||
});
|
||||
|
||||
@ -1,3 +1,5 @@
|
||||
#![warn(clippy::needless_continue)]
|
||||
|
||||
macro_rules! zero {
|
||||
($x:expr) => {
|
||||
$x == 0
|
||||
@ -10,7 +12,6 @@ macro_rules! nonzero {
|
||||
};
|
||||
}
|
||||
|
||||
#[warn(clippy::needless_continue)]
|
||||
fn main() {
|
||||
let mut i = 1;
|
||||
while i < 10 {
|
||||
@ -49,3 +50,66 @@ fn main() {
|
||||
println!("bleh");
|
||||
}
|
||||
}
|
||||
|
||||
mod issue_2329 {
|
||||
fn condition() -> bool {
|
||||
unimplemented!()
|
||||
}
|
||||
fn update_condition() {}
|
||||
|
||||
// only the outer loop has a label
|
||||
fn foo() {
|
||||
'outer: loop {
|
||||
println!("Entry");
|
||||
while condition() {
|
||||
update_condition();
|
||||
if condition() {
|
||||
println!("foo-1");
|
||||
} else {
|
||||
continue 'outer; // should not lint here
|
||||
}
|
||||
println!("foo-2");
|
||||
|
||||
update_condition();
|
||||
if condition() {
|
||||
continue 'outer; // should not lint here
|
||||
} else {
|
||||
println!("foo-3");
|
||||
}
|
||||
println!("foo-4");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// both loops have labels
|
||||
fn bar() {
|
||||
'outer: loop {
|
||||
println!("Entry");
|
||||
'inner: while condition() {
|
||||
update_condition();
|
||||
if condition() {
|
||||
println!("bar-1");
|
||||
} else {
|
||||
continue 'outer; // should not lint here
|
||||
}
|
||||
println!("bar-2");
|
||||
|
||||
update_condition();
|
||||
if condition() {
|
||||
println!("bar-3");
|
||||
} else {
|
||||
continue 'inner; // should lint here
|
||||
}
|
||||
println!("bar-4");
|
||||
|
||||
update_condition();
|
||||
if condition() {
|
||||
continue; // should lint here
|
||||
} else {
|
||||
println!("bar-5");
|
||||
}
|
||||
println!("bar-6");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@ -1,6 +1,6 @@
|
||||
error: This else block is redundant.
|
||||
|
||||
--> $DIR/needless_continue.rs:27:16
|
||||
--> $DIR/needless_continue.rs:28:16
|
||||
|
|
||||
LL | } else {
|
||||
| ________________^
|
||||
@ -37,7 +37,7 @@ LL | | }
|
||||
|
||||
error: There is no need for an explicit `else` block for this `if` expression
|
||||
|
||||
--> $DIR/needless_continue.rs:42:9
|
||||
--> $DIR/needless_continue.rs:43:9
|
||||
|
|
||||
LL | / if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 {
|
||||
LL | | continue;
|
||||
@ -55,5 +55,47 @@ LL | | }
|
||||
println!("Jabber");
|
||||
...
|
||||
|
||||
error: aborting due to 2 previous errors
|
||||
error: This else block is redundant.
|
||||
|
||||
--> $DIR/needless_continue.rs:100:24
|
||||
|
|
||||
LL | } else {
|
||||
| ________________________^
|
||||
LL | | continue 'inner; // should lint here
|
||||
LL | | }
|
||||
| |_________________^
|
||||
|
|
||||
= help: Consider dropping the else clause and merging the code that follows (in the loop) with the if block, like so:
|
||||
if condition() {
|
||||
println!("bar-3");
|
||||
// Merged code follows...println!("bar-4");
|
||||
update_condition();
|
||||
if condition() {
|
||||
continue; // should lint here
|
||||
} else {
|
||||
println!("bar-5");
|
||||
}
|
||||
println!("bar-6");
|
||||
}
|
||||
|
||||
|
||||
error: There is no need for an explicit `else` block for this `if` expression
|
||||
|
||||
--> $DIR/needless_continue.rs:106:17
|
||||
|
|
||||
LL | / if condition() {
|
||||
LL | | continue; // should lint here
|
||||
LL | | } else {
|
||||
LL | | println!("bar-5");
|
||||
LL | | }
|
||||
| |_________________^
|
||||
|
|
||||
= help: Consider dropping the else clause, and moving out the code in the else block, like so:
|
||||
if condition() {
|
||||
continue;
|
||||
}
|
||||
println!("bar-5");
|
||||
...
|
||||
|
||||
error: aborting due to 4 previous errors
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user