Add new lint iter_not_returning_iterator#7610
Add new lint iter_not_returning_iterator#7610bors merged 1 commit intorust-lang:masterfrom Labelray:conflict
iter_not_returning_iterator#7610Conversation
|
r? @camsteffen (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| let parameters = fn_sig.decl.inputs; | ||
| if parameters.is_empty() || parameters.len() > 1 { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why check for number of parameters?
Shouldn't this lint work for an iter that takes parameters other than self?
Or maybe this is covered by another lint?
There was a problem hiding this comment.
I think it should only detect fn iter(&self) -> _ before, because I think if it has more parameters it may have a different meaning, so this lint may not suit it.
There was a problem hiding this comment.
You can use if let [param] = fn_sig.decl.inputs. The if_chains can be collapsed. Can you also check that the parameter is &self?
| if parameters.is_empty() || parameters.len() > 1 { | ||
| return; | ||
| } | ||
| let iter_trait_id = get_trait_def_id(cx, &paths::ITERATOR).unwrap(); |
There was a problem hiding this comment.
This will crash on any no_std crate or maybe even when linting the standard library itself. You should just return when this trait can't be found.
|
@Labelray Thanks for the PR. This is a good idea. I do have one concern though... When GAT arrives (coming soon according to this blog post), iterators that are not |
|
Shouldn't this be covered by |
I think not. [ |
|
☔ The latest upstream changes (presumably #7604) made this pull request unmergeable. Please resolve the merge conflicts. |
|
|
|
I think it's fine to have this lint until GATs arrive; the streaming iter trait might end up using a different signature, or we can check for both. Bit concerned the lint name is too long though. |
Will it be a good idea to name it as |
camsteffen
left a comment
camsteffen
left a comment
There was a problem hiding this comment.
For the lint name I'd suggest iter_not_returning_iterator.
| let parameters = fn_sig.decl.inputs; | ||
| if parameters.is_empty() || parameters.len() > 1 { | ||
| return; | ||
| } |
There was a problem hiding this comment.
You can use if let [param] = fn_sig.decl.inputs. The if_chains can be collapsed. Can you also check that the parameter is &self?
|
Can you do a rebase, not a merge? |
Labelray
changed the title
returned_iter_not_implementing_iteratoriter_not_returning_iterator
camsteffen
left a comment
camsteffen
left a comment
There was a problem hiding this comment.
This lint can be in the pedantic group. Otherwise this is ready!
|
Thanks! @bors r+ |
|
📌 Commit 8f88acd has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
8 participants
Add new lint [
iter_not_returning_iterator] to detect methoditer()oriter_mut()returning a type not implementingIteratorchangelog: Add new lint [
iter_not_returning_iterator]