Add iterator reduction coverage to never_loop #16222

cuongleqq · 2025-12-12T03:13:21Z

Extend never_loop to also lint iterator reduction methods (e.g. for_each, try_for_each, fold, try_fold, reduce, all, any) when the closure’s body diverges (return type !). Add a UI test never_loop_iterator_reduction to cover these cases.

Testing:

TESTNAME=never_loop_iterator_reduction cargo uitest

changelog: [never_loop]: lint diverging iterator reduction closures like for_each and fold

rustbot · 2025-12-12T03:13:25Z

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Jarcho · 2025-12-13T04:09:57Z

clippy_lints/src/loops/mod.rs

+        if let ExprKind::MethodCall(path, recv, args, _) = expr.kind
+            && matches!(
+                path.ident.name,
+                sym::for_each | sym::try_for_each | sym::fold | sym::try_fold | sym::reduce | sym::all | sym::any
+            )
+            && ty::get_iterator_item_ty(cx, cx.typeck_results().expr_ty(recv)).is_some()
+        {
+            never_loop::check_iterator_reduction(cx, expr, recv, args);
+        }


This should be combined with the previous method check as something like

if let ExprKind::MethodCall(path, recv, args, _) = expr.kind { match (path.ident.name, args) { // methods here } }

I used a single MethodCall destructure as suggested, but I kept 2 if branches instead of a single match because some methods like for_each, all, any are handled by both lints. Match will short-circuit to only the first matching arm.

Still make it a single match. A match arm can do multiple things.

Ok! I switched it to a single match as suggested. For the overlapping methods, the match arm now calls both unused_enumerate_index and never_loop. Other arms handle the single-lint cases.

Jarcho · 2025-12-13T04:11:40Z

clippy_lints/src/loops/never_loop.rs

+            expr.span,
+            "this iterator reduction never loops (closure always diverges)",
+            |diag| {
+                let mut app = Applicability::Unspecified;


Should be WithPlaceholders

Jarcho · 2025-12-13T04:13:38Z

clippy_lints/src/loops/never_loop.rs

+            "this iterator reduction never loops (closure always diverges)",
+            |diag| {
+                let mut app = Applicability::Unspecified;
+                let recv_snip = make_iterator_snippet(cx, recv, &mut app);


This should just be snippet_with_context. We already know the receiver is an iterator.

Jarcho · 2025-12-13T04:15:14Z

clippy_lints/src/loops/mod.rs

+                path.ident.name,
+                sym::for_each | sym::try_for_each | sym::fold | sym::try_fold | sym::reduce | sym::all | sym::any
+            )
+            && ty::get_iterator_item_ty(cx, cx.typeck_results().expr_ty(recv)).is_some()


This should be recv.ty_based_def(cx).assoc_fn_parent(cx).is_diag_item(cx, sym::Iterator).

rustbot · 2025-12-13T04:15:44Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

cuongleqq · 2025-12-13T14:44:35Z

I pushed an update to address all 4 comments

Combined the method check for both unused_enumerate_index and never_loop
Use a more direct check for Iterator trait
Use HasPlaceholders for the applicability
Use snippet_with_context instead of make_iterator_snippet

@rustbot ready

cuongleqq · 2025-12-14T03:12:26Z

Addressed the requested changes:

Switched to a single match for the MethodCall handling

@rustbot ready

Jarcho · 2025-12-14T14:07:04Z

clippy_lints/src/loops/never_loop.rs

+    // identify which argument is the closure based on the method kind
+    let Some(method_name) = (match expr.kind {
+        ExprKind::MethodCall(path, ..) => Some(path.ident.name),
+        _ => None,
+    }) else {
+        return;
+    };
+
+    let closure_arg = match method_name {
+        sym::for_each | sym::try_for_each | sym::reduce | sym::all | sym::any => args.first(),
+        sym::fold | sym::try_fold => args.get(1),
+        _ => None,
+    };
+
+    let Some(closure_arg) = closure_arg else {
+        return;
+    };


You should be splitting this before getting to this function.

Jarcho · 2025-12-14T14:11:46Z

clippy_lints/src/loops/mod.rs

+                    unused_enumerate_index::check_method(cx, expr, recv, arg);
+                    if is_iterator_method {
+                        never_loop::check_iterator_reduction(cx, expr, recv, args);
+                    }


Can you pull out the checks shared between the two lints to happen here. Both the check for ExprKind::Closure and the iterator check are the same for both of them.

Jarcho · 2025-12-14T14:12:52Z

clippy_lints/src/loops/mod.rs

+            let is_iterator_method = cx
+                .ty_based_def(expr)
+                .assoc_fn_parent(cx)
+                .is_diag_item(cx, sym::Iterator);


This should be delayed until after the name check. Type checking and item identification is notably slower than matching on a symbol.

Defining a closure would be fine.

cuongleqq · 2025-12-15T08:33:59Z

Thanks for the quick review!

I pushed an update to address the comments:

is_iterator_method() stays behind the cheap name/arg checks
moved the shared iterator + closure checks into check_expr
pass the closure into never_loop / unused_enumerate_index instead of re-parsing inside

@rustbot ready

@Jarcho: I’m repeating if let ExprKind::Closure(closure) = arg.kind && is_iterator_method() in all match arms to keep the expensive check last. Do you want me to make a small helper for this, or is it fine as-is?

Jarcho

Thank you.

View changes since this review

Jarcho · 2025-12-15T12:04:28Z

I’m repeating if let ExprKind::Closure(closure) = arg.kind && is_iterator_method() in all match arms to keep the expensive check last. Do you want me to make a small helper for this, or is it fine as-is?

A helper function would end up harder to read. It's two steps that are, without external context, unrelated to each other. Having to jump to another spot to just to read what it does is also a pain.

Add iterator reduction coverage to never_loop

36201bd

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 12, 2025

rustbot assigned Jarcho Dec 12, 2025

cuongleqq added 2 commits December 12, 2025 10:24

Fix fmt and dogfood lints for never_loop iterator reduction

89a848c

Clean up unused try_reduce entry in sym

fefd58b

cuongleqq mentioned this pull request Dec 12, 2025

never_loop: Also check all known iterator reductions. #16061

Closed

Jarcho requested changes Dec 13, 2025

View reviewed changes

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 13, 2025

rustbot added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Dec 13, 2025

Refine never_loop iterator reduction check

51580d3

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 13, 2025

Refactor loops MethodCall handling into single match

9dca2a8

Jarcho requested changes Dec 14, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 14, 2025

loops: reuse iterator checks for unused_enumerate_index

c59da4f

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 15, 2025

Jarcho approved these changes Dec 15, 2025

View reviewed changes

Jarcho added this pull request to the merge queue Dec 15, 2025

Merged via the queue into rust-lang:master with commit d36794f Dec 15, 2025
11 checks passed

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 15, 2025

Add iterator reduction coverage to never_loop #16222

Add iterator reduction coverage to never_loop #16222

Conversation

cuongleqq commented Dec 12, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Dec 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Dec 13, 2025

Uh oh!

cuongleqq commented Dec 13, 2025

Uh oh!

cuongleqq commented Dec 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cuongleqq commented Dec 15, 2025

Uh oh!

Jarcho left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jarcho commented Dec 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cuongleqq commented Dec 12, 2025 •

edited by rustbot

Loading

Jarcho left a comment •

edited by rustbot

Loading