Skip to content

Commit

Permalink
Move await_holding_* lints to suspicious and improve doc
Browse files Browse the repository at this point in the history
Even though the FP for that the lints were moved to pedantic isn't fixed
yet, running the lintcheck tool over the most popular 279 crates didn't
trigger this lint once. I would say that this lint is valuable enough,
despite the known FP, to be warn-by-default. Especially since a pretty
nice workaround exists.
  • Loading branch information
flip1995 committed Feb 17, 2022
1 parent c570941 commit ea0ce7b
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 30 deletions.
88 changes: 60 additions & 28 deletions clippy_lints/src/await_holding_invalid.rs
Expand Up @@ -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 Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.register_all.rs
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
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
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

0 comments on commit ea0ce7b

Please sign in to comment.