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

Add unused_enumerate_index lint #10404

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

dnbln
Copy link
Contributor

@dnbln dnbln commented Feb 26, 2023

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 correct Enumerate 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

@rustbot
Copy link
Collaborator

rustbot commented Feb 26, 2023

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.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 26, 2023
@bors
Copy link
Contributor

bors commented Apr 29, 2023

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

@blyxyas
Copy link
Member

blyxyas commented Oct 7, 2023

Hi @dnbln, Philipp doesn't have enough time to review PRs anymore, so I'll take the review from now on. Are you still interested in continuing with this development? ฅ^•ﻌ•^ฅ

r? @blyxyas

@rustbot rustbot assigned blyxyas and unassigned flip1995 Oct 7, 2023
@dnbln dnbln force-pushed the feat/unused_enumerate_index branch from a4c8c15 to 06454c8 Compare October 9, 2023 17:28
@dnbln
Copy link
Contributor Author

dnbln commented Oct 9, 2023

Hi @blyxyas, yes, certainly. I have resolved the merge conflicts; do let me know if there's anything else I should do.

Copy link
Member

@blyxyas blyxyas left a 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! ❤️

clippy_lints/src/loops/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/unused_enumerate_index.rs Outdated Show resolved Hide resolved
Copy link
Member

@blyxyas blyxyas left a 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.

clippy_lints/src/loops/unused_enumerate_index.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@blyxyas blyxyas left a 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! ❤️ ^•ﻌ•^ฅ♡

tests/ui/unused_enumerate_index.rs Show resolved Hide resolved
@blyxyas
Copy link
Member

blyxyas commented Oct 22, 2023

We have a slight block here (don't worry, I'll fix it soon(=´∇`=)). We have this deprecated paths module, and yesterday I started an effort to convert these paths to diagnostic items and convert existing uses of these paths to use diagnostic items.

It won't take long. I'll update you with time.
There isn't anything more to do on your end.

@blyxyas
Copy link
Member

blyxyas commented Oct 26, 2023

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 😅

Copy link
Member

@blyxyas blyxyas left a 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),
Copy link
Member

Choose a reason for hiding this comment

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

Pretty smart move.

Copy link
Contributor Author

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?

/// 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,
}
}

Copy link
Member

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

tests/ui/unused_enumerate_index.rs Show resolved Hide resolved
@dnbln dnbln force-pushed the feat/unused_enumerate_index branch from 56f315a to 98f0c2c Compare October 31, 2023 16:53
Co-authored-by: Alejandra González <[email protected]>
@dnbln dnbln force-pushed the feat/unused_enumerate_index branch from 98f0c2c to 14b8290 Compare October 31, 2023 17:22
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.
@dnbln dnbln force-pushed the feat/unused_enumerate_index branch from f209845 to bb9cc6d Compare November 1, 2023 13:19
Copy link
Member

@blyxyas blyxyas left a 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! ❤️

@blyxyas
Copy link
Member

blyxyas commented Nov 1, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Nov 1, 2023

📌 Commit bb9cc6d has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 1, 2023

⌛ Testing commit bb9cc6d with merge 919f698...

@bors
Copy link
Contributor

bors commented Nov 1, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 919f698 to master...

@bors bors merged commit 919f698 into rust-lang:master Nov 1, 2023
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.

6 participants