Skip to content

Commit 1c9c34d

Browse files
borsbors
committed
Auto merge of #9858 - DesmondWillowbrook:never_loop, r=dswij
`never_loop`: don't emit AlwaysBreaks if it targets a block ref: #9837 (comment) The previous fix (#9837) was too simple and ignored all break commands inside a labelled block, regardless of whether their destination was a labelled block or a loop. This fix tracks all the labelled blocks in scope to ensure that only breaks targeting loops are considered. changelog: [`never_loop`]: prevent false negatives from `breaks` nested in labelled blocks
2 parents dac600e + bc3cd34 commit 1c9c34d

File tree

3 files changed

+94
-46
lines changed

3 files changed

+94
-46
lines changed

‎clippy_lints/src/loops/never_loop.rs‎

Lines changed: 71 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use clippy_utils::diagnostics::span_lint_and_then;
44
use clippy_utils::higher::ForLoop;
55
use clippy_utils::source::snippet;
66
use rustc_errors::Applicability;
7-
use rustc_hir::{Block, Expr, ExprKind, HirId, InlineAsmOperand, Pat, Stmt, StmtKind};
7+
use rustc_hir::{Block, Destination, Expr, ExprKind, HirId, InlineAsmOperand, Pat, Stmt, StmtKind};
88
use rustc_lint::LateContext;
99
use rustc_span::Span;
1010
use std::iter::{once, Iterator};
@@ -16,7 +16,7 @@ pub(super) fn check(
1616
span: Span,
1717
for_loop: Option<&ForLoop<'_>>,
1818
) {
19-
match never_loop_block(block, loop_id) {
19+
match never_loop_block(block, &mut Vec::new(), loop_id) {
2020
NeverLoopResult::AlwaysBreak => {
2121
span_lint_and_then(cx, NEVER_LOOP, span, "this loop never actually loops", |diag| {
2222
if let Some(ForLoop {
@@ -92,35 +92,34 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult
9292
}
9393
}
9494

95-
fn never_loop_block(block: &Block<'_>, main_loop_id: HirId) -> NeverLoopResult {
96-
let mut iter = block
95+
fn never_loop_block(block: &Block<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: HirId) -> NeverLoopResult {
96+
let iter = block
9797
.stmts
9898
.iter()
9999
.filter_map(stmt_to_expr)
100100
.chain(block.expr.map(|expr| (expr, None)));
101-
never_loop_expr_seq(&mut iter, main_loop_id)
102-
}
103101

104-
fn never_loop_expr_seq<'a, T: Iterator<Item = (&'a Expr<'a>, Option<&'a Block<'a>>)>>(
105-
es: &mut T,
106-
main_loop_id: HirId,
107-
) -> NeverLoopResult {
108-
es.map(|(e, els)| {
109-
let e = never_loop_expr(e, main_loop_id);
110-
els.map_or(e, |els| combine_branches(e, never_loop_block(els, main_loop_id)))
102+
iter.map(|(e, els)| {
103+
let e = never_loop_expr(e, ignore_ids, main_loop_id);
104+
// els is an else block in a let...else binding
105+
els.map_or(e, |els| {
106+
combine_branches(e, never_loop_block(els, ignore_ids, main_loop_id))
107+
})
111108
})
112109
.fold(NeverLoopResult::Otherwise, combine_seq)
113110
}
114111

115112
fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<(&'tcx Expr<'tcx>, Option<&'tcx Block<'tcx>>)> {
116113
match stmt.kind {
117-
StmtKind::Semi(e, ..) | StmtKind::Expr(e, ..) => Some((e, None)),
114+
StmtKind::Semi(e) | StmtKind::Expr(e) => Some((e, None)),
115+
// add the let...else expression (if present)
118116
StmtKind::Local(local) => local.init.map(|init| (init, local.els)),
119117
StmtKind::Item(..) => None,
120118
}
121119
}
122120

123-
fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
121+
#[allow(clippy::too_many_lines)]
122+
fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: HirId) -> NeverLoopResult {
124123
match expr.kind {
125124
ExprKind::Box(e)
126125
| ExprKind::Unary(_, e)
@@ -129,48 +128,56 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
129128
| ExprKind::Field(e, _)
130129
| ExprKind::AddrOf(_, _, e)
131130
| ExprKind::Repeat(e, _)
132-
| ExprKind::DropTemps(e) => never_loop_expr(e, main_loop_id),
133-
ExprKind::Let(let_expr) => never_loop_expr(let_expr.init, main_loop_id),
134-
ExprKind::Array(es) | ExprKind::Tup(es) => never_loop_expr_all(&mut es.iter(), main_loop_id),
135-
ExprKind::MethodCall(_, receiver, es, _) => {
136-
never_loop_expr_all(&mut std::iter::once(receiver).chain(es.iter()), main_loop_id)
137-
},
131+
| ExprKind::DropTemps(e) => never_loop_expr(e, ignore_ids, main_loop_id),
132+
ExprKind::Let(let_expr) => never_loop_expr(let_expr.init, ignore_ids, main_loop_id),
133+
ExprKind::Array(es) | ExprKind::Tup(es) => never_loop_expr_all(&mut es.iter(), ignore_ids, main_loop_id),
134+
ExprKind::MethodCall(_, receiver, es, _) => never_loop_expr_all(
135+
&mut std::iter::once(receiver).chain(es.iter()),
136+
ignore_ids,
137+
main_loop_id,
138+
),
138139
ExprKind::Struct(_, fields, base) => {
139-
let fields = never_loop_expr_all(&mut fields.iter().map(|f| f.expr), main_loop_id);
140+
let fields = never_loop_expr_all(&mut fields.iter().map(|f| f.expr), ignore_ids, main_loop_id);
140141
if let Some(base) = base {
141-
combine_both(fields, never_loop_expr(base, main_loop_id))
142+
combine_both(fields, never_loop_expr(base, ignore_ids, main_loop_id))
142143
} else {
143144
fields
144145
}
145146
},
146-
ExprKind::Call(e, es) => never_loop_expr_all(&mut once(e).chain(es.iter()), main_loop_id),
147+
ExprKind::Call(e, es) => never_loop_expr_all(&mut once(e).chain(es.iter()), ignore_ids, main_loop_id),
147148
ExprKind::Binary(_, e1, e2)
148149
| ExprKind::Assign(e1, e2, _)
149150
| ExprKind::AssignOp(_, e1, e2)
150-
| ExprKind::Index(e1, e2) => never_loop_expr_all(&mut [e1, e2].iter().copied(), main_loop_id),
151+
| ExprKind::Index(e1, e2) => never_loop_expr_all(&mut [e1, e2].iter().copied(), ignore_ids, main_loop_id),
151152
ExprKind::Loop(b, _, _, _) => {
152153
// Break can come from the inner loop so remove them.
153-
absorb_break(never_loop_block(b, main_loop_id))
154+
absorb_break(never_loop_block(b, ignore_ids, main_loop_id))
154155
},
155156
ExprKind::If(e, e2, e3) => {
156-
let e1 = never_loop_expr(e, main_loop_id);
157-
let e2 = never_loop_expr(e2, main_loop_id);
158-
let e3 = e3
159-
.as_ref()
160-
.map_or(NeverLoopResult::Otherwise, |e| never_loop_expr(e, main_loop_id));
157+
let e1 = never_loop_expr(e, ignore_ids, main_loop_id);
158+
let e2 = never_loop_expr(e2, ignore_ids, main_loop_id);
159+
let e3 = e3.as_ref().map_or(NeverLoopResult::Otherwise, |e| {
160+
never_loop_expr(e, ignore_ids, main_loop_id)
161+
});
161162
combine_seq(e1, combine_branches(e2, e3))
162163
},
163164
ExprKind::Match(e, arms, _) => {
164-
let e = never_loop_expr(e, main_loop_id);
165+
let e = never_loop_expr(e, ignore_ids, main_loop_id);
165166
if arms.is_empty() {
166167
e
167168
} else {
168-
let arms = never_loop_expr_branch(&mut arms.iter().map(|a| a.body), main_loop_id);
169+
let arms = never_loop_expr_branch(&mut arms.iter().map(|a| a.body), ignore_ids, main_loop_id);
169170
combine_seq(e, arms)
170171
}
171172
},
172-
ExprKind::Block(b, None) => never_loop_block(b, main_loop_id),
173-
ExprKind::Block(b, Some(_label)) => absorb_break(never_loop_block(b, main_loop_id)),
173+
ExprKind::Block(b, l) => {
174+
if l.is_some() {
175+
ignore_ids.push(b.hir_id);
176+
}
177+
let ret = never_loop_block(b, ignore_ids, main_loop_id);
178+
ignore_ids.pop();
179+
ret
180+
},
174181
ExprKind::Continue(d) => {
175182
let id = d
176183
.target_id
@@ -181,20 +188,32 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
181188
NeverLoopResult::AlwaysBreak
182189
}
183190
},
191+
// checks if break targets a block instead of a loop
192+
ExprKind::Break(Destination { target_id: Ok(t), .. }, e) if ignore_ids.contains(&t) => e
193+
.map_or(NeverLoopResult::Otherwise, |e| {
194+
combine_seq(never_loop_expr(e, ignore_ids, main_loop_id), NeverLoopResult::Otherwise)
195+
}),
184196
ExprKind::Break(_, e) | ExprKind::Ret(e) => e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| {
185-
combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak)
197+
combine_seq(
198+
never_loop_expr(e, ignore_ids, main_loop_id),
199+
NeverLoopResult::AlwaysBreak,
200+
)
186201
}),
187202
ExprKind::InlineAsm(asm) => asm
188203
.operands
189204
.iter()
190205
.map(|(o, _)| match o {
191206
InlineAsmOperand::In { expr, .. } | InlineAsmOperand::InOut { expr, .. } => {
192-
never_loop_expr(expr, main_loop_id)
207+
never_loop_expr(expr, ignore_ids, main_loop_id)
193208
},
194-
InlineAsmOperand::Out { expr, .. } => never_loop_expr_all(&mut expr.iter().copied(), main_loop_id),
195-
InlineAsmOperand::SplitInOut { in_expr, out_expr, .. } => {
196-
never_loop_expr_all(&mut once(*in_expr).chain(out_expr.iter().copied()), main_loop_id)
209+
InlineAsmOperand::Out { expr, .. } => {
210+
never_loop_expr_all(&mut expr.iter().copied(), ignore_ids, main_loop_id)
197211
},
212+
InlineAsmOperand::SplitInOut { in_expr, out_expr, .. } => never_loop_expr_all(
213+
&mut once(*in_expr).chain(out_expr.iter().copied()),
214+
ignore_ids,
215+
main_loop_id,
216+
),
198217
InlineAsmOperand::Const { .. }
199218
| InlineAsmOperand::SymFn { .. }
200219
| InlineAsmOperand::SymStatic { .. } => NeverLoopResult::Otherwise,
@@ -209,13 +228,21 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
209228
}
210229
}
211230

212-
fn never_loop_expr_all<'a, T: Iterator<Item = &'a Expr<'a>>>(es: &mut T, main_loop_id: HirId) -> NeverLoopResult {
213-
es.map(|e| never_loop_expr(e, main_loop_id))
231+
fn never_loop_expr_all<'a, T: Iterator<Item = &'a Expr<'a>>>(
232+
es: &mut T,
233+
ignore_ids: &mut Vec<HirId>,
234+
main_loop_id: HirId,
235+
) -> NeverLoopResult {
236+
es.map(|e| never_loop_expr(e, ignore_ids, main_loop_id))
214237
.fold(NeverLoopResult::Otherwise, combine_both)
215238
}
216239

217-
fn never_loop_expr_branch<'a, T: Iterator<Item = &'a Expr<'a>>>(e: &mut T, main_loop_id: HirId) -> NeverLoopResult {
218-
e.map(|e| never_loop_expr(e, main_loop_id))
240+
fn never_loop_expr_branch<'a, T: Iterator<Item = &'a Expr<'a>>>(
241+
e: &mut T,
242+
ignore_ids: &mut Vec<HirId>,
243+
main_loop_id: HirId,
244+
) -> NeverLoopResult {
245+
e.map(|e| never_loop_expr(e, ignore_ids, main_loop_id))
219246
.fold(NeverLoopResult::AlwaysBreak, combine_branches)
220247
}
221248

‎tests/ui/never_loop.rs‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,13 +234,22 @@ pub fn test19() {
234234
fn thing(iter: impl Iterator) {
235235
for _ in iter {
236236
'b: {
237-
// error goes away if we just have the block's value be ().
238237
break 'b;
239238
}
240239
}
241240
}
242241
}
243242

243+
pub fn test20() {
244+
'a: loop {
245+
'b: {
246+
break 'b 'c: {
247+
break 'a;
248+
};
249+
}
250+
}
251+
}
252+
244253
fn main() {
245254
test1();
246255
test2();

‎tests/ui/never_loop.stderr‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,5 +114,17 @@ LL | | break x;
114114
LL | | };
115115
| |_____^
116116

117-
error: aborting due to 10 previous errors
117+
error: this loop never actually loops
118+
--> $DIR/never_loop.rs:244:5
119+
|
120+
LL | / 'a: loop {
121+
LL | | 'b: {
122+
LL | | break 'b 'c: {
123+
LL | | break 'a;
124+
LL | | };
125+
LL | | }
126+
LL | | }
127+
| |_____^
128+
129+
error: aborting due to 11 previous errors
118130

0 commit comments

Comments
 (0)