diff --git a/clippy_lints/src/await_holding_invalid.rs b/clippy_lints/src/await_holding_invalid.rs index 367534499fd0..2000bdae363f 100644 --- a/clippy_lints/src/await_holding_invalid.rs +++ b/clippy_lints/src/await_holding_invalid.rs @@ -60,12 +60,12 @@ impl LateLintPass<'_> for AwaitHoldingLock { }; let def_id = cx.tcx.hir().body_owner_def_id(body_id); let typeck_results = cx.tcx.typeck(def_id); - check_interior_types(cx, &typeck_results.generator_interior_types, body.value.span); + check_interior_types_lock(cx, &typeck_results.generator_interior_types, body.value.span); } } } -fn check_interior_types(cx: &LateContext<'_>, ty_causes: &[GeneratorInteriorTypeCause<'_>], span: Span) { +fn check_interior_types_lock(cx: &LateContext<'_>, ty_causes: &[GeneratorInteriorTypeCause<'_>], span: Span) { for ty_cause in ty_causes { if let rustc_middle::ty::Adt(adt, _) = ty_cause.ty.kind() { if is_mutex_guard(cx, adt.did) { @@ -90,3 +90,79 @@ fn is_mutex_guard(cx: &LateContext<'_>, def_id: DefId) -> bool { || match_def_path(cx, def_id, &paths::PARKING_LOT_RWLOCK_READ_GUARD) || match_def_path(cx, def_id, &paths::PARKING_LOT_RWLOCK_WRITE_GUARD) } + +declare_clippy_lint! { + /// **What it does:** 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:** None. + /// + /// **Example:** + /// + /// ```rust,ignore + /// use std::cell::RefCell; + /// + /// async fn foo(x: &RefCell) { + /// let b = x.borrow_mut()(); + /// *ref += 1; + /// bar.await; + /// } + /// ``` + /// + /// Use instead: + /// ```rust,ignore + /// use std::cell::RefCell; + /// + /// async fn foo(x: &RefCell) { + /// { + /// let b = x.borrow_mut(); + /// *ref += 1; + /// } + /// bar.await; + /// } + /// ``` + pub AWAIT_HOLDING_REFCELL_REF, + pedantic, + "Inside an async function, holding a RefCell ref while calling await" +} + +declare_lint_pass!(AwaitHoldingRefCellRef => [AWAIT_HOLDING_REFCELL_REF]); + +impl LateLintPass<'_> for AwaitHoldingRefCellRef { + fn check_body(&mut self, cx: &LateContext<'_>, body: &'_ Body<'_>) { + use AsyncGeneratorKind::{Block, Closure, Fn}; + if let Some(GeneratorKind::Async(Block | Closure | Fn)) = body.generator_kind { + let body_id = BodyId { + hir_id: body.value.hir_id, + }; + let def_id = cx.tcx.hir().body_owner_def_id(body_id); + let typeck_results = cx.tcx.typeck(def_id); + check_interior_types_refcell(cx, &typeck_results.generator_interior_types, body.value.span); + } + } +} + +fn check_interior_types_refcell(cx: &LateContext<'_>, ty_causes: &[GeneratorInteriorTypeCause<'_>], span: Span) { + for ty_cause in ty_causes { + if let rustc_middle::ty::Adt(adt, _) = ty_cause.ty.kind() { + if is_refcell_ref(cx, adt.did) { + span_lint_and_note( + 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", + ); + } + } + } +} + +fn is_refcell_ref(cx: &LateContext<'_>, def_id: DefId) -> bool { + match_def_path(cx, def_id, &paths::REFCELL_REF) || match_def_path(cx, def_id, &paths::REFCELL_REFMUT) +} diff --git a/clippy_lints/src/await_holding_refcell_ref.rs b/clippy_lints/src/await_holding_refcell_ref.rs deleted file mode 100644 index 9a75911acbea..000000000000 --- a/clippy_lints/src/await_holding_refcell_ref.rs +++ /dev/null @@ -1,83 +0,0 @@ -use crate::utils::{match_def_path, paths, span_lint_and_note}; -use rustc_hir::def_id::DefId; -use rustc_hir::{AsyncGeneratorKind, Body, BodyId, GeneratorKind}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::GeneratorInteriorTypeCause; -use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::Span; - -declare_clippy_lint! { - /// **What it does:** 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:** None. - /// - /// **Example:** - /// - /// ```rust,ignore - /// use std::cell::RefCell; - /// - /// async fn foo(x: &RefCell) { - /// let b = x.borrow_mut()(); - /// *ref += 1; - /// bar.await; - /// } - /// ``` - /// - /// Use instead: - /// ```rust,ignore - /// use std::cell::RefCell; - /// - /// async fn foo(x: &RefCell) { - /// { - /// let b = x.borrow_mut(); - /// *ref += 1; - /// } - /// bar.await; - /// } - /// ``` - pub AWAIT_HOLDING_REFCELL_REF, - pedantic, - "Inside an async function, holding a RefCell ref while calling await" -} - -declare_lint_pass!(AwaitHoldingRefCellRef => [AWAIT_HOLDING_REFCELL_REF]); - -impl LateLintPass<'_> for AwaitHoldingRefCellRef { - fn check_body(&mut self, cx: &LateContext<'_>, body: &'_ Body<'_>) { - use AsyncGeneratorKind::{Block, Closure, Fn}; - if let Some(GeneratorKind::Async(Block | Closure | Fn)) = body.generator_kind { - let body_id = BodyId { - hir_id: body.value.hir_id, - }; - let def_id = cx.tcx.hir().body_owner_def_id(body_id); - let typeck_results = cx.tcx.typeck(def_id); - check_interior_types(cx, &typeck_results.generator_interior_types, body.value.span); - } - } -} - -fn check_interior_types(cx: &LateContext<'_>, ty_causes: &[GeneratorInteriorTypeCause<'_>], span: Span) { - for ty_cause in ty_causes { - if let rustc_middle::ty::Adt(adt, _) = ty_cause.ty.kind() { - if is_refcell_ref(cx, adt.did) { - span_lint_and_note( - 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", - ); - } - } - } -} - -fn is_refcell_ref(cx: &LateContext<'_>, def_id: DefId) -> bool { - match_def_path(cx, def_id, &paths::REFCELL_REF) || match_def_path(cx, def_id, &paths::REFCELL_REFMUT) -} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 47cc0c903229..0c5baf96970c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -161,7 +161,6 @@ mod async_yields_async; mod atomic_ordering; mod attrs; mod await_holding_invalid; -mod await_holding_refcell_ref; mod bit_mask; mod blacklisted_name; mod blocks_in_if_conditions; @@ -511,7 +510,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &attrs::UNKNOWN_CLIPPY_LINTS, &attrs::USELESS_ATTRIBUTE, &await_holding_invalid::AWAIT_HOLDING_LOCK, - &await_holding_refcell_ref::AWAIT_HOLDING_REFCELL_REF, + &await_holding_invalid::AWAIT_HOLDING_REFCELL_REF, &bit_mask::BAD_BIT_MASK, &bit_mask::INEFFECTIVE_BIT_MASK, &bit_mask::VERBOSE_BIT_MASK, @@ -908,7 +907,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: // end register lints, do not remove this comment, it’s used in `update_lints` store.register_late_pass(|| box await_holding_invalid::AwaitHoldingLock); - store.register_late_pass(|| box await_holding_refcell_ref::AwaitHoldingRefCellRef); + store.register_late_pass(|| box await_holding_invalid::AwaitHoldingRefCellRef); store.register_late_pass(|| box serde_api::SerdeAPI); store.register_late_pass(|| box utils::internal_lints::CompilerLintFunctions::new()); store.register_late_pass(|| box utils::internal_lints::LintWithoutLintPass::default()); @@ -1192,7 +1191,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: 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_refcell_ref::AWAIT_HOLDING_REFCELL_REF), + LintId::of(&await_holding_invalid::AWAIT_HOLDING_REFCELL_REF), LintId::of(&bit_mask::VERBOSE_BIT_MASK), LintId::of(&checked_conversions::CHECKED_CONVERSIONS), LintId::of(&copies::MATCH_SAME_ARMS), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 63e9220ccd50..c955f37b83a3 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -72,7 +72,7 @@ vec![ group: "pedantic", desc: "Inside an async function, holding a RefCell ref while calling await", deprecation: None, - module: "await_holding_refcell_ref", + module: "await_holding_invalid", }, Lint { name: "bad_bit_mask", diff --git a/tests/ui/await_holding_invalid.rs b/tests/ui/await_holding_invalid.rs index 0458950edee1..45aa3e64292e 100644 --- a/tests/ui/await_holding_invalid.rs +++ b/tests/ui/await_holding_invalid.rs @@ -1,14 +1,15 @@ // edition:2018 -#![warn(clippy::await_holding_lock)] +#![warn(clippy::await_holding_lock, clippy::await_holding_refcell_ref)] +use std::cell::RefCell; use std::sync::Mutex; -async fn bad(x: &Mutex) -> u32 { +async fn bad_lock(x: &Mutex) -> u32 { let guard = x.lock().unwrap(); baz().await } -async fn good(x: &Mutex) -> u32 { +async fn good_lock(x: &Mutex) -> u32 { { let guard = x.lock().unwrap(); let y = *guard + 1; @@ -22,7 +23,7 @@ async fn baz() -> u32 { 42 } -async fn also_bad(x: &Mutex) -> u32 { +async fn also_bad_lock(x: &Mutex) -> u32 { let first = baz().await; let guard = x.lock().unwrap(); @@ -34,7 +35,7 @@ async fn also_bad(x: &Mutex) -> u32 { first + second + third } -async fn not_good(x: &Mutex) -> u32 { +async fn not_good_lock(x: &Mutex) -> u32 { let first = baz().await; let second = { @@ -48,18 +49,97 @@ async fn not_good(x: &Mutex) -> u32 { } #[allow(clippy::manual_async_fn)] -fn block_bad(x: &Mutex) -> impl std::future::Future + '_ { +fn block_bad_lock(x: &Mutex) -> impl std::future::Future + '_ { async move { let guard = x.lock().unwrap(); baz().await } } -fn main() { - let m = Mutex::new(100); - good(&m); - bad(&m); - also_bad(&m); - not_good(&m); - block_bad(&m); +async fn bad_refcell(x: &RefCell) -> u32 { + let b = x.borrow(); + baz().await +} + +async fn bad_mut_refcell(x: &RefCell) -> u32 { + let b = x.borrow_mut(); + baz().await +} + +async fn good_refcell(x: &RefCell) -> u32 { + { + let b = x.borrow_mut(); + let y = *b + 1; + } + baz().await; + let b = x.borrow_mut(); + 47 +} + +async fn also_bad_refcell(x: &RefCell) -> u32 { + let first = baz().await; + + let b = x.borrow_mut(); + + let second = baz().await; + + let third = baz().await; + + first + second + third +} + +async fn less_bad_refcell(x: &RefCell) -> u32 { + let first = baz().await; + + let b = x.borrow_mut(); + + let second = baz().await; + + drop(b); + + let third = baz().await; + + first + second + third +} + +async fn not_good_refcell(x: &RefCell) -> u32 { + let first = baz().await; + + let second = { + let b = x.borrow_mut(); + baz().await + }; + + let third = baz().await; + + first + second + third +} + +#[allow(clippy::manual_async_fn)] +fn block_bad_refcell(x: &RefCell) -> impl std::future::Future + '_ { + async move { + let b = x.borrow_mut(); + baz().await + } +} + +fn main() { + { + let m = Mutex::new(100); + good_lock(&m); + bad_lock(&m); + also_bad_lock(&m); + not_good_lock(&m); + block_bad_lock(&m); + } + { + let rc = RefCell::new(100); + good_refcell(&rc); + bad_refcell(&rc); + bad_mut_refcell(&rc); + also_bad_refcell(&rc); + less_bad_refcell(&rc); + not_good_refcell(&rc); + block_bad_refcell(&rc); + } } diff --git a/tests/ui/await_holding_invalid.stderr b/tests/ui/await_holding_invalid.stderr index 315d5731b962..c8d49820c020 100644 --- a/tests/ui/await_holding_invalid.stderr +++ b/tests/ui/await_holding_invalid.stderr @@ -1,12 +1,12 @@ error: this MutexGuard is held across an 'await' point. Consider using an async-aware Mutex type or ensuring the MutexGuard is dropped before calling await. - --> $DIR/await_holding_invalid.rs:7:9 + --> $DIR/await_holding_invalid.rs:8:9 | LL | let guard = x.lock().unwrap(); | ^^^^^ | = note: `-D clippy::await-holding-lock` implied by `-D warnings` note: these are all the await points this lock is held through - --> $DIR/await_holding_invalid.rs:7:5 + --> $DIR/await_holding_invalid.rs:8:5 | LL | / let guard = x.lock().unwrap(); LL | | baz().await @@ -14,13 +14,13 @@ LL | | } | |_^ error: this MutexGuard is held across an 'await' point. Consider using an async-aware Mutex type or ensuring the MutexGuard is dropped before calling await. - --> $DIR/await_holding_invalid.rs:28:9 + --> $DIR/await_holding_invalid.rs:29:9 | LL | let guard = x.lock().unwrap(); | ^^^^^ | note: these are all the await points this lock is held through - --> $DIR/await_holding_invalid.rs:28:5 + --> $DIR/await_holding_invalid.rs:29:5 | LL | / let guard = x.lock().unwrap(); LL | | @@ -32,13 +32,13 @@ LL | | } | |_^ error: this MutexGuard is held across an 'await' point. Consider using an async-aware Mutex type or ensuring the MutexGuard is dropped before calling await. - --> $DIR/await_holding_invalid.rs:41:13 + --> $DIR/await_holding_invalid.rs:42:13 | LL | let guard = x.lock().unwrap(); | ^^^^^ | note: these are all the await points this lock is held through - --> $DIR/await_holding_invalid.rs:41:9 + --> $DIR/await_holding_invalid.rs:42:9 | LL | / let guard = x.lock().unwrap(); LL | | baz().await @@ -46,18 +46,111 @@ LL | | }; | |_____^ error: this MutexGuard is held across an 'await' point. Consider using an async-aware Mutex type or ensuring the MutexGuard is dropped before calling await. - --> $DIR/await_holding_invalid.rs:53:13 + --> $DIR/await_holding_invalid.rs:54:13 | LL | let guard = x.lock().unwrap(); | ^^^^^ | note: these are all the await points this lock is held through - --> $DIR/await_holding_invalid.rs:53:9 + --> $DIR/await_holding_invalid.rs:54:9 | LL | / let guard = x.lock().unwrap(); LL | | baz().await LL | | } | |_____^ -error: aborting due to 4 previous errors +error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. + --> $DIR/await_holding_invalid.rs:60:9 + | +LL | let b = x.borrow(); + | ^ + | + = note: `-D clippy::await-holding-refcell-ref` implied by `-D warnings` +note: these are all the await points this ref is held through + --> $DIR/await_holding_invalid.rs:60:5 + | +LL | / let b = x.borrow(); +LL | | baz().await +LL | | } + | |_^ + +error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. + --> $DIR/await_holding_invalid.rs:65:9 + | +LL | let b = x.borrow_mut(); + | ^ + | +note: these are all the await points this ref is held through + --> $DIR/await_holding_invalid.rs:65:5 + | +LL | / let b = x.borrow_mut(); +LL | | baz().await +LL | | } + | |_^ + +error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. + --> $DIR/await_holding_invalid.rs:82:9 + | +LL | let b = x.borrow_mut(); + | ^ + | +note: these are all the await points this ref is held through + --> $DIR/await_holding_invalid.rs:82:5 + | +LL | / let b = x.borrow_mut(); +LL | | +LL | | let second = baz().await; +LL | | +... | +LL | | first + second + third +LL | | } + | |_^ + +error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. + --> $DIR/await_holding_invalid.rs:94:9 + | +LL | let b = x.borrow_mut(); + | ^ + | +note: these are all the await points this ref is held through + --> $DIR/await_holding_invalid.rs:94:5 + | +LL | / let b = x.borrow_mut(); +LL | | +LL | | let second = baz().await; +LL | | +... | +LL | | first + second + third +LL | | } + | |_^ + +error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. + --> $DIR/await_holding_invalid.rs:109:13 + | +LL | let b = x.borrow_mut(); + | ^ + | +note: these are all the await points this ref is held through + --> $DIR/await_holding_invalid.rs:109:9 + | +LL | / let b = x.borrow_mut(); +LL | | baz().await +LL | | }; + | |_____^ + +error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. + --> $DIR/await_holding_invalid.rs:121:13 + | +LL | let b = x.borrow_mut(); + | ^ + | +note: these are all the await points this ref is held through + --> $DIR/await_holding_invalid.rs:121:9 + | +LL | / let b = x.borrow_mut(); +LL | | baz().await +LL | | } + | |_____^ + +error: aborting due to 10 previous errors diff --git a/tests/ui/await_holding_refcell_ref.rs b/tests/ui/await_holding_refcell_ref.rs deleted file mode 100644 index 88841597bb60..000000000000 --- a/tests/ui/await_holding_refcell_ref.rs +++ /dev/null @@ -1,86 +0,0 @@ -// edition:2018 -#![warn(clippy::await_holding_refcell_ref)] - -use std::cell::RefCell; - -async fn bad(x: &RefCell) -> u32 { - let b = x.borrow(); - baz().await -} - -async fn bad_mut(x: &RefCell) -> u32 { - let b = x.borrow_mut(); - baz().await -} - -async fn good(x: &RefCell) -> u32 { - { - let b = x.borrow_mut(); - let y = *b + 1; - } - baz().await; - let b = x.borrow_mut(); - 47 -} - -async fn baz() -> u32 { - 42 -} - -async fn also_bad(x: &RefCell) -> u32 { - let first = baz().await; - - let b = x.borrow_mut(); - - let second = baz().await; - - let third = baz().await; - - first + second + third -} - -async fn less_bad(x: &RefCell) -> u32 { - let first = baz().await; - - let b = x.borrow_mut(); - - let second = baz().await; - - drop(b); - - let third = baz().await; - - first + second + third -} - -async fn not_good(x: &RefCell) -> u32 { - let first = baz().await; - - let second = { - let b = x.borrow_mut(); - baz().await - }; - - let third = baz().await; - - first + second + third -} - -#[allow(clippy::manual_async_fn)] -fn block_bad(x: &RefCell) -> impl std::future::Future + '_ { - async move { - let b = x.borrow_mut(); - baz().await - } -} - -fn main() { - let rc = RefCell::new(100); - good(&rc); - bad(&rc); - bad_mut(&rc); - also_bad(&rc); - less_bad(&rc); - not_good(&rc); - block_bad(&rc); -} diff --git a/tests/ui/await_holding_refcell_ref.stderr b/tests/ui/await_holding_refcell_ref.stderr deleted file mode 100644 index b504f0454913..000000000000 --- a/tests/ui/await_holding_refcell_ref.stderr +++ /dev/null @@ -1,95 +0,0 @@ -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. - --> $DIR/await_holding_refcell_ref.rs:7:9 - | -LL | let b = x.borrow(); - | ^ - | - = note: `-D clippy::await-holding-refcell-ref` implied by `-D warnings` -note: these are all the await points this ref is held through - --> $DIR/await_holding_refcell_ref.rs:7:5 - | -LL | / let b = x.borrow(); -LL | | baz().await -LL | | } - | |_^ - -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. - --> $DIR/await_holding_refcell_ref.rs:12:9 - | -LL | let b = x.borrow_mut(); - | ^ - | -note: these are all the await points this ref is held through - --> $DIR/await_holding_refcell_ref.rs:12:5 - | -LL | / let b = x.borrow_mut(); -LL | | baz().await -LL | | } - | |_^ - -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. - --> $DIR/await_holding_refcell_ref.rs:33:9 - | -LL | let b = x.borrow_mut(); - | ^ - | -note: these are all the await points this ref is held through - --> $DIR/await_holding_refcell_ref.rs:33:5 - | -LL | / let b = x.borrow_mut(); -LL | | -LL | | let second = baz().await; -LL | | -... | -LL | | first + second + third -LL | | } - | |_^ - -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. - --> $DIR/await_holding_refcell_ref.rs:45:9 - | -LL | let b = x.borrow_mut(); - | ^ - | -note: these are all the await points this ref is held through - --> $DIR/await_holding_refcell_ref.rs:45:5 - | -LL | / let b = x.borrow_mut(); -LL | | -LL | | let second = baz().await; -LL | | -... | -LL | | first + second + third -LL | | } - | |_^ - -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. - --> $DIR/await_holding_refcell_ref.rs:60:13 - | -LL | let b = x.borrow_mut(); - | ^ - | -note: these are all the await points this ref is held through - --> $DIR/await_holding_refcell_ref.rs:60:9 - | -LL | / let b = x.borrow_mut(); -LL | | baz().await -LL | | }; - | |_____^ - -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await. - --> $DIR/await_holding_refcell_ref.rs:72:13 - | -LL | let b = x.borrow_mut(); - | ^ - | -note: these are all the await points this ref is held through - --> $DIR/await_holding_refcell_ref.rs:72:9 - | -LL | / let b = x.borrow_mut(); -LL | | baz().await -LL | | } - | |_____^ - -error: aborting due to 6 previous errors -