feat(manual_is_variant): Check for .iter().any() - #15989
Conversation
|
r? @Alexendoo rustbot has assigned @Alexendoo. Use |
|
If the input uses |
This comment has been minimized.
This comment has been minimized.
414b252 to
844c1c0
Compare
This comment has been minimized.
This comment has been minimized.
| // suggest `any(|x| ..)` instead of `any(|&x| ..)` for `find(|&x| ..).is_some()` | ||
| if matches!(closure_arg.pat.kind, PatKind::Ref(..)) { | ||
| let mut applicability = Applicability::MachineApplicable; | ||
| let suggestion = | ||
| snippet_with_applicability(cx, closure.span, "..", &mut applicability).replacen('&', "", 1); | ||
| return Some(DerefClosure { | ||
| applicability, | ||
| suggestion, | ||
| }); | ||
| } | ||
| if !matches!(strip_pat_refs(closure_arg.pat).kind, PatKind::Binding(..)) { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
I believe this should still live outside of the clippy_utils method, though it's not currently used elsewhere it's specified as being generally applicable
There was a problem hiding this comment.
With this patch, both search_is_some and manual_is_variant_and use it.
There was a problem hiding this comment.
Yeah for those it might be fine, but may not be applicable to other future uses
cc @Jarcho on this though since I'm not 100% sure on what deref_closure_args is doing
There was a problem hiding this comment.
The deref_closure_args function changes a closure that closure that takes the reference into one that takes the value. A closure can take a reference in these forms: |x| *x, or |&x| x. Without my patch, only the search_is_some lint does so to the closure. The first form is handled in the deref_closure_args function. The second form is handled in the file for the lint. It makes sense for other lints, such as the manual_is_variant that I am modifying, to modify both forms if it needs to make the closure take the value. So I moved the logic for handling both forms into the library function.
This comment has been minimized.
This comment has been minimized.
844c1c0 to
2083830
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2083830 to
6904962
Compare
This comment has been minimized.
This comment has been minimized.
|
@profetia Can you please take a look? You worked on the same check recently. |
This comment has been minimized.
This comment has been minimized.
6904962 to
eb2ac26
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The lint `search_is_some` involves converting a closure that takes the reference into one that takes the value. The code is split between the function for the lint and a shared function. The new feature that we are adding involves the same transformation. To do so it needs some of the same code that is currently in the `search_is_some` lint. So it is moved from the lint into the shared function. Before this patch, line 799 in the file `clippy_utils/src/sugg.rs` had a check. It made the function return none in case the closure did not dereference the argument. However, the code for the lint `search_is_some` would then fall back to using the original closure. That achieved the same effect as the check not existing. So the check is now removed.
Now the linter checks for calling the `iter` or `into_iter` method followed by calling the `all` or `any` method. We have used these methods by mistake ourselves. The developer was not familiar with the `is_some_and` method. He tried to use the `any` method on an option. The compiler suggested using `iter` first. The developer did not get to learn about the idiomatic method. changelog: [`manual_is_variant`]: check for calling the `iter` or `into_iter` method followed by calling the `all` or `any` method
changelog: [`manual_is_variant`]: skip the check when the MSRV is before 1.70
eb2ac26 to
2dba3d5
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. |
|
No reviewer has said things for a long time. Does that mean we had better give up on the pull request? |
|
No reviewer has said things for a long time. Does that mean we had better give up on the pull request? |
|
You can use |
|
r? clippy |
|
@ada4a This patch has taken a long time. Is the reviewer busy or is there a problem with the patch? |
|
You might use |
|
It takes too long to get this patch reviewed. |
View all comments
Now the linter checks for calling the
iterorinto_itermethodfollowed by calling the
alloranymethod.We have used these methods by mistake ourselves. The developer was not
familiar with the
is_some_andmethod. He tried to use theanymethod on an option. The compiler suggested using
iterfirst. Thedeveloper did not get to learn about the idiomatic method.
The lint
search_is_someinvolves converting a closure that takes thereference into one that takes the value. The code is split between the
function for the lint and a shared function. The new feature that we
are adding involves the same transformation. To do so it needs some of
the same code that is currently in the
search_is_somelint. So it ismoved from the lint into the shared function.
Before this patch, line 799 in the file
clippy_utils/src/sugg.rshad acheck. It made the function return none in case the closure did not
dereference the argument. However, the code for the lint
search_is_somewould then fall back to using the original closure.That achieved the same effect as the check not existing. So the check
is now removed.
changelog: [
manual_is_variant]: check for calling theiterorinto_itermethod followed by calling thealloranymethodchangelog: [
manual_is_variant]: skip the check when the MSRV is before1.70