From c231b4188782c249428a66d70b3f705757390ce5 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Thu, 9 Feb 2023 20:32:44 +0100 Subject: [PATCH 1/4] Remove useless call to combine_seq `combine_seq(x, NeverLoopResult::Otherwise)` always returns `x` --- clippy_lints/src/loops/never_loop.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index 14f161f51026..5bc0f4226fb2 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -191,7 +191,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec, 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) + 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( From 1fec2927c5cf02de85c08a63dbc8909077661d1b Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Mon, 13 Feb 2023 20:36:41 +0100 Subject: [PATCH 2/4] Replace combine_both by combine_seq All evaluations now happen in order. --- clippy_lints/src/loops/never_loop.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index 5bc0f4226fb2..9d16949c81f4 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -68,18 +68,6 @@ fn combine_seq(first: NeverLoopResult, second: NeverLoopResult) -> NeverLoopResu } } -// 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::AlwaysBreak) => NeverLoopResult::AlwaysBreak, - (NeverLoopResult::Otherwise, NeverLoopResult::Otherwise) => NeverLoopResult::Otherwise, - } -} - // Combine two results where only one of the part may have been executed. #[must_use] fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult { @@ -139,7 +127,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec, 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 } @@ -218,7 +206,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec, 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 +222,7 @@ fn never_loop_expr_all<'a, T: Iterator>>( 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>>( From e9dffa391085f4b3c84f90e8ab82bdd568d1f17e Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Tue, 14 Feb 2023 09:12:51 +0100 Subject: [PATCH 3/4] Fix a bug in never_loop when anonymous blocks are nested in named blocks The following code ``` loop { 'a: { { } break 'a; } } ``` was detected as a never-looping loop. --- clippy_lints/src/loops/never_loop.rs | 4 +++- tests/ui/never_loop.rs | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index 9d16949c81f4..3cb5b1ffc7b3 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -163,7 +163,9 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec, main_loop_id: H ignore_ids.push(b.hir_id); } let ret = never_loop_block(b, ignore_ids, main_loop_id); - ignore_ids.pop(); + if l.is_some() { + ignore_ids.pop(); + } ret }, ExprKind::Continue(d) => { diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs index 28e8f459d442..a177d5e06aa6 100644 --- a/tests/ui/never_loop.rs +++ b/tests/ui/never_loop.rs @@ -250,6 +250,15 @@ pub fn test20() { } } +pub fn test21() { + loop { + 'a: { + { } + break 'a; + } + } +} + fn main() { test1(); test2(); From 657ee48bec2c02ad6885176ea37658bddf6ce59a Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Mon, 13 Feb 2023 20:37:36 +0100 Subject: [PATCH 4/4] Ignore instructions following a break from block in never_loop lint 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. --- clippy_lints/src/loops/never_loop.rs | 35 +++++++++++++++++++------ tests/ui/never_loop.rs | 38 +++++++++++++++++++++++++++- tests/ui/never_loop.stderr | 15 ++++++++++- 3 files changed, 78 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index 3cb5b1ffc7b3..ea7630ce56dd 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -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,15 +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::AlwaysBreak | NeverLoopResult::MayContinueMainLoop | NeverLoopResult::IgnoreUntilEnd(_) => { + first + }, 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 @@ -91,7 +106,7 @@ fn never_loop_block(block: &Block<'_>, ignore_ids: &mut Vec, 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) @@ -147,7 +162,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec, 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); @@ -166,7 +181,10 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec, main_loop_id: H if l.is_some() { ignore_ids.pop(); } - ret + match ret { + NeverLoopResult::IgnoreUntilEnd(a) if a == b.hir_id => NeverLoopResult::Otherwise, + _ => ret, + } }, ExprKind::Continue(d) => { let id = d @@ -180,7 +198,7 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec, 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| { + .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| { @@ -232,8 +250,9 @@ fn never_loop_expr_branch<'a, T: Iterator>>( ignore_ids: &mut Vec, 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 { diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs index a177d5e06aa6..29821ff96fc0 100644 --- a/tests/ui/never_loop.rs +++ b/tests/ui/never_loop.rs @@ -253,12 +253,48 @@ 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(); diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr index b7029bf8bed4..704d448644e2 100644 --- a/tests/ui/never_loop.stderr +++ b/tests/ui/never_loop.stderr @@ -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