Bad suggestion and false positive `redundant_iter_cloned`

[ilyagr]

Summary

After updating nightly rust today, I got a bad lint for the code at https://github.com/jj-vcs/jj/tree/a5b61b2cc331e8b8c7636b706ca1be9f1a33815b.

See jj-vcs/jj#7557 for details (mostly copied below)

Lint Name

redundant_iter_cloned

Reproducer

I tried this code:

 let commits: std::collections::HashMap<CommitId, _> = bookmark_list_items
            .iter()
            .filter_map(|item| item.primary.target().added_ids().next())  // Iterator of type &CommitId
            .cloned()
            .map(|commit_id| {
                store
                    .get_commit(&commit_id)
                    .map(|commit| (commit_id, commit.store_commit().clone()))
            })
            .try_collect()?;

I saw this happen:

warning: unneeded cloning of iterator items
   --> cli/src/commands/bookmark/list.rs:251:19
    |
251 |           commits = bookmark_list_items
    |  ___________________^
252 | |             .iter()
253 | |             .filter_map(|item| item.primary.target().added_ids().next())
254 | |             .cloned()
...   |
258 | |                     .map(|commit| (commit_id, commit.store_commit().clone()))
259 | |             })
    | |______________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_iter_cloned
    = note: `#[warn(clippy::redundant_iter_cloned)]` on by default
help: try
    |
253 ~             .filter_map(|item| item.primary.target().added_ids().next())
254 +             .map(|commit_id| {
255 +                 store
256 +                     .get_commit(&commit_id)
257 +                     .map(|commit| (commit_id, commit.store_commit().clone()))
258 +             })
    |

Following the suggestion and removing the .cloned() results in an error:

The error:

error[E0277]: a value of type `std::collections::HashMap<jj_lib::backend::CommitId, std::sync::Arc<jj_lib::backend::Commit>>` cannot be built from an iterator over elements of type `(&jj_lib::backend::CommitId, std::sync::Arc<jj_lib::backend::Commit>)`
    --> cli/src/commands/bookmark/list.rs:259:14
     |
 259 |             .try_collect()?;
     |              ^^^^^^^^^^^ value of type `std::collections::HashMap<jj_lib::backend::CommitId, std::sync::Arc<jj_lib::backend::Commit>>` cannot be built from `std::iter::Iterator<Item=(&jj_lib::backend::CommitId, std::sync::Arc<jj_lib::backend::Commit>)>`
     |
     = help: the trait `FromIterator<(&jj_lib::backend::CommitId, std::sync::Arc<_>)>` is not implemented for `std::collections::HashMap<jj_lib::backend::CommitId, std::sync::Arc<jj_lib::backend::Commit>>`
             but trait `FromIterator<(jj_lib::backend::CommitId, std::sync::Arc<_>)>` is implemented for it
     = help: for that trait implementation, expected `jj_lib::backend::CommitId`, found `&jj_lib::backend::CommitId`
     = note: required for `Result<HashMap<CommitId, Arc<Commit>>, BackendError>` to implement `FromIterator<Result<(&CommitId, Arc<Commit>), BackendError>>`
note: required by a bound in `itertools::Itertools::try_collect`
    --> /Users/ilyagr/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/itertools-0.14.0/src/lib.rs:2381:23
     |
2378 |     fn try_collect<T, U, E>(self) -> Result<U, E>
     |        ----------- required by a bound in this associated function
...
2381 |         Result<U, E>: FromIterator<Result<T, E>>,
     |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Itertools::try_collect`
     = note: the full name for the type has been written to '/Users/ilyagr/dev/jj/target/debug/deps/jj_cli-442101cf9493e967.long-type-2991360816845051416.txt'
     = note: consider using `--verbose` to print the full type name to the console

---

The following diff satisfies clippy, but it doesn't seem much better than the original version, and certainly differs from what clippy suggested.

diff --git a/cli/src/commands/bookmark/list.rs b/cli/src/commands/bookmark/list.rs
index bd362dc9bf..bb9f26bc07 100644
--- a/cli/src/commands/bookmark/list.rs
+++ b/cli/src/commands/bookmark/list.rs
@@ -251,11 +251,10 @@
         commits = bookmark_list_items
             .iter()
             .filter_map(|item| item.primary.target().added_ids().next())
-            .cloned()
             .map(|commit_id| {
                 store
-                    .get_commit(&commit_id)
-                    .map(|commit| (commit_id, commit.store_commit().clone()))
+                    .get_commit(commit_id)
+                    .map(|commit| (commit_id.clone(), commit.store_commit().clone()))
             })
             .try_collect()?;
     }

Version

rustc 1.92.0-nightly (0be8e1608 2025-09-19)
binary: rustc
commit-hash: 0be8e16088894483a7012c5026c3247c14a0c3c2
commit-date: 2025-09-19
host: aarch64-apple-darwin
release: 1.92.0-nightly
LLVM version: 21.1.1

Additional Labels

I-suggestion-causes-error

No response

[samueltardieu]

Note: part of the nursery allow-by-default lint redundant_clone has been split into a warn-by-default perf lint redundant_iter_cloned in #15277, which is why it appears now. The problem was present before the split as well.

[samueltardieu]

This should be fixed before and merged in on Oct 15 or later, or it would need to be backported after this date to make sure it doesn't go in Rust 1.92 as warn-by-default.

[ilyagr]

@yuja pointed out in jj-vcs/jj#7557 that, while clippy's suggestion was unhelpful, the refactor it forced me to do (in the diff above) does make things slightly better. It avoids the clone if .get_commit() returns Err.