Auto merge of rust-lang#6029 - Daniel-B-Smith:refcell_ref_await, r=flip1995

Add lint for holding RefCell Ref across an await

Fixes rust-lang#6008

This introduces the lint await_holding_refcell_ref. For async functions, we iterate
over all types in generator_interior_types and look for `core::cell::Ref` or `core::cell::RefMut`. If we find one then we emit a lint.

Heavily cribs from: rust-lang/rust-clippy#5439

changelog: introduce the await_holding_refcell_ref lint

Author: bors

CHANGELOG.md

@@ -1632,6 +1632,7 @@ Released 2018-09-13
 [`assign_ops`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_ops
 [`async_yields_async`]: https://rust-lang.github.io/rust-clippy/master/index.html#async_yields_async
 [`await_holding_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock
+[`await_holding_refcell_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_refcell_ref
 [`bad_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#bad_bit_mask
 [`bind_instead_of_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#bind_instead_of_map
 [`blacklisted_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name

clippy_lints/src/await_holding_lock.rs renamed to clippy_lints/src/await_holding_invalid.rs

@@ -45,13 +45,52 @@ declare_clippy_lint! {
     /// }
     /// ```
     pub AWAIT_HOLDING_LOCK,
-    pedantic,
+    correctness,
     "Inside an async function, holding a MutexGuard while calling await"
 }
 
-declare_lint_pass!(AwaitHoldingLock => [AWAIT_HOLDING_LOCK]);
+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,
+    correctness,
+    "Inside an async function, holding a RefCell ref while calling await"
+}
 
-impl LateLintPass<'_> for AwaitHoldingLock {
+declare_lint_pass!(AwaitHolding => [AWAIT_HOLDING_LOCK, AWAIT_HOLDING_REFCELL_REF]);
+
+impl LateLintPass<'_> for AwaitHolding {
     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 {
@@ -78,6 +117,16 @@ fn check_interior_types(cx: &LateContext<'_>, ty_causes: &[GeneratorInteriorType
                     "these are all the await points this lock is held through",
                 );
             }
+            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",
+                    );
+            }
         }
     }
 }
@@ -90,3 +139,7 @@ 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)
 }
+
+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/lib.rs

@@ -160,7 +160,7 @@ mod assign_ops;
 mod async_yields_async;
 mod atomic_ordering;
 mod attrs;
-mod await_holding_lock;
+mod await_holding_invalid;
 mod bit_mask;
 mod blacklisted_name;
 mod blocks_in_if_conditions;
@@ -509,7 +509,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &attrs::MISMATCHED_TARGET_OS,
         &attrs::UNKNOWN_CLIPPY_LINTS,
         &attrs::USELESS_ATTRIBUTE,
-        &await_holding_lock::AWAIT_HOLDING_LOCK,
+        &await_holding_invalid::AWAIT_HOLDING_LOCK,
+        &await_holding_invalid::AWAIT_HOLDING_REFCELL_REF,
         &bit_mask::BAD_BIT_MASK,
         &bit_mask::INEFFECTIVE_BIT_MASK,
         &bit_mask::VERBOSE_BIT_MASK,
@@ -906,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_lock::AwaitHoldingLock);
+    store.register_late_pass(|| box await_holding_invalid::AwaitHolding);
     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());
@@ -1190,7 +1191,6 @@ 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_lock::AWAIT_HOLDING_LOCK),
         LintId::of(&bit_mask::VERBOSE_BIT_MASK),
         LintId::of(&checked_conversions::CHECKED_CONVERSIONS),
         LintId::of(&copies::MATCH_SAME_ARMS),
@@ -1290,6 +1290,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&attrs::MISMATCHED_TARGET_OS),
         LintId::of(&attrs::UNKNOWN_CLIPPY_LINTS),
         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),
@@ -1736,6 +1738,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         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(&booleans::LOGIC_BUG),

clippy_lints/src/utils/paths.rs

@@ -93,6 +93,8 @@ pub const RANGE_ARGUMENT_TRAIT: [&str; 3] = ["core", "ops", "RangeBounds"];
 pub const RC: [&str; 3] = ["alloc", "rc", "Rc"];
 pub const RC_PTR_EQ: [&str; 4] = ["alloc", "rc", "Rc", "ptr_eq"];
 pub const RECEIVER: [&str; 4] = ["std", "sync", "mpsc", "Receiver"];
+pub const REFCELL_REF: [&str; 3] = ["core", "cell", "Ref"];
+pub const REFCELL_REFMUT: [&str; 3] = ["core", "cell", "RefMut"];
 pub const REGEX_BUILDER_NEW: [&str; 5] = ["regex", "re_builder", "unicode", "RegexBuilder", "new"];
 pub const REGEX_BYTES_BUILDER_NEW: [&str; 5] = ["regex", "re_builder", "bytes", "RegexBuilder", "new"];
 pub const REGEX_BYTES_NEW: [&str; 4] = ["regex", "re_bytes", "Regex", "new"];

src/lintlist/mod.rs

@@ -62,10 +62,17 @@ vec![
     },
     Lint {
         name: "await_holding_lock",
-        group: "pedantic",
+        group: "correctness",
         desc: "Inside an async function, holding a MutexGuard while calling await",
         deprecation: None,
-        module: "await_holding_lock",
+        module: "await_holding_invalid",
+    },
+    Lint {
+        name: "await_holding_refcell_ref",
+        group: "correctness",
+        desc: "Inside an async function, holding a RefCell ref while calling await",
+        deprecation: None,
+        module: "await_holding_invalid",
     },
     Lint {
         name: "bad_bit_mask",

tests/ui/await_holding_refcell_ref.rs

@@ -0,0 +1,86 @@
+// edition:2018
+#![warn(clippy::await_holding_refcell_ref)]
+
+use std::cell::RefCell;
+
+async fn bad(x: &RefCell<u32>) -> u32 {
+    let b = x.borrow();
+    baz().await
+}
+
+async fn bad_mut(x: &RefCell<u32>) -> u32 {
+    let b = x.borrow_mut();
+    baz().await
+}
+
+async fn good(x: &RefCell<u32>) -> 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>) -> 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>) -> 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>) -> 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<u32>) -> impl std::future::Future<Output = u32> + '_ {
+    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);
+}

tests/ui/await_holding_refcell_ref.stderr

@@ -0,0 +1,95 @@
+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
+