Fix `any()` not taking reference in `search_is_some` lint #7463

ThibsG · 2021-07-14T09:24:41Z

find gives reference to the item, but any does not, so suggestion is broken in some specific cases.

Fixes: #7392

changelog: [search_is_some] Fix suggestion for any() not taking item by reference

rust-highfive · 2021-07-14T09:24:43Z

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

Manishearth

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

Manishearth · 2021-07-14T15:49:35Z

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?

camsteffen · 2021-07-14T15:55:16Z

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.

ThibsG · 2021-07-19T09:18:50Z

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?

camsteffen · 2021-07-19T14:18:19Z

For me, explicit deref is removed here:

Yes, right idea, but should not use String::replace.

tests/ui/search_is_some.stderr

Manishearth · 2021-08-10T16:36:51Z

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!

xFrednet · 2021-08-10T18:23:41Z

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

xFrednet

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. 🙃

clippy_lints/src/methods/search_is_some.rs

tests/ui/search_is_some.rs

xFrednet

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 ✨ 😉

xFrednet · 2021-08-25T15:37:30Z

clippy_lints/src/methods/search_is_some.rs

+// 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

tests/ui/search_is_some_fixable_none.rs

clippy_lints/src/methods/search_is_some.rs

ThibsG · 2021-09-07T13:35:18Z

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.

xFrednet

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

#![allow(dead_code)]
#![warn(clippy::search_is_some)]

fn please(x: &u32) -> bool {
    *x == 9
}

fn main() {
    let x = 19;
    let ppx: &u32 = &x;
    let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_some();
    // Lint suggestion:  any(|ppp_x: &&&u32| please(&ppp_x))
    // - The closure type is wrong, this is something I also missed until now.
    // - The suggestion to replace the two derefs with on ref is also suspicius

    // The applied suggestion with the correct type:
    let _ = [ppx].iter().any(|ppp_x: &&u32| please(&ppp_x));
    // This triggers `clippy::needless_borrow` and suggests only using `ppp_x`,
    // while this would be okay, it would be nicer if we can avoid this

    let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_some();
    // Lint suggestion:                      any(|s| &s.len() == 2)
    // - The first argument of a method call expression is the caller in this case s.
}

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

xFrednet · 2021-09-11T12:22:52Z

clippy_lints/src/methods/search_is_some.rs

+// 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 🙃

xFrednet · 2021-09-11T12:35:27Z

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 🙃

xFrednet

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. 🙃

clippy_lints/src/methods/search_is_some.rs

tests/ui/search_is_some_fixable_none.rs

ThibsG · 2021-11-20T14:58:56Z

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 ! 😉

xFrednet · 2021-11-21T13:44:57Z

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 🙃

xFrednet

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! 🙃

clippy_utils/src/sugg.rs

xFrednet · 2021-11-27T14:01:08Z

clippy_utils/src/sugg.rs

+            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

clippy_utils/src/sugg.rs

…remove unneeded statements

xFrednet · 2021-12-02T17:19:33Z

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+

bors · 2021-12-02T17:19:34Z

📌 Commit a8e7fed has been approved by xFrednet

bors · 2021-12-02T17:27:57Z

⌛ Testing commit a8e7fed with merge d5d830a...

bors · 2021-12-02T17:42:10Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing d5d830a to master...

rust-highfive assigned Manishearth Jul 14, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 14, 2021

ThibsG force-pushed the find_any_7392 branch from de89e5c to 3d3103b Compare July 14, 2021 09:31

Manishearth reviewed Jul 14, 2021

View reviewed changes

clippy_lints/src/methods/search_is_some.rs Outdated Show resolved Hide resolved

ThibsG force-pushed the find_any_7392 branch from 5827c3c to 3fcab10 Compare August 9, 2021 10:19

ThibsG commented Aug 9, 2021

View reviewed changes

tests/ui/search_is_some.stderr Outdated Show resolved Hide resolved

ThibsG force-pushed the find_any_7392 branch from 3fcab10 to 476e1b2 Compare August 9, 2021 10:37

rust-highfive assigned xFrednet and unassigned Manishearth Aug 10, 2021

xFrednet requested changes Aug 16, 2021

View reviewed changes

xFrednet added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 16, 2021

ThibsG force-pushed the find_any_7392 branch from 476e1b2 to 8e4540c Compare August 18, 2021 16:17

xFrednet requested changes Aug 25, 2021

View reviewed changes

xFrednet reviewed Sep 11, 2021

View reviewed changes

ThibsG force-pushed the find_any_7392 branch from 089b231 to 059fb4b Compare September 12, 2021 09:46

xFrednet added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 12, 2021

ThibsG force-pushed the find_any_7392 branch from 89fc1c9 to b74190c Compare September 14, 2021 13:01

xFrednet reviewed Sep 15, 2021

View reviewed changes

xFrednet removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 15, 2021

ThibsG added 6 commits November 20, 2021 09:40

Add test case for references bindings

268ef40

Add some notes about MethodCall cases

abaaf74

Add tests with closure

7221999

Rephrase the fn checking for a double ref, not only one

2ff702c

Handle closures with type annotations on args

1176b8e

Fix full projection identifier + move applicability to MaybeIncorrect

5ebede0

ThibsG force-pushed the find_any_7392 branch 2 times, most recently from db90e2c to 4393aa3 Compare November 20, 2021 14:42

Move deref closure builder to clippy_utils::sugg module

092fe20

ThibsG force-pushed the find_any_7392 branch from 4393aa3 to 092fe20 Compare November 20, 2021 14:48

xFrednet added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Nov 21, 2021

xFrednet reviewed Nov 27, 2021

View reviewed changes

xFrednet added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 27, 2021

ThibsG added 2 commits November 27, 2021 19:22

Add Index when checking projs in a call, rename some variables and …

c5ce7ff

…remove unneeded statements

Rewrite comment when handling special case for ProjectionKind::Deref

917fdb1

ThibsG force-pushed the find_any_7392 branch from 91529e6 to 4c3e09b Compare November 28, 2021 08:59

Add a note about type annotation on closure param

a8e7fed

ThibsG force-pushed the find_any_7392 branch from 4c3e09b to a8e7fed Compare November 28, 2021 09:22

xFrednet approved these changes Dec 2, 2021

View reviewed changes

xFrednet mentioned this pull request Dec 2, 2021

Make search_is_some suggestion MachineApplicable in 1.59.0 or 1.60.0 #8062

Closed

bors merged commit d5d830a into rust-lang:master Dec 2, 2021

ThibsG deleted the find_any_7392 branch December 2, 2021 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `any()` not taking reference in `search_is_some` lint #7463

Fix `any()` not taking reference in `search_is_some` lint #7463

ThibsG commented Jul 14, 2021 •

edited

Loading

rust-highfive commented Jul 14, 2021

Manishearth left a comment

Manishearth commented Jul 14, 2021

camsteffen commented Jul 14, 2021

ThibsG commented Jul 19, 2021

camsteffen commented Jul 19, 2021

Manishearth commented Aug 10, 2021

xFrednet commented Aug 10, 2021

xFrednet left a comment

xFrednet left a comment •

edited

Loading

xFrednet Aug 25, 2021 •

edited

Loading

xFrednet Sep 11, 2021

xFrednet Oct 6, 2021

ThibsG commented Sep 7, 2021

xFrednet left a comment

xFrednet Sep 11, 2021

xFrednet commented Sep 11, 2021

xFrednet left a comment •

edited

Loading

ThibsG commented Nov 20, 2021

xFrednet commented Nov 21, 2021 •

edited

Loading

xFrednet left a comment

xFrednet Nov 27, 2021

xFrednet commented Dec 2, 2021

bors commented Dec 2, 2021

bors commented Dec 2, 2021

bors commented Dec 2, 2021

Fix any() not taking reference in search_is_some lint #7463

Fix any() not taking reference in search_is_some lint #7463

Conversation

ThibsG commented Jul 14, 2021 • edited Loading

rust-highfive commented Jul 14, 2021

Manishearth left a comment

Choose a reason for hiding this comment

Manishearth commented Jul 14, 2021

camsteffen commented Jul 14, 2021

ThibsG commented Jul 19, 2021

camsteffen commented Jul 19, 2021

Manishearth commented Aug 10, 2021

xFrednet commented Aug 10, 2021

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

xFrednet Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

xFrednet Sep 11, 2021

Choose a reason for hiding this comment

xFrednet Oct 6, 2021

Choose a reason for hiding this comment

ThibsG commented Sep 7, 2021

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Sep 11, 2021

Choose a reason for hiding this comment

xFrednet commented Sep 11, 2021

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

ThibsG commented Nov 20, 2021

xFrednet commented Nov 21, 2021 • edited Loading

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Nov 27, 2021

Choose a reason for hiding this comment

xFrednet commented Dec 2, 2021

bors commented Dec 2, 2021

bors commented Dec 2, 2021

bors commented Dec 2, 2021

Fix `any()` not taking reference in `search_is_some` lint #7463

Fix `any()` not taking reference in `search_is_some` lint #7463

ThibsG commented Jul 14, 2021 •

edited

Loading

xFrednet left a comment •

edited

Loading

xFrednet Aug 25, 2021 •

edited

Loading

xFrednet left a comment •

edited

Loading

xFrednet commented Nov 21, 2021 •

edited

Loading