Add new `trivial_map_over_range` lint

[rspencer01]

This lint checks for code that looks like

  let something : Vec<_> = (0..100).map(|_| {
    1 + 2 + 3
  }).collect();

which is more clear as

  let something : Vec<_> = std::iter::repeat_with(|| {
    1 + 2 + 3
  }).take(100).collect();

That is, a map over a range which does nothing with the parameter passed to it is simply a function (or closure) being called n times and could be more semantically expressed using take.

• [x] Followed [lint naming conventions][lint_naming]
• [x] Added passing UI tests (including committed .stderr file)
• [x] cargo test passes locally
• [x] Executed cargo dev update_lints
• [x] Added lint documentation
• [x] Run cargo dev fmt

changelog: new lint: [trivial_map_over_range] restriction

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @y21 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[bors]

:umbrella: The latest upstream changes (presumably #12873) made this pull request unmergeable. Please resolve the merge conflicts.

[y21]

I can agree on that this is more explicit and better expresses intent, but I feel like the range map pattern is pretty common and an accepted way of collecting into vecs and generally considered idiomatic, so I'm a bit unsure about the category here and this being warn-by-default.

Also the Map<Range> type implements DoubleEndedIterator while Take<RepeatWith> does not, and perhaps some other traits I can't think of right now, so that could be a source of potential false positives.

[rspencer01]

Thank you for your speedy response!

I would argue that if you are using a DoubleEndedIterator on a map that is independent of the input, you are probably not expecting the behaviour that you will get. On the other hand, I could see this being used in a dyn impl somewhere[^1]. Your point about other traits is also good.

I always have a mental hiccough when I see this pattern and have to think about why it was written that way (does the author want to highlight that really we aren't taking end-start elements, but we are morally acting on something [start..end]? I know it is quite common though. I have a hunch that this is an attempt at rewriting for _ in 0..n { ... } in a more iterator-y way though (certainly that's why I did it in the past). This doesn't feel like it should be idiomatic, even if it has become so.

At the end of the day though this is perhaps a slightly personal-stylistic and opinionated lint. @y21, would you consider this under a different category?

[^1]: Though it is sad that Take<RepeatWith> doesn't implement DoubleEndedIterator.

[bors]

:umbrella: The latest upstream changes (presumably #10155) made this pull request unmergeable. Please resolve the merge conflicts.

[y21]

The reason for why I brought up the missing DoubleEndedIterator impl is because this warns on code in the regex crate where this lint's suggestion would not work.

Other warnings from a lintcheck run

target/lintcheck/sources/crossbeam-deque-0.8.5/src/deque.rs:42:43 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/crossbeam-utils-0.8.20/src/sync/sharded_lock.rs:104:110 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/rayon-1.10.0/src/iter/par_bridge.rs:89:89 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/rayon-core-1.12.1/src/broadcast/mod.rs:110:111 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/rayon-core-1.12.1/src/broadcast/mod.rs:141:147 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/rayon-core-1.12.1/src/registry.rs:241:251 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/rayon-core-1.12.1/src/registry.rs:254:259 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/rayon-core-1.12.1/src/scope/mod.rs:560:560 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/rayon-core-1.12.1/src/scope/mod.rs:653:656 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/rayon-core-1.12.1/src/sleep/mod.rs:64:64 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/regex-automata-0.4.7/src/nfa/thompson/compiler.rs:1330:1330 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/sharded-slab-0.1.7/src/shard.rs:97:97 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
target/lintcheck/sources/thread_local-1.1.8/src/lib.rs:511:515 clippy::trivial_map_over_range "map of a trivial closure (not dependent on parameter) over a range"
| clippy::trivial_map_over_range                     |    13 |

Most of them are true positives though, except for the mentioned regex case.

---

For the category, I would personally put this in pedantic or even restriction because I still feel like this is such a common pattern (e.g. clippy itself has a suggestion that goes against this lint), but we can also wait until the FCP where other team members have a last look over this and we can see what others think. (It's entirely possible that more people disagree with my opinion :))

[rspencer01]

Thank you for your detailed comments. This is my first clippy lint and my first time working with things like hir, so apologies for the rough edges. I will look at and address your comments, move the lint to pedantic or restriction and then get back to you.

[y21]

No worries, I'll change the label here to clean up my queue, you can write @rustbot ready to change it back (or if you're stuck, feel free to to ask for help).

@rustbot author

[rspencer01]

@rustbot ready

[bors]

:umbrella: The latest upstream changes (presumably #13125) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

• 8ab22a477d3b56ce841bc6b6aa352cb0217a0c5c

[rspencer01]

@rustbot ready

[rspencer01]

Thanks for guiding this PR @y21 . I've made your suggested changes and rebased, but am happy to take further suggestions/nix the lint if we decide against it.

[y21]

The implementation looks good to me. I opened a thread on zulip for the FCP process that we require for new lints: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.3A.20map_with_unused_argument_over_ranges

Would be good if you could squash the commits, but that can also wait until after the FCP is complete since you might end up having to rebase anyway

[rspencer01]

• [x] squash and rebase

[bors]

:umbrella: The latest upstream changes (presumably #13421) made this pull request unmergeable. Please resolve the merge conflicts.

[y21]

It's been a few weeks since the FCP has started. A few things have come up:

• The lint category should be restriction (see the poll)
• Instead of repeat(<element>).take(<count>) we can suggest repeat_n(<element>, <count>) if the MSRV is >=1.83

[xFrednet]

Hey @rspencer01, this is a ping from triage, since there hasn't been any activity in some time. Have you seen the latest comment? Once that part is fixed, this should be good to go :D

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

[rspencer01]

Thanks, this is on my todo list! Sorry its taken me a while to get back to it. I hope to have fixes for the two comments soon.

[rspencer01]

Checks seem to be failing due to an unrelated GLIBC error.

@rustbot ready

[y21]

Hm, might be spurious? Let's see what's up with that error. (I'd also like to see the lintcheck results before merging)

@bors try

[bors]

:hourglass: Trying commit 208a7e64a936d7cabf828fedb5166f5d690516d4 with merge 6a37ddc4057b5282cb4ff8e121e34aca01e233b6...

[bors]

:sunny: Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Build commit: 6a37ddc4057b5282cb4ff8e121e34aca01e233b6 (6a37ddc4057b5282cb4ff8e121e34aca01e233b6)

[y21]

The lintcheck failure is weird, no idea what's up with that. Maybe try another rebase? Haven't seen this in other PRs

[bors]

:umbrella: The latest upstream changes (presumably #13437) made this pull request unmergeable. Please resolve the merge conflicts.

[y21]

Looks all good to me now. Can you rebase one last time? Then I can r+

[y21]

Thanks!

@bors r+

[bors]

:pushpin: Commit acc3842d438028bf5649e9715b74791ea4ec6c83 has been approved by y21

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit acc3842d438028bf5649e9715b74791ea4ec6c83 with merge 15ad8245b23cdb537454b3fb6f34d5c96a6a35ea...

[bors]

:sunny: Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Approved by: y21 Pushing 15ad8245b23cdb537454b3fb6f34d5c96a6a35ea to master...