From ea0ce7bb76bb01741fc3ba2ca32a80c75a380d0f Mon Sep 17 00:00:00 2001 From: flip1995 Date: Thu, 17 Feb 2022 17:57:39 +0100 Subject: [PATCH] Move await_holding_* lints to suspicious and improve doc 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. --- clippy_lints/src/await_holding_invalid.rs | 88 ++++++++++++++------- clippy_lints/src/lib.register_all.rs | 2 + clippy_lints/src/lib.register_pedantic.rs | 2 - clippy_lints/src/lib.register_suspicious.rs | 2 + 4 files changed, 64 insertions(+), 30 deletions(-) diff --git a/clippy_lints/src/await_holding_invalid.rs b/clippy_lints/src/await_holding_invalid.rs index f7bc8395c82..f0979840ff8 100644 --- a/clippy_lints/src/await_holding_invalid.rs +++ b/clippy_lints/src/await_holding_invalid.rs @@ -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 @@ -22,41 +21,57 @@ 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) { - /// let guard = x.lock().unwrap(); + /// let mut guard = x.lock().unwrap(); /// *guard += 1; - /// bar.await; + /// baz().await; + /// } + /// + /// async fn bar(x: &Mutex) { + /// 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) { /// { - /// let guard = x.lock().unwrap(); + /// let mut guard = x.lock().unwrap(); /// *guard += 1; /// } - /// bar.await; + /// baz().await; + /// } + /// + /// async fn bar(x: &Mutex) { + /// { + /// 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 @@ -64,35 +79,52 @@ declare_clippy_lint! { /// 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) { /// let mut y = x.borrow_mut(); /// *y += 1; - /// bar.await; + /// baz().await; + /// } + /// + /// async fn bar(x: &RefCell) { + /// 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) { /// { /// let mut y = x.borrow_mut(); /// *y += 1; /// } - /// bar.await; + /// baz().await; + /// } + /// + /// async fn bar(x: &RefCell) { + /// { + /// 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]); diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index de0ff536907..a94f6b528b4 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -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), diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs index 1292675f4a9..00d30513181 100644 --- a/clippy_lints/src/lib.register_pedantic.rs +++ b/clippy_lints/src/lib.register_pedantic.rs @@ -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), diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 10f8ae4b7f7..da56f800804 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/clippy_lints/src/lib.register_suspicious.rs @@ -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),