mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #10311 - samueltardieu:issue-10304, r=Jarcho

Browse Source 
[never_loop] Fix #10304

It is not sufficient to ignore break from a block inside the loop. Instructions after the break must be ignored, as they are unreachable. This is also true for all instructions in outer blocks and loops until the right block is reached.

Fixes #10304

---

changelog: FP: [`never_loop`]: No longer lints, for statements following break statements for outer blocks.
[#10311](https://github.com/rust-lang/rust-clippy/pull/10311)
<!-- changelog_checked-->
This commit is contained in:
bors 2023-02-14 17:33:28 +00:00
commit a182a67cea
3 changed files with 93 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

59
clippy_lints/src/loops/never_loop.rs
View File

@ -39,6 +39,7 @@ pub(super) fn check(
});
},
NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Otherwise => (),
NeverLoopResult::IgnoreUntilEnd(_) => unreachable!(),
}
}
@ -48,6 +49,8 @@ enum NeverLoopResult {
AlwaysBreak,
// A continue may occur for the main loop.
MayContinueMainLoop,
// Ignore everything until the end of the block with this id
IgnoreUntilEnd(HirId),
Otherwise,
}
@ -56,6 +59,7 @@ fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
match arg {
NeverLoopResult::AlwaysBreak | NeverLoopResult::Otherwise => NeverLoopResult::Otherwise,
NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop,
NeverLoopResult::IgnoreUntilEnd(id) => NeverLoopResult::IgnoreUntilEnd(id),
}
}
@ -63,27 +67,26 @@ fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult {
#[must_use]
fn combine_seq(first: NeverLoopResult, second: NeverLoopResult) -> NeverLoopResult {
match first {
NeverLoopResult::AlwaysBreak | NeverLoopResult::MayContinueMainLoop => first,
NeverLoopResult::Otherwise => second,
}
}
// Combine two results where both parts are called but not necessarily in order.
#[must_use]
fn combine_both(left: NeverLoopResult, right: NeverLoopResult) -> NeverLoopResult {
match (left, right) {
(NeverLoopResult::MayContinueMainLoop, _) | (_, NeverLoopResult::MayContinueMainLoop) => {
NeverLoopResult::MayContinueMainLoop
NeverLoopResult::AlwaysBreak | NeverLoopResult::MayContinueMainLoop | NeverLoopResult::IgnoreUntilEnd(_) => {
first
},
(NeverLoopResult::AlwaysBreak, _) | (_, NeverLoopResult::AlwaysBreak) => NeverLoopResult::AlwaysBreak,
(NeverLoopResult::Otherwise, NeverLoopResult::Otherwise) => NeverLoopResult::Otherwise,
NeverLoopResult::Otherwise => second,
}
}
// Combine two results where only one of the part may have been executed.
#[must_use]
fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult {
fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult, ignore_ids: &[HirId]) -> NeverLoopResult {
match (b1, b2) {
(NeverLoopResult::IgnoreUntilEnd(a), NeverLoopResult::IgnoreUntilEnd(b)) => {
if ignore_ids.iter().find(|&e| e == &a || e == &b).unwrap() == &a {
NeverLoopResult::IgnoreUntilEnd(b)
} else {
NeverLoopResult::IgnoreUntilEnd(a)
}
},
(i @ NeverLoopResult::IgnoreUntilEnd(_), NeverLoopResult::AlwaysBreak)
| (NeverLoopResult::AlwaysBreak, i @ NeverLoopResult::IgnoreUntilEnd(_)) => i,
(NeverLoopResult::AlwaysBreak, NeverLoopResult::AlwaysBreak) => NeverLoopResult::AlwaysBreak,
(NeverLoopResult::MayContinueMainLoop, _) | (_, NeverLoopResult::MayContinueMainLoop) => {
NeverLoopResult::MayContinueMainLoop
@ -103,7 +106,7 @@ fn never_loop_block(block: &Block<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id
let e = never_loop_expr(e, ignore_ids, main_loop_id);
// els is an else block in a let...else binding
els.map_or(e, |els| {
combine_branches(e, never_loop_block(els, ignore_ids, main_loop_id))
combine_branches(e, never_loop_block(els, ignore_ids, main_loop_id), ignore_ids)
})
})
.fold(NeverLoopResult::Otherwise, combine_seq)
@ -139,7 +142,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
ExprKind::Struct(_, fields, base) => {
let fields = never_loop_expr_all(&mut fields.iter().map(|f| f.expr), ignore_ids, main_loop_id);
if let Some(base) = base {
combine_both(fields, never_loop_expr(base, ignore_ids, main_loop_id))
combine_seq(fields, never_loop_expr(base, ignore_ids, main_loop_id))
} else {
fields
}
@ -159,7 +162,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
let e3 = e3.as_ref().map_or(NeverLoopResult::Otherwise, |e| {
never_loop_expr(e, ignore_ids, main_loop_id)
});
combine_seq(e1, combine_branches(e2, e3))
combine_seq(e1, combine_branches(e2, e3, ignore_ids))
},
ExprKind::Match(e, arms, _) => {
let e = never_loop_expr(e, ignore_ids, main_loop_id);
@ -175,8 +178,13 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
ignore_ids.push(b.hir_id);
}
let ret = never_loop_block(b, ignore_ids, main_loop_id);
ignore_ids.pop();
ret
if l.is_some() {
ignore_ids.pop();
}
match ret {
NeverLoopResult::IgnoreUntilEnd(a) if a == b.hir_id => NeverLoopResult::Otherwise,
_ => ret,
}
},
ExprKind::Continue(d) => {
let id = d
@ -190,8 +198,8 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
},
// checks if break targets a block instead of a loop
ExprKind::Break(Destination { target_id: Ok(t), .. }, e) if ignore_ids.contains(&t) => e
.map_or(NeverLoopResult::Otherwise, |e| {
combine_seq(never_loop_expr(e, ignore_ids, main_loop_id), NeverLoopResult::Otherwise)
.map_or(NeverLoopResult::IgnoreUntilEnd(t), |e| {
never_loop_expr(e, ignore_ids, main_loop_id)
}),
ExprKind::Break(_, e) | ExprKind::Ret(e) => e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| {
combine_seq(
@ -218,7 +226,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
| InlineAsmOperand::SymFn { .. }
| InlineAsmOperand::SymStatic { .. } => NeverLoopResult::Otherwise,
})
.fold(NeverLoopResult::Otherwise, combine_both),
.fold(NeverLoopResult::Otherwise, combine_seq),
ExprKind::Yield(_, _)
| ExprKind::Closure { .. }
| ExprKind::Path(_)
@ -234,7 +242,7 @@ fn never_loop_expr_all<'a, T: Iterator<Item = &'a Expr<'a>>>(
main_loop_id: HirId,
) -> NeverLoopResult {
es.map(|e| never_loop_expr(e, ignore_ids, main_loop_id))
.fold(NeverLoopResult::Otherwise, combine_both)
.fold(NeverLoopResult::Otherwise, combine_seq)
}
fn never_loop_expr_branch<'a, T: Iterator<Item = &'a Expr<'a>>>(
@ -242,8 +250,9 @@ fn never_loop_expr_branch<'a, T: Iterator<Item = &'a Expr<'a>>>(
ignore_ids: &mut Vec<HirId>,
main_loop_id: HirId,
) -> NeverLoopResult {
e.map(|e| never_loop_expr(e, ignore_ids, main_loop_id))
.fold(NeverLoopResult::AlwaysBreak, combine_branches)
e.fold(NeverLoopResult::AlwaysBreak, |a, b| {
combine_branches(a, never_loop_expr(b, ignore_ids, main_loop_id), ignore_ids)
})
}
fn for_to_if_let_sugg(cx: &LateContext<'_>, iterator: &Expr<'_>, pat: &Pat<'_>) -> String {

45
tests/ui/never_loop.rs
View File

@ -250,6 +250,51 @@ pub fn test20() {
}
}
pub fn test21() {
loop {
'a: {
{}
break 'a;
}
}
}
// Issue 10304: code after break from block was not considered
// unreachable code and was considered for further analysis of
// whether the loop would ever be executed or not.
pub fn test22() {
for _ in 0..10 {
'block: {
break 'block;
return;
}
println!("looped");
}
}
pub fn test23() {
for _ in 0..10 {
'block: {
for _ in 0..20 {
break 'block;
}
}
println!("looped");
}
}
pub fn test24() {
'a: for _ in 0..10 {
'b: {
let x = Some(1);
match x {
None => break 'a,
Some(_) => break 'b,
}
}
}
}
fn main() {
test1();
test2();

15
tests/ui/never_loop.stderr
View File

@ -126,5 +126,18 @@ LL | | }
LL | | }
| |_____^
error: aborting due to 11 previous errors
error: this loop never actually loops
--> $DIR/never_loop.rs:278:13
|
LL | / for _ in 0..20 {
LL | | break 'block;
LL | | }
| |_____________^
|
help: if you need the first element of the iterator, try writing
|
LL | if let Some(_) = (0..20).next() {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to 12 previous errors