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

Extend iter_on lints to catch more cases #11038

Closed
wants to merge 1 commit into from

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Jun 27, 2023

I've moved this to pedantic, unfortunately not style as there are still a few FPs that need to be sorted out.

changelog: Enhancement: [iter_on_single_items], [iter_on_empty_collections]: Catch more cases

@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2023

r? @dswij

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 27, 2023
@Centri3 Centri3 changed the title Extend iter_on to catch more cases Extend iter_on lints to catch more cases Jun 27, 2023
@dswij
Copy link
Member

dswij commented Jun 29, 2023

as there are still a few FPs that need to be sorted out.

Can you list out the FPs to be sorted out? Will be good if we track them in an issue.


@blyxyas Can you help to review this one? 😄

r? @blyxyas

@rust-lang rust-lang deleted a comment from rustbot Jun 29, 2023
@Centri3
Copy link
Member Author

Centri3 commented Jun 29, 2023

@blyxyas
Copy link
Member

blyxyas commented Jun 29, 2023

Yeah, I'll review it once we know what FP got fixed 😅
Yeehaw 🤠

@bors
Copy link
Collaborator

bors commented Jul 13, 2023

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

@Centri3
Copy link
Member Author

Centri3 commented Jul 14, 2023

I gave making the get_expr_use_or_unification_node check work another try but I still have no idea why it isn't working...

@bors
Copy link
Collaborator

bors commented Jul 23, 2023

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

@bors
Copy link
Collaborator

bors commented Jul 24, 2023

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

@dswij
Copy link
Member

dswij commented Jul 25, 2023

Assigning this to you since you've taken a look and have review privilege now

r? @blyxyas

@blyxyas
Copy link
Member

blyxyas commented Jul 26, 2023

rust-lang/rust#114070 was accepted, please wait until the next sync (which will also change Clippy's toolchain version) to do this change (it won't work before that :) ). It will sync about 2 or 3 days from now AFAIK.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 26, 2023
Add `sym::iter_mut` + `sym::as_mut_ptr` for Clippy

We currently have `sym::iter` and `sym::iter_repeat`, this PR adds `sym::iter_mut` as it's useful for rust-lang/rust-clippy#11038 and another Clippy lint, it also adds `sym::as_mut_ptr` as it's useful for rust-lang/rust-clippy#10962.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jul 28, 2023
Add `sym::iter_mut` + `sym::as_mut_ptr` for Clippy

We currently have `sym::iter` and `sym::iter_repeat`, this PR adds `sym::iter_mut` as it's useful for rust-lang#11038 and another Clippy lint, it also adds `sym::as_mut_ptr` as it's useful for rust-lang#10962.
@bors
Copy link
Collaborator

bors commented Jul 28, 2023

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

@blyxyas
Copy link
Member

blyxyas commented Jul 29, 2023

Good news! #11250 got merged, merging 0404b6b (#11250), the change can now be made :)

@Centri3
Copy link
Member Author

Centri3 commented Aug 1, 2023

@blyxyas
Copy link
Member

blyxyas commented Aug 8, 2023

Forgot to ask, I assumed that you wanted to fix those 2 FPs and uplift all in this PR. Do you wish to fix those in a future PR? Is this ready for another review?

@Centri3
Copy link
Member Author

Centri3 commented Aug 8, 2023

Yeah this is ready for another, I think the 2nd was caused by this rewrite so I intend to fix that but I'm unsure about the other. Ideally both would be fixed in this PR

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

Just some finishing touches and I think this is pretty ready.

///
/// ### Why is this bad?
/// It is simpler to use the once function from the standard library.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// It is simpler to use the once function from the standard library.
/// It is simpler to use the [`once`](https://doc.rust-lang.org/std/iter/fn.once.html) function from the standard library.

Comment on lines +2440 to +2441
/// ### 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.

It should probably mention the reason, that it is a core::iter::sources::once::Once<T> instead of a core::slice::iter::Iter<T>, but it should work fine as Once also implements Iterator

///
/// It is simpler to use the empty function from the standard library:
/// ### 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.

(Same as comment above 🐮 )

@blyxyas
Copy link
Member

blyxyas commented Aug 9, 2023

Also, I didn't really understand: Will you work on this PR to fix those FPs, or open another PR in the future uplifting this lint to style?

@Centri3
Copy link
Member Author

Centri3 commented Aug 9, 2023

Ideally, yes, I want this to be style after this PR if possible

@bors
Copy link
Collaborator

bors commented Aug 11, 2023

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

@rust-lang rust-lang deleted a comment from bors Sep 20, 2023
@xFrednet
Copy link
Member

Hey this is triage, I'm closing this due to inactivity. Currently, @Centri3 sadly doesn't have the time to continue this implementation. If anyone is interested in continuing this PR, you're more than welcome to create a new PR and push it over the finish line. :D

Thank you to @Centri3 and the reviewers for the time, that you already put into this!

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review

@xFrednet xFrednet closed this Mar 31, 2024
@rustbot rustbot added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants