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

new lint: iter_without_into_iter #11527

Merged
merged 1 commit into from Sep 29, 2023
Merged

Conversation

y21
Copy link
Member

@y21 y21 commented Sep 17, 2023

Closes #9736

A new lint that looks for iter (and iter_mut) method implementations without the type implementing IntoIterator for &Type.
Imo this seems rather pedantic, so I went with that, but I can be convinced to change it to style like the linked issue asked for.
Writing a machine applicable suggestion seems a bit tricky and tedious, so for now this relies on the user adding remaining lifetimes.

changelog: new lint: iter_without_into_iter

@rustbot
Copy link
Collaborator

rustbot commented Sep 17, 2023

r? @Jarcho

(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 Sep 17, 2023
clippy_lints/src/iter_without_into_iter.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_without_into_iter.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_without_into_iter.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_without_into_iter.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_without_into_iter.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Sep 26, 2023

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

Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

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

Just two minor things, otherwise this looks good. Feel free to squash everything down.

clippy_lints/src/iter_without_into_iter.rs Outdated Show resolved Hide resolved
clippy_lints/src/iter_without_into_iter.rs Outdated Show resolved Hide resolved
@Jarcho
Copy link
Contributor

Jarcho commented Sep 29, 2023

Thank you. @bors r+

@bors
Copy link
Collaborator

bors commented Sep 29, 2023

📌 Commit 330ebbb has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 29, 2023

⌛ Testing commit 330ebbb with merge d38fa1a...

@bors
Copy link
Collaborator

bors commented Sep 29, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing d38fa1a to master...

@bors bors merged commit d38fa1a into rust-lang:master Sep 29, 2023
8 checks passed
@Kixunil
Copy link

Kixunil commented Sep 29, 2023

The reason I picked style is that not doing this causes for x in &container to not work. Also do I understand correctly that this doesn't do the opposite - suggest iter() when there's IntoIter for &T?

@y21
Copy link
Member Author

y21 commented Sep 29, 2023

Right, this doesn't lint the opposite, must have missed that when reading the issue 😅. Could reopen the issue then, and mention that only the lint for a missing IntoIterator impl is implemented now

@Kixunil
Copy link

Kixunil commented Sep 29, 2023

Yes please, reopen.

@y21
Copy link
Member Author

y21 commented Sep 29, 2023

I can't do that since I'm not on the team and don't have perms for it. I think you as the issue creator can do it though? Or is that different when the issue is autoclosed by a PR? 🤔 I'm not sure.
If not, maybe we need a new issue for it, or someone else with permissions can do it

@y21
Copy link
Member Author

y21 commented Sep 29, 2023

Regardless, I'm going to try to implement the other version later today and submit another PR for it

bors added a commit that referenced this pull request Sep 30, 2023
new lint: `into_iter_without_iter`

Closes #9736 (part 2)

This implements the other lint that my earlier PR missed: given an `IntoIterator for &Type` impl, check that there exists an inherent `fn iter(&self)` method.

changelog: new lint: `into_iter_without_iter`

r? `@Jarcho` since you reviewed #11527 I figured it makes sense for you to review this as well?
@Kixunil
Copy link

Kixunil commented Oct 3, 2023

@y21 I couldn't, thanks anyway!

@lopopolo
Copy link

lopopolo commented Oct 21, 2023

Hey folks, the suggested fix this lint produces doesn't compile in at least 5 ways on some toy code I've tried:

It looks like this PR doesn't include any tests for the suggested code to ensure that it compiles.

@y21
Copy link
Member Author

y21 commented Oct 21, 2023

As I've said in the issue, it's expected that the current suggestion does not compile (it even has a comment up top), and has the unspecified applicability. It's mostly meant as a nudge in the right direction to the user, not an autofix.
So testing the suggestion doesn't really make sense since we know it won't compile in any case.

You do make some valid points in the issue and at least some trivial things should have been included in the suggestion, and I'll try to look into improving the suggestion here a bit later.

@lopopolo
Copy link

As I've said in the issue, it's expected that the current suggestion does not compile (it even has a comment up top), and has the unspecified applicability. It's mostly meant as a nudge in the right direction to the user, not an autofix.
So testing the suggestion doesn't really make sense since we know it won't compile in any case.

ahh gotcha. Thanks for clarifying @y21. I didn't read this PR thread or the code. I only left a comment here to cross-post my bug to make sure the rule author (you 😄) came across it.

FWIW, that lack of machine applicability is not made clear in the warning output. Based on my familiarity with clippy's other suggestions, I expected to be able to copy and paste:

warning: `iter` method without an `IntoIterator` impl for `&Links<T>`
  --> src/link.rs:68:5
   |
68 | /     pub fn iter(&self) -> Iter<'_, Link<T>, usize> {
69 | |         self.registry.iter()
70 | |     }
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#iter_without_into_iter
note: the lint level is defined here
  --> src/lib.rs:11:9
   |
11 | #![warn(clippy::pedantic)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(clippy::iter_without_into_iter)]` implied by `#[warn(clippy::pedantic)]`
help: consider implementing `IntoIterator` for `&Links<T>`
   |
33 +
34 + impl IntoIterator for &Links<T> {
35 +     type IntoIter = hashbrown::hash_map::Iter<'_, link::Link<T>, usize>;
36 +     type Iter = (&link::Link<T>, &usize);
37 +     fn into_iter() -> Self::IntoIter {
38 +         self.iter()
39 +     }
40 + }
   |

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.

Suggest implementing IntoIterator for &'_ T where T has iter() method returning an iterator (or vice versa)
6 participants