-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
r? @Jarcho (rustbot has picked a reviewer for you, use r? to override) |
☔ The latest upstream changes (presumably #11556) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this 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.
6a58a19
to
330ebbb
Compare
Thank you. @bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
The reason I picked |
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 |
Yes please, reopen. |
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. |
Regardless, I'm going to try to implement the other version later today and submit another PR for it |
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?
@y21 I couldn't, thanks anyway! |
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. |
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. 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. |
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:
|
Closes #9736
A new lint that looks for
iter
(anditer_mut
) method implementations without the type implementingIntoIterator
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