Skip to content

Commit 138c735

Browse files
ada4aada4a
committed
fix(mut_mutex_lock): FP in projection chains
1 parent d7fd373 commit 138c735

File tree

4 files changed

+593
-7
lines changed

4 files changed

+593
-7
lines changed

‎clippy_lints/src/methods/mut_mutex_lock.rs‎

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,44 @@ use clippy_utils::expr_custom_deref_adjustment;
33
use clippy_utils::res::{MaybeDef, MaybeTypeckRes};
44
use clippy_utils::ty::peel_and_count_ty_refs;
55
use rustc_errors::Applicability;
6-
use rustc_hir::{Expr, Mutability};
6+
use rustc_hir::{Expr, ExprKind, Mutability};
77
use rustc_lint::LateContext;
88
use rustc_span::{Span, sym};
99

1010
use super::MUT_MUTEX_LOCK;
1111

1212
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, recv: &'tcx Expr<'tcx>, name_span: Span) {
13-
if matches!(expr_custom_deref_adjustment(cx, recv), None | Some(Mutability::Mut))
14-
// NOTE: the reason we don't use `expr_ty_adjusted` here is that a call to `Mutex::lock` by itself
15-
// adjusts the receiver to be `&Mutex`
16-
&& let (_, _, Some(Mutability::Mut)) = peel_and_count_ty_refs(cx.typeck_results().expr_ty(recv))
17-
&& cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::Mutex)
13+
// given a `recv` like `a.b.mutex`, this returns `[a.b.mutex, a.b, a]`
14+
let mut projection_chain = std::iter::successors(Some(recv), |recv| {
15+
if let ExprKind::Index(r, ..) | ExprKind::Field(r, _) = recv.kind {
16+
Some(r)
17+
} else {
18+
None
19+
}
20+
});
21+
22+
// If, somewhere along the projection chain, we stumble upon a field of type `&T`, or dereference a
23+
// type like `Arc<T>` to `&T`, we no longer have mutable access to the undelying `Mutex`
24+
if projection_chain.all(|recv| {
25+
let expr_ty = cx.typeck_results().expr_ty(recv);
26+
// The reason we don't use `expr_ty_adjusted` here is twofold:
27+
//
28+
// Consider code like this:
29+
// ```rs
30+
// struct Foo(Mutex<i32>);
31+
//
32+
// fn fun(f: &Foo) {
33+
// f.0.lock()
34+
// }
35+
// ```
36+
// - In the outermost receiver (`f.0`), the adjusted type would be `&Mutex`, due to an adjustment
37+
// performed by `Mutex::lock`.
38+
// - In the intermediary receivers (here, only `f`), the adjusted type would be fully dereferenced
39+
// (`Foo`), which would make us miss the fact that `f` is actually behind a `&` -- this
40+
// information is preserved in the pre-adjustment type (`&Foo`)
41+
peel_and_count_ty_refs(expr_ty).2 != Some(Mutability::Not)
42+
&& expr_custom_deref_adjustment(cx, recv) != Some(Mutability::Not)
43+
}) && cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::Mutex)
1844
{
1945
span_lint_and_sugg(
2046
cx,

‎tests/ui/mut_mutex_lock.fixed‎

Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,4 +37,243 @@ fn mut_ref_ref_mutex_lock() {
3737
*guard += 1;
3838
}
3939

40+
mod issue16253 {
41+
use std::sync::{Arc, Mutex};
42+
43+
struct RefMutex(&'static Mutex<i32>);
44+
impl RefMutex {
45+
fn foo(self, refself: &Self, refmutself: &mut Self) {
46+
// since `.0` is `&Mutex`, we can't get to `&mut Mutex` no matter what `self` is
47+
refself.0.lock().unwrap();
48+
refmutself.0.lock().unwrap();
49+
self.0.lock().unwrap();
50+
}
51+
}
52+
53+
struct ArcMutex(Arc<Mutex<i32>>);
54+
impl ArcMutex {
55+
fn foo(self, refself: &Self, refmutself: &mut Self) {
56+
// since `.0` is `Arc<Mutex> = &Mutex`, we can't get a `&mut Mutex` no matter what `self` is
57+
refself.0.lock().unwrap();
58+
refmutself.0.lock().unwrap();
59+
self.0.lock().unwrap();
60+
}
61+
}
62+
63+
struct RefMutMutex(&'static mut Mutex<i32>);
64+
impl RefMutMutex {
65+
fn foo(self, refself: &Self, refmutself: &mut Self) {
66+
// since `.0` is `&mut Mutex`, we _can_ get a `&mut Mutex` if we have `self`/`&mut self`
67+
refself.0.lock().unwrap();
68+
refmutself.0.get_mut().unwrap();
69+
//~^ mut_mutex_lock
70+
self.0.get_mut().unwrap();
71+
//~^ mut_mutex_lock
72+
}
73+
}
74+
75+
struct ValueMutex(Mutex<i32>);
76+
impl ValueMutex {
77+
fn foo(mut self, refself: &Self, refmutself: &mut Self) {
78+
// since `.0` is `Mutex`, we _can_ get a `&mut Mutex` if we have `self`/`&mut self`
79+
refself.0.lock().unwrap();
80+
refmutself.0.get_mut().unwrap();
81+
//~^ mut_mutex_lock
82+
self.0.get_mut().unwrap();
83+
//~^ mut_mutex_lock
84+
}
85+
}
86+
87+
struct RefRefMutex(&'static RefMutex);
88+
impl RefRefMutex {
89+
fn foo(self, refself: &Self, refmutself: &mut Self) {
90+
// since `.0` is `&RefMutex = &&Mutex`,
91+
// we can't get to `&mut Mutex` no matter what `self` is
92+
refself.0.0.lock().unwrap();
93+
refmutself.0.0.lock().unwrap();
94+
self.0.0.lock().unwrap();
95+
}
96+
}
97+
98+
struct ArcRefMutex(Arc<RefMutex>);
99+
impl ArcRefMutex {
100+
fn foo(self, refself: &Self, refmutself: &mut Self) {
101+
// since `.0` is `Arc<RefMutex> = &RefMutex = &&Mutex`,
102+
// we can't get to `&mut Mutex` no matter what `self` is
103+
refself.0.0.lock().unwrap();
104+
refmutself.0.0.lock().unwrap();
105+
self.0.0.lock().unwrap();
106+
}
107+
}
108+
109+
struct RefMutRefMutex(&'static mut RefMutex);
110+
impl RefMutRefMutex {
111+
fn foo(self, refself: &Self, refmutself: &mut Self) {
112+
// since `.0` is `&mut RefMutex = &mut &Mutex`,
113+
// we can't get to `&mut Mutex` no matter what `self` is
114+
refself.0.0.lock().unwrap();
115+
refmutself.0.0.lock().unwrap();
116+
self.0.0.lock().unwrap();
117+
}
118+
}
119+
120+
struct ValueRefMutex(RefMutex);
121+
impl ValueRefMutex {
122+
fn foo(self, refself: &Self, refmutself: &mut Self) {
123+
// since `.0` is `RefMutex = &Mutex`,
124+
// we can't get to `&mut Mutex` no matter what `self` is
125+
refself.0.0.lock().unwrap();
126+
refmutself.0.0.lock().unwrap();
127+
self.0.0.lock().unwrap();
128+
}
129+
}
130+
131+
struct RefArcMutex(&'static ArcMutex);
132+
impl RefArcMutex {
133+
fn foo(self, refself: &Self, refmutself: &mut Self) {
134+
// since `.0` is `Arc<RefMutex> = &RefMutex = &&Mutex`,
135+
// we can't get to `&mut Mutex` no matter what `self` is
136+
refself.0.0.lock().unwrap();
137+
refmutself.0.0.lock().unwrap();
138+
self.0.0.lock().unwrap();
139+
}
140+
}
141+
142+
struct ArcArcMutex(Arc<ArcMutex>);
143+
impl ArcArcMutex {
144+
fn foo(self, refself: &Self, refmutself: &mut Self) {
145+
// since `.0` is `Arc<ArcMutex> = &ArcMutex = &&Mutex`,
146+
// we can't get to `&mut Mutex` no matter what `self` is
147+
refself.0.0.lock().unwrap();
148+
refmutself.0.0.lock().unwrap();
149+
self.0.0.lock().unwrap();
150+
}
151+
}
152+
153+
struct RefMutArcMutex(&'static mut ArcMutex);
154+
impl RefMutArcMutex {
155+
fn foo(self, refself: &Self, refmutself: &mut Self) {
156+
// since `.0` is `&mut ArcMutex = &mut &Mutex`,
157+
// we can't get to `&mut Mutex` no matter what `self` is
158+
refself.0.0.lock().unwrap();
159+
refmutself.0.0.lock().unwrap();
160+
self.0.0.lock().unwrap();
161+
}
162+
}
163+
164+
struct ValueArcMutex(ArcMutex);
165+
impl ValueArcMutex {
166+
fn foo(self, refself: &Self, refmutself: &mut Self) {
167+
// since `.0` is `ArcMutex = &Mutex`,
168+
// we can't get to `&mut Mutex` no matter what `self` is
169+
refself.0.0.lock().unwrap();
170+
refmutself.0.0.lock().unwrap();
171+
self.0.0.lock().unwrap();
172+
}
173+
}
174+
175+
struct RefRefMutMutex(&'static RefMutMutex);
176+
impl RefRefMutMutex {
177+
fn foo(self, refself: &Self, refmutself: &mut Self) {
178+
// since `.0` is `&RefMutMutex = &&mut Mutex`,
179+
// we can't get to `&mut Mutex` no matter what `self` is
180+
refself.0.0.lock().unwrap();
181+
refmutself.0.0.lock().unwrap();
182+
self.0.0.lock().unwrap();
183+
}
184+
}
185+
186+
struct ArcRefMutMutex(Arc<RefMutMutex>);
187+
impl ArcRefMutMutex {
188+
fn foo(self, refself: &Self, refmutself: &mut Self) {
189+
// since `.0` is `Arc<RefMutMutex> = &RefMutMutex = &&mut Mutex`,
190+
// we can't get to `&mut Mutex` no matter what `self` is
191+
refself.0.0.lock().unwrap();
192+
refmutself.0.0.lock().unwrap();
193+
self.0.0.lock().unwrap();
194+
}
195+
}
196+
197+
struct RefMutRefMutMutex(&'static mut RefMutMutex);
198+
impl RefMutRefMutMutex {
199+
fn foo(self, refself: &Self, refmutself: &mut Self) {
200+
// since `.0` is `&mut RefMutMutex = &mut &mut Mutex`,
201+
// we _can_ get to `&mut Mutex` if we have `self`/`&mut self`
202+
refself.0.0.lock().unwrap();
203+
refmutself.0.0.get_mut().unwrap();
204+
//~^ mut_mutex_lock
205+
self.0.0.get_mut().unwrap();
206+
//~^ mut_mutex_lock
207+
}
208+
}
209+
210+
struct ValueRefMutMutex(RefMutMutex);
211+
impl ValueRefMutMutex {
212+
fn foo(self, refself: &Self, refmutself: &mut Self) {
213+
// since `.0` is `RefMutMutex = &mut Mutex`,
214+
// we _can_ get to `&mut Mutex` if we have `self`/`&mut self`
215+
refself.0.0.lock().unwrap();
216+
refmutself.0.0.get_mut().unwrap();
217+
//~^ mut_mutex_lock
218+
self.0.0.get_mut().unwrap();
219+
//~^ mut_mutex_lock
220+
}
221+
}
222+
223+
struct RefValueMutex(&'static ValueMutex);
224+
impl RefValueMutex {
225+
fn foo(self, refself: &Self, refmutself: &mut Self) {
226+
// since `.0` is `&ValueMutex = &Mutex`,
227+
// we can't get to `&mut Mutex` no matter what `self` is
228+
refself.0.0.lock().unwrap();
229+
refmutself.0.0.lock().unwrap();
230+
self.0.0.lock().unwrap();
231+
}
232+
}
233+
234+
struct ArcValueMutex(Arc<ValueMutex>);
235+
impl ArcValueMutex {
236+
fn foo(self, refself: &Self, refmutself: &mut Self) {
237+
// since `.0` is `Arc<ValueMutex> = &ValueMutex = &Mutex`,
238+
// we can't get to `&mut Mutex` no matter what `self` is
239+
refself.0.0.lock().unwrap();
240+
refmutself.0.0.lock().unwrap();
241+
self.0.0.lock().unwrap();
242+
}
243+
}
244+
245+
struct RefMutValueMutex(&'static mut ValueMutex);
246+
impl RefMutValueMutex {
247+
fn foo(self, refself: &Self, refmutself: &mut Self) {
248+
// since `.0` is `&mut ValueMutex = &mut Mutex`,
249+
// we _can_ get to `&mut Mutex` if we have `self`/`&mut self`
250+
refself.0.0.lock().unwrap();
251+
refmutself.0.0.get_mut().unwrap();
252+
//~^ mut_mutex_lock
253+
self.0.0.get_mut().unwrap();
254+
//~^ mut_mutex_lock
255+
}
256+
}
257+
258+
struct ValueValueMutex(ValueMutex);
259+
impl ValueValueMutex {
260+
fn foo(mut self, refself: &Self, refmutself: &mut Self) {
261+
// since `.0` is `ValueMutex = Mutex`,
262+
// we _can_ get to `&mut Mutex` if we have `self`/`&mut self`
263+
refself.0.0.lock().unwrap();
264+
refmutself.0.0.get_mut().unwrap();
265+
//~^ mut_mutex_lock
266+
self.0.0.get_mut().unwrap();
267+
//~^ mut_mutex_lock
268+
}
269+
}
270+
271+
fn index(mutexes: &mut [Mutex<u32>]) {
272+
let exes: &_ = mutexes;
273+
exes[0].lock().unwrap();
274+
mutexes[0].get_mut().unwrap();
275+
//~^ mut_mutex_lock
276+
}
277+
}
278+
40279
fn main() {}

0 commit comments

Comments
 (0)