Skip to content

Commit 6543a09

Browse files
blyxyasblyxyas
authored
Fix never_loop forget to remove break in suggestion (rust-lang#15064)
Closes rust-lang/rust-clippy#15007 Removes the `break` stmt and the dead code after it when appropriate. changelog: [`never_loop`] Make sure to remove `break` in suggestions
2 parents 72243f4 + 8d941d2 commit 6543a09

File tree

3 files changed

+210
-32
lines changed

3 files changed

+210
-32
lines changed

‎clippy_lints/src/loops/never_loop.rs

Lines changed: 110 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@ use clippy_utils::macros::root_macro_call_first_node;
66
use clippy_utils::source::snippet;
77
use clippy_utils::visitors::{Descend, for_each_expr_without_closures};
88
use rustc_errors::Applicability;
9-
use rustc_hir::{Block, Destination, Expr, ExprKind, HirId, InlineAsmOperand, Pat, Stmt, StmtKind, StructTailExpr};
9+
use rustc_hir::{
10+
Block, Destination, Expr, ExprKind, HirId, InlineAsmOperand, Node, Pat, Stmt, StmtKind, StructTailExpr,
11+
};
1012
use rustc_lint::LateContext;
11-
use rustc_span::{Span, sym};
13+
use rustc_span::{BytePos, Span, sym};
1214
use std::iter::once;
1315
use std::ops::ControlFlow;
1416

@@ -20,7 +22,7 @@ pub(super) fn check<'tcx>(
2022
for_loop: Option<&ForLoop<'_>>,
2123
) {
2224
match never_loop_block(cx, block, &mut Vec::new(), loop_id) {
23-
NeverLoopResult::Diverging => {
25+
NeverLoopResult::Diverging { ref break_spans } => {
2426
span_lint_and_then(cx, NEVER_LOOP, span, "this loop never actually loops", |diag| {
2527
if let Some(ForLoop {
2628
arg: iterator,
@@ -38,10 +40,15 @@ pub(super) fn check<'tcx>(
3840
Applicability::Unspecified
3941
};
4042

41-
diag.span_suggestion_verbose(
43+
let mut suggestions = vec![(
4244
for_span.with_hi(iterator.span.hi()),
43-
"if you need the first element of the iterator, try writing",
4445
for_to_if_let_sugg(cx, iterator, pat),
46+
)];
47+
// Make sure to clear up the diverging sites when we remove a loopp.
48+
suggestions.extend(break_spans.iter().map(|span| (*span, String::new())));
49+
diag.multipart_suggestion_verbose(
50+
"if you need the first element of the iterator, try writing",
51+
suggestions,
4552
app,
4653
);
4754
}
@@ -70,22 +77,22 @@ fn contains_any_break_or_continue(block: &Block<'_>) -> bool {
7077
/// The first two bits of information are in this enum, and the last part is in the
7178
/// `local_labels` variable, which contains a list of `(block_id, reachable)` pairs ordered by
7279
/// scope.
73-
#[derive(Copy, Clone)]
80+
#[derive(Clone)]
7481
enum NeverLoopResult {
7582
/// A continue may occur for the main loop.
7683
MayContinueMainLoop,
7784
/// We have not encountered any main loop continue,
7885
/// but we are diverging (subsequent control flow is not reachable)
79-
Diverging,
86+
Diverging { break_spans: Vec<Span> },
8087
/// We have not encountered any main loop continue,
8188
/// and subsequent control flow is (possibly) reachable
8289
Normal,
8390
}
8491

8592
#[must_use]
86-
fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
93+
fn absorb_break(arg: &NeverLoopResult) -> NeverLoopResult {
8794
match arg {
88-
NeverLoopResult::Diverging | NeverLoopResult::Normal => NeverLoopResult::Normal,
95+
NeverLoopResult::Diverging { .. } | NeverLoopResult::Normal => NeverLoopResult::Normal,
8996
NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop,
9097
}
9198
}
@@ -94,7 +101,7 @@ fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
94101
#[must_use]
95102
fn combine_seq(first: NeverLoopResult, second: impl FnOnce() -> NeverLoopResult) -> NeverLoopResult {
96103
match first {
97-
NeverLoopResult::Diverging | NeverLoopResult::MayContinueMainLoop => first,
104+
NeverLoopResult::Diverging { .. } | NeverLoopResult::MayContinueMainLoop => first,
98105
NeverLoopResult::Normal => second(),
99106
}
100107
}
@@ -103,7 +110,7 @@ fn combine_seq(first: NeverLoopResult, second: impl FnOnce() -> NeverLoopResult)
103110
#[must_use]
104111
fn combine_seq_many(iter: impl IntoIterator<Item = NeverLoopResult>) -> NeverLoopResult {
105112
for e in iter {
106-
if let NeverLoopResult::Diverging | NeverLoopResult::MayContinueMainLoop = e {
113+
if let NeverLoopResult::Diverging { .. } | NeverLoopResult::MayContinueMainLoop = e {
107114
return e;
108115
}
109116
}
@@ -118,7 +125,19 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult
118125
NeverLoopResult::MayContinueMainLoop
119126
},
120127
(NeverLoopResult::Normal, _) | (_, NeverLoopResult::Normal) => NeverLoopResult::Normal,
121-
(NeverLoopResult::Diverging, NeverLoopResult::Diverging) => NeverLoopResult::Diverging,
128+
(
129+
NeverLoopResult::Diverging {
130+
break_spans: mut break_spans1,
131+
},
132+
NeverLoopResult::Diverging {
133+
break_spans: mut break_spans2,
134+
},
135+
) => {
136+
break_spans1.append(&mut break_spans2);
137+
NeverLoopResult::Diverging {
138+
break_spans: break_spans1,
139+
}
140+
},
122141
}
123142
}
124143

@@ -136,15 +155,15 @@ fn never_loop_block<'tcx>(
136155
combine_seq_many(iter.map(|(e, els)| {
137156
let e = never_loop_expr(cx, e, local_labels, main_loop_id);
138157
// els is an else block in a let...else binding
139-
els.map_or(e, |els| {
158+
els.map_or(e.clone(), |els| {
140159
combine_seq(e, || match never_loop_block(cx, els, local_labels, main_loop_id) {
141160
// Returning MayContinueMainLoop here means that
142161
// we will not evaluate the rest of the body
143162
NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop,
144163
// An else block always diverges, so the Normal case should not happen,
145164
// but the analysis is approximate so it might return Normal anyway.
146165
// Returning Normal here says that nothing more happens on the main path
147-
NeverLoopResult::Diverging | NeverLoopResult::Normal => NeverLoopResult::Normal,
166+
NeverLoopResult::Diverging { .. } | NeverLoopResult::Normal => NeverLoopResult::Normal,
148167
})
149168
})
150169
}))
@@ -159,6 +178,45 @@ fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<(&'tcx Expr<'tcx>, Option<&'t
159178
}
160179
}
161180

181+
fn stmt_source_span(stmt: &Stmt<'_>) -> Span {
182+
let call_span = stmt.span.source_callsite();
183+
// if it is a macro call, the span will be missing the trailing semicolon
184+
if stmt.span == call_span {
185+
return call_span;
186+
}
187+
188+
// An expression without a trailing semi-colon (must have unit type).
189+
if let StmtKind::Expr(..) = stmt.kind {
190+
return call_span;
191+
}
192+
193+
call_span.with_hi(call_span.hi() + BytePos(1))
194+
}
195+
196+
/// Returns a Vec of all the individual spans after the highlighted expression in a block
197+
fn all_spans_after_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> Vec<Span> {
198+
if let Node::Stmt(stmt) = cx.tcx.parent_hir_node(expr.hir_id) {
199+
if let Node::Block(block) = cx.tcx.parent_hir_node(stmt.hir_id) {
200+
return block
201+
.stmts
202+
.iter()
203+
.skip_while(|inner| inner.hir_id != stmt.hir_id)
204+
.map(stmt_source_span)
205+
.chain(if let Some(e) = block.expr { vec![e.span] } else { vec![] })
206+
.collect();
207+
}
208+
209+
return vec![stmt.span];
210+
}
211+
212+
vec![]
213+
}
214+
215+
fn is_label_for_block(cx: &LateContext<'_>, dest: &Destination) -> bool {
216+
dest.target_id
217+
.is_ok_and(|hir_id| matches!(cx.tcx.hir_node(hir_id), Node::Block(_)))
218+
}
219+
162220
#[allow(clippy::too_many_lines)]
163221
fn never_loop_expr<'tcx>(
164222
cx: &LateContext<'tcx>,
@@ -197,7 +255,7 @@ fn never_loop_expr<'tcx>(
197255
ExprKind::Loop(b, _, _, _) => {
198256
// We don't attempt to track reachability after a loop,
199257
// just assume there may have been a break somewhere
200-
absorb_break(never_loop_block(cx, b, local_labels, main_loop_id))
258+
absorb_break(&never_loop_block(cx, b, local_labels, main_loop_id))
201259
},
202260
ExprKind::If(e, e2, e3) => {
203261
let e1 = never_loop_expr(cx, e, local_labels, main_loop_id);
@@ -212,9 +270,10 @@ fn never_loop_expr<'tcx>(
212270
ExprKind::Match(e, arms, _) => {
213271
let e = never_loop_expr(cx, e, local_labels, main_loop_id);
214272
combine_seq(e, || {
215-
arms.iter().fold(NeverLoopResult::Diverging, |a, b| {
216-
combine_branches(a, never_loop_expr(cx, b.body, local_labels, main_loop_id))
217-
})
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+
})
218277
})
219278
},
220279
ExprKind::Block(b, _) => {
@@ -224,7 +283,7 @@ fn never_loop_expr<'tcx>(
224283
let ret = never_loop_block(cx, b, local_labels, main_loop_id);
225284
let jumped_to = b.targeted_by_break && local_labels.pop().unwrap().1;
226285
match ret {
227-
NeverLoopResult::Diverging if jumped_to => NeverLoopResult::Normal,
286+
NeverLoopResult::Diverging { .. } if jumped_to => NeverLoopResult::Normal,
228287
_ => ret,
229288
}
230289
},
@@ -235,25 +294,39 @@ fn never_loop_expr<'tcx>(
235294
if id == main_loop_id {
236295
NeverLoopResult::MayContinueMainLoop
237296
} else {
238-
NeverLoopResult::Diverging
297+
NeverLoopResult::Diverging {
298+
break_spans: all_spans_after_expr(cx, expr),
299+
}
239300
}
240301
},
241-
ExprKind::Break(_, e) | ExprKind::Ret(e) => {
302+
ExprKind::Ret(e) => {
242303
let first = e.as_ref().map_or(NeverLoopResult::Normal, |e| {
243304
never_loop_expr(cx, e, local_labels, main_loop_id)
244305
});
245306
combine_seq(first, || {
246307
// checks if break targets a block instead of a loop
247-
if let ExprKind::Break(Destination { target_id: Ok(t), .. }, _) = expr.kind
248-
&& let Some((_, reachable)) = local_labels.iter_mut().find(|(label, _)| *label == t)
249-
{
250-
*reachable = true;
308+
mark_block_as_reachable(expr, local_labels);
309+
NeverLoopResult::Diverging { break_spans: vec![] }
310+
})
311+
},
312+
ExprKind::Break(dest, e) => {
313+
let first = e.as_ref().map_or(NeverLoopResult::Normal, |e| {
314+
never_loop_expr(cx, e, local_labels, main_loop_id)
315+
});
316+
combine_seq(first, || {
317+
// checks if break targets a block instead of a loop
318+
mark_block_as_reachable(expr, local_labels);
319+
NeverLoopResult::Diverging {
320+
break_spans: if is_label_for_block(cx, &dest) {
321+
vec![]
322+
} else {
323+
all_spans_after_expr(cx, expr)
324+
},
251325
}
252-
NeverLoopResult::Diverging
253326
})
254327
},
255328
ExprKind::Become(e) => combine_seq(never_loop_expr(cx, e, local_labels, main_loop_id), || {
256-
NeverLoopResult::Diverging
329+
NeverLoopResult::Diverging { break_spans: vec![] }
257330
}),
258331
ExprKind::InlineAsm(asm) => combine_seq_many(asm.operands.iter().map(|(o, _)| match o {
259332
InlineAsmOperand::In { expr, .. } | InlineAsmOperand::InOut { expr, .. } => {
@@ -283,12 +356,12 @@ fn never_loop_expr<'tcx>(
283356
};
284357
let result = combine_seq(result, || {
285358
if cx.typeck_results().expr_ty(expr).is_never() {
286-
NeverLoopResult::Diverging
359+
NeverLoopResult::Diverging { break_spans: vec![] }
287360
} else {
288361
NeverLoopResult::Normal
289362
}
290363
});
291-
if let NeverLoopResult::Diverging = result
364+
if let NeverLoopResult::Diverging { .. } = result
292365
&& let Some(macro_call) = root_macro_call_first_node(cx, expr)
293366
&& let Some(sym::todo_macro) = cx.tcx.get_diagnostic_name(macro_call.def_id)
294367
{
@@ -316,3 +389,11 @@ fn for_to_if_let_sugg(cx: &LateContext<'_>, iterator: &Expr<'_>, pat: &Pat<'_>)
316389

317390
format!("if let Some({pat_snippet}) = {iter_snippet}.next()")
318391
}
392+
393+
fn mark_block_as_reachable(expr: &Expr<'_>, local_labels: &mut [(HirId, bool)]) {
394+
if let ExprKind::Break(Destination { target_id: Ok(t), .. }, _) = expr.kind
395+
&& let Some((_, reachable)) = local_labels.iter_mut().find(|(label, _)| *label == t)
396+
{
397+
*reachable = true;
398+
}
399+
}

‎tests/ui/never_loop.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -466,3 +466,35 @@ fn main() {
466466
test13();
467467
test14();
468468
}
469+
470+
fn issue15059() {
471+
'a: for _ in 0..1 {
472+
//~^ never_loop
473+
break 'a;
474+
}
475+
476+
let mut b = 1;
477+
'a: for i in 0..1 {
478+
//~^ never_loop
479+
match i {
480+
0 => {
481+
b *= 2;
482+
break 'a;
483+
},
484+
x => {
485+
b += x;
486+
break 'a;
487+
},
488+
}
489+
}
490+
491+
#[allow(clippy::unused_unit)]
492+
for v in 0..10 {
493+
//~^ never_loop
494+
break;
495+
println!("{v}");
496+
// This is comment and should be kept
497+
println!("This is a comment");
498+
()
499+
}
500+
}

‎tests/ui/never_loop.stderr

Lines changed: 68 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -176,8 +176,10 @@ LL | | }
176176
|
177177
help: if you need the first element of the iterator, try writing
178178
|
179-
LL - for v in 0..10 {
180-
LL + if let Some(v) = (0..10).next() {
179+
LL ~ if let Some(v) = (0..10).next() {
180+
LL |
181+
LL ~
182+
LL ~
181183
|
182184

183185
error: this loop never actually loops
@@ -232,5 +234,68 @@ LL | | break 'inner;
232234
LL | | }
233235
| |_________^
234236

235-
error: aborting due to 21 previous errors
237+
error: this loop never actually loops
238+
--> tests/ui/never_loop.rs:471:5
239+
|
240+
LL | / 'a: for _ in 0..1 {
241+
LL | |
242+
LL | | break 'a;
243+
LL | | }
244+
| |_____^
245+
|
246+
help: if you need the first element of the iterator, try writing
247+
|
248+
LL ~ if let Some(_) = (0..1).next() {
249+
LL |
250+
LL ~
251+
|
252+
253+
error: this loop never actually loops
254+
--> tests/ui/never_loop.rs:477:5
255+
|
256+
LL | / 'a: for i in 0..1 {
257+
LL | |
258+
LL | | match i {
259+
LL | | 0 => {
260+
... |
261+
LL | | }
262+
| |_____^
263+
|
264+
help: if you need the first element of the iterator, try writing
265+
|
266+
LL ~ if let Some(i) = (0..1).next() {
267+
LL |
268+
...
269+
LL | b *= 2;
270+
LL ~
271+
LL | },
272+
LL | x => {
273+
LL | b += x;
274+
LL ~
275+
|
276+
277+
error: this loop never actually loops
278+
--> tests/ui/never_loop.rs:492:5
279+
|
280+
LL | / for v in 0..10 {
281+
LL | |
282+
LL | | break;
283+
LL | | println!("{v}");
284+
... |
285+
LL | | ()
286+
LL | | }
287+
| |_____^
288+
|
289+
help: if you need the first element of the iterator, try writing
290+
|
291+
LL ~ if let Some(v) = (0..10).next() {
292+
LL |
293+
LL ~
294+
LL ~
295+
LL | // This is comment and should be kept
296+
LL ~
297+
LL ~
298+
|
299+
300+
error: aborting due to 24 previous errors
236301

0 commit comments

Comments
 (0)