Skip to content

Commit 828ee51

Browse files
blyxyasblyxyas
authored
Fix never_loop forget to remove break in nested loop (#15356)
Closes rust-lang/rust-clippy#15350 Follow up of rust-lang/rust-clippy#15064 changelog: [`never_loop`] add to remove `break` in nested loop
2 parents e9b7045 + 40ff076 commit 828ee51

File tree

5 files changed

+170
-14
lines changed

5 files changed

+170
-14
lines changed

‎clippy_lints/src/loops/never_loop.rs‎

Lines changed: 48 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@ pub(super) fn check<'tcx>(
2222
for_loop: Option<&ForLoop<'_>>,
2323
) {
2424
match never_loop_block(cx, block, &mut Vec::new(), loop_id) {
25-
NeverLoopResult::Diverging { ref break_spans } => {
25+
NeverLoopResult::Diverging {
26+
ref break_spans,
27+
ref never_spans,
28+
} => {
2629
span_lint_and_then(cx, NEVER_LOOP, span, "this loop never actually loops", |diag| {
2730
if let Some(ForLoop {
2831
arg: iterator,
@@ -34,12 +37,16 @@ pub(super) fn check<'tcx>(
3437
{
3538
// If the block contains a break or continue, or if the loop has a label, `MachineApplicable` is not
3639
// appropriate.
37-
let app = if !contains_any_break_or_continue(block) && label.is_none() {
40+
let mut app = if !contains_any_break_or_continue(block) && label.is_none() {
3841
Applicability::MachineApplicable
3942
} else {
4043
Applicability::Unspecified
4144
};
4245

46+
if !never_spans.is_empty() {
47+
app = Applicability::HasPlaceholders;
48+
}
49+
4350
let mut suggestions = vec![(
4451
for_span.with_hi(iterator.span.hi()),
4552
for_to_if_let_sugg(cx, iterator, pat),
@@ -51,6 +58,13 @@ pub(super) fn check<'tcx>(
5158
suggestions,
5259
app,
5360
);
61+
62+
for span in never_spans {
63+
diag.span_help(
64+
*span,
65+
"this code is unreachable. Consider moving the reachable parts out",
66+
);
67+
}
5468
}
5569
});
5670
},
@@ -77,13 +91,16 @@ fn contains_any_break_or_continue(block: &Block<'_>) -> bool {
7791
/// The first two bits of information are in this enum, and the last part is in the
7892
/// `local_labels` variable, which contains a list of `(block_id, reachable)` pairs ordered by
7993
/// scope.
80-
#[derive(Clone)]
94+
#[derive(Clone, Debug)]
8195
enum NeverLoopResult {
8296
/// A continue may occur for the main loop.
8397
MayContinueMainLoop,
8498
/// We have not encountered any main loop continue,
8599
/// but we are diverging (subsequent control flow is not reachable)
86-
Diverging { break_spans: Vec<Span> },
100+
Diverging {
101+
break_spans: Vec<Span>,
102+
never_spans: Vec<Span>,
103+
},
87104
/// We have not encountered any main loop continue,
88105
/// and subsequent control flow is (possibly) reachable
89106
Normal,
@@ -128,14 +145,18 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult
128145
(
129146
NeverLoopResult::Diverging {
130147
break_spans: mut break_spans1,
148+
never_spans: mut never_spans1,
131149
},
132150
NeverLoopResult::Diverging {
133151
break_spans: mut break_spans2,
152+
never_spans: mut never_spans2,
134153
},
135154
) => {
136155
break_spans1.append(&mut break_spans2);
156+
never_spans1.append(&mut never_spans2);
137157
NeverLoopResult::Diverging {
138158
break_spans: break_spans1,
159+
never_spans: never_spans1,
139160
}
140161
},
141162
}
@@ -207,6 +228,8 @@ fn all_spans_after_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> Vec<Span> {
207228
}
208229

209230
return vec![stmt.span];
231+
} else if let Node::Block(_) = cx.tcx.parent_hir_node(expr.hir_id) {
232+
return vec![expr.span];
210233
}
211234

212235
vec![]
@@ -270,10 +293,13 @@ fn never_loop_expr<'tcx>(
270293
ExprKind::Match(e, arms, _) => {
271294
let e = never_loop_expr(cx, e, local_labels, main_loop_id);
272295
combine_seq(e, || {
273-
arms.iter()
274-
.fold(NeverLoopResult::Diverging { break_spans: vec![] }, |a, b| {
275-
combine_branches(a, never_loop_expr(cx, b.body, local_labels, main_loop_id))
276-
})
296+
arms.iter().fold(
297+
NeverLoopResult::Diverging {
298+
break_spans: vec![],
299+
never_spans: vec![],
300+
},
301+
|a, b| combine_branches(a, never_loop_expr(cx, b.body, local_labels, main_loop_id)),
302+
)
277303
})
278304
},
279305
ExprKind::Block(b, _) => {
@@ -296,6 +322,7 @@ fn never_loop_expr<'tcx>(
296322
} else {
297323
NeverLoopResult::Diverging {
298324
break_spans: all_spans_after_expr(cx, expr),
325+
never_spans: vec![],
299326
}
300327
}
301328
},
@@ -306,7 +333,10 @@ fn never_loop_expr<'tcx>(
306333
combine_seq(first, || {
307334
// checks if break targets a block instead of a loop
308335
mark_block_as_reachable(expr, local_labels);
309-
NeverLoopResult::Diverging { break_spans: vec![] }
336+
NeverLoopResult::Diverging {
337+
break_spans: vec![],
338+
never_spans: vec![],
339+
}
310340
})
311341
},
312342
ExprKind::Break(dest, e) => {
@@ -322,11 +352,15 @@ fn never_loop_expr<'tcx>(
322352
} else {
323353
all_spans_after_expr(cx, expr)
324354
},
355+
never_spans: vec![],
325356
}
326357
})
327358
},
328359
ExprKind::Become(e) => combine_seq(never_loop_expr(cx, e, local_labels, main_loop_id), || {
329-
NeverLoopResult::Diverging { break_spans: vec![] }
360+
NeverLoopResult::Diverging {
361+
break_spans: vec![],
362+
never_spans: vec![],
363+
}
330364
}),
331365
ExprKind::InlineAsm(asm) => combine_seq_many(asm.operands.iter().map(|(o, _)| match o {
332366
InlineAsmOperand::In { expr, .. } | InlineAsmOperand::InOut { expr, .. } => {
@@ -356,7 +390,10 @@ fn never_loop_expr<'tcx>(
356390
};
357391
let result = combine_seq(result, || {
358392
if cx.typeck_results().expr_ty(expr).is_never() {
359-
NeverLoopResult::Diverging { break_spans: vec![] }
393+
NeverLoopResult::Diverging {
394+
break_spans: vec![],
395+
never_spans: all_spans_after_expr(cx, expr),
396+
}
360397
} else {
361398
NeverLoopResult::Normal
362399
}

‎tests/ui/never_loop.rs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,3 +498,27 @@ fn issue15059() {
498498
()
499499
}
500500
}
501+
502+
fn issue15350() {
503+
'bar: for _ in 0..100 {
504+
//~^ never_loop
505+
loop {
506+
//~^ never_loop
507+
println!("This will still run");
508+
break 'bar;
509+
}
510+
}
511+
512+
'foo: for _ in 0..100 {
513+
//~^ never_loop
514+
loop {
515+
//~^ never_loop
516+
println!("This will still run");
517+
loop {
518+
//~^ never_loop
519+
println!("This will still run");
520+
break 'foo;
521+
}
522+
}
523+
}
524+
}

‎tests/ui/never_loop.stderr‎

Lines changed: 96 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,19 @@ LL | | return;
193193
LL | | }
194194
| |_____^
195195
|
196+
help: this code is unreachable. Consider moving the reachable parts out
197+
--> tests/ui/never_loop.rs:436:9
198+
|
199+
LL | / loop {
200+
LL | |
201+
LL | | break 'outer;
202+
LL | | }
203+
| |_________^
204+
help: this code is unreachable. Consider moving the reachable parts out
205+
--> tests/ui/never_loop.rs:440:9
206+
|
207+
LL | return;
208+
| ^^^^^^^
196209
help: if you need the first element of the iterator, try writing
197210
|
198211
LL - 'outer: for v in 0..10 {
@@ -297,5 +310,87 @@ LL ~
297310
LL ~
298311
|
299312

300-
error: aborting due to 24 previous errors
313+
error: this loop never actually loops
314+
--> tests/ui/never_loop.rs:503:5
315+
|
316+
LL | / 'bar: for _ in 0..100 {
317+
LL | |
318+
LL | | loop {
319+
... |
320+
LL | | }
321+
| |_____^
322+
|
323+
help: this code is unreachable. Consider moving the reachable parts out
324+
--> tests/ui/never_loop.rs:505:9
325+
|
326+
LL | / loop {
327+
LL | |
328+
LL | | println!("This will still run");
329+
LL | | break 'bar;
330+
LL | | }
331+
| |_________^
332+
help: if you need the first element of the iterator, try writing
333+
|
334+
LL - 'bar: for _ in 0..100 {
335+
LL + if let Some(_) = (0..100).next() {
336+
|
337+
338+
error: this loop never actually loops
339+
--> tests/ui/never_loop.rs:505:9
340+
|
341+
LL | / loop {
342+
LL | |
343+
LL | | println!("This will still run");
344+
LL | | break 'bar;
345+
LL | | }
346+
| |_________^
347+
348+
error: this loop never actually loops
349+
--> tests/ui/never_loop.rs:512:5
350+
|
351+
LL | / 'foo: for _ in 0..100 {
352+
LL | |
353+
LL | | loop {
354+
... |
355+
LL | | }
356+
| |_____^
357+
|
358+
help: this code is unreachable. Consider moving the reachable parts out
359+
--> tests/ui/never_loop.rs:514:9
360+
|
361+
LL | / loop {
362+
LL | |
363+
LL | | println!("This will still run");
364+
LL | | loop {
365+
... |
366+
LL | | }
367+
| |_________^
368+
help: if you need the first element of the iterator, try writing
369+
|
370+
LL - 'foo: for _ in 0..100 {
371+
LL + if let Some(_) = (0..100).next() {
372+
|
373+
374+
error: this loop never actually loops
375+
--> tests/ui/never_loop.rs:514:9
376+
|
377+
LL | / loop {
378+
LL | |
379+
LL | | println!("This will still run");
380+
LL | | loop {
381+
... |
382+
LL | | }
383+
| |_________^
384+
385+
error: this loop never actually loops
386+
--> tests/ui/never_loop.rs:517:13
387+
|
388+
LL | / loop {
389+
LL | |
390+
LL | | println!("This will still run");
391+
LL | | break 'foo;
392+
LL | | }
393+
| |_____________^
394+
395+
error: aborting due to 29 previous errors
301396

‎tests/ui/never_loop_fixable.fixed‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#![allow(clippy::iter_next_slice, clippy::needless_return)]
1+
#![allow(clippy::iter_next_slice, clippy::needless_return, clippy::redundant_pattern_matching)]
22

33
fn no_break_or_continue_loop() {
44
if let Some(i) = [1, 2, 3].iter().next() {

‎tests/ui/never_loop_fixable.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#![allow(clippy::iter_next_slice, clippy::needless_return)]
1+
#![allow(clippy::iter_next_slice, clippy::needless_return, clippy::redundant_pattern_matching)]
22

33
fn no_break_or_continue_loop() {
44
for i in [1, 2, 3].iter() {

0 commit comments

Comments
 (0)