New lint: iter_missing_exact_size#17395
Conversation
This comment has been minimized.
This comment has been minimized.
e4c0d8c to
2b8d2b7
Compare
|
Not exactly sure if the new name matches the guidelines, I didn't want the name to be too long, suggestions welcome @rustbot label +A-lint |
|
Error: Unknown labels: A-lints Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
|
Lintcheck changes for cfb5ade
This comment will be updated if you push new changes |
143523a to
a9e1af0
Compare
|
So the first round of CI said that I should add rust-clippy/tests/symbols-used.rs Lines 41 to 44 in 9006d6c sym, I was using use clippy_utils::sym as clippy_sym; and use rustc_span::sym;, I'll switch them
|
All symbols of |
| span_lint_and_help( | ||
| cx, | ||
| ITER_MISSING_EXACT_SIZE, | ||
| lint_span, |
There was a problem hiding this comment.
| lint_span, | |
| item.span.into(), |
There was a problem hiding this comment.
Is there a reason to not have the label pointing to size_hint?
There was a problem hiding this comment.
I believe I wrote this because they bound a simple expression to a variable and just used it just once. I would think that the size hint would be good as the help span. Something like "size_hint is based on an ExactSizeIterator here"
There was a problem hiding this comment.
adjusted to have the size hint as a help span
This comment has been minimized.
This comment has been minimized.
2dc95c6 to
c864c24
Compare
This comment has been minimized.
This comment has been minimized.
c864c24 to
aaae638
Compare
Is it possible to have the name be part of what is decided in the FCP? |
Yes, feel free to join the discussion on Zulip (once the FCP starts). I think this currently wouldn't fire if the user has fn size_hint(&self) -> (usize, Option<usize>) {
return self.range.size_hint();
}If you can please add a test for this? |
I added support for function bodies where the only statement is a return (and optionally there might be a trailing expression that is dead code and is ignored) |
This comment has been minimized.
This comment has been minimized.
eb066f0 to
ca1028b
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Squashed and rebased |
There was a problem hiding this comment.
@Gri-ffin @fpdotmonkey @samueltardieu do you think this is ready for FCP?
It's starting to look solid, but I'm not a maintainer, I'm just helping in the reviews, I'll leave that call to the maintainers.
|
Likewise, I am but a community reviewer. I think it looks good, though. |
|
Okay, @Gri-ffin do you have any more feedback? If not I'll start the FCP |
|
@fpdotmonkey @Gri-ffin if you think the PR is ready then mark it as approved; that's what signals triagebot to assign a maintainer. @DanielEScherzer The process for FCPs has a team member starting it. We could try and have it start once the the community review phase is done, but we'll need to see how the reviews play out first. |
Oh, I thought I just needed to add |
| error: iterator can implement `ExactSizeIterator` | ||
| --> tests/ui/iter_missing_exact_size.rs:13:1 | ||
| | | ||
| LL | / impl Iterator for StringRepeater1 { | ||
| LL | | | ||
| LL | | type Item = String; | ||
| LL | | fn next(&mut self) -> Option<Self::Item> { | ||
| ... | | ||
| LL | | } | ||
| | |_^ | ||
| | | ||
| help: this `size_hint()` implementation delegates to to the `size_hint()` of an `ExactSizeIterator`, so the overall iterator is likely to have an exact size | ||
| --> tests/ui/iter_missing_exact_size.rs:20:9 | ||
| | | ||
| LL | self.range.size_hint() | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ | ||
| = note: `-D clippy::iter-missing-exact-size` implied by `-D warnings` | ||
| = help: to override `-D warnings` add `#[allow(clippy::iter_missing_exact_size)]` |
There was a problem hiding this comment.
I personally find this confusing but it's nothing currently blocking.
There was a problem hiding this comment.
which part is confusing?
There was a problem hiding this comment.
IMO if I see this suggestion I would assume I need to modify the impl block, which isn't the case, personally I would prefer to only highlight size_hint, but it's only a personal opinion, it can be discussed later.
There was a problem hiding this comment.
Ah, okay. But since the issue is with the overall implementation (impl Iterator with a delegating size_hint but without also implementing ExactSizeIterator) I think pointing to the overall impl block is still helpful
Can you mark this as an approval in GitHub so that it counts as a community review and triggers the team review process? |
View all comments
.stderrfile)cargo testpasses locallycargo dev update_lintscargo dev fmtFixes #17393
changelog: [
iter_missing_exact_size]: add lint