[`unused_enumerate_index`]: trigger on method calls #12432

Ethiraric · 2024-03-07T18:13:50Z

The lint used to check for patterns looking like:

for (_, x) in some_iter.enumerate() {
    // Index is ignored
}

This commit further checks for chained method calls constructs where we
can detect that the index is unused. Currently, this checks only for the
following patterns:

some_iter.enumerate().map_function(|(_, x)| ..)
let x = some_iter.enumerate();
x.map_function(|(_, x)| ..)

where map_function is one of all, any, filter_map, find_map,
flat_map, for_each or map.

Fixes #12411.

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: [unused_enumerate_index]: add detection for method chains such as iter.enumerate().map(|(_, x)| x)

rustbot · 2024-03-07T18:13:55Z

r? @y21

rustbot has assigned @y21.
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

GuillaumeGomez · 2024-03-07T18:20:32Z

clippy_lints/src/loops/unused_enumerate_index.rs

+        && pat_is_wild(cx, &index.kind, body)
+        && let ty::Adt(base, _) = *ty.kind()
+        && let name = cx.tcx.def_path_str(base.did())
+        && (name == "std::iter::Enumerate" || name == "core::iter::Enumerate")


Please use match_def_path instead. Otherwise if they used a type alias, I'm not sure it'll work.

I did and also changed the other string check a bit below for std::iter::Iterator::enumerate.

GuillaumeGomez

Looks good to me, thanks! Documentation is particularly appreciated, thanks for adding it! :)

y21

This is a very nice addition to the lint, thanks for working on this! I also like the refactor you did in loops/. Looks good with let chains, and +1 for the documentation

I have some initial comments

clippy_lints/src/loops/unused_enumerate_index.rs

clippy_lints/src/methods/unused_enumerate_index.rs

tests/ui/unused_enumerate_index.rs

clippy_lints/src/methods/unused_enumerate_index.rs

y21

Two (or three) more things, then this LGTM

y21 · 2024-03-08T16:29:07Z

clippy_lints/src/methods/unused_enumerate_index.rs

+        // iter.enumerate().map(|(_, x)| x)
+        // ^^^^^^^^^^^^^^^^ `recv`, a call to `std::iter::Iterator::enumerate`
+        // ````
+        if !trigger_lint(cx, recv, closure_param.span, elem.span) {


Can we simplify this by always calling expr_or_init and having that trigger_lint logic inlined again? As in,

let recv = expr_or_init(recv); if let ExprKind::MethodCall(_, enumerate_recv, _, enumerate_span) = recv.kind && ...

expr_or_init will just return the passed expression right away if it's not a path to a local, so this should pretty much have the same effect, I think.

We definitely can! trigger_lint was an ugly solution to not duplicate code.

I've changed it and it passes all tests.

y21 · 2024-03-08T16:35:51Z

clippy_lints/src/methods/unused_enumerate_index.rs

+    {
+        // If it is, we can suggest removing the tuple from the closure and the preceding call to
+        // `enumerate`, whose span we can get from the `MethodCall`.
+        span_lint_and_then(


Now that we're potentially using a primary span from somewhere else (.enumerate() on a variable, which may be on a different line), this should IMO use span_lint_hir_and_then, with recv.hir_id as the HirId.

Otherwise I can see how this could lead to confusion when users want to locally allow the lint.
That is, this doesn't work right now, even though the lint points at the enumerate line:

#[allow(clippy::unused_enumerate_index)] let v = (...).enumerate(); v.map();

because span_lint_and_then only respects attributes on the currently active node, which would be the map call.

(would also be a good test)

Indeed! A nice corner case.

I would however assume that I need the hir_id of the return of expr_or_init(cx, recv) rather than that of recv?

Writing the test made me notice that I do not handle the following case:

[].iter().enumerate().map(|_| {})

I'm gonna fix this and add it as a test before I push.

Hmm. No, I am not gonna fix this in this PR, but rather with the next one where I also handle .map(|x| x.1).

y21 · 2024-03-08T17:01:29Z

clippy_lints/src/methods/unused_enumerate_index.rs

+            enumerate_span,
+            "you seem to use `.enumerate()` and immediately discard the index",
+            |diag| {
+                multispan_sugg(


FYI (or maybe this was intentional), multispan_sugg uses a non-machine-applicable suggestion, meaning that tools like rustfix won't try to autofix it (unlike multispan_sugg_with_applicability(Applicability::MachineApplicable)).

But there are also subtle edge cases where the current suggestion breaks code. E.g. it currently removes the parameter's type annotation, which can break type inference (#9122 for one example where this has happened before).

We can leave it as is though, it's correct for this to not be machine applicable as is, and it doesn't need to have one. Just thought I'd bring it up in case you intended this to be an autofixable suggestion.

This was not at all intentional. I would like it to be machine applicable, if that's at all possible. Is there some kind of testing we could do to find these edge cases?

Hm, thinking a bit more about it, the only edge case I can think of is that this removes the parameter's type annotation.
So if we can also replace the tuple type annotation (if it exists) with the second tuple element type then that seems good enough to be machine applicable. I.e.

.enumerate().map(|(_, v): (usize, i32)| ...); // to .map(|v: i32| ...);

You can then use multispan_sugg_with_applicability instead of multispan_sugg, with Applicability::MachineApplicable.

Other than just knowing about edge cases from experience I'm not aware of any way we could find them ^^. There is the lintcheck tool which runs clippy on a set of crates, and it has an option to also try auto applying suggestions, but I also did a lintcheck run with this PR and it didn't find anything.

This should be fixed with my latest push.

The documentation of multispan_sugg_with_applicability states:

rustfix currently doesn’t support the automatic application of suggestions with multiple spans. This is tracked in issue rustfix#141. Suggestions with multiple spans will be silently ignored.

Is this out of date? The issue that is linked is from a repository that was archived, and the issue was closed.

Yeah, that looks outdated

hamirmahal · 2024-03-11T07:53:53Z

clippy_lints/src/methods/unused_enumerate_index.rs

+/// where `map_function` is one of `all`, `any`, `filter_map`, `find_map`, `flat_map`, `for_each` or
+/// `map`.
+///
+/// # Preconditons


Suggested change

/// # Preconditons

/// # Preconditions

Good catch! This'll be fixed in my next push! <3

The lint used to check for patterns looking like: ```rs for (_, x) in some_iter.enumerate() { // Index is ignored } ``` This commit further checks for chained method calls constructs where we can detect that the index is unused. Currently, this checks only for the following patterns: ```rs some_iter.enumerate().map_function(|(_, x)| ..) let x = some_iter.enumerate(); x.map_function(|(_, x)| ..) ``` where `map_function` is one of `all`, `any`, `filter_map`, `find_map`, `flat_map`, `for_each` or `map`. Fixes rust-lang#12411.

y21

One last nit and then it looks good to go

y21 · 2024-03-14T19:09:26Z

clippy_lints/src/methods/unused_enumerate_index.rs

+            // Fallback to `..` if we fail getting either snippet.
+            Some(ty_span) => snippet_opt(cx, elem.span)
+                .and_then(|binding_name| snippet_opt(cx, ty_span).map(|ty_name| format!("{binding_name}: {ty_name}")))
+                .unwrap_or("..".to_string()),


Suggested change

.unwrap_or("..".to_string()),

.unwrap_or_else(|| "..".to_string()),

Do we prefer .unwrap_or_else( because it's lazily evaluated, which might help with performance?

unwrap_or("..".to_string()) will unconditionally construct the String to call the function. This requires a memory allocation, that we will mostly immediately release.

On the other side, unwrap_or_else(|| "..".to_string()) will execute the closure only when we have an Err. Only in this scenario will the closure be executed and the allocation be done.

The observable behaviour will be the same in both cases, but the latter is an optimization. The documentation for unwrap_or explains this as well.

yeah, good summary :)
in this specific case, taking a snippet can realistically never fail, so lazy evaluation lets us avoid allocating it unnecessarily.
The only scenario where that can happen afaik is when taking a snippet from code in the standard library and the user doesn't have the rust-src component installed. The methods lint pass checks that none of the arguments are from expansions, so that won't happen.

Prior to this change, it might be that the lint would remove an explicit type that was necessary for the type system to keep track of types. This should be the last change that prevented this lint to be machine applicable.

y21 · 2024-03-14T22:25:00Z

All looks good, thank you for working on this!

@bors r+

bors · 2024-03-14T22:25:02Z

📌 Commit dadcd94 has been approved by y21

It is now in the queue for this repository.

bors · 2024-03-14T22:26:08Z

⌛ Testing commit dadcd94 with merge 5a11fef...

Ethiraric · 2024-03-14T22:28:43Z

Thank you very much @y21! You've been a great help in my learning of some parts of clippy. Your reviews were extremely detailed and it was a real pleasure working with you!

Keep being awesome! ❤️

bors · 2024-03-14T22:35:41Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 5a11fef to master...

rustbot assigned y21 Mar 7, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 7, 2024

GuillaumeGomez reviewed Mar 7, 2024

View reviewed changes

[unused_enumerate_index]: Use if-let chain

43f4ec3

Ethiraric force-pushed the fix-12411 branch from 9042556 to bc67039 Compare March 7, 2024 18:26

GuillaumeGomez approved these changes Mar 7, 2024

View reviewed changes

y21 reviewed Mar 7, 2024

View reviewed changes

Ethiraric force-pushed the fix-12411 branch from bc67039 to 874b4f3 Compare March 7, 2024 23:24

y21 reviewed Mar 8, 2024

View reviewed changes

Ethiraric force-pushed the fix-12411 branch from 874b4f3 to 49523f2 Compare March 8, 2024 19:55

hamirmahal suggested changes Mar 11, 2024

View reviewed changes

Ethiraric force-pushed the fix-12411 branch from 49523f2 to 0aaba70 Compare March 13, 2024 19:39

y21 approved these changes Mar 14, 2024

View reviewed changes

[unused_enumerate_index]: Keep explicit element type

dadcd94

Prior to this change, it might be that the lint would remove an explicit type that was necessary for the type system to keep track of types. This should be the last change that prevented this lint to be machine applicable.

Ethiraric force-pushed the fix-12411 branch from 0aaba70 to dadcd94 Compare March 14, 2024 22:12

bors merged commit 5a11fef into rust-lang:master Mar 14, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`unused_enumerate_index`]: trigger on method calls #12432

[`unused_enumerate_index`]: trigger on method calls #12432

Ethiraric commented Mar 7, 2024

rustbot commented Mar 7, 2024

GuillaumeGomez Mar 7, 2024

Ethiraric Mar 7, 2024

GuillaumeGomez left a comment

y21 left a comment

y21 left a comment

y21 Mar 8, 2024

Ethiraric Mar 8, 2024

y21 Mar 8, 2024

Ethiraric Mar 8, 2024

Ethiraric Mar 8, 2024

y21 Mar 8, 2024

Ethiraric Mar 8, 2024

y21 Mar 10, 2024 •

edited

Ethiraric Mar 13, 2024

y21 Mar 14, 2024

hamirmahal Mar 11, 2024

Ethiraric Mar 13, 2024

y21 left a comment

y21 Mar 14, 2024

hamirmahal Mar 14, 2024

Ethiraric Mar 14, 2024

y21 Mar 14, 2024

y21 commented Mar 14, 2024

bors commented Mar 14, 2024

bors commented Mar 14, 2024

Ethiraric commented Mar 14, 2024

bors commented Mar 14, 2024

	.unwrap_or("..".to_string()),
	.unwrap_or_else(\|\| "..".to_string()),

[unused_enumerate_index]: trigger on method calls #12432

[unused_enumerate_index]: trigger on method calls #12432

Conversation

Ethiraric commented Mar 7, 2024

rustbot commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez left a comment

Choose a reason for hiding this comment

y21 left a comment

Choose a reason for hiding this comment

y21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y21 Mar 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y21 commented Mar 14, 2024

bors commented Mar 14, 2024

bors commented Mar 14, 2024

Ethiraric commented Mar 14, 2024

bors commented Mar 14, 2024

[`unused_enumerate_index`]: trigger on method calls #12432

[`unused_enumerate_index`]: trigger on method calls #12432

y21 Mar 10, 2024 •

edited