Lint for holding locks across await points

Fixes rust-lang#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.

Author: rokob

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,100 @@
+use crate::utils::span_lint_and_note;
+use if_chain::if_chain;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{Body, FnDecl, HirId, IsAsync};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::{Span, Symbol};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for calls to await while holding a MutexGuard.
+    ///
+    /// **Why is this bad?** This is almost certainly an error which can result
+    /// in a deadlock because the reactor will invoke code not visible to the
+    /// currently visible scope.
+    ///
+    /// **Known problems:** Detects only specifically named guard types:
+    /// MutexGuard, RwLockReadGuard, and RwLockWriteGuard.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// use std::sync::Mutex;
+    ///
+    /// async fn foo(x: &Mutex<u32>) {
+    ///   let guard = x.lock().unwrap();
+    ///   *guard += 1;
+    ///   bar.await;
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// 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"
+}
+
+const MUTEX_GUARD_TYPES: [&str; 3] = ["MutexGuard", "RwLockReadGuard", "RwLockWriteGuard"];
+
+declare_lint_pass!(AwaitHoldingLock => [AWAIT_HOLDING_LOCK]);
+
+impl LateLintPass<'_, '_> for AwaitHoldingLock {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'_, '_>,
+        fn_kind: FnKind<'_>,
+        _: &FnDecl<'_>,
+        _: &Body<'_>,
+        span: Span,
+        _: HirId,
+    ) {
+        if !is_async_fn(fn_kind) {
+            return;
+        }
+
+        for ty_clause in &cx.tables.generator_interior_types {
+            if_chain! {
+              if let rustc_middle::ty::Adt(adt, _) = ty_clause.ty.kind;
+              if let Some(&sym) = cx.get_def_path(adt.did).iter().last();
+              if is_symbol_mutex_guard(sym);
+              then {
+                span_lint_and_note(
+                      cx,
+                      AWAIT_HOLDING_LOCK,
+                      ty_clause.span,
+                      "this MutexGuard is held across an 'await' point",
+                      ty_clause.scope_span.unwrap_or(span),
+                      "these are all the await points this lock is held through"
+                    );
+              }
+            }
+        }
+    }
+}
+
+fn is_async_fn(fn_kind: FnKind<'_>) -> bool {
+    fn_kind.header().map_or(false, |h| match h.asyncness {
+        IsAsync::Async => true,
+        IsAsync::NotAsync => false,
+    })
+}
+
+fn is_symbol_mutex_guard(sym: Symbol) -> bool {
+    let sym_str = sym.as_str();
+    for ty in &MUTEX_GUARD_TYPES {
+        if sym_str == *ty {
+            return true;
+        }
+    }
+    false
+}

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;
@@ -494,6 +495,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,
@@ -856,6 +858,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());
@@ -1090,6 +1093,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),

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,42 @@
+// 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
+}
+
+fn main() {
+    let m = Mutex::new(100);
+    good(&m);
+    bad(&m);
+    also_bad(&m);
+}

tests/ui/await_holding_lock.stderr

@@ -0,0 +1,35 @@
+error: this MutexGuard is held across an 'await' point
+  --> $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
+  --> $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: aborting due to 2 previous errors
+