fix(mut_mutex_lock): don't lint on indexing and field accesses

Author: ada4a

clippy_lints/src/methods/mut_mutex_lock.rs

@@ -3,16 +3,32 @@ use clippy_utils::expr_custom_deref_adjustment;
 use clippy_utils::res::MaybeDef;
 use clippy_utils::ty::peel_and_count_ty_refs;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, Mutability};
+use rustc_hir::{Expr, ExprKind, Mutability};
 use rustc_lint::LateContext;
 use rustc_span::{Span, sym};
 
 use super::MUT_MUTEX_LOCK;
 
 pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, recv: &'tcx Expr<'tcx>, name_span: Span) {
-    if matches!(expr_custom_deref_adjustment(cx, recv), None | Some(Mutability::Mut))
-        // NOTE: the reason we don't use `expr_ty_adjusted` here is that a call to `Mutex::lock` by itself
-        // adjusts the receiver to be `&Mutex`
+    // We don't want to lint on indexing and field accesses, as both of those would take exclusive
+    // access to only part of a value -- which would conflict with any immutable reborrow over
+    // the whole value
+    if !matches!(recv.kind, ExprKind::Field(..) | ExprKind::Index(..))
+        // Make sure that we have a mutable access to `Mutex`:
+        //
+        // 1. Check that there are no custom deref adjustments turning the access immutable (e.g.
+        //    `Arc<Mutex>` derefs to `&Mutex`)
+        // TODO: add tests for this
+        && matches!(expr_custom_deref_adjustment(cx, recv), None | Some(Mutability::Mut))
+        // 2. Check that the receiver is behind one or more `&mut`s -- we want to have mutable
+        //    access, while not outright owning it -- if the latter were the case, then changing
+        //    `.lock()` to `.get_mut()`, could easily result in a conflict with other existing
+        //    immutable borrows.
+        //
+        // NOTE: `mutbl` being `Some` is enough to determine that there was at least one layer of
+        // references, so no need to check `n`
+        // NOTE: the reason we don't use `expr_ty_adjusted` here is that a call
+        // to `Mutex::lock` by itself adjusts the receiver to be `&Mutex`
         && let (recv_ty, _, Some(Mutability::Mut)) = peel_and_count_ty_refs(cx.typeck_results().expr_ty(recv))
         && recv_ty.is_diag_item(cx, sym::Mutex)
     {

tests/ui/mut_mutex_lock.fixed

@@ -37,4 +37,47 @@ fn mut_ref_ref_mutex_lock() {
     *guard += 1;
 }
 
+mod issue16253 {
+    use std::sync::{Arc, Mutex};
+
+    // Do not lint, even if the mutex is owned
+    fn dont_lint_owned(m: Mutex<i32>) {
+        m.lock();
+    }
+
+    struct Wrapper {
+        arc: Arc<Mutex<i32>>,
+        ref_: &'static Mutex<i32>,
+        ref_mut: &'static mut Mutex<i32>,
+        owned: Mutex<i32>,
+    }
+
+    // Do not lint, even if the projection chain would theoretically allow mutable access
+    fn field(w: Wrapper, ref_w: &Wrapper, refmut_w: &mut Wrapper) {
+        w.arc.lock();
+        w.ref_.lock();
+        w.ref_mut.lock();
+        w.owned.lock();
+        ref_w.arc.lock();
+        ref_w.ref_.lock();
+        ref_w.ref_mut.lock();
+        ref_w.owned.lock();
+        refmut_w.arc.lock();
+        refmut_w.ref_.lock();
+        refmut_w.ref_mut.lock();
+        refmut_w.owned.lock();
+    }
+
+    // Do not lint, even if the `.index()` could've been turned into `.index_mut()`
+    fn index(mutexes: &mut [Mutex<u32>]) {
+        // even though `[0]` is _currently_ an `.index(0)`, it can be turned into `.index_mut()` to
+        // enable mutable access: `&mut [Mutex] -> &mut Mutex`
+        mutexes[0].lock().unwrap();
+
+        // `exes` is `&[Mutex] = &Mutex`, so we can't get to `&mut Mutex` no matter what
+        let exes: &_ = mutexes;
+        exes[0].lock().unwrap();
+    }
+}
+
 fn main() {}

tests/ui/mut_mutex_lock.rs

@@ -37,4 +37,47 @@ fn mut_ref_ref_mutex_lock() {
     *guard += 1;
 }
 
+mod issue16253 {
+    use std::sync::{Arc, Mutex};
+
+    // Do not lint, even if the mutex is owned
+    fn dont_lint_owned(m: Mutex<i32>) {
+        m.lock();
+    }
+
+    struct Wrapper {
+        arc: Arc<Mutex<i32>>,
+        ref_: &'static Mutex<i32>,
+        ref_mut: &'static mut Mutex<i32>,
+        owned: Mutex<i32>,
+    }
+
+    // Do not lint, even if the projection chain would theoretically allow mutable access
+    fn field(w: Wrapper, ref_w: &Wrapper, refmut_w: &mut Wrapper) {
+        w.arc.lock();
+        w.ref_.lock();
+        w.ref_mut.lock();
+        w.owned.lock();
+        ref_w.arc.lock();
+        ref_w.ref_.lock();
+        ref_w.ref_mut.lock();
+        ref_w.owned.lock();
+        refmut_w.arc.lock();
+        refmut_w.ref_.lock();
+        refmut_w.ref_mut.lock();
+        refmut_w.owned.lock();
+    }
+
+    // Do not lint, even if the `.index()` could've been turned into `.index_mut()`
+    fn index(mutexes: &mut [Mutex<u32>]) {
+        // even though `[0]` is _currently_ an `.index(0)`, it can be turned into `.index_mut()` to
+        // enable mutable access: `&mut [Mutex] -> &mut Mutex`
+        mutexes[0].lock().unwrap();
+
+        // `exes` is `&[Mutex] = &Mutex`, so we can't get to `&mut Mutex` no matter what
+        let exes: &_ = mutexes;
+        exes[0].lock().unwrap();
+    }
+}
+
 fn main() {}