From d6e55e97ff7060a2d752ff2413211adc7f8f4469 Mon Sep 17 00:00:00 2001 From: Andy Weiss Date: Thu, 16 Apr 2020 23:21:49 -0700 Subject: [PATCH] Make lint also capture blocks and closures, adjust language to mention other mutex types --- clippy_lints/src/await_holding_lock.rs | 81 +++++++++++++------------- tests/ui/await_holding_lock.rs | 22 +++++++ tests/ui/await_holding_lock.stderr | 34 ++++++++++- 3 files changed, 95 insertions(+), 42 deletions(-) diff --git a/clippy_lints/src/await_holding_lock.rs b/clippy_lints/src/await_holding_lock.rs index 1b2b020bd55..b2aa3437923 100644 --- a/clippy_lints/src/await_holding_lock.rs +++ b/clippy_lints/src/await_holding_lock.rs @@ -1,20 +1,24 @@ use crate::utils::{match_def_path, paths, span_lint_and_note}; use rustc_hir::def_id::DefId; -use rustc_hir::intravisit::FnKind; -use rustc_hir::{Body, FnDecl, HirId, IsAsync}; +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 MutexGuard. + /// **What it does:** Checks for calls to await while holding a + /// non-async-aware 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. + /// **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. /// - /// **Known problems:** Detects only specifically named guard types: - /// MutexGuard, RwLockReadGuard, and RwLockWriteGuard. + /// 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:** /// @@ -27,6 +31,7 @@ declare_clippy_lint! { /// bar.await; /// } /// ``` + /// /// Use instead: /// ```rust,ignore /// use std::sync::Mutex; @@ -47,41 +52,39 @@ declare_clippy_lint! { 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_cause in &cx.tables.generator_interior_types { - 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", - ty_cause.scope_span.unwrap_or(span), - "these are all the await points this lock is held through", - ); - } - } + 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 is_async_fn(fn_kind: FnKind<'_>) -> bool { - fn_kind.header().map_or(false, |h| match h.asyncness { - IsAsync::Async => true, - IsAsync::NotAsync => false, - }) +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.unwrap_or(span), + "these are all the await points this lock is held through", + ); + } + } + } } fn is_mutex_guard(cx: &LateContext<'_, '_>, def_id: DefId) -> bool { diff --git a/tests/ui/await_holding_lock.rs b/tests/ui/await_holding_lock.rs index fab31f37ffc..5c1fdd83efb 100644 --- a/tests/ui/await_holding_lock.rs +++ b/tests/ui/await_holding_lock.rs @@ -34,9 +34,31 @@ async fn also_bad(x: &Mutex) -> u32 { first + second + third } +async fn not_good(x: &Mutex) -> 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) -> 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); } diff --git a/tests/ui/await_holding_lock.stderr b/tests/ui/await_holding_lock.stderr index 8d4fd0c20a9..8c47cb37d8c 100644 --- a/tests/ui/await_holding_lock.stderr +++ b/tests/ui/await_holding_lock.stderr @@ -1,4 +1,4 @@ -error: this MutexGuard is held across an 'await' point +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(); @@ -13,7 +13,7 @@ LL | | baz().await LL | | } | |_^ -error: this MutexGuard is held across an 'await' point +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(); @@ -31,5 +31,33 @@ LL | | first + second + third LL | | } | |_^ -error: aborting due to 2 previous errors +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