Skip to content

Commit 4f18d7f

Browse files
profetiaprofetia
committed
fix: never_loop forget to remove break in suggestion
1 parent 97815d0 commit 4f18d7f

File tree

3 files changed

+180
-26
lines changed

3 files changed

+180
-26
lines changed

‎clippy_lints/src/loops/never_loop.rs

Lines changed: 83 additions & 23 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 inner) => {
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,14 @@ 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+
suggestions.extend(inner.iter().map(|span| (*span, String::new())));
48+
diag.multipart_suggestion_verbose(
49+
"if you need the first element of the iterator, try writing",
50+
suggestions,
4551
app,
4652
);
4753
}
@@ -70,22 +76,22 @@ fn contains_any_break_or_continue(block: &Block<'_>) -> bool {
7076
/// The first two bits of information are in this enum, and the last part is in the
7177
/// `local_labels` variable, which contains a list of `(block_id, reachable)` pairs ordered by
7278
/// scope.
73-
#[derive(Copy, Clone)]
79+
#[derive(Clone)]
7480
enum NeverLoopResult {
7581
/// A continue may occur for the main loop.
7682
MayContinueMainLoop,
7783
/// We have not encountered any main loop continue,
7884
/// but we are diverging (subsequent control flow is not reachable)
79-
Diverging,
85+
Diverging(Vec<Span>),
8086
/// We have not encountered any main loop continue,
8187
/// and subsequent control flow is (possibly) reachable
8288
Normal,
8389
}
8490

8591
#[must_use]
86-
fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
92+
fn absorb_break(arg: &NeverLoopResult) -> NeverLoopResult {
8793
match arg {
88-
NeverLoopResult::Diverging | NeverLoopResult::Normal => NeverLoopResult::Normal,
94+
NeverLoopResult::Diverging(..) | NeverLoopResult::Normal => NeverLoopResult::Normal,
8995
NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop,
9096
}
9197
}
@@ -94,7 +100,7 @@ fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
94100
#[must_use]
95101
fn combine_seq(first: NeverLoopResult, second: impl FnOnce() -> NeverLoopResult) -> NeverLoopResult {
96102
match first {
97-
NeverLoopResult::Diverging | NeverLoopResult::MayContinueMainLoop => first,
103+
NeverLoopResult::Diverging(..) | NeverLoopResult::MayContinueMainLoop => first,
98104
NeverLoopResult::Normal => second(),
99105
}
100106
}
@@ -103,7 +109,7 @@ fn combine_seq(first: NeverLoopResult, second: impl FnOnce() -> NeverLoopResult)
103109
#[must_use]
104110
fn combine_seq_many(iter: impl IntoIterator<Item = NeverLoopResult>) -> NeverLoopResult {
105111
for e in iter {
106-
if let NeverLoopResult::Diverging | NeverLoopResult::MayContinueMainLoop = e {
112+
if let NeverLoopResult::Diverging(..) | NeverLoopResult::MayContinueMainLoop = e {
107113
return e;
108114
}
109115
}
@@ -118,7 +124,10 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult
118124
NeverLoopResult::MayContinueMainLoop
119125
},
120126
(NeverLoopResult::Normal, _) | (_, NeverLoopResult::Normal) => NeverLoopResult::Normal,
121-
(NeverLoopResult::Diverging, NeverLoopResult::Diverging) => NeverLoopResult::Diverging,
127+
(NeverLoopResult::Diverging(mut inner1), NeverLoopResult::Diverging(mut inner2)) => {
128+
inner1.append(&mut inner2);
129+
NeverLoopResult::Diverging(inner1)
130+
},
122131
}
123132
}
124133

@@ -136,15 +145,15 @@ fn never_loop_block<'tcx>(
136145
combine_seq_many(iter.map(|(e, els)| {
137146
let e = never_loop_expr(cx, e, local_labels, main_loop_id);
138147
// els is an else block in a let...else binding
139-
els.map_or(e, |els| {
148+
els.map_or(e.clone(), |els| {
140149
combine_seq(e, || match never_loop_block(cx, els, local_labels, main_loop_id) {
141150
// Returning MayContinueMainLoop here means that
142151
// we will not evaluate the rest of the body
143152
NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop,
144153
// An else block always diverges, so the Normal case should not happen,
145154
// but the analysis is approximate so it might return Normal anyway.
146155
// Returning Normal here says that nothing more happens on the main path
147-
NeverLoopResult::Diverging | NeverLoopResult::Normal => NeverLoopResult::Normal,
156+
NeverLoopResult::Diverging(..) | NeverLoopResult::Normal => NeverLoopResult::Normal,
148157
})
149158
})
150159
}))
@@ -159,6 +168,38 @@ fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<(&'tcx Expr<'tcx>, Option<&'t
159168
}
160169
}
161170

171+
fn stmt_source_span(stmt: &Stmt<'_>) -> Span {
172+
let call_span = stmt.span.source_callsite();
173+
// if it is a macro call, the span will missing the trailing semicolon
174+
if stmt.span == call_span {
175+
return call_span;
176+
}
177+
call_span.with_hi(call_span.hi() + BytePos(1))
178+
}
179+
180+
fn expr_find_remove_span(cx: &LateContext<'_>, expr: &Expr<'_>) -> Vec<Span> {
181+
if let Node::Stmt(stmt) = cx.tcx.parent_hir_node(expr.hir_id) {
182+
if let Node::Block(block) = cx.tcx.parent_hir_node(stmt.hir_id) {
183+
return block
184+
.stmts
185+
.iter()
186+
.skip_while(|inner| inner.hir_id != stmt.hir_id)
187+
.map(stmt_source_span)
188+
.chain(if let Some(e) = block.expr { vec![e.span] } else { vec![] })
189+
.collect();
190+
}
191+
192+
return vec![stmt.span];
193+
}
194+
195+
vec![]
196+
}
197+
198+
fn is_label_for_block(cx: &LateContext<'_>, dest: &Destination) -> bool {
199+
dest.target_id
200+
.is_ok_and(|hir_id| matches!(cx.tcx.hir_node(hir_id), Node::Block(_)))
201+
}
202+
162203
#[allow(clippy::too_many_lines)]
163204
fn never_loop_expr<'tcx>(
164205
cx: &LateContext<'tcx>,
@@ -197,7 +238,7 @@ fn never_loop_expr<'tcx>(
197238
ExprKind::Loop(b, _, _, _) => {
198239
// We don't attempt to track reachability after a loop,
199240
// just assume there may have been a break somewhere
200-
absorb_break(never_loop_block(cx, b, local_labels, main_loop_id))
241+
absorb_break(&never_loop_block(cx, b, local_labels, main_loop_id))
201242
},
202243
ExprKind::If(e, e2, e3) => {
203244
let e1 = never_loop_expr(cx, e, local_labels, main_loop_id);
@@ -212,7 +253,7 @@ fn never_loop_expr<'tcx>(
212253
ExprKind::Match(e, arms, _) => {
213254
let e = never_loop_expr(cx, e, local_labels, main_loop_id);
214255
combine_seq(e, || {
215-
arms.iter().fold(NeverLoopResult::Diverging, |a, b| {
256+
arms.iter().fold(NeverLoopResult::Diverging(vec![]), |a, b| {
216257
combine_branches(a, never_loop_expr(cx, b.body, local_labels, main_loop_id))
217258
})
218259
})
@@ -224,7 +265,7 @@ fn never_loop_expr<'tcx>(
224265
let ret = never_loop_block(cx, b, local_labels, main_loop_id);
225266
let jumped_to = b.targeted_by_break && local_labels.pop().unwrap().1;
226267
match ret {
227-
NeverLoopResult::Diverging if jumped_to => NeverLoopResult::Normal,
268+
NeverLoopResult::Diverging(..) if jumped_to => NeverLoopResult::Normal,
228269
_ => ret,
229270
}
230271
},
@@ -235,10 +276,10 @@ fn never_loop_expr<'tcx>(
235276
if id == main_loop_id {
236277
NeverLoopResult::MayContinueMainLoop
237278
} else {
238-
NeverLoopResult::Diverging
279+
NeverLoopResult::Diverging(expr_find_remove_span(cx, expr))
239280
}
240281
},
241-
ExprKind::Break(_, e) | ExprKind::Ret(e) => {
282+
ExprKind::Ret(e) => {
242283
let first = e.as_ref().map_or(NeverLoopResult::Normal, |e| {
243284
never_loop_expr(cx, e, local_labels, main_loop_id)
244285
});
@@ -249,11 +290,30 @@ fn never_loop_expr<'tcx>(
249290
{
250291
*reachable = true;
251292
}
252-
NeverLoopResult::Diverging
293+
NeverLoopResult::Diverging(vec![])
294+
})
295+
},
296+
ExprKind::Break(dest, e) => {
297+
let first = e.as_ref().map_or(NeverLoopResult::Normal, |e| {
298+
never_loop_expr(cx, e, local_labels, main_loop_id)
299+
});
300+
combine_seq(first, || {
301+
// checks if break targets a block instead of a loop
302+
if let ExprKind::Break(Destination { target_id: Ok(t), .. }, _) = expr.kind
303+
&& let Some((_, reachable)) = local_labels.iter_mut().find(|(label, _)| *label == t)
304+
{
305+
*reachable = true;
306+
}
307+
308+
NeverLoopResult::Diverging(if is_label_for_block(cx, &dest) {
309+
vec![]
310+
} else {
311+
expr_find_remove_span(cx, expr)
312+
})
253313
})
254314
},
255315
ExprKind::Become(e) => combine_seq(never_loop_expr(cx, e, local_labels, main_loop_id), || {
256-
NeverLoopResult::Diverging
316+
NeverLoopResult::Diverging(vec![])
257317
}),
258318
ExprKind::InlineAsm(asm) => combine_seq_many(asm.operands.iter().map(|(o, _)| match o {
259319
InlineAsmOperand::In { expr, .. } | InlineAsmOperand::InOut { expr, .. } => {
@@ -283,12 +343,12 @@ fn never_loop_expr<'tcx>(
283343
};
284344
let result = combine_seq(result, || {
285345
if cx.typeck_results().expr_ty(expr).is_never() {
286-
NeverLoopResult::Diverging
346+
NeverLoopResult::Diverging(vec![])
287347
} else {
288348
NeverLoopResult::Normal
289349
}
290350
});
291-
if let NeverLoopResult::Diverging = result
351+
if let NeverLoopResult::Diverging(..) = result
292352
&& let Some(macro_call) = root_macro_call_first_node(cx, expr)
293353
&& let Some(sym::todo_macro) = cx.tcx.get_diagnostic_name(macro_call.def_id)
294354
{

‎tests/ui/never_loop.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -466,3 +466,33 @@ 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+
for v in 0..10 {
492+
//~^ never_loop
493+
break;
494+
println!("{v}");
495+
// This is comment and should be kept
496+
println!("This is a comment");
497+
}
498+
}

‎tests/ui/never_loop.stderr

Lines changed: 67 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,67 @@ 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:491:5
279+
|
280+
LL | / for v in 0..10 {
281+
LL | |
282+
LL | | break;
283+
LL | | println!("{v}");
284+
LL | | // This is comment and should be kept
285+
LL | | println!("This is a comment");
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+
|
298+
299+
error: aborting due to 24 previous errors
236300

0 commit comments

Comments
 (0)