Auto merge of #5439 - rokob:lock-await, r=Manishearth

Lint for holding locks across await points

Fixes #4226

This introduces the lint await_holding_lock. For async functions, we iterate
over all types in generator_interior_types and look for types named MutexGuard,
RwLockReadGuard, or RwLockWriteGuard. If we find one then we emit a lint.

changelog: introduce the await_holding_lock lint

CHANGELOG.md

@@ -1188,6 +1188,7 @@ Released 2018-09-13
 [`assertions_on_constants`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants
 [`assign_op_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
 [`assign_ops`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_ops
+[`await_holding_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock
 [`bad_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#bad_bit_mask
 [`blacklisted_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name
 [`block_in_if_condition_expr`]: https://rust-lang.github.io/rust-clippy/master/index.html#block_in_if_condition_expr

clippy_lints/src/await_holding_lock.rs

@@ -0,0 +1,97 @@
+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
+    /// non-async-aware MutexGuard.
+    ///
+    /// **Why is this bad?** The Mutex types found in syd::sync and parking_lot
+    /// are not designed to operator in an async context across await points.
+    ///
+    /// There are two potential solutions. One is to use an asynx-aware Mutex
+    /// type. Many asynchronous foundation crates provide such a Mutex type. The
+    /// other solution is to ensure the mutex is unlocked before calling await,
+    /// either by introducing a scope or an explicit call to Drop::drop.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust,ignore
+    /// use std::sync::Mutex;
+    ///
+    /// async fn foo(x: &Mutex<u32>) {
+    ///   let guard = x.lock().unwrap();
+    ///   *guard += 1;
+    ///   bar.await;
+    /// }
+    /// ```
+    ///
+    /// Use instead:
+    /// ```rust,ignore
+    /// use std::sync::Mutex;
+    ///
+    /// async fn foo(x: &Mutex<u32>) {
+    ///   {
+    ///     let guard = x.lock().unwrap();
+    ///     *guard += 1;
+    ///   }
+    ///   bar.await;
+    /// }
+    /// ```
+    pub AWAIT_HOLDING_LOCK,
+    pedantic,
+    "Inside an async function, holding a MutexGuard while calling await"
+}
+
+declare_lint_pass!(AwaitHoldingLock => [AWAIT_HOLDING_LOCK]);
+
+impl LateLintPass<'_, '_> for AwaitHoldingLock {
+    fn check_body(&mut self, cx: &LateContext<'_, '_>, body: &'_ Body<'_>) {
+        use AsyncGeneratorKind::{Block, Closure, Fn};
+        match body.generator_kind {
+            Some(GeneratorKind::Async(Block))
+            | Some(GeneratorKind::Async(Closure))
+            | Some(GeneratorKind::Async(Fn)) => {
+                let body_id = BodyId {
+                    hir_id: body.value.hir_id,
+                };
+                let def_id = cx.tcx.hir().body_owner_def_id(body_id);
+                let tables = cx.tcx.typeck_tables_of(def_id);
+                check_interior_types(cx, &tables.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_mutex_guard(cx, adt.did) {
+                span_lint_and_note(
+                    cx,
+                    AWAIT_HOLDING_LOCK,
+                    ty_cause.span,
+                    "this MutexGuard is held across an 'await' point. Consider using an async-aware Mutex type or ensuring the MutexGuard is dropped before calling await.",
+                    ty_cause.scope_span.or(Some(span)),
+                    "these are all the await points this lock is held through",
+                );
+            }
+        }
+    }
+}
+
+fn is_mutex_guard(cx: &LateContext<'_, '_>, def_id: DefId) -> bool {
+    match_def_path(cx, def_id, &paths::MUTEX_GUARD)
+        || match_def_path(cx, def_id, &paths::RWLOCK_READ_GUARD)
+        || match_def_path(cx, def_id, &paths::RWLOCK_WRITE_GUARD)
+        || match_def_path(cx, def_id, &paths::PARKING_LOT_MUTEX_GUARD)
+        || match_def_path(cx, def_id, &paths::PARKING_LOT_RWLOCK_READ_GUARD)
+        || match_def_path(cx, def_id, &paths::PARKING_LOT_RWLOCK_WRITE_GUARD)
+}

clippy_lints/src/lib.rs

@@ -177,6 +177,7 @@ mod assertions_on_constants;
 mod assign_ops;
 mod atomic_ordering;
 mod attrs;
+mod await_holding_lock;
 mod bit_mask;
 mod blacklisted_name;
 mod block_in_if_condition;
@@ -497,6 +498,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &attrs::INLINE_ALWAYS,
         &attrs::UNKNOWN_CLIPPY_LINTS,
         &attrs::USELESS_ATTRIBUTE,
+        &await_holding_lock::AWAIT_HOLDING_LOCK,
         &bit_mask::BAD_BIT_MASK,
         &bit_mask::INEFFECTIVE_BIT_MASK,
         &bit_mask::VERBOSE_BIT_MASK,
@@ -864,6 +866,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 serde_api::SerdeAPI);
     store.register_late_pass(|| box utils::internal_lints::CompilerLintFunctions::new());
     store.register_late_pass(|| box utils::internal_lints::LintWithoutLintPass::default());
@@ -1102,6 +1105,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_lock::AWAIT_HOLDING_LOCK),
         LintId::of(&checked_conversions::CHECKED_CONVERSIONS),
         LintId::of(&copies::MATCH_SAME_ARMS),
         LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION),

clippy_lints/src/utils/paths.rs

@@ -72,6 +72,9 @@ pub const ORD: [&str; 3] = ["core", "cmp", "Ord"];
 pub const OS_STRING: [&str; 4] = ["std", "ffi", "os_str", "OsString"];
 pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString", "as_os_str"];
 pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"];
+pub const PARKING_LOT_MUTEX_GUARD: [&str; 2] = ["parking_lot", "MutexGuard"];
+pub const PARKING_LOT_RWLOCK_READ_GUARD: [&str; 2] = ["parking_lot", "RwLockReadGuard"];
+pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 2] = ["parking_lot", "RwLockWriteGuard"];
 pub const PATH: [&str; 3] = ["std", "path", "Path"];
 pub const PATH_BUF: [&str; 3] = ["std", "path", "PathBuf"];
 pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"];

src/lintlist/mod.rs

@@ -52,6 +52,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "assign_ops",
     },
+    Lint {
+        name: "await_holding_lock",
+        group: "pedantic",
+        desc: "Inside an async function, holding a MutexGuard while calling await",
+        deprecation: None,
+        module: "await_holding_lock",
+    },
     Lint {
         name: "bad_bit_mask",
         group: "correctness",

tests/ui/await_holding_lock.rs

@@ -0,0 +1,64 @@
+// edition:2018
+#![warn(clippy::await_holding_lock)]
+
+use std::sync::Mutex;
+
+async fn bad(x: &Mutex<u32>) -> u32 {
+    let guard = x.lock().unwrap();
+    baz().await
+}
+
+async fn good(x: &Mutex<u32>) -> u32 {
+    {
+        let guard = x.lock().unwrap();
+        let y = *guard + 1;
+    }
+    baz().await;
+    let guard = x.lock().unwrap();
+    47
+}
+
+async fn baz() -> u32 {
+    42
+}
+
+async fn also_bad(x: &Mutex<u32>) -> u32 {
+    let first = baz().await;
+
+    let guard = x.lock().unwrap();
+
+    let second = baz().await;
+
+    let third = baz().await;
+
+    first + second + third
+}
+
+async fn not_good(x: &Mutex<u32>) -> u32 {
+    let first = baz().await;
+
+    let second = {
+        let guard = x.lock().unwrap();
+        baz().await
+    };
+
+    let third = baz().await;
+
+    first + second + third
+}
+
+fn block_bad(x: &Mutex<u32>) -> impl std::future::Future<Output = u32> + '_ {
+    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);
+}

tests/ui/await_holding_lock.stderr

@@ -0,0 +1,63 @@
+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_lock.rs:7: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_lock.rs:7:5
+   |
+LL | /     let guard = x.lock().unwrap();
+LL | |     baz().await
+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_lock.rs:28:9
+   |
+LL |     let guard = x.lock().unwrap();
+   |         ^^^^^
+   |
+note: these are all the await points this lock is held through
+  --> $DIR/await_holding_lock.rs:28:5
+   |
+LL | /     let guard = x.lock().unwrap();
+LL | |
+LL | |     let second = baz().await;
+LL | |
+...  |
+LL | |     first + second + third
+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_lock.rs:41:13
+   |
+LL |         let guard = x.lock().unwrap();
+   |             ^^^^^
+   |
+note: these are all the await points this lock is held through
+  --> $DIR/await_holding_lock.rs:41:9
+   |
+LL | /         let guard = x.lock().unwrap();
+LL | |         baz().await
+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_lock.rs:52:13
+   |
+LL |         let guard = x.lock().unwrap();
+   |             ^^^^^
+   |
+note: these are all the await points this lock is held through
+  --> $DIR/await_holding_lock.rs:52:9
+   |
+LL | /         let guard = x.lock().unwrap();
+LL | |         baz().await
+LL | |     }
+   | |_____^
+
+error: aborting due to 4 previous errors
+