Move refcell lint into shared module

Author: Daniel Smith

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<u32>) {
+    ///   let b = x.borrow_mut()();
+    ///   *ref += 1;
+    ///   bar.await;
+    /// }
+    /// ```
+    ///
+    /// Use instead:
+    /// ```rust,ignore
+    /// use std::cell::RefCell;
+    ///
+    /// async fn foo(x: &RefCell<u32>) {
+    ///   {
+    ///     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)
+}

clippy_lints/src/await_holding_refcell_ref.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),

clippy_lints/src/lib.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",

src/lintlist/mod.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>) -> u32 {
+async fn bad_lock(x: &Mutex<u32>) -> u32 {
     let guard = x.lock().unwrap();
     baz().await
 }
 
-async fn good(x: &Mutex<u32>) -> u32 {
+async fn good_lock(x: &Mutex<u32>) -> 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>) -> u32 {
+async fn also_bad_lock(x: &Mutex<u32>) -> u32 {
     let first = baz().await;
 
     let guard = x.lock().unwrap();
@@ -34,7 +35,7 @@ async fn also_bad(x: &Mutex<u32>) -> u32 {
     first + second + third
 }
 
-async fn not_good(x: &Mutex<u32>) -> u32 {
+async fn not_good_lock(x: &Mutex<u32>) -> u32 {
     let first = baz().await;
 
     let second = {
@@ -48,18 +49,97 @@ async fn not_good(x: &Mutex<u32>) -> u32 {
 }
 
 #[allow(clippy::manual_async_fn)]
-fn block_bad(x: &Mutex<u32>) -> impl std::future::Future<Output = u32> + '_ {
+fn block_bad_lock(x: &Mutex<u32>) -> impl std::future::Future<Output = u32> + '_ {
     async move {
         let guard = x.lock().unwrap();
         baz().await
     }
 }
 
+async fn bad_refcell(x: &RefCell<u32>) -> u32 {
+    let b = x.borrow();
+    baz().await
+}
+
+async fn bad_mut_refcell(x: &RefCell<u32>) -> u32 {
+    let b = x.borrow_mut();
+    baz().await
+}
+
+async fn good_refcell(x: &RefCell<u32>) -> 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>) -> 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>) -> 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>) -> 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<u32>) -> impl std::future::Future<Output = u32> + '_ {
+    async move {
+        let b = x.borrow_mut();
+        baz().await
+    }
+}
+
 fn main() {
-    let m = Mutex::new(100);
-    good(&m);
-    bad(&m);
-    also_bad(&m);
-    not_good(&m);
-    block_bad(&m);
+    {
+        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);
+    }
 }