Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix await_holding_lock not linting parking_lot Mutex/RwLock #8419

Merged
merged 4 commits into from
Feb 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 82 additions & 37 deletions clippy_lints/src/await_holding_invalid.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use clippy_utils::diagnostics::span_lint_and_note;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::{match_def_path, paths};
use rustc_hir::def_id::DefId;
use rustc_hir::{AsyncGeneratorKind, Body, BodyId, GeneratorKind};
Expand All @@ -9,8 +9,7 @@ use rustc_span::Span;

declare_clippy_lint! {
/// ### What it does
/// Checks for calls to await while holding a
/// non-async-aware MutexGuard.
/// Checks for calls to await while holding a non-async-aware MutexGuard.
///
/// ### Why is this bad?
/// The Mutex types found in std::sync and parking_lot
Expand All @@ -22,77 +21,110 @@ declare_clippy_lint! {
/// either by introducing a scope or an explicit call to Drop::drop.
///
/// ### Known problems
/// Will report false positive for explicitly dropped guards ([#6446](https://github.com/rust-lang/rust-clippy/issues/6446)).
/// Will report false positive for explicitly dropped guards
/// ([#6446](https://github.com/rust-lang/rust-clippy/issues/6446)). A workaround for this is
/// to wrap the `.lock()` call in a block instead of explicitly dropping the guard.
///
/// ### Example
/// ```rust,ignore
/// use std::sync::Mutex;
///
/// ```rust
/// # use std::sync::Mutex;
/// # async fn baz() {}
/// async fn foo(x: &Mutex<u32>) {
/// let guard = x.lock().unwrap();
/// let mut guard = x.lock().unwrap();
/// *guard += 1;
/// bar.await;
/// baz().await;
/// }
///
/// async fn bar(x: &Mutex<u32>) {
/// let mut guard = x.lock().unwrap();
/// *guard += 1;
/// drop(guard); // explicit drop
/// baz().await;
/// }
/// ```
///
/// Use instead:
/// ```rust,ignore
/// use std::sync::Mutex;
///
/// ```rust
/// # use std::sync::Mutex;
/// # async fn baz() {}
/// async fn foo(x: &Mutex<u32>) {
/// {
/// let guard = x.lock().unwrap();
/// let mut guard = x.lock().unwrap();
/// *guard += 1;
/// }
/// bar.await;
/// baz().await;
/// }
///
/// async fn bar(x: &Mutex<u32>) {
/// {
/// let mut guard = x.lock().unwrap();
/// *guard += 1;
/// } // guard dropped here at end of scope
/// baz().await;
/// }
/// ```
#[clippy::version = "1.45.0"]
pub AWAIT_HOLDING_LOCK,
pedantic,
"Inside an async function, holding a MutexGuard while calling await"
suspicious,
"inside an async function, holding a `MutexGuard` while calling `await`"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for calls to await while holding a
/// `RefCell` `Ref` or `RefMut`.
/// Checks for calls to await while holding a `RefCell` `Ref` or `RefMut`.
///
/// ### Why is this bad?
/// `RefCell` refs only check for exclusive mutable access
/// at runtime. Holding onto a `RefCell` ref across an `await` suspension point
/// risks panics from a mutable ref shared while other refs are outstanding.
///
/// ### Known problems
/// Will report false positive for explicitly dropped refs ([#6353](https://github.com/rust-lang/rust-clippy/issues/6353)).
/// Will report false positive for explicitly dropped refs
/// ([#6353](https://github.com/rust-lang/rust-clippy/issues/6353)). A workaround for this is
/// to wrap the `.borrow[_mut]()` call in a block instead of explicitly dropping the ref.
///
/// ### Example
/// ```rust,ignore
/// use std::cell::RefCell;
///
/// ```rust
/// # use std::cell::RefCell;
/// # async fn baz() {}
/// async fn foo(x: &RefCell<u32>) {
/// let mut y = x.borrow_mut();
/// *y += 1;
/// bar.await;
/// baz().await;
/// }
///
/// async fn bar(x: &RefCell<u32>) {
/// let mut y = x.borrow_mut();
/// *y += 1;
/// drop(y); // explicit drop
/// baz().await;
/// }
/// ```
///
/// Use instead:
/// ```rust,ignore
/// use std::cell::RefCell;
///
/// ```rust
/// # use std::cell::RefCell;
/// # async fn baz() {}
/// async fn foo(x: &RefCell<u32>) {
/// {
/// let mut y = x.borrow_mut();
/// *y += 1;
/// }
/// bar.await;
/// baz().await;
/// }
///
/// async fn bar(x: &RefCell<u32>) {
/// {
/// let mut y = x.borrow_mut();
/// *y += 1;
/// } // y dropped here at end of scope
/// baz().await;
/// }
/// ```
#[clippy::version = "1.49.0"]
pub AWAIT_HOLDING_REFCELL_REF,
pedantic,
"Inside an async function, holding a RefCell ref while calling await"
suspicious,
"inside an async function, holding a `RefCell` ref while calling `await`"
}

declare_lint_pass!(AwaitHolding => [AWAIT_HOLDING_LOCK, AWAIT_HOLDING_REFCELL_REF]);
Expand All @@ -118,23 +150,36 @@ fn check_interior_types(cx: &LateContext<'_>, ty_causes: &[GeneratorInteriorType
for ty_cause in ty_causes {
if let rustc_middle::ty::Adt(adt, _) = ty_cause.ty.kind() {
if is_mutex_guard(cx, adt.did) {
span_lint_and_note(
span_lint_and_then(
cx,
AWAIT_HOLDING_LOCK,
ty_cause.span,
"this MutexGuard is held across an 'await' point. Consider using an async-aware Mutex type or ensuring the MutexGuard is dropped before calling await",
ty_cause.scope_span.or(Some(span)),
"these are all the await points this lock is held through",
"this `MutexGuard` is held across an `await` point",
|diag| {
diag.help(
"consider using an async-aware `Mutex` type or ensuring the \
`MutexGuard` is dropped before calling await",
);
diag.span_note(
ty_cause.scope_span.unwrap_or(span),
"these are all the `await` points this lock is held through",
);
},
);
}
if is_refcell_ref(cx, adt.did) {
span_lint_and_note(
span_lint_and_then(
cx,
AWAIT_HOLDING_REFCELL_REF,
ty_cause.span,
"this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await",
ty_cause.scope_span.or(Some(span)),
"these are all the await points this ref is held through",
"this `RefCell` reference is held across an `await` point",
|diag| {
diag.help("ensure the reference is dropped before calling `await`");
diag.span_note(
ty_cause.scope_span.unwrap_or(span),
"these are all the `await` points this reference is held through",
);
},
);
}
}
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.register_all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
LintId::of(attrs::DEPRECATED_SEMVER),
LintId::of(attrs::MISMATCHED_TARGET_OS),
LintId::of(attrs::USELESS_ATTRIBUTE),
LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
LintId::of(bit_mask::BAD_BIT_MASK),
LintId::of(bit_mask::INEFFECTIVE_BIT_MASK),
LintId::of(blacklisted_name::BLACKLISTED_NAME),
Expand Down
2 changes: 0 additions & 2 deletions clippy_lints/src/lib.register_pedantic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
LintId::of(attrs::INLINE_ALWAYS),
LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
LintId::of(bit_mask::VERBOSE_BIT_MASK),
LintId::of(borrow_as_ptr::BORROW_AS_PTR),
LintId::of(bytecount::NAIVE_BYTECOUNT),
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.register_suspicious.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec![
LintId::of(assign_ops::MISREFACTORED_ASSIGN_OP),
LintId::of(attrs::BLANKET_CLIPPY_RESTRICTION_LINTS),
LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
LintId::of(eval_order_dependence::EVAL_ORDER_DEPENDENCE),
LintId::of(float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS),
LintId::of(formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),
Expand Down
6 changes: 3 additions & 3 deletions clippy_utils/src/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString",
pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"];
pub const PARKING_LOT_RAWMUTEX: [&str; 3] = ["parking_lot", "raw_mutex", "RawMutex"];
pub const PARKING_LOT_RAWRWLOCK: [&str; 3] = ["parking_lot", "raw_rwlock", "RawRwLock"];
pub const PARKING_LOT_MUTEX_GUARD: [&str; 2] = ["parking_lot", "MutexGuard"];
pub const PARKING_LOT_RWLOCK_READ_GUARD: [&str; 2] = ["parking_lot", "RwLockReadGuard"];
pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 2] = ["parking_lot", "RwLockWriteGuard"];
pub const PARKING_LOT_MUTEX_GUARD: [&str; 3] = ["lock_api", "mutex", "MutexGuard"];
pub const PARKING_LOT_RWLOCK_READ_GUARD: [&str; 3] = ["lock_api", "rwlock", "RwLockReadGuard"];
pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 3] = ["lock_api", "rwlock", "RwLockWriteGuard"];
pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"];
pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"];
pub const PERMISSIONS: [&str; 3] = ["std", "fs", "Permissions"];
Expand Down
Loading