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.

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

Open

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

Add iterator reduction coverage to never_loop #16222

Are you sure you want to change the base?

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cuongleqq commented Dec 12, 2025 •

edited by rustbot

Loading