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 lint recommending using std::iter::once and std::iter::empty #9187

Merged
merged 6 commits into from
Aug 14, 2022

Conversation

sgued
Copy link
Contributor

@sgued sgued commented Jul 16, 2022

changelog: [`iter_once`]: add new lint
changelog: [`iter_empty`]: add new lint

fixes #9186

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

The lint doesn't really follow the naming conventions. I don't have any better idea so I'm open to suggestions.

Sorry, something went wrong.

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 16, 2022
clippy_lints/src/iter_once_empty.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_once_empty.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_once_empty.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_once_empty.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_once_empty.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_once_empty.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_once_empty.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jul 18, 2022

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

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
///
/// ### Known problems
///
/// The type of the resulting iterator might become incompatible with its usage
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little concerned about this. From how many allows are necessary in Clippy already, I think this is a bigger problem, that has to be addressed before we can merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does clippy have a way to know whether a suggestion type-checks?
Maybe we could limit it to expressions within a chain, which would likely lead to less false positives but not 0. It would also create false negatives in cases like:

let iter = if some_condition {
    [a].into_iter()
} else {
    [b].into_iter()
}
  
iter.chain(other_iter)

Copy link
Member

Choose a reason for hiding this comment

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

Do you have examples where applying the suggestion does not type check? Looking at all the allows you added to Clippy, I don't really see why you couldn't use iter::empty there.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, if I'd had to guess, I would say that this fails type checking when using iter::empty because it can't infer the type anymore? I don't think this is a problem for iter_once though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Say you have something like this:

let iter = match smth {
    "a" => [1,2].iter(),
    "b" => [3].iter(),
    _ => [].iter(),
};

The first branch forces iter to be of type slice::Iter<i32> but suggestions would be to change it to:

let iter = match smth {
    "a" => [1,2].iter(),
    "b" => iter::once(&3),
    _ => iter::empty(),
};

so now iter must be at the same time Once<i32>, slice::Iter<i32> and Empty<i32>

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Running it on the top 50 crates, those lints were only emitted on serde_derive and all 3 cases were fixable. Maybe I making a mountain out of a molehill here.

@flip1995 have you ensured that the lint was enabled when you ran it on those crates, or could it be that they haven't enabled pedantic lints?

Copy link
Member

Choose a reason for hiding this comment

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

I used @mikerites tool to check those lints. I'm pretty sure they were enabled. I mean one of those triggered on serde_derive.

Copy link
Contributor Author

@sgued sgued Jul 23, 2022

Choose a reason for hiding this comment

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

I ran the tool on the top 200 crates to have a better idea.

The results are:

Crate Count False Positive (FP) Partial FP
chrono-0.4.19 1 0 0
petgraph-0.6.2 2 2 0
rayon-1.5.3 1 1 0
serde_derive-1.0.140 3 0 0
synstructure-0.12.6 1 0 0
tokio-1.20.0 2 0 2
unicode-normalization-0.1.21 6 6 1
clippy 7 2 0

Partial FP means the lint is right to trigger but the suggestion doesn't typecheck.

petgraph and rayon

False positive, a slice::Iter(Mut) is needed

serde_derive (1 2 3) and chrono

Each time the perfect true positive for this lint

tokio

Two "false positive". In the sense that the suggestion doesn't TypeCheck but by changing some type annotations it does. This could have required a breaking change if it hadn't been on some private impls though.

unicode-normalization

6 times the same false positive for type checking, because a trait is implemented specifically for Option::IntoIter, but it might be interesting for them to implement the trait also for iter::Once. I don't know how the crate is used and whether it would be useful though.

Clippy itself (in this PR)

The lint is fixed 5 times and ignored twice. There was one "partial false positive" which required manually writing the type of Empty<ty::subst::GenericArg<'_>>, which was a bit of a pain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3b45a28 based on this fixed the FP in petgraph and clippy itself. The Lint still triggers in Tokio and unicode-normalization though.

clippy_lints/src/methods/iter_once_empty.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/iter_once_empty.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/iter_once_empty.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/iter_once_empty.rs Outdated Show resolved Hide resolved
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

I'll be thinking about a a lint name before merging this.

@flip1995 flip1995 added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Jul 26, 2022
@flip1995
Copy link
Member

Thanks for doing the analysis (#9187 (comment)). I nominated this PR for the meeting this evening to figure out if we want to get this lint into pedantic or have it in nursery for starters.

The meeting is at 3pm UTC: https://everytimezone.com/s/deeec323

@flip1995
Copy link
Member

So in the Clippy meeting, we decided to place those lints in nursery for now and lift them up to pedantic once at least the most obvious FPs are addressed. Jarcho left some instructions on how this could be done here. However I don't want to block this PR on this.

@sgued
Copy link
Contributor Author

sgued commented Jul 28, 2022

I see on Zulip you proposed iter_on_empty_collection for the name of iter_empty. I like it. I suggest iter_on_singleton for iter_once

@flip1995
Copy link
Member

flip1995 commented Jul 29, 2022

I think of something different when I think about singletons. How about iter_on_single_items?

also make sure to use the plural form, so iter_on_empty_collections. That sounds better with allow.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

This LGTM now. After renaming the lints, please squash some (doesn't have to be all) of the commits and this should be ready to go.

@sgued sgued force-pushed the iter-once branch 2 times, most recently from 2d4ff2d to ee9eb22 Compare July 30, 2022 13:38
@flip1995
Copy link
Member

flip1995 commented Aug 1, 2022

Can you squash the commits a bit further, please? Most of those are trivial/one-line changes, that we don't really need in the Clippy history.

@sgued sgued force-pushed the iter-once branch 2 times, most recently from e7c9a6a to ece5f6b Compare August 3, 2022 18:23
@bors
Copy link
Contributor

bors commented Aug 8, 2022

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

@flip1995 flip1995 removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Aug 9, 2022
@sgued
Copy link
Contributor Author

sgued commented Aug 13, 2022

Is it squashed enough now or do you want even more?

@flip1995
Copy link
Member

Commits f30d7c2 and b247594 could also be squashed just looking at the commit message. But that's just me being pedantic, so I won't block this PR on that.

Thanks for squashing. That's how I (almost 😋) like my commit messages.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 14, 2022

📌 Commit af4885c has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 14, 2022

⌛ Testing commit af4885c with merge 679fa9f...

@bors
Copy link
Contributor

bors commented Aug 14, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 679fa9f to master...

@bors bors merged commit 679fa9f into rust-lang:master Aug 14, 2022
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.

Lints recommeding to use std::iter::once and std::iter::empty
6 participants