-
Notifications
You must be signed in to change notification settings - Fork 1.8k
allow explicit deref
or deref_mut
method calls in impl of Deref
or DerefMut
trait
#15688
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
base: master
Are you sure you want to change the base?
Conversation
Lintcheck changes for 6129b52
This comment will be updated if you push new changes |
This comment has been minimized.
This comment has been minimized.
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. |
r? clippy |
deref
or deref_mut
impl of `D…deref
impl of Deref
trait
… or `DerefMut` trait
deref
impl of Deref
traitderef
or deref_mut
method calls in impl of Deref
or DerefMut
trait
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the possible confusion @relaxcn – I'm not actually a member of the Clippy Team and so I unfortunately can't merge this PR. But I think it's in a pretty good shape now, and we'll just need to wait for the actual reviewer to take a final look and approve – the reviewers are usually quite busy, so it can take a bit 😅
mutbl, | ||
} => { | ||
// Skip when inside implementation of the `Deref` or `DerefMut` trait. | ||
if in_deref_or_deref_mut_impl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will end up allowing deref
in DerefMut
and vice versa, which could technically be solved by having a separate bool for each.. but honestly it might just be okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had created a PR to rustc
, but it seems that they don't want to add a new diag item refer rust-lang/rust#147643.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly the suggestion to look-up both diagnostic and language items in the same call makes a lot of sense. I'll open a Zulip thread to discuss it with the team, if I don't forget. Thank you for giving it a go though:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
/// let _ = <Foo as Deref>::deref(&foo); | ||
/// ``` | ||
/// | ||
/// This lint also excludes explicit `deref` or `deref_mut` method calls inside implementations of the `Deref` or `DerefMut` traits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think such additional information should go into the "What it does" section
@y21 friendly ping |
Fixes: #15392
changelog: [
explicit_deref_methods
]: Allow explicitderef
orderef_mut
method calls in implementation of the theDeref
orDerefMut
trait.