Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new trivial_map_over_range lint #13034

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

rspencer01
Copy link
Contributor

@rspencer01 rspencer01 commented Jul 3, 2024

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.

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

changelog: new lint: [trivial_map_over_range] restriction

@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2024

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 3, 2024
@bors
Copy link
Collaborator

bors commented Jul 4, 2024

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

@y21
Copy link
Member

y21 commented Jul 4, 2024

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
Copy link
Contributor Author

rspencer01 commented Jul 4, 2024

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 somewhere1. 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?

Footnotes

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

@bors
Copy link
Collaborator

bors commented Jul 4, 2024

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

clippy_lints/src/trivial_map_over_range.rs Outdated Show resolved Hide resolved
clippy_lints/src/trivial_map_over_range.rs Outdated Show resolved Hide resolved
clippy_lints/src/trivial_map_over_range.rs Outdated Show resolved Hide resolved
clippy_lints/src/trivial_map_over_range.rs Outdated Show resolved Hide resolved
clippy_lints/src/trivial_map_over_range.rs Outdated Show resolved Hide resolved
tests/ui/trivial_map_over_range.stderr Outdated Show resolved Hide resolved
clippy_lints/src/trivial_map_over_range.rs Outdated Show resolved Hide resolved
tests/ui/repeat_vec_with_capacity.stderr Outdated Show resolved Hide resolved
tests/ui/trivial_map_over_range.rs Outdated Show resolved Hide resolved
clippy_lints/src/trivial_map_over_range.rs Outdated Show resolved Hide resolved
@y21
Copy link
Member

y21 commented Jul 4, 2024

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
Copy link
Contributor Author

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
Copy link
Member

y21 commented Jul 5, 2024

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

@rustbot 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 Jul 5, 2024
@rspencer01
Copy link
Contributor Author

@rustbot ready

@rustbot 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 Jul 6, 2024
@bors
Copy link
Collaborator

bors commented Jul 25, 2024

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

Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking a bit longer to respond here, have been a bit busy recently 😓

clippy_lints/src/methods/mod.rs Show resolved Hide resolved
tests/ui/trivial_map_over_range.stderr Outdated Show resolved Hide resolved
clippy_lints/src/trivial_map_over_range.rs Outdated Show resolved Hide resolved
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Aug 11, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 11, 2024

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:

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Aug 11, 2024
@rspencer01
Copy link
Contributor Author

@rustbot ready

Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three small things then I think this should be good :)

@rspencer01
Copy link
Contributor Author

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.

@bors
Copy link
Collaborator

bors commented Sep 19, 2024

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

@y21
Copy link
Member

y21 commented Sep 26, 2024

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 xFrednet added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) labels Oct 13, 2024
@xFrednet
Copy link
Member

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
Copy link
Contributor Author

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 rspencer01 force-pushed the trivial_map_over_range branch 3 times, most recently from 52d35de to 208a7e6 Compare October 16, 2024 21:06
@rspencer01
Copy link
Contributor Author

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

@rustbot ready

@rustbot 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 Oct 16, 2024
@y21
Copy link
Member

y21 commented Oct 18, 2024

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 added a commit that referenced this pull request Oct 18, 2024
Add new `trivial_map_over_range` lint

This lint checks for code that looks like
```rust
  let something : Vec<_> = (0..100).map(|_| {
    1 + 2 + 3
  }).collect();
```
which is more clear as
```rust
  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`
@bors
Copy link
Collaborator

bors commented Oct 18, 2024

⌛ Trying commit 208a7e6 with merge 6a37ddc...

@bors
Copy link
Collaborator

bors commented Oct 18, 2024

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

/// let f : Vec<_> = std::iter::repeat( 3 + 1 ).take(10).collect();
/// ```
///
/// ### Known Issues
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good news: Take<Repeat> and Take<RepeatWith> now implement DoubleEndedIterator and ExactSizeIterator as of 1.82.0, so I think we can remove this note?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I'd missed that in the notes. Does Take<RepeatWith> implement DoubleEndedIterator though? The playground (and release notes) suggest not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it doesn't. I only actually looked at ExactSizeIterator and made wrong assumptions about DoubleEndedIterator

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_config/src/msrvs.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches/single_match.rs Show resolved Hide resolved
Comment on lines 122 to 127
ex.span.shrink_to_hi(),
if use_take {
format!(".take({count})")
} else {
String::new()
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These empty replacements (empty suggestion and empty span) trigger a debug assertion in the compiler, so this needs to conditionally push the replacement into the vec

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be easier to just filter the vec. Let me know if you disagree stylistically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the filtering is easier than something like

let mut parts = vec![
    (receiver.span.to(method_call_span), format!("std::iter::{method_to_use_name}")),
    new_span,
];
if use_take {
    parts.push((ex.span.shrink_to_hi(), format!(".take({count})")));
}

I also find it a lot clearer because the current filtering makes me think that there could be other replacements that are empty when it's (as far as I can tell?) really only the last one, and the last one can only be empty because of an explicit String::new() so it would be IMO clearer to just not create an empty replacement like that in the first place and only push a non-empty replacement if necessary.

(The debug assertion also only exists to find bugs in suggestions, so it feels more like a workaround)

@y21
Copy link
Member

y21 commented Oct 27, 2024

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

@bors
Copy link
Collaborator

bors commented Oct 29, 2024

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

@y21
Copy link
Member

y21 commented Oct 29, 2024

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

This lint checks for code that looks like
```rust
  let something : Vec<_> = (0..100).map(|_| {
    1 + 2 + 3
  }).collect();
```
which is more clear as
```rust
  let something : Vec<_> = std::iter::repeat_with(|| {
    1 + 2 + 3
  }).take(100).collect();
```
or
```rust
  let something : Vec<_> =
      std::iter::repeat_n(1 + 2 + 3, 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`.
@y21
Copy link
Member

y21 commented Oct 29, 2024

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 29, 2024

📌 Commit acc3842 has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 29, 2024

⌛ Testing commit acc3842 with merge 15ad824...

@bors
Copy link
Collaborator

bors commented Oct 29, 2024

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

@bors bors merged commit 15ad824 into rust-lang:master Oct 29, 2024
11 checks passed
@rspencer01
Copy link
Contributor Author

Again, thank you @y21 for your guidance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants