Skip to content

Commit 20728fb

Browse files
camsteffencamsteffen
committed
fix never_loop
1 parent 892cc28 commit 20728fb

File tree

5 files changed

+125
-64
lines changed

5 files changed

+125
-64
lines changed

‎README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,7 @@ name
295295
[needless_return](https://github.com/Manishearth/rust-clippy/wiki#needless_return) | warn | using a return statement like `return expr;` where an expression would suffice
296296
[needless_update](https://github.com/Manishearth/rust-clippy/wiki#needless_update) | warn | using `Foo { ..base }` when there are no missing fields
297297
[neg_multiply](https://github.com/Manishearth/rust-clippy/wiki#neg_multiply) | warn | multiplying integers with -1
298-
[never_loop](https://github.com/Manishearth/rust-clippy/wiki#never_loop) | allow | any loop with an unconditional `break` or `return` statement
298+
[never_loop](https://github.com/Manishearth/rust-clippy/wiki#never_loop) | warn | any loop that will always `break` or `return`
299299
[new_ret_no_self](https://github.com/Manishearth/rust-clippy/wiki#new_ret_no_self) | warn | not returning `Self` in a `new` method
300300
[new_without_default](https://github.com/Manishearth/rust-clippy/wiki#new_without_default) | warn | `fn new() -> Self` method without `Default` implementation
301301
[new_without_default_derive](https://github.com/Manishearth/rust-clippy/wiki#new_without_default_derive) | warn | `fn new() -> Self` without `#[derive]`able `Default` implementation

‎clippy_lints/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
324324
enum_variants::STUTTER,
325325
if_not_else::IF_NOT_ELSE,
326326
items_after_statements::ITEMS_AFTER_STATEMENTS,
327-
loops::NEVER_LOOP,
328327
matches::SINGLE_MATCH_ELSE,
329328
mem_forget::MEM_FORGET,
330329
methods::FILTER_MAP,
@@ -420,6 +419,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
420419
loops::FOR_LOOP_OVER_RESULT,
421420
loops::ITER_NEXT_LOOP,
422421
loops::NEEDLESS_RANGE_LOOP,
422+
loops::NEVER_LOOP,
423423
loops::REVERSE_RANGE_LOOP,
424424
loops::UNUSED_COLLECT,
425425
loops::WHILE_LET_LOOP,

‎clippy_lints/src/loops.rs

Lines changed: 79 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -285,24 +285,22 @@ declare_lint! {
285285
"looping on a map using `iter` when `keys` or `values` would do"
286286
}
287287

288-
/// **What it does:** Checks for loops that contain an unconditional `break`
289-
/// or `return`.
288+
/// **What it does:** Checks for loops that will always `break`, `return` or
289+
/// `continue` an outer loop.
290290
///
291291
/// **Why is this bad?** This loop never loops, all it does is obfuscating the
292292
/// code.
293293
///
294-
/// **Known problems:** Ignores `continue` statements in the loop that create
295-
/// nontrivial control flow. Therefore set to `Allow` by default.
296-
/// See https://github.com/Manishearth/rust-clippy/issues/1586
294+
/// **Known problems:** None
297295
///
298296
/// **Example:**
299297
/// ```rust
300298
/// loop { ..; break; }
301299
/// ```
302300
declare_lint! {
303301
pub NEVER_LOOP,
304-
Allow,
305-
"any loop with an unconditional `break` or `return` statement"
302+
Warn,
303+
"any loop that will always `break` or `return`"
306304
}
307305

308306
#[derive(Copy, Clone)]
@@ -344,7 +342,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
344342
"empty `loop {}` detected. You may want to either use `panic!()` or add \
345343
`std::thread::sleep(..);` to the loop body.");
346344
}
347-
if never_loop_block(block) {
345+
if never_loop(block, &expr.id) {
348346
span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops");
349347
}
350348

@@ -424,47 +422,100 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
424422
}
425423
}
426424

427-
fn never_loop_block(block: &Block) -> bool {
428-
block.stmts.iter().any(never_loop_stmt) || block.expr.as_ref().map_or(false, |e| never_loop_expr(e))
425+
fn never_loop(block: &Block, id: &NodeId) -> bool {
426+
!contains_continue_block(block, id) && loop_exit_block(block)
427+
}
428+
429+
fn contains_continue_block(block: &Block, dest: &NodeId) -> bool {
430+
block.stmts.iter().any(|e| contains_continue_stmt(e, dest))
431+
|| block.expr.as_ref().map_or(false, |e| contains_continue_expr(e, dest))
429432
}
430433

431-
fn never_loop_stmt(stmt: &Stmt) -> bool {
434+
fn contains_continue_stmt(stmt: &Stmt, dest: &NodeId) -> bool {
432435
match stmt.node {
433436
StmtSemi(ref e, _) |
434-
StmtExpr(ref e, _) => never_loop_expr(e),
435-
StmtDecl(ref d, _) => never_loop_decl(d),
437+
StmtExpr(ref e, _) => contains_continue_expr(e, dest),
438+
StmtDecl(ref d, _) => contains_continue_decl(d, dest),
436439
}
437440
}
438441

439-
fn never_loop_decl(decl: &Decl) -> bool {
440-
if let DeclLocal(ref local) = decl.node {
441-
local.init.as_ref().map_or(false, |e| never_loop_expr(e))
442-
} else {
443-
false
442+
fn contains_continue_decl(decl: &Decl, dest: &NodeId) -> bool {
443+
match decl.node {
444+
DeclLocal(ref local) => local.init.as_ref().map_or(false, |e| contains_continue_expr(e, dest)),
445+
_ => false
444446
}
445447
}
446448

447-
fn never_loop_expr(expr: &Expr) -> bool {
449+
fn contains_continue_expr(expr: &Expr, dest: &NodeId) -> bool {
448450
match expr.node {
449-
ExprBreak(..) | ExprRet(..) => true,
450451
ExprBox(ref e) |
451452
ExprUnary(_, ref e) |
452-
ExprBinary(_, ref e, _) | // because short-circuiting
453453
ExprCast(ref e, _) |
454454
ExprType(ref e, _) |
455455
ExprField(ref e, _) |
456456
ExprTupField(ref e, _) |
457-
ExprRepeat(ref e, _) |
458-
ExprAddrOf(_, ref e) => never_loop_expr(e),
457+
ExprAddrOf(_, ref e) |
458+
ExprRepeat(ref e, _) => contains_continue_expr(e, dest),
459+
ExprBinary(_, ref e1, ref e2) |
459460
ExprAssign(ref e1, ref e2) |
460461
ExprAssignOp(_, ref e1, ref e2) |
461-
ExprIndex(ref e1, ref e2) => never_loop_expr(e1) || never_loop_expr(e2),
462+
ExprIndex(ref e1, ref e2) => [e1, e2].iter().any(|e| contains_continue_expr(e, dest)),
462463
ExprArray(ref es) |
463464
ExprTup(ref es) |
464-
ExprMethodCall(_, _, ref es) => es.iter().any(|e| never_loop_expr(e)),
465-
ExprCall(ref e, ref es) => never_loop_expr(e) || es.iter().any(|e| never_loop_expr(e)),
466-
ExprBlock(ref block) => never_loop_block(block),
467-
ExprStruct(_, _, ref base) => base.as_ref().map_or(false, |e| never_loop_expr(e)),
465+
ExprMethodCall(_, _, ref es) => es.iter().any(|e| contains_continue_expr(e, dest)),
466+
ExprCall(ref e, ref es) => contains_continue_expr(e, dest) || es.iter().any(|e| contains_continue_expr(e, dest)),
467+
ExprBlock(ref block) => contains_continue_block(block, dest),
468+
ExprStruct(_, _, ref base) => base.as_ref().map_or(false, |e| contains_continue_expr(e, dest)),
469+
ExprAgain(d) => d.target_id.opt_id().map_or(false, |id| id == *dest),
470+
_ => false,
471+
}
472+
}
473+
474+
fn loop_exit_block(block: &Block) -> bool {
475+
block.stmts.iter().any(|e| loop_exit_stmt(e))
476+
|| block.expr.as_ref().map_or(false, |e| loop_exit_expr(e))
477+
}
478+
479+
fn loop_exit_stmt(stmt: &Stmt) -> bool {
480+
match stmt.node {
481+
StmtSemi(ref e, _) |
482+
StmtExpr(ref e, _) => loop_exit_expr(e),
483+
StmtDecl(ref d, _) => loop_exit_decl(d),
484+
}
485+
}
486+
487+
fn loop_exit_decl(decl: &Decl) -> bool {
488+
match decl.node {
489+
DeclLocal(ref local) => local.init.as_ref().map_or(false, |e| loop_exit_expr(e)),
490+
_ => false
491+
}
492+
}
493+
494+
fn loop_exit_expr(expr: &Expr) -> bool {
495+
match expr.node {
496+
ExprBox(ref e) |
497+
ExprUnary(_, ref e) |
498+
ExprCast(ref e, _) |
499+
ExprType(ref e, _) |
500+
ExprField(ref e, _) |
501+
ExprTupField(ref e, _) |
502+
ExprAddrOf(_, ref e) |
503+
ExprRepeat(ref e, _) => loop_exit_expr(e),
504+
ExprMethodCall(_, _, ref es) => es.iter().any(|e| loop_exit_expr(e)),
505+
ExprArray(ref es) |
506+
ExprTup(ref es) => es.iter().any(|e| loop_exit_expr(e)),
507+
ExprCall(ref e, ref es) => loop_exit_expr(e) || es.iter().any(|e| loop_exit_expr(e)),
508+
ExprBinary(_, ref e1, ref e2) |
509+
ExprAssign(ref e1, ref e2) |
510+
ExprAssignOp(_, ref e1, ref e2) |
511+
ExprIndex(ref e1, ref e2) => [e1, e2].iter().any(|e| loop_exit_expr(e)),
512+
ExprIf(ref e, ref e2, ref e3) => loop_exit_expr(e) || e3.as_ref().map_or(false, |e| loop_exit_expr(e)) && loop_exit_expr(e2),
513+
ExprWhile(ref e, ref b, _) => loop_exit_expr(e) || loop_exit_block(b),
514+
ExprMatch(ref e, ref arms, _) => loop_exit_expr(e) || arms.iter().all(|a| loop_exit_expr(&a.body)),
515+
ExprBlock(ref b) => loop_exit_block(b),
516+
ExprBreak(_, _) |
517+
ExprAgain(_) |
518+
ExprRet(_) => true,
468519
_ => false,
469520
}
470521
}

‎clippy_tests/examples/never_loop.rs

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,33 +2,56 @@
22
#![plugin(clippy)]
33

44
#![warn(never_loop)]
5-
#![allow(dead_code, unused)]
5+
#![allow(single_match, while_true)]
66

7-
fn main() {
7+
fn break_stmt() {
88
loop {
9-
println!("This is only ever printed once");
109
break;
1110
}
11+
}
1212

13-
let x = 1;
13+
fn conditional_break() {
14+
let mut x = 5;
1415
loop {
15-
println!("This, too"); // but that's OK
16+
x -= 1;
1617
if x == 1 {
17-
break;
18+
break
1819
}
1920
}
21+
}
2022

23+
fn nested_loop() {
2124
loop {
22-
loop {
23-
// another one
24-
break;
25+
while true {
26+
break
2527
}
26-
break;
28+
break
2729
}
30+
}
2831

32+
fn if_false() {
33+
let x = 1;
2934
loop {
30-
loop {
31-
if x == 1 { return; }
35+
if x == 1 {
36+
return
3237
}
3338
}
3439
}
40+
41+
fn match_false() {
42+
let x = 1;
43+
loop {
44+
match x {
45+
1 => return,
46+
_ => (),
47+
}
48+
}
49+
}
50+
51+
fn main() {
52+
break_stmt();
53+
conditional_break();
54+
nested_loop();
55+
if_false();
56+
match_false();
57+
}

‎clippy_tests/examples/never_loop.stderr

Lines changed: 9 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,38 +2,25 @@ error: this loop never actually loops
22
--> never_loop.rs:8:5
33
|
44
8 | / loop {
5-
9 | | println!("This is only ever printed once");
6-
10 | | break;
7-
11 | | }
5+
9 | | break;
6+
10 | | }
87
| |_____^
98
|
109
= note: `-D never-loop` implied by `-D warnings`
1110

1211
error: this loop never actually loops
13-
--> never_loop.rs:21:5
12+
--> never_loop.rs:24:5
1413
|
15-
21 | / loop {
16-
22 | | loop {
17-
23 | | // another one
18-
24 | | break;
19-
25 | | }
20-
26 | | break;
21-
27 | | }
14+
24 | / loop {
15+
25 | | while true {
16+
26 | | break
17+
27 | | }
18+
28 | | break
19+
29 | | }
2220
| |_____^
2321
|
2422
= note: `-D never-loop` implied by `-D warnings`
2523

26-
error: this loop never actually loops
27-
--> never_loop.rs:22:9
28-
|
29-
22 | / loop {
30-
23 | | // another one
31-
24 | | break;
32-
25 | | }
33-
| |_________^
34-
|
35-
= note: `-D never-loop` implied by `-D warnings`
36-
3724
error: aborting due to previous error(s)
3825

3926
error: Could not compile `clippy_tests`.

0 commit comments

Comments
 (0)