New lint: `iter_overeager_cloned` #8203

pmnoxx · 2021-12-31T06:45:00Z

changelog: New lint: [iter_overeager_cloned]

rust-highfive · 2021-12-31T06:45:03Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

llogiq

This looks good to me, modulo running update_lints and checking in the changes. I have an idea how we could make that lint even better. If you want to do that within this PR, go ahead, otherwise let me know and I'll r+.

llogiq · 2021-12-31T07:00:11Z

clippy_lints/src/methods/cloned_last.rs

+            expr.span,
+            msg,
+            "try this",
+            format!("{}.last().cloned()", iter_snippet),


If the .last() is unwrap()ed or ?d, we could even suggest to .clone() that. Just a suggestion for extending the lint, this won't stop us from merging.

@llogiq That's a great suggestion.

I see: for Option<> / Result<>.
recv.cloned().unwrap()
could be
recv.unwrap().clone.

I see that cloned should be moved furthermost the chain as long as possible, not just for last.
For example here:

vec.iter().cloned().filter(...).skip(5).take(10).last().unwrap()

That could be just written as

let val = vec.cloned().filter(...).skip(5).take(10).last().unwrap().clone()

This may even allow us to drop the clone() entirely, in some cases, when clone() is not needed.

It may be a good idea to propagate clone from before, to after

last

take

skip

next.

unwrap

filter // has lambda, so requires some more checks

find.// has lambda,so requires some more checks

map - vec.iter().cloned().map(|x|x.len()) <-- this probably doesn't need clone either.

foreach - maybe? vec.iter().cloned().foreach_each(|x|println!{"{}", x)) <-- this doesn't need clone

... probably some others as well

map/for_each/try_foreach require detecting whenever lambda can accept reference instead

The question is, should each be a separate lint, or should they be combined together?

I think your example is a bit off, but otherwise full ack.

I'd be OK with multiple lints if there are useful distinctions to make. E.g. if one part of the check can have false positives, it would be a good idea to put that into a different nursery lint. Or if one part could reasonably be style and another perf. Otherwise, putting all the checks into one lint seems natural. One thing I that comes to mind is that we already have the needless_clone lint, so we could add cases where the cloned can be removed outright to that, making the rest early_iter_cloned or some such.

Again, I'm also OK if you restrict the scope of this PR and do a followup later. Have fun!

clippy_lints/src/methods/cloned_last.rs

llogiq · 2021-12-31T11:27:51Z

I'd be OK with merging this with the .cloned().next() replaced by .cloned().last() in the two comments, or if you want you can tackle making the lint more powerful. Ping me if you need any help!

tests/ui/cloned_last.rs

pmnoxx · 2021-12-31T12:22:02Z

I'd be OK with merging this with the .cloned().next() replaced by .cloned().last() in the two comments, or if you want you can tackle making the lint more powerful. Ping me if you need any help!

I added implementation for

skip
take
count
last
next
nth

TODO

update documentation
change lint name

Questions:

what name should be used for new lint? needless_cloned ?
should it be applied to clippy::all or clippy::nursery
maybe this should only be applies to types ,which are bigger than 8 bytes, as it's more efficient to deference types 8 bytes and shorter

I think that support for .cloned().next().unwrap() should be in a separate PR.

Fixing option_receiver.cloned().unwrap() or result_receiver.cloned().unwrap(), shouldn't be hard to to either, I'll take a look at it later.

llogiq · 2021-12-31T12:27:53Z

My name suggestion as per comment above is: Add the cases where the cloned call can be removed outright to needless_clone, the others could be needless_iter_cloned or early_iter_cloned or overeager_iter_cloned. For last and skip, the latter seems to be most appropriate.

llogiq · 2021-12-31T12:29:32Z

Oh, and I would like to run some tests before deciding whether to put it in style, perf or nursery. If we find false positives or lots of noise, I'd suggest the latter. Otherwise I'd be OK with either of the former.

tests/ui/cloned_last.rs

llogiq · 2021-12-31T13:41:34Z

clippy_lints/src/methods/mod.rs

@@ -107,6 +108,29 @@ declare_clippy_lint! {
    "used `cloned` where `copied` could be used instead"
 }

+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of `_.cloned().last()`.


The documentation should also reflect the complete lint.

@llogiq I updated the documentation.

clippy_lints/src/methods/mod.rs

pmnoxx · 2021-12-31T14:15:19Z

My name suggestion as per comment above is: Add the cases where the cloned call can be removed outright to needless_clone, the others could be needless_iter_cloned or early_iter_cloned or overeager_iter_cloned. For last and skip, the latter seems to be most appropriate.

I agree, as a use of clippy I think like to see it divided maybe into

needless_cloned - count, len, is_empty - cloned is remoed
overeager_iter_cloned - skip, take, last, nth - affects performance; cloned is used less often` - has side effect
early_iter_cloned - next - clone can be done later, perhaps this will allow further optimizations later on; - moves clone further into the chain, may allow further performance optimizations.
filter - can be added later to overeager_iter_cloned
map. flat_map - not sure, sometimes may be map / flat_map doesn't need cloned version of the item.

llogiq · 2021-12-31T14:28:30Z

I'm ok with the needless_clone / overeager_iter_cloned split.

I would avoid adding needless_cloned, because a) that's very similar to needless_clone which we already have and b) also conceptually similar enough we may as well just extend that lint.

I don't think the early_iter_cloned warrants it's own lint. As I explained above, code changes, and doing it now has no drawbacks and benefits compile time even if only slightly.

Note that we'll have to extend the needless_clone documentation to include the cases if we go that route.

clippy_lints/src/methods/iter_overeager_cloned.rs

pmnoxx · 2021-12-31T19:19:32Z

@llogiq I think some more changes are needed.

--) Cloning small values, references of references is actually beneficial.

Consider following

/// cloned() should not be removed
let _ = [0u8, 0u8].iter().cloned()....
/// cloned() should not be removed
let _ = Vec::<&&String>.iter().cloned()....

Cloning &u8 is a good idea, this will improve performance, as there is no need to do dereferencing.
Same thing can be said about &&String.

I think we should apply the same rules as clippy_lints/src/pass_by_ref_or_value.rs uses.

If the type after cloning is <=8 bytes, then we should keep the cloned, otherwise postpone it.
For example, changing &&String to &String, keep the size the same, but removes one extra deference needed.

llogiq · 2021-12-31T19:36:42Z

That should be .copied(), not .cloned(), but we already have that lint. Regardless, not linting if the element type is of size <= size_of::<usize>() makes sense.

pmnoxx · 2021-12-31T19:48:24Z

That should be .copied(), not .cloned(), but we already have that lint. Regardless, not linting if the element type is of size <= size_of::<usize>() makes sense.

I added a test, and I see that this would be applied automatically:

diff --git i/tests/ui/iter_overeager_cloned.fixed w/tests/ui/iter_overeager_cloned.fixed
index 01b64ff7a..7295ad871 100644
--- i/tests/ui/iter_overeager_cloned.fixed
+++ w/tests/ui/iter_overeager_cloned.fixed
@@ -39,4 +39,7 @@ fn main() {
 
     // Not implemented yet
     let _ = vec.iter().cloned().any(|x| x.len() == 1);
+
+    // Should probably stay as it is.
+    let _ = [0, 1, 2, 3, 4].iter().take(10).cloned();
 }
diff --git i/tests/ui/iter_overeager_cloned.rs w/tests/ui/iter_overeager_cloned.rs
index fede4c733..dd04e33a4 100644
--- i/tests/ui/iter_overeager_cloned.rs
+++ w/tests/ui/iter_overeager_cloned.rs
@@ -41,4 +41,7 @@ fn main() {
 
     // Not implemented yet
     let _ = vec.iter().cloned().any(|x| x.len() == 1);
+
+    // Should probably stay as it is.
+    let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);
 }
diff --git i/tests/ui/iter_overeager_cloned.stderr w/tests/ui/iter_overeager_cloned.stderr
index 4a58687f4..343614e75 100644
--- i/tests/ui/iter_overeager_cloned.stderr
+++ w/tests/ui/iter_overeager_cloned.stderr
@@ -54,5 +54,11 @@ LL ~     let _ = [Some(Some("str".to_string())), Some(Some("str".to_string()))]
 LL ~         .iter().flatten().cloned();
    |
 
-error: aborting due to 7 previous errors
+error: called `cloned().take(...)` on an `Iterator`. It may be more efficient to call`.take(...).cloned()` instead
+  --> $DIR/iter_overeager_cloned.rs:46:13
+   |
+LL |     let _ = [0, 1, 2, 3, 4].iter().cloned().take(10);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `[0, 1, 2, 3, 4].iter().take(10).cloned()`
+
+error: aborting due to 8 previous errors

clippy_lints/src/methods/mod.rs

llogiq · 2022-01-01T09:07:32Z

Cool stuff! My remaining nits are with the documentation, otherwise this looks great.

llogiq

A few more nits to fix. Do you need help with the size check?

clippy_lints/src/methods/iter_overeager_cloned.rs

pmnoxx · 2022-01-01T20:50:23Z

A few more nits to fix. Do you need help with the size check?

I'm still thinking whenever the check is needed. For example, for

count -

pass_by_ref_or_value

I'm not entirely convinced whenever the check is needed. For example.

[0u8, 1u8, 2u8, 3u8].iter().cloned().count() /// cloned() should be removed
 [0, 1, 2, 3, 4].last().cloned().last();          /// it's better to not deference values, and move cloned() last
 [0, 1, 2, 3, 4].last().cloned().nth(40);          /// same here
 [0, 1, 2, 3, 4].last().cloned().skip(40);          /// same here
 [0, 1, 2, 3, 4].last().cloned().next();     // probably it's better not to print - not sure
 [0, 1, 2, 3, 4].last().cloned().take(50);     // probably it's better not to print - not sure
 [0, 1, 2, 3, 4].last().cloned().flatten();     // probably it's better not to print - not sure

@llogiq I'm not sure how to get the type from the iterator. I'll have to spend some time and figure out a better way to navigate through code. Type hinting doesn't seem to work correctly with Rust plugin for Intellij, which makes things harder.

My understanding is that the receiver is some generic iterator, that implements Iterator<Item = SomeItem>. I'm not sure yet how to do get it.

Anyway, I should probably take a look at CLONED_INSTEAD_OF_COPIED.

llogiq · 2022-01-04T23:17:54Z

I just had a discussion with someone from libs. There's a Rust PR #90209 that aims to remove more clones through specialization. Apparently the libs team thinks that as there never was any guarantee of current behavior, they're free to change it. Which is fine by me. That somewhat reduces the need for this lint, but doesn't render it moot, because as we all know specialization is a fickle beast and may sometimes fail to apply. So we should still add this lint, but perhaps note the possibility of not seeing a perf win in the docs.

bors · 2022-01-12T14:18:05Z

☔ The latest upstream changes (presumably #8198) made this pull request unmergeable. Please resolve the merge conflicts.

llogiq · 2022-01-16T14:16:02Z

That PR was closed, so we're left with the clippy lint and some docs on Iterator::cloned I just wrote. Hopefully this will suffice to have people avoid most needless clones on iterating.

pmnoxx · 2022-01-16T15:00:55Z

That PR was closed, so we're left with the clippy lint and some docs on Iterator::cloned I just wrote. Hopefully this will suffice to have people avoid most needless clones on iterating.

Ok, just let me know what you think is best. Whenever we should ships this PR as it is. Or improve it, or close this PR.

llogiq · 2022-01-16T17:26:48Z

Re-reading this, I think we can look into refining the lint later. For now let's just merge it.

@bors r+

bors · 2022-01-16T17:26:49Z

📌 Commit e209ff8 has been approved by llogiq

bors · 2022-01-16T17:26:53Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout piotr-next-lint (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self piotr-next-lint --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging clippy_lints/src/methods/mod.rs
CONFLICT (content): Merge conflict in clippy_lints/src/methods/mod.rs
Auto-merging clippy_lints/src/lib.register_perf.rs
Auto-merging clippy_lints/src/lib.register_lints.rs
Auto-merging clippy_lints/src/lib.register_all.rs
Auto-merging CHANGELOG.md
Automatic merge failed; fix conflicts and then commit the result.

llogiq · 2022-01-16T17:27:01Z

@bors r-

llogiq · 2022-01-16T17:27:30Z

I overlooked that you need to rebase the PR on current master. Shouldn't be too hard.

pmnoxx · 2022-01-16T18:01:07Z

I overlooked that you need to rebase the PR on current master. Shouldn't be too hard.

I rebased the PR.

llogiq · 2022-01-16T20:02:24Z

@bors r+

bors · 2022-01-16T20:02:25Z

📌 Commit 36396c6 has been approved by llogiq

bors · 2022-01-16T20:02:31Z

⌛ Testing commit 36396c6 with merge 93cad4a...

bors · 2022-01-16T20:20:24Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 93cad4a to master...

rust-highfive assigned llogiq Dec 31, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 31, 2021

pmnoxx mentioned this pull request Dec 31, 2021

New lint: iter_overeager_cloned #8202

Closed

pmnoxx force-pushed the piotr-next-lint branch from 3360d24 to ec208b4 Compare December 31, 2021 06:45

llogiq approved these changes Dec 31, 2021

View reviewed changes

pmnoxx commented Dec 31, 2021

View reviewed changes

clippy_lints/src/methods/cloned_last.rs Outdated Show resolved Hide resolved

pmnoxx force-pushed the piotr-next-lint branch from 9c9f804 to 04619ef Compare December 31, 2021 08:37

llogiq reviewed Dec 31, 2021

View reviewed changes

tests/ui/cloned_last.rs Outdated Show resolved Hide resolved

llogiq reviewed Dec 31, 2021

View reviewed changes

tests/ui/cloned_last.rs Outdated Show resolved Hide resolved

llogiq reviewed Dec 31, 2021

View reviewed changes

pmnoxx force-pushed the piotr-next-lint branch from 8da266c to baad6c2 Compare December 31, 2021 15:25

pmnoxx commented Dec 31, 2021

View reviewed changes

clippy_lints/src/methods/iter_overeager_cloned.rs Outdated Show resolved Hide resolved

pmnoxx force-pushed the piotr-next-lint branch 3 times, most recently from 9f175f5 to 77dd958 Compare December 31, 2021 17:32

llogiq reviewed Jan 1, 2022

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

llogiq reviewed Jan 1, 2022

View reviewed changes

clippy_lints/src/methods/iter_overeager_cloned.rs Outdated Show resolved Hide resolved

clippy_lints/src/methods/iter_overeager_cloned.rs Outdated Show resolved Hide resolved

pmnoxx force-pushed the piotr-next-lint branch from f1b6ebe to ecf660f Compare January 2, 2022 07:28

pmnoxx force-pushed the piotr-next-lint branch 2 times, most recently from 58fdacd to cf95b86 Compare January 3, 2022 14:09

pmnoxx changed the title ~~New lint: cloned_next~~ New lint: iter_overeager_cloned Jan 4, 2022

llogiq mentioned this pull request Jan 6, 2022

Elide clone operation when calling iter::Cloned::advance_{back}_by() rust-lang/rust#90209

Closed

New line: cloned_next

1c9b31d

pmnoxx force-pushed the piotr-next-lint branch from e209ff8 to 1c9b31d Compare January 16, 2022 17:59

pmnoxx added 2 commits January 16, 2022 10:19

Fix clippy warnings

d48d247

Fix tests

36396c6

pmnoxx force-pushed the piotr-next-lint branch from d5075d9 to 36396c6 Compare January 16, 2022 18:33

bors merged commit 93cad4a into rust-lang:master Jan 16, 2022

pmnoxx deleted the piotr-next-lint branch January 16, 2022 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint: `iter_overeager_cloned` #8203

New lint: `iter_overeager_cloned` #8203

pmnoxx commented Dec 31, 2021 •

edited by llogiq

Loading

rust-highfive commented Dec 31, 2021

llogiq left a comment •

edited

Loading

llogiq Dec 31, 2021 •

edited

Loading

pmnoxx Dec 31, 2021 •

edited

Loading

llogiq Dec 31, 2021

llogiq commented Dec 31, 2021

pmnoxx commented Dec 31, 2021 •

edited

Loading

llogiq commented Dec 31, 2021

llogiq commented Dec 31, 2021

llogiq Dec 31, 2021

pmnoxx Dec 31, 2021

pmnoxx commented Dec 31, 2021

llogiq commented Dec 31, 2021

pmnoxx commented Dec 31, 2021

llogiq commented Dec 31, 2021

pmnoxx commented Dec 31, 2021

llogiq commented Jan 1, 2022

llogiq left a comment

pmnoxx commented Jan 1, 2022 •

edited

Loading

llogiq commented Jan 4, 2022

bors commented Jan 12, 2022

llogiq commented Jan 16, 2022

pmnoxx commented Jan 16, 2022

llogiq commented Jan 16, 2022

bors commented Jan 16, 2022

bors commented Jan 16, 2022

llogiq commented Jan 16, 2022

llogiq commented Jan 16, 2022

pmnoxx commented Jan 16, 2022

llogiq commented Jan 16, 2022

bors commented Jan 16, 2022

bors commented Jan 16, 2022

bors commented Jan 16, 2022

New lint: iter_overeager_cloned #8203

New lint: iter_overeager_cloned #8203

Conversation

pmnoxx commented Dec 31, 2021 • edited by llogiq Loading

rust-highfive commented Dec 31, 2021

llogiq left a comment • edited Loading

Choose a reason for hiding this comment

llogiq Dec 31, 2021 • edited Loading

Choose a reason for hiding this comment

pmnoxx Dec 31, 2021 • edited Loading

Choose a reason for hiding this comment

llogiq Dec 31, 2021

Choose a reason for hiding this comment

llogiq commented Dec 31, 2021

pmnoxx commented Dec 31, 2021 • edited Loading

llogiq commented Dec 31, 2021

llogiq commented Dec 31, 2021

llogiq Dec 31, 2021

Choose a reason for hiding this comment

pmnoxx Dec 31, 2021

Choose a reason for hiding this comment

pmnoxx commented Dec 31, 2021

llogiq commented Dec 31, 2021

pmnoxx commented Dec 31, 2021

llogiq commented Dec 31, 2021

pmnoxx commented Dec 31, 2021

llogiq commented Jan 1, 2022

llogiq left a comment

Choose a reason for hiding this comment

pmnoxx commented Jan 1, 2022 • edited Loading

llogiq commented Jan 4, 2022

bors commented Jan 12, 2022

llogiq commented Jan 16, 2022

pmnoxx commented Jan 16, 2022

llogiq commented Jan 16, 2022

bors commented Jan 16, 2022

bors commented Jan 16, 2022

llogiq commented Jan 16, 2022

llogiq commented Jan 16, 2022

pmnoxx commented Jan 16, 2022

llogiq commented Jan 16, 2022

bors commented Jan 16, 2022

bors commented Jan 16, 2022

bors commented Jan 16, 2022

New lint: `iter_overeager_cloned` #8203

New lint: `iter_overeager_cloned` #8203

pmnoxx commented Dec 31, 2021 •

edited by llogiq

Loading

llogiq left a comment •

edited

Loading

llogiq Dec 31, 2021 •

edited

Loading

pmnoxx Dec 31, 2021 •

edited

Loading

pmnoxx commented Dec 31, 2021 •

edited

Loading

pmnoxx commented Jan 1, 2022 •

edited

Loading