From 13915be54a8e003d34e5037865675662af716763 Mon Sep 17 00:00:00 2001 From: MassimilianoBaglioni <3d.massimilianobaglioni@gmail.com> Date: Mon, 1 Jun 2026 15:38:37 +0200 Subject: [PATCH] enhance never_loop to emit diagnostic notes for non obvious divergence --- clippy_lints/src/loops/never_loop.rs | 82 +++++++++++++++- tests/ui/never_loop.rs | 103 ++++++++++++++++++++ tests/ui/never_loop.stderr | 139 ++++++++++++++++++++++++++- 3 files changed, 321 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index ff0ac575550c..bf21cf26d477 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -28,6 +28,7 @@ pub(super) fn check<'tcx>( NeverLoopResult::Diverging { ref break_spans, ref never_spans, + ref non_obvious_exprs, } => { span_lint_and_then(cx, NEVER_LOOP, span, "this loop never actually loops", |diag| { if let Some(ForLoop { @@ -54,7 +55,7 @@ pub(super) fn check<'tcx>( for_span.with_hi(iterator.span.hi()), for_to_if_let_sugg(cx, iterator, pat), )]; - // Make sure to clear up the diverging sites when we remove a loopp. + // Make sure to clear up the diverging sites when we remove a loop. suggestions.extend(break_spans.iter().map(|span| (*span, String::new()))); diag.multipart_suggestion( "if you need the first element of the iterator, try writing", @@ -69,6 +70,15 @@ pub(super) fn check<'tcx>( ); } } + + let non_obvious_spans = non_obvious_exprs + .iter() + .map(|hir_id| cx.tcx.hir_expect_expr(*hir_id)) + .flat_map(|expr| find_non_obvious_spans(cx, expr)); + + for span in non_obvious_spans { + diag.span_note(span, "this expression never returns"); + } }); }, NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Normal => (), @@ -135,6 +145,7 @@ enum NeverLoopResult { Diverging { break_spans: Vec, never_spans: Vec, + non_obvious_exprs: Vec, }, /// We have not encountered any main loop continue, /// and subsequent control flow is (possibly) reachable @@ -181,17 +192,21 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult NeverLoopResult::Diverging { break_spans: mut break_spans1, never_spans: mut never_spans1, + non_obvious_exprs: mut non_obvious_exprs1, }, NeverLoopResult::Diverging { break_spans: mut break_spans2, never_spans: mut never_spans2, + non_obvious_exprs: mut non_obvious_exprs2, }, ) => { break_spans1.append(&mut break_spans2); never_spans1.append(&mut never_spans2); + non_obvious_exprs1.append(&mut non_obvious_exprs2); NeverLoopResult::Diverging { break_spans: break_spans1, never_spans: never_spans1, + non_obvious_exprs: non_obvious_exprs1, } }, } @@ -278,7 +293,7 @@ fn is_label_for_block(cx: &LateContext<'_>, dest: &Destination) -> bool { #[expect(clippy::too_many_lines)] fn never_loop_expr<'tcx>( cx: &LateContext<'tcx>, - expr: &Expr<'tcx>, + expr: &'tcx Expr<'tcx>, local_labels: &mut Vec<(HirId, bool)>, main_loop_id: HirId, ) -> NeverLoopResult { @@ -335,6 +350,7 @@ fn never_loop_expr<'tcx>( NeverLoopResult::Diverging { break_spans: vec![], never_spans: vec![], + non_obvious_exprs: vec![], }, |a, b| combine_branches(a, never_loop_expr(cx, b.body, local_labels, main_loop_id)), ) @@ -361,6 +377,7 @@ fn never_loop_expr<'tcx>( NeverLoopResult::Diverging { break_spans: all_spans_after_expr(cx, expr), never_spans: vec![], + non_obvious_exprs: vec![], } } }, @@ -374,6 +391,7 @@ fn never_loop_expr<'tcx>( NeverLoopResult::Diverging { break_spans: vec![], never_spans: vec![], + non_obvious_exprs: vec![], } }) }, @@ -391,6 +409,7 @@ fn never_loop_expr<'tcx>( all_spans_after_expr(cx, expr) }, never_spans: vec![], + non_obvious_exprs: vec![], } }) }, @@ -398,6 +417,7 @@ fn never_loop_expr<'tcx>( NeverLoopResult::Diverging { break_spans: vec![], never_spans: vec![], + non_obvious_exprs: vec![], } }), ExprKind::InlineAsm(asm) => combine_seq_many(asm.operands.iter().map(|(o, _)| match o { @@ -434,11 +454,13 @@ fn never_loop_expr<'tcx>( | ExprKind::Lit(_) | ExprKind::Err(_) => NeverLoopResult::Normal, }; + let result = combine_seq(result, || { if cx.typeck_results().expr_ty(expr).is_never() { NeverLoopResult::Diverging { break_spans: vec![], never_spans: all_spans_after_expr(cx, expr), + non_obvious_exprs: vec![expr.hir_id], } } else { NeverLoopResult::Normal @@ -480,3 +502,59 @@ fn mark_block_as_reachable(expr: &Expr<'_>, local_labels: &mut [(HirId, bool)]) *reachable = true; } } + +fn find_non_obvious_spans<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) -> Vec { + let mut spans = vec![]; + + for_each_expr_without_closures(e, |expr: &'tcx Expr<'tcx>| -> ControlFlow<(), Descend> { + if cx.typeck_results().expr_ty(expr).is_never() && !expr.span.from_expansion() { + match expr.kind { + // The first arm handles both directly divergent expressions and expressions + // that contain divergence indirectly. The latter are inspected to identify + // possible inner non-trivial divergent expressions. + ExprKind::Break(..) + | ExprKind::Continue(..) + | ExprKind::Ret(..) + | ExprKind::Become(..) + | ExprKind::Loop(..) + | ExprKind::Block(..) + | ExprKind::Match(..) + | ExprKind::If(..) => { + return ControlFlow::Continue(Descend::Yes); + }, + ExprKind::ConstBlock(..) + | ExprKind::Array(..) + | ExprKind::Call(..) + | ExprKind::MethodCall(..) + | ExprKind::Use(..) + | ExprKind::Tup(..) + | ExprKind::Binary(..) + | ExprKind::Unary(..) + | ExprKind::Lit(..) + | ExprKind::Cast(..) + | ExprKind::Type(..) + | ExprKind::DropTemps(..) + | ExprKind::Let(..) + | ExprKind::Closure(..) + | ExprKind::Assign(..) + | ExprKind::AssignOp(..) + | ExprKind::Field(..) + | ExprKind::Index(..) + | ExprKind::Path(..) + | ExprKind::AddrOf(..) + | ExprKind::InlineAsm(..) + | ExprKind::OffsetOf(..) + | ExprKind::Struct(..) + | ExprKind::Repeat(..) + | ExprKind::Yield(..) + | ExprKind::UnsafeBinderCast(..) + | ExprKind::Err(..) => { + spans.push(expr.span); + return ControlFlow::Continue(Descend::No); + }, + } + } + ControlFlow::Continue(Descend::Yes) + }); + spans +} diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs index 7b2d66450f17..5d9d93dc49c3 100644 --- a/tests/ui/never_loop.rs +++ b/tests/ui/never_loop.rs @@ -541,3 +541,106 @@ fn issue16462() { n >= 0 || break; } } + +fn issue16056_basic() { + let x = || Some(panic!()); + + //~v never_loop + loop { + x().unwrap(); + } +} + +fn issue16056_nested_loops() { + let x = || Some(panic!()); + + //~v never_loop + loop { + //~v never_loop + loop { + x().unwrap(); + } + } +} + +fn issue16056_nested_block() { + let x = || Some(panic!()); + + //~v never_loop + loop { + { + x().unwrap(); + } + } +} + +fn issue16056_nested_blocks() { + let x = || Some(panic!()); + + //~v never_loop + loop { + { + { + x().unwrap(); + } + } + } +} + +fn issue16056_match() { + let x = || Some(panic!()); + + let mut y = 1; + //~v never_loop + loop { + y += 1; + match y { + 5 => return, + _ => { + x().unwrap(); + }, + } + } +} + +fn issue16056_if_no_trigger() { + let x = || Some(panic!()); + let mut y = 0; + //~v never_loop + loop { + y += 1; + if y == 1 { + // No extra note for this + x().unwrap(); + } + break; + } +} + +fn issue16056_if_trigger() { + let x = || Some(panic!()); + + //~v never_loop + loop { + if true { + x().unwrap(); + } else { + break; + } + } +} + +macro_rules! test_macro { + ($e:expr) => { + $e + }; +} + +fn issue16056_macro() { + let x = || Some(panic!()); + + //~v never_loop + loop { + test_macro!(x().unwrap()); + } +} diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr index 815758107884..7bc53289e76f 100644 --- a/tests/ui/never_loop.stderr +++ b/tests/ui/never_loop.stderr @@ -404,5 +404,142 @@ LL | | return; LL | | } | |_____^ -error: aborting due to 30 previous errors +error: this loop never actually loops + --> tests/ui/never_loop.rs:549:5 + | +LL | / loop { +LL | | x().unwrap(); +LL | | } + | |_____^ + | +note: this expression never returns + --> tests/ui/never_loop.rs:550:9 + | +LL | x().unwrap(); + | ^^^^^^^^^^^^ + +error: this loop never actually loops + --> tests/ui/never_loop.rs:558:5 + | +LL | / loop { +LL | | +LL | | loop { +LL | | x().unwrap(); +LL | | } +LL | | } + | |_____^ + | +note: this expression never returns + --> tests/ui/never_loop.rs:561:13 + | +LL | x().unwrap(); + | ^^^^^^^^^^^^ + +error: this loop never actually loops + --> tests/ui/never_loop.rs:560:9 + | +LL | / loop { +LL | | x().unwrap(); +LL | | } + | |_________^ + | +note: this expression never returns + --> tests/ui/never_loop.rs:561:13 + | +LL | x().unwrap(); + | ^^^^^^^^^^^^ + +error: this loop never actually loops + --> tests/ui/never_loop.rs:570:5 + | +LL | / loop { +LL | | { +LL | | x().unwrap(); +LL | | } +LL | | } + | |_____^ + | +note: this expression never returns + --> tests/ui/never_loop.rs:572:13 + | +LL | x().unwrap(); + | ^^^^^^^^^^^^ + +error: this loop never actually loops + --> tests/ui/never_loop.rs:581:5 + | +LL | / loop { +LL | | { +LL | | { +LL | | x().unwrap(); +... | +LL | | } + | |_____^ + | +note: this expression never returns + --> tests/ui/never_loop.rs:584:17 + | +LL | x().unwrap(); + | ^^^^^^^^^^^^ + +error: this loop never actually loops + --> tests/ui/never_loop.rs:595:5 + | +LL | / loop { +LL | | y += 1; +LL | | match y { +LL | | 5 => return, +... | +LL | | } + | |_____^ + | +note: this expression never returns + --> tests/ui/never_loop.rs:600:17 + | +LL | x().unwrap(); + | ^^^^^^^^^^^^ + +error: this loop never actually loops + --> tests/ui/never_loop.rs:610:5 + | +LL | / loop { +LL | | y += 1; +LL | | if y == 1 { +... | +LL | | break; +LL | | } + | |_____^ + +error: this loop never actually loops + --> tests/ui/never_loop.rs:624:5 + | +LL | / loop { +LL | | if true { +LL | | x().unwrap(); +LL | | } else { +... | +LL | | } + | |_____^ + | +note: this expression never returns + --> tests/ui/never_loop.rs:626:13 + | +LL | x().unwrap(); + | ^^^^^^^^^^^^ + +error: this loop never actually loops + --> tests/ui/never_loop.rs:643:5 + | +LL | / loop { +LL | | test_macro!(x().unwrap()); +LL | | } + | |_____^ + | +note: this expression never returns + --> tests/ui/never_loop.rs:644:21 + | +LL | test_macro!(x().unwrap()); + | ^^^^^^^^^^^^ + +error: aborting due to 39 previous errors