Skip to content

Commit 02f3c17

Browse files
borsbors
committed
Auto merge of #8419 - flip1995:await_parking_alot, r=llogiq
Fix `await_holding_lock` not linting `parking_lot` Mutex/RwLock This adds tests for `RwLock` and `parking_lot::{Mutex, RwLock}`, which were added before in 2dc8c08, but never tested in UI tests. I noticed this while reading [fasterthanli.me](https://fasterthanli.me/articles/a-rust-match-made-in-hell) latest blog post, complaining that Clippy doesn't catch this for `parking_lot`. (Too many people read his blog, he's too powerful) Some more things: - Adds a test for #6446 - Improves the lint message changelog: [`await_holding_lock`]: Now also lints for `parking_lot::{Mutex, RwLock}`
2 parents 76f91b6 + ea0ce7b commit 02f3c17

8 files changed

+454
-128
lines changed

‎clippy_lints/src/await_holding_invalid.rs

+82-37
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use clippy_utils::diagnostics::span_lint_and_note;
1+
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::{match_def_path, paths};
33
use rustc_hir::def_id::DefId;
44
use rustc_hir::{AsyncGeneratorKind, Body, BodyId, GeneratorKind};
@@ -9,8 +9,7 @@ use rustc_span::Span;
99

1010
declare_clippy_lint! {
1111
/// ### What it does
12-
/// Checks for calls to await while holding a
13-
/// non-async-aware MutexGuard.
12+
/// Checks for calls to await while holding a non-async-aware MutexGuard.
1413
///
1514
/// ### Why is this bad?
1615
/// The Mutex types found in std::sync and parking_lot
@@ -22,77 +21,110 @@ declare_clippy_lint! {
2221
/// either by introducing a scope or an explicit call to Drop::drop.
2322
///
2423
/// ### Known problems
25-
/// Will report false positive for explicitly dropped guards ([#6446](https://github.com/rust-lang/rust-clippy/issues/6446)).
24+
/// Will report false positive for explicitly dropped guards
25+
/// ([#6446](https://github.com/rust-lang/rust-clippy/issues/6446)). A workaround for this is
26+
/// to wrap the `.lock()` call in a block instead of explicitly dropping the guard.
2627
///
2728
/// ### Example
28-
/// ```rust,ignore
29-
/// use std::sync::Mutex;
30-
///
29+
/// ```rust
30+
/// # use std::sync::Mutex;
31+
/// # async fn baz() {}
3132
/// async fn foo(x: &Mutex<u32>) {
32-
/// let guard = x.lock().unwrap();
33+
/// let mut guard = x.lock().unwrap();
3334
/// *guard += 1;
34-
/// bar.await;
35+
/// baz().await;
36+
/// }
37+
///
38+
/// async fn bar(x: &Mutex<u32>) {
39+
/// let mut guard = x.lock().unwrap();
40+
/// *guard += 1;
41+
/// drop(guard); // explicit drop
42+
/// baz().await;
3543
/// }
3644
/// ```
3745
///
3846
/// Use instead:
39-
/// ```rust,ignore
40-
/// use std::sync::Mutex;
41-
///
47+
/// ```rust
48+
/// # use std::sync::Mutex;
49+
/// # async fn baz() {}
4250
/// async fn foo(x: &Mutex<u32>) {
4351
/// {
44-
/// let guard = x.lock().unwrap();
52+
/// let mut guard = x.lock().unwrap();
4553
/// *guard += 1;
4654
/// }
47-
/// bar.await;
55+
/// baz().await;
56+
/// }
57+
///
58+
/// async fn bar(x: &Mutex<u32>) {
59+
/// {
60+
/// let mut guard = x.lock().unwrap();
61+
/// *guard += 1;
62+
/// } // guard dropped here at end of scope
63+
/// baz().await;
4864
/// }
4965
/// ```
5066
#[clippy::version = "1.45.0"]
5167
pub AWAIT_HOLDING_LOCK,
52-
pedantic,
53-
"Inside an async function, holding a MutexGuard while calling await"
68+
suspicious,
69+
"inside an async function, holding a `MutexGuard` while calling `await`"
5470
}
5571

5672
declare_clippy_lint! {
5773
/// ### What it does
58-
/// Checks for calls to await while holding a
59-
/// `RefCell` `Ref` or `RefMut`.
74+
/// Checks for calls to await while holding a `RefCell` `Ref` or `RefMut`.
6075
///
6176
/// ### Why is this bad?
6277
/// `RefCell` refs only check for exclusive mutable access
6378
/// at runtime. Holding onto a `RefCell` ref across an `await` suspension point
6479
/// risks panics from a mutable ref shared while other refs are outstanding.
6580
///
6681
/// ### Known problems
67-
/// Will report false positive for explicitly dropped refs ([#6353](https://github.com/rust-lang/rust-clippy/issues/6353)).
82+
/// Will report false positive for explicitly dropped refs
83+
/// ([#6353](https://github.com/rust-lang/rust-clippy/issues/6353)). A workaround for this is
84+
/// to wrap the `.borrow[_mut]()` call in a block instead of explicitly dropping the ref.
6885
///
6986
/// ### Example
70-
/// ```rust,ignore
71-
/// use std::cell::RefCell;
72-
///
87+
/// ```rust
88+
/// # use std::cell::RefCell;
89+
/// # async fn baz() {}
7390
/// async fn foo(x: &RefCell<u32>) {
7491
/// let mut y = x.borrow_mut();
7592
/// *y += 1;
76-
/// bar.await;
93+
/// baz().await;
94+
/// }
95+
///
96+
/// async fn bar(x: &RefCell<u32>) {
97+
/// let mut y = x.borrow_mut();
98+
/// *y += 1;
99+
/// drop(y); // explicit drop
100+
/// baz().await;
77101
/// }
78102
/// ```
79103
///
80104
/// Use instead:
81-
/// ```rust,ignore
82-
/// use std::cell::RefCell;
83-
///
105+
/// ```rust
106+
/// # use std::cell::RefCell;
107+
/// # async fn baz() {}
84108
/// async fn foo(x: &RefCell<u32>) {
85109
/// {
86110
/// let mut y = x.borrow_mut();
87111
/// *y += 1;
88112
/// }
89-
/// bar.await;
113+
/// baz().await;
114+
/// }
115+
///
116+
/// async fn bar(x: &RefCell<u32>) {
117+
/// {
118+
/// let mut y = x.borrow_mut();
119+
/// *y += 1;
120+
/// } // y dropped here at end of scope
121+
/// baz().await;
90122
/// }
91123
/// ```
92124
#[clippy::version = "1.49.0"]
93125
pub AWAIT_HOLDING_REFCELL_REF,
94-
pedantic,
95-
"Inside an async function, holding a RefCell ref while calling await"
126+
suspicious,
127+
"inside an async function, holding a `RefCell` ref while calling `await`"
96128
}
97129

98130
declare_lint_pass!(AwaitHolding => [AWAIT_HOLDING_LOCK, AWAIT_HOLDING_REFCELL_REF]);
@@ -118,23 +150,36 @@ fn check_interior_types(cx: &LateContext<'_>, ty_causes: &[GeneratorInteriorType
118150
for ty_cause in ty_causes {
119151
if let rustc_middle::ty::Adt(adt, _) = ty_cause.ty.kind() {
120152
if is_mutex_guard(cx, adt.did) {
121-
span_lint_and_note(
153+
span_lint_and_then(
122154
cx,
123155
AWAIT_HOLDING_LOCK,
124156
ty_cause.span,
125-
"this MutexGuard is held across an 'await' point. Consider using an async-aware Mutex type or ensuring the MutexGuard is dropped before calling await",
126-
ty_cause.scope_span.or(Some(span)),
127-
"these are all the await points this lock is held through",
157+
"this `MutexGuard` is held across an `await` point",
158+
|diag| {
159+
diag.help(
160+
"consider using an async-aware `Mutex` type or ensuring the \
161+
`MutexGuard` is dropped before calling await",
162+
);
163+
diag.span_note(
164+
ty_cause.scope_span.unwrap_or(span),
165+
"these are all the `await` points this lock is held through",
166+
);
167+
},
128168
);
129169
}
130170
if is_refcell_ref(cx, adt.did) {
131-
span_lint_and_note(
171+
span_lint_and_then(
132172
cx,
133173
AWAIT_HOLDING_REFCELL_REF,
134174
ty_cause.span,
135-
"this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await",
136-
ty_cause.scope_span.or(Some(span)),
137-
"these are all the await points this ref is held through",
175+
"this `RefCell` reference is held across an `await` point",
176+
|diag| {
177+
diag.help("ensure the reference is dropped before calling `await`");
178+
diag.span_note(
179+
ty_cause.scope_span.unwrap_or(span),
180+
"these are all the `await` points this reference is held through",
181+
);
182+
},
138183
);
139184
}
140185
}

‎clippy_lints/src/lib.register_all.rs

+2
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
1414
LintId::of(attrs::DEPRECATED_SEMVER),
1515
LintId::of(attrs::MISMATCHED_TARGET_OS),
1616
LintId::of(attrs::USELESS_ATTRIBUTE),
17+
LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
18+
LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
1719
LintId::of(bit_mask::BAD_BIT_MASK),
1820
LintId::of(bit_mask::INEFFECTIVE_BIT_MASK),
1921
LintId::of(blacklisted_name::BLACKLISTED_NAME),

‎clippy_lints/src/lib.register_pedantic.rs

-2
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,6 @@
44

55
store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
66
LintId::of(attrs::INLINE_ALWAYS),
7-
LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
8-
LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
97
LintId::of(bit_mask::VERBOSE_BIT_MASK),
108
LintId::of(borrow_as_ptr::BORROW_AS_PTR),
119
LintId::of(bytecount::NAIVE_BYTECOUNT),

‎clippy_lints/src/lib.register_suspicious.rs

+2
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec![
66
LintId::of(assign_ops::MISREFACTORED_ASSIGN_OP),
77
LintId::of(attrs::BLANKET_CLIPPY_RESTRICTION_LINTS),
8+
LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
9+
LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
810
LintId::of(eval_order_dependence::EVAL_ORDER_DEPENDENCE),
911
LintId::of(float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS),
1012
LintId::of(formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),

‎clippy_utils/src/paths.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,9 @@ pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString",
105105
pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"];
106106
pub const PARKING_LOT_RAWMUTEX: [&str; 3] = ["parking_lot", "raw_mutex", "RawMutex"];
107107
pub const PARKING_LOT_RAWRWLOCK: [&str; 3] = ["parking_lot", "raw_rwlock", "RawRwLock"];
108-
pub const PARKING_LOT_MUTEX_GUARD: [&str; 2] = ["parking_lot", "MutexGuard"];
109-
pub const PARKING_LOT_RWLOCK_READ_GUARD: [&str; 2] = ["parking_lot", "RwLockReadGuard"];
110-
pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 2] = ["parking_lot", "RwLockWriteGuard"];
108+
pub const PARKING_LOT_MUTEX_GUARD: [&str; 3] = ["lock_api", "mutex", "MutexGuard"];
109+
pub const PARKING_LOT_RWLOCK_READ_GUARD: [&str; 3] = ["lock_api", "rwlock", "RwLockReadGuard"];
110+
pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 3] = ["lock_api", "rwlock", "RwLockWriteGuard"];
111111
pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"];
112112
pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"];
113113
pub const PERMISSIONS: [&str; 3] = ["std", "fs", "Permissions"];

0 commit comments

Comments
 (0)