fix(mut_mutex_lock): don't lint on indexing and field accesses#16435
fix(mut_mutex_lock): don't lint on indexing and field accesses#16435ada4a wants to merge 4 commits into
Conversation
rustbot
added
the
S-waiting-on-review
|
Like in the other PR you are still going to need to find the projection base (i.e. walk deref calls) and check that all derefs can be switched to their mutable form. A "known problems" section needs to be added to the lint to explain how the lint doesn't take lifetimes into account as well. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // | ||
| // 1. Check that there are no custom deref adjustments turning the access immutable (e.g. | ||
| // `Arc<Mutex>` derefs to `&Mutex`) | ||
| if matches!(expr_custom_deref_adjustment(cx, recv), None | Some(Mutability::Mut)) |
There was a problem hiding this comment.
Why is this happening? Calling lock through a custom deref will never be mutable. The check you do after also makes this completely redundant.
There was a problem hiding this comment.
Apologies, this was left over from a previous version of the lint, as I honestly wasn't sure if it's still relevant. Removed it now
| && cx | ||
| .qpath_res(p, r.hir_id) | ||
| .opt_def_id() | ||
| .and_then(|id| cx.tcx.static_mutability(id)) | ||
| == Some(Mutability::Not) | ||
| { | ||
| // The mutex is stored in a `static`, and we don't want to suggest making that | ||
| // mutable | ||
| return; | ||
| } |
There was a problem hiding this comment.
We don't want to lint any statics, mutable or not.
| // mutable one -- we need to check if a mutable deref would've been possible, i.e. if | ||
| // `ty: DerefMut<Target = target>` (we don't need to check the `Target` part, as `Deref` and | ||
| // `DerefMut` impls necessarily have the same one) | ||
| DerefAdjustKind::Overloaded(_) => impls_deref_mut(ty).then_some(target), |
There was a problem hiding this comment.
Why allow custom derefs here, but not earlier.
| // Peel off one projection | ||
| match r.kind { | ||
| ExprKind::Unary(UnOp::Deref, base) => { | ||
| if impls_deref_mut(typeck.expr_ty_adjusted(base)) { |
There was a problem hiding this comment.
Why allow custom derefs here, but not earlier.
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
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. |
|
Addressed the review comments in third commit. Also, extracted projection checking into a separate function, as that makes the lint structure a bit clearer imo -- that's a separate commit as well, so it can be easily removed if you disagree. @rustbot ready |
rustbot
added
S-waiting-on-review
- remove the first adjustment check, as it's redundant - don't lint on _any_ statics
makes the structure of the lint a bit clearer imo
3 participants
A v2 of #16261, due to #16261 (comment)
Fixes #16253
changelog: [
mut_mutex_lock]: don't lint on indexing and field accessesr? @Jarcho