-
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
Add unused_enumerate_index
lint
#10404
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon. Please see the contribution instructions for more information. |
☔ The latest upstream changes (presumably #10647) made this pull request unmergeable. Please resolve the merge conflicts. |
a4c8c15
to
06454c8
Compare
Hi @blyxyas, yes, certainly. I have resolved the merge conflicts; do let me know if there's anything else I should do. |
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 a first review. Very good for a first review, and welcome to contributing to Clippy! ❤️
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.
I did a little bit of trolling in my last review hehe :3
Seems pretty good, this is the last review before doing the final complete one.
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 some more tests and this is it! Thanks for your patience and I hope this new lint (quite a useful one, also) gets merged as soon as possible! ❤️ ^•ﻌ•^ฅ♡
We have a slight block here (don't worry, I'll fix it soon(=´∇`=)). We have this deprecated It won't take long. I'll update you with time. |
It seems like it will take more than expected (without even counting the reviewing + sync time), so I'll unblock this PR and just fix it later in my rust-lang/rust branch. I don't think it's worth it making you wait even longer haha 😅 |
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.
Ok, that's it! Add a test implementing std::iter::Iterator
for a Dummy3
structure and then squash the branch into 1 or 2 commits. It should be merge-ready then!
fn pat_is_wild<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx PatKind<'_>, body: &'tcx Expr<'_>) -> bool { | ||
match *pat { | ||
PatKind::Wild => true, | ||
PatKind::Binding(_, id, ident, None) if ident.as_str().starts_with('_') => !is_local_used(cx, body, id), |
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.
Pretty smart move.
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.
I've used the FOR_KV_MAP
lint for reference, this function is taken straight from there. Maybe it could be moved to an util function, since with this it would already be defined 3 times?
rust-clippy/clippy_lints/src/loops/for_kv_map.rs
Lines 59 to 66 in 7d34406
/// Returns `true` if the pattern is a `PatWild` or an ident prefixed with `_`. | |
fn pat_is_wild<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx PatKind<'_>, body: &'tcx Expr<'_>) -> bool { | |
match *pat { | |
PatKind::Wild => true, | |
PatKind::Binding(_, id, ident, None) if ident.as_str().starts_with('_') => !is_local_used(cx, body, id), | |
_ => false, | |
} | |
} |
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.
Oh yes then, move to it clippy_utils
56f315a
to
98f0c2c
Compare
Co-authored-by: Alejandra González <[email protected]>
98f0c2c
to
14b8290
Compare
This function was previously defined for the iter_kv_map, for_kw_map, and unused_enumerate_index lints. This commit extracts it into clippy_utils.
f209845
to
bb9cc6d
Compare
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.
LGTM, thanks for the lint, and sorry for taking so long to review! ❤️
@bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
A lint for unused
.enumerate()
indexes (for (_, x) in iter.enumerate()
).I wasn't able to find a
rustc_span::sym::Enumerate
, so the code for checking that it's the correctEnumerate
iterator is a bit weird.changelog: New lint: [
unused_enumerate_index
]: A new lint for checking that the indexes from.enumerate()
calls are used.#10404