diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 53bcd319bc86..79fcaade3052 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2519,6 +2519,10 @@ declare_clippy_lint! { /// guarantee that the mutex isn't locked, instead of just a runtime /// guarantee. /// + /// ### Known problems + /// + /// This lint doesn't take lifetimes into account. + /// /// ### Example /// ```no_run /// use std::sync::{Arc, Mutex}; @@ -5592,7 +5596,7 @@ impl Methods { } }, (sym::lock, []) => { - mut_mutex_lock::check(cx, expr, recv, span); + mut_mutex_lock::check(cx, recv, span); }, (name @ (sym::map | sym::map_err), [m_arg]) => { if name == sym::map { diff --git a/clippy_lints/src/methods/mut_mutex_lock.rs b/clippy_lints/src/methods/mut_mutex_lock.rs index 4ef9169e4daf..bd7d1ddb025f 100644 --- a/clippy_lints/src/methods/mut_mutex_lock.rs +++ b/clippy_lints/src/methods/mut_mutex_lock.rs @@ -1,21 +1,41 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::expr_custom_deref_adjustment; use clippy_utils::res::MaybeDef as _; -use clippy_utils::ty::peel_and_count_ty_refs; +use clippy_utils::ty::{implements_trait, peel_and_count_ty_refs}; use rustc_errors::Applicability; -use rustc_hir::{Expr, Mutability}; +use rustc_hir::{Expr, ExprKind, Mutability, UnOp}; use rustc_lint::LateContext; +use rustc_middle::ty::adjustment::{Adjust, DerefAdjustKind}; use rustc_span::{Span, sym}; use super::MUT_MUTEX_LOCK; -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, ex: &'tcx Expr<'tcx>, recv: &'tcx Expr<'tcx>, name_span: Span) { - if matches!(expr_custom_deref_adjustment(cx, recv), None | Some(Mutability::Mut)) - && let (_, _, Some(Mutability::Mut)) = peel_and_count_ty_refs(cx.typeck_results().expr_ty(recv)) - && let Some(method_id) = cx.typeck_results().type_dependent_def_id(ex.hir_id) - && let Some(impl_id) = cx.tcx.impl_of_assoc(method_id) - && cx.tcx.type_of(impl_id).is_diag_item(cx, sym::Mutex) +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, recv: &'tcx Expr<'tcx>, name_span: Span) { + // Make sure that we have a mutable access to `Mutex`: + // 1. 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` + if 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) + // 2. The mutex was accessed either directly (`mutex.lock()`), or through a series of + // deref/field/indexing projections. Since the final `.lock()` call only requires `&Mutex`, + // those might be immutable, and so we need to manually check whether mutable projections + // would've been possible. For that, we'll repeatedly peel off projections and check each + // intermediary receiver. + && let Some(recv) = check_projections(cx, recv) { + if let ExprKind::Path(ref p) = recv.kind + && let Some(did) = cx.qpath_res(p, recv.hir_id).opt_def_id() + && cx.tcx.is_static(did) + { + // Don't lint on statics + return; + } + span_lint_and_sugg( cx, MUT_MUTEX_LOCK, @@ -27,3 +47,60 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, ex: &'tcx Expr<'tcx>, recv: &' ); } } + +/// See the comment at the callsite. +/// +/// Returns the innermost receiver if all the projections could be made mutable. +fn check_projections<'tcx>(cx: &LateContext<'tcx>, mut recv: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { + let deref_mut_trait = cx.tcx.lang_items().deref_mut_trait(); + let impls_deref_mut = |ty| deref_mut_trait.is_some_and(|trait_id| implements_trait(cx, ty, trait_id, &[])); + let typeck = cx.typeck_results(); + + loop { + // Check that the (deref) adjustments could be made mutable + #[expect(clippy::question_mark, reason = "the suggested form is confusing")] + if (typeck.expr_adjustments(recv)) + .iter() + .map_while(|a| match a.kind { + Adjust::Deref(x) => Some((a.target, x)), + _ => None, + }) + .try_fold(typeck.expr_ty(recv), |ty, (target, deref)| match deref { + // There has been an overloaded deref, most likely an immutable one, as `.lock()` didn't require a + // mutable one -- we need to check if a mutable deref would've been possible, i.e. if + // `ty: DerefMut` (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 has been a simple deref; if it happened on a `&T`, then we know it will can't be changed to + // provide mutable access + DerefAdjustKind::Builtin => (ty.ref_mutability() != Some(Mutability::Not)).then_some(target), + DerefAdjustKind::Pin => Some(target), + }) + .is_none() + { + return None; + } + + // Peel off one projection + match recv.kind { + ExprKind::Unary(UnOp::Deref, base) => { + if impls_deref_mut(typeck.expr_ty_adjusted(base)) { + recv = base; + } else { + return None; + } + }, + ExprKind::Index(..) | ExprKind::Field(..) => { + // 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 + return None; + }, + _ => { + // No more projections to check + break; + }, + } + } + Some(recv) +} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index b352529078f7..9503c228fce5 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -467,10 +467,9 @@ pub fn trait_ref_of_method<'tcx>(cx: &LateContext<'tcx>, owner: OwnerId) -> Opti /// This method will return tuple of projection stack and root of the expression, /// used in `can_mut_borrow_both`. /// -/// For example, if `e` represents the `v[0].a.b[x]` -/// this method will return a tuple, composed of a `Vec` -/// containing the `Expr`s for `v[0], v[0].a, v[0].a.b, v[0].a.b[x]` -/// and an `Expr` for root of them, `v` +/// For example, if `e` represents the `v[0].a.b[x]` this method will return a tuple, composed of: +/// - a `Vec` containing the `Expr`s for `v[0], v[0].a, v[0].a.b, v[0].a.b[x]` +/// - and an `Expr` for root of them, `v` fn projection_stack<'a, 'hir>( mut e: &'a Expr<'hir>, ctxt: SyntaxContext, @@ -500,7 +499,8 @@ pub fn expr_custom_deref_adjustment(cx: &LateContext<'_>, e: &Expr<'_>) -> Optio Adjust::Deref(DerefAdjustKind::Builtin) => None, _ => Some(None), }) - .and_then(|x| x) + // if there were no adjustments to begin with, trivially none of them are of the custom-deref kind + .unwrap_or(None) } /// Checks if two expressions can be mutably borrowed simultaneously diff --git a/tests/ui/mut_mutex_lock.fixed b/tests/ui/mut_mutex_lock.fixed index 44587b89ec83..53bf902e1ecb 100644 --- a/tests/ui/mut_mutex_lock.fixed +++ b/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 on an owned value since it has a good chance of already having another active borrow. + fn dont_lint_owned(m: Mutex) { + m.lock(); + } + + struct Wrapper { + arc: Arc>, + ref_: &'static Mutex, + ref_mut: &'static mut Mutex, + owned: Mutex, + } + + // 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 be turned into `.index_mut()` + fn index(mutexes: &mut [Mutex]) { + // 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() {} diff --git a/tests/ui/mut_mutex_lock.rs b/tests/ui/mut_mutex_lock.rs index eef4f26c0959..1b2ab140c698 100644 --- a/tests/ui/mut_mutex_lock.rs +++ b/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 on an owned value since it has a good chance of already having another active borrow. + fn dont_lint_owned(m: Mutex) { + m.lock(); + } + + struct Wrapper { + arc: Arc>, + ref_: &'static Mutex, + ref_mut: &'static mut Mutex, + owned: Mutex, + } + + // 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 be turned into `.index_mut()` + fn index(mutexes: &mut [Mutex]) { + // 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() {}