Skip to content

[needless_continue]: lint if the last stmt in for/while/loop is `co… #11546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[needless_continue]: lint if the last stmt in for/while/loop is `co… #11546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clippy_lints/src/methods/unnecessary_to_owned.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ fn can_change_type<'a>(cx: &LateContext<'a>, mut expr: &'a Expr<'a>, mut ty: Ty<
for (_, node) in cx.tcx.hir().parent_iter(expr.hir_id) {
match node {
Node::Stmt(_) => return true,
Node::Block(..) => continue,
Node::Block(..) => {}
Node::Item(item) => {
if let ItemKind::Fn(_, _, body_id) = &item.kind
&& let output_ty = return_ty(cx, item.owner_id)
Expand Down
108 changes: 91 additions & 17 deletions clippy_lints/src/needless_continue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
//! This lint is **warn** by default.
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::source::{indent_of, snippet, snippet_block};
use rustc_ast::ast;
use rustc_ast::{ast, Block, Label};
use rustc_lint::{EarlyContext, EarlyLintPass, LintContext};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::Span;
Expand All @@ -46,6 +46,7 @@ declare_clippy_lint! {
/// that contain a `continue` statement in either their main blocks or their
/// `else`-blocks, when omitting the `else`-block possibly with some
/// rearrangement of code can make the code easier to understand.
/// The lint also checks if the last statement in the loop is a `continue`
///
/// ### Why is this bad?
/// Having explicit `else` blocks for `if` statements
Expand Down Expand Up @@ -110,6 +111,45 @@ declare_clippy_lint! {
/// # break;
/// }
/// ```
///
/// ```rust
/// fn foo() -> std::io::ErrorKind { std::io::ErrorKind::NotFound }
/// for _ in 0..10 {
/// match foo() {
/// std::io::ErrorKind::NotFound => {
/// eprintln!("not found");
/// continue
/// }
/// std::io::ErrorKind::TimedOut => {
/// eprintln!("timeout");
/// continue
/// }
/// _ => {
/// eprintln!("other error");
/// continue
/// }
/// }
/// }
/// ```
/// Could be rewritten as
///
///
/// ```rust
/// fn foo() -> std::io::ErrorKind { std::io::ErrorKind::NotFound }
/// for _ in 0..10 {
/// match foo() {
/// std::io::ErrorKind::NotFound => {
/// eprintln!("not found");
/// }
/// std::io::ErrorKind::TimedOut => {
/// eprintln!("timeout");
/// }
/// _ => {
/// eprintln!("other error");
/// }
/// }
/// }
/// ```
#[clippy::version = "pre 1.29.0"]
pub NEEDLESS_CONTINUE,
pedantic,
Expand Down Expand Up @@ -361,24 +401,58 @@ fn suggestion_snippet_for_continue_inside_else(cx: &EarlyContext<'_>, data: &Lin
)
}

fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) {
if_chain! {
if let ast::ExprKind::Loop(loop_block, ..) = &expr.kind;
if let Some(last_stmt) = loop_block.stmts.last();
if let ast::StmtKind::Expr(inner_expr) | ast::StmtKind::Semi(inner_expr) = &last_stmt.kind;
if let ast::ExprKind::Continue(_) = inner_expr.kind;
then {
span_lint_and_help(
cx,
NEEDLESS_CONTINUE,
last_stmt.span,
MSG_REDUNDANT_CONTINUE_EXPRESSION,
None,
DROP_CONTINUE_EXPRESSION_MSG,
);
}
fn check_last_stmt_in_expr<F>(inner_expr: &ast::Expr, func: &F)
where
F: Fn(Option<&ast::Label>, Span),
{
match &inner_expr.kind {
ast::ExprKind::Continue(continue_label) => {
func(continue_label.as_ref(), inner_expr.span);
},
ast::ExprKind::If(_, then_block, else_block) => {
check_last_stmt_in_block(then_block, func);
if let Some(else_block) = else_block {
check_last_stmt_in_expr(else_block, func);
}
},
ast::ExprKind::Match(_, arms) => {
for arm in arms {
check_last_stmt_in_expr(&arm.body, func);
}
},
ast::ExprKind::Block(b, _) => {
check_last_stmt_in_block(b, func);
},
_ => {},
}
}

fn check_last_stmt_in_block<F>(b: &Block, func: &F)
where
F: Fn(Option<&ast::Label>, Span),
{
if let Some(last_stmt) = b.stmts.last() &&
let ast::StmtKind::Expr(inner_expr) | ast::StmtKind::Semi(inner_expr) = &last_stmt.kind {
check_last_stmt_in_expr(inner_expr, func);
}
}

fn check_and_warn(cx: &EarlyContext<'_>, expr: &ast::Expr) {
with_loop_block(expr, |loop_block, label| {
let p = |continue_label: Option<&Label>, span: Span| {
if compare_labels(label, continue_label) {
span_lint_and_help(
cx,
NEEDLESS_CONTINUE,
span,
MSG_REDUNDANT_CONTINUE_EXPRESSION,
None,
DROP_CONTINUE_EXPRESSION_MSG,
);
}
};
check_last_stmt_in_block(loop_block, &p);

for (i, stmt) in loop_block.stmts.iter().enumerate() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it's possible for the same lint to emit two warnings for the following code:

loop {
  if true {

  } else { // redundant `else`
    continue; // redundant `continue`
  }
}
Output 
warning: this `continue` expression is redundant
   --> test.rs:171:13
    |
171 |             continue;
    |             ^^^^^^^^

warning: this `else` block is redundant
   --> test.rs:170:16
    |
170 |           } else {
    |  ________________^
171 | |             continue;
172 | |         }
    | |_________^
    |

I like the other warning a bit more in this case because it suggests removing the whole else block altogether.

@Alexendoo What do you think? Is this fine to ignore for now or would you consider it a potential blocker? Looks like it possibly has a bit of overlap with your suggestion to split up the lint

If this is a good idea and worth fixing in this PR, we could probably just move this check_last_stmt_in_expr call into the loop and guard it behind an i == stmts.len() - 1 check so that it only runs on the last statement, inline the with_if_expr function (it's really short and only used here, anyway) and add continue; in the one lint case that is emitted here so that it skips checking the new case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good catch, we don't want to be linting the same thing twice

The else winning would be preferable, but if it's easier implementation wise to ignore it that's fine too, there's a lint for empty else blocks that would pick it up

with_if_expr(stmt, |if_expr, cond, then_block, else_expr| {
let data = &LintData {
Expand Down
1 change: 0 additions & 1 deletion clippy_lints/src/redundant_else.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ impl EarlyLintPass for RedundantElse {
ExprKind::If(_, next_then, Some(next_els)) => {
then = next_then;
els = next_els;
continue;
},
// else if without else
ExprKind::If(..) => return,
Expand Down
4 changes: 0 additions & 4 deletions clippy_lints/src/transmute/transmute_undefined_repr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,16 @@ pub(super) fn check<'tcx>(
| (ReducedTy::UnorderedFields(from_sub_ty), ReducedTy::OrderedFields(_, Some(to_sub_ty))) => {
from_ty = from_sub_ty;
to_ty = to_sub_ty;
continue;
},
(ReducedTy::OrderedFields(_, Some(from_sub_ty)), ReducedTy::Other(to_sub_ty)) if reduced_tys.to_fat_ptr => {
from_ty = from_sub_ty;
to_ty = to_sub_ty;
continue;
},
(ReducedTy::Other(from_sub_ty), ReducedTy::OrderedFields(_, Some(to_sub_ty)))
if reduced_tys.from_fat_ptr =>
{
from_ty = from_sub_ty;
to_ty = to_sub_ty;
continue;
},

// ptr <-> ptr
Expand All @@ -52,7 +49,6 @@ pub(super) fn check<'tcx>(
{
from_ty = from_sub_ty;
to_ty = to_sub_ty;
continue;
},

// fat ptr <-> (*size, *size)
Expand Down
2 changes: 1 addition & 1 deletion tests/missing-test-files.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ fn explore_directory(dir: &Path) -> Vec<String> {
missing_files.push(path.to_str().unwrap().to_string());
}
},
_ => continue,
_ => {},
};
}
}
Expand Down
39 changes: 39 additions & 0 deletions tests/ui/needless_continue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ fn simple_loop4() {
}
}

fn simple_loop5() {
loop {
println!("bleh");
{ continue }
//~^ ERROR: this `continue` expression is redundant
}
}

mod issue_2329 {
fn condition() -> bool {
unimplemented!()
Expand Down Expand Up @@ -151,3 +159,34 @@ mod issue_2329 {
}
}
}

mod issue_4077 {
fn main() {
'outer: loop {
'inner: loop {
do_something();
if some_expr() {
println!("bar-7");
continue 'outer;
} else if !some_expr() {
println!("bar-8");
continue 'inner;
} else {
println!("bar-9");
continue 'inner;
}
}
}
}

// The contents of these functions are irrelevant, the purpose of this file is
// shown in main.

fn do_something() {
std::process::exit(0);
}

fn some_expr() -> bool {
true
}
}
34 changes: 29 additions & 5 deletions tests/ui/needless_continue.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,15 @@ error: this `continue` expression is redundant
--> $DIR/needless_continue.rs:60:9
|
LL | continue;
| ^^^^^^^^^
| ^^^^^^^^
|
= help: consider dropping the `continue` expression

error: this `continue` expression is redundant
--> $DIR/needless_continue.rs:68:9
|
LL | continue;
| ^^^^^^^^^
| ^^^^^^^^
|
= help: consider dropping the `continue` expression

Expand All @@ -91,8 +91,16 @@ LL | continue
|
= help: consider dropping the `continue` expression

error: this `continue` expression is redundant
--> $DIR/needless_continue.rs:93:11
|
LL | { continue }
| ^^^^^^^^
|
= help: consider dropping the `continue` expression

error: this `else` block is redundant
--> $DIR/needless_continue.rs:136:24
--> $DIR/needless_continue.rs:144:24
|
LL | } else {
| ________________________^
Expand All @@ -117,7 +125,7 @@ LL | | }
}

error: there is no need for an explicit `else` block for this `if` expression
--> $DIR/needless_continue.rs:143:17
--> $DIR/needless_continue.rs:151:17
|
LL | / if condition() {
LL | |
Expand All @@ -136,5 +144,21 @@ LL | | }
println!("bar-5");
}

error: aborting due to 8 previous errors
error: this `continue` expression is redundant
--> $DIR/needless_continue.rs:173:21
|
LL | continue 'inner;
| ^^^^^^^^^^^^^^^
|
= help: consider dropping the `continue` expression

error: this `continue` expression is redundant
--> $DIR/needless_continue.rs:176:21
|
LL | continue 'inner;
| ^^^^^^^^^^^^^^^
|
= help: consider dropping the `continue` expression

error: aborting due to 11 previous errors