[useless_asref]: check that the clone receiver is the parameter
#12136
Conversation
|
r? @Jarcho (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
Thanks for jumping on this so quickly. |
|
Thanks for fixing it! |
| hir::Param { | ||
| pat: | ||
| hir::Pat { | ||
| kind: hir::PatKind::Binding(_, local_id, ..), |
There was a problem hiding this comment.
You also need to check for Ref.
There was a problem hiding this comment.
I assume you mean |&x| patterns (PatKind::Ref)? Do you mean that we should also lint on .as_ref().map(|&x| x.clone())? Or do you mean that we shouldn't lint when ref is on the binding?
There was a problem hiding this comment.
I'm not sure exactly what this Ref pattern is checking exactly. I looked at map_clone (which lint for something similar) to check how they did and they handle both so I think you should as well here.
There was a problem hiding this comment.
Hmm, looking more into this, (correct me if I'm wrong) it looks like map_clone used to lint on .as_ref().map(|&x| x) because it's the same as .copied(), but as of recently doesn't anymore, because of a should_run_lint check that lets useless_asref check it.
But useless_asref only seems to handle .clone() calls, so there's nothing to catch that case anymore.
I suppose we could extend the logic in should_run_lint to only return false when clone is called in the closure, so that map_clone can continue linting on .map(|x| *x) and .map(|&x| x). We won't need a Ref check here, then (because it's already handled elsewhere).
This is getting pretty close to duplicating the logic from useless_asref inside should_run_lint though. I'm wondering if we could just move the clone checking that's currently in useless_asref to map_clone so they don't need this sort of coordination?
There was a problem hiding this comment.
Oh but you still can trick the lint:
let x = Some(String::new());
let x = x.as_ref();
x.map(|&x| x.clone()); // or whateverIf this is duplicating the logic from map_clone, then might be worth having some code in common instead. :)
There was a problem hiding this comment.
Fixed in be5707c . Is that what you had in mind? (This also moved that let else from before to an if let guard because I think it looks nicer and less indentation, shouldn't change anything functionally)
There was a problem hiding this comment.
opened a new issue for the other one: #12142, so this can't be forgotten :)
There was a problem hiding this comment.
Unless I missed something, you still only handle Binding and still not Ref (Ref is used here in map_clone).
There was a problem hiding this comment.
The call to strip_pat_refs removes any ref patterns, so |&&x|, |&x|, |x| will all be handled the same way
|
Thank you. @bors r+ |
[`useless_asref`]: check that the clone receiver is the parameter Fixes #12135 There was no check for the receiver of the `clone` call in the map closure. This makes sure that it's a path to the parameter. changelog: [`useless_asref`]: check that the clone receiver is the closure parameter
|
💔 Test failed - checks-action_test |
|
@bors retry |
[`useless_asref`]: check that the clone receiver is the parameter Fixes #12135 There was no check for the receiver of the `clone` call in the map closure. This makes sure that it's a path to the parameter. changelog: [`useless_asref`]: check that the clone receiver is the closure parameter
|
💔 Test failed - checks-action_test |
|
@bors retry Come on macos, you can do it. |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #12135
There was no check for the receiver of the
clonecall in the map closure. This makes sure that it's a path to the parameter.changelog: [
useless_asref]: check that the clone receiver is the closure parameter