Fix `any()` not taking reference in `search_is_some` lint by ThibsG · Pull Reque...
source link: https://github.com/rust-lang/rust-clippy/pull/7463
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
New issue
Fix any() not taking reference in search_is_some lint
#7463
Conversation
r? @Manishearth
(rust-highfive has picked a reviewer for you, use r? to override)
I'm not a big fan of direct string replacing, unsure if there's a better way to do it
clippy_lints/src/methods/search_is_some.rs
Outdated Show resolved
cc @camsteffen got ideas for how to do this nicely without string replacing? Perhaps using a multipart suggestion to replace specific spans after running an ExprUseVisitor?
Perhaps using a multipart suggestion to replace specific spans after running an ExprUseVisitor?
Yes I would do that. String replacement could go wrong in a lot of ways. Also, we should handle cases where the variable is derefed. Autoderef requires no change. Explicit deref can be removed.
I agree about string replacement, I never used multipart suggestion, it's time to experience it !
For me, explicit deref is removed here: https://github.com/rust-lang/rust-clippy/pull/7463/files#diff-c7288bb6febe070c5809cdc2ebe9fc138a849a1ded710011daf9b363888a744bR51
Are you talking about different situation?
For me, explicit deref is removed here:
Yes, right idea, but should not use String::replace.
r? @xFrednet
I might not have time to complete this review in a timely manner at the moment, hoping someone else can pick this up!
Sure, I can pick this PR up! 
I'll have a quick scan over it now and then try to do the proper review tomorrow . ThibsG could you please also include the lint name as part of your changelog entry. A format like this is usually very helpful:
changelog: [`<lint_name>`] message
Hey, I've marked some points that can be improved IMO. I'm impressed by this work. The API you used is a bit confusing and not as well documented, but you used it correctly from what I can tell. 
It would be nice to directly construct the search closure. I suggested a possible solution here, that should in theory work. However, that might require some more work and the PR as it is would already be a great improvement. Therefore, I would also be happy to merge it with the newly added help messages. You can decide what you prefer.
Thank you for the work, and sorry that the review took so long.
The new suggestions are already looking really nice, thank you!
Looking at the .stderr files, I think we can shorten the lint message a bit by replacing the use ... instead: with a try:. This shortens the message a lot, but might be a bit harder to understand. I'm not entirely sure if it's worth it. What are your thoughts about it?
Side note: I've done a quick local test, and this suggestion building could in theory also be used to create suggestion over multiple lines:
error: called `is_none()` after searching an `Iterator` with `find`
--> $DIR/aaa.rs:6:13
|
LL | let _ = (0..17).find(|x| {
| _____________^
LL | | *x == 7
LL | | || *x == 9
LL | | || *x == 17
LL | | }).is_none();
| |________________^
|
= note: `-D clippy::search-is-some` implied by `-D warnings`
help: use `!_.any()` instead
|
LL ~ let _ = !(0..17).any(|x| {
LL + x == 7
LL + || x == 9
LL + || x == 17
LL ~ });
|In my tests I've put the line limit to 5 which ensures that the suggestion is still displayed to the user. This would be a nice feature but should be added in a followup PR, as it would also require dealing with Delegate::mutate. Just an idea if you started to enjoy working with Span-Magic 
// Build suggestion gradually by handling closure arg specific usages,
// such as explicit deref and borrowing cases.
// Returns `None` if no such use cases have been triggered in closure body
fn get_closure_suggestion<'tcx>(
This is a nice function, where I can image that it will become useful to other lints as well. Could you move it to clippy_utils (probably the sugg module) and also add an example to the doc comment?
It should also be noted that it currently doesn't support mutations
The refactorings are looking good. I'm guessing we'll leave this comment open until the suggestion is build correctly, so we can keep the review comments
This function should get a more descriptive name for inside clippy_utils::sugg maybe something like create_closure_deref_args_sugg or something similar, it's hard to find a good descriptive name. I'm also not happy with my suggestion yet 
clippy_lints/src/methods/search_is_some.rs
Outdated Show resolved
clippy_lints/src/methods/search_is_some.rs
Outdated Show resolved
clippy_lints/src/methods/search_is_some.rs
Outdated Show resolved
clippy_lints/src/methods/search_is_some.rs
Outdated Show resolved
clippy_lints/src/methods/search_is_some.rs
Outdated Show resolved
clippy_lints/src/methods/search_is_some.rs
Outdated Show resolved
clippy_lints/src/methods/search_is_some.rs
Outdated Show resolved
tests/ui/search_is_some_fixable_none.rs
Show resolved
clippy_lints/src/methods/search_is_some.rs
Outdated Show resolved
Looking at the .stderr files, I think we can shorten the lint message a bit by replacing the use ... instead: with a try:. This shortens the message a lot, but might be a bit harder to understand. I'm not entirely sure if it's worth it. What are your thoughts about it?
We can move the reference to the use of !_.any() in message instead. But I don't have strong opinion about this, so we can also remove it completely or ask Clippy's team their feeling about it.
Hey, I've reviewed your recent changes, which already improved the suggestion building
I've also done some testing on your branch and found a few instances where the suggestion can be improved. These would also be great test cases, along tests for the other projection types.
My test cases
Thank you for putting in this work. I've underestimated the complexity a bit 
. If we get this working, it will be a nice addition to Clippy IMO . Also thank you for waiting on the review, university has again taken up a lot of time which made the review take longer
clippy_lints/src/methods/search_is_some.rs
Outdated Show resolved
// Build suggestion gradually by handling closure arg specific usages,
// such as explicit deref and borrowing cases.
// Returns `None` if no such use cases have been triggered in closure body
fn get_closure_suggestion<'tcx>(
The refactorings are looking good. I'm guessing we'll leave this comment open until the suggestion is build correctly, so we can keep the review comments
We can move the reference to the use of !_.any() in message instead. But I don't have strong opinion about this, so we can also remove it completely or ask Clippy's team their feeling about it.
Good point that we should mention it somewhere. On second thought, I would say it's good where it is right now. But I'll let it run through my head a bit more
Heyho, thank you for the update and all the additional work you're putting in after my suggestion to use Span-magic ! I've reviewed your code and found some edge cases. One of them made me wonder if it would be simpler to use a normal Visitor and manually handle the projections etc. See comment I'm not sure if that's the best course of action, though. This was just an idea that popped in my head.
I've now mainly reviewed the seach_is_some.rs file. I've skimmed over the tests, which already looks good, but there might be some NITs in the next review.
force-pushed the find_any_7392
branch
2 times, most recently
from
db90e2c
to
4393aa3
21 days ago
Alright should be good for final review.
I am also looking forward to see this merged into Clippy 
Thank you for your valuable guidance and patience !
Alright should be good for final review.
Fantastic, it's on my to-do list for text week.
Thank you for your valuable guidance and patience !
Same back to you! I wanted to say something similar once this gets merged. Your patience with my reviews has also helped, it looks like we're both happy with how it turns out. I've also learned a lot while reviewing this
Alright, I'm done with an in-depth review. The number of tests in this PR are remarkable and frightening at once. The output looks excellent, though. I marked some code pieces that could be adjusted a bit more (Sorry ^^). And that's it, I hope that this is not too much, and we can get this merged soon!
Outdated Show resolved
closure_arg_is_double_ref,
next_pos: search_expr.span.lo(),
suggestion_start: String::new(),
applicability: Applicability::MaybeIncorrect,
Reminder @xFrednet, create an issue to set this to Applicability::MashineApplicable in 1.59.0 or 1.60.0
Outdated Show resolved
Show resolved
Outdated Show resolved
Outdated Show resolved
Outdated Show resolved
Outdated Show resolved
Alright, I believe that this PR is ready to be merged. It has grown quite a bit and I might have missed something, but the tests and previous reviews give me confidence that it works. And even in the worth case, this will only affect the suggestion.
@ThibsG Thank you very much for all the changes and the tremendous amount of tests! This has been the largest PR I've ever reviewed 
I'll create a new issue to have this suggestion be MachineApplicable in one or two releases.
And now, after 111 comments on this PR, I'm happy to add this one with:
@bors r+
Commit a8e7fed has been approved by xFrednet
Testing commit Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
No milestone
Successfully merging this pull request may close these issues.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK