From cdf9a28006b1216653eac1bdc45a83e436fac9ba Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sat, 12 Feb 2022 10:23:07 +0100 Subject: [PATCH 1/4] Improve lint message of await_holding_* Improves the message of the lints await_holding_lock and await_holding_refcell_ref. Now also actually tests RwLock. --- clippy_lints/src/await_holding_invalid.rs | 31 +++++++---- tests/ui/await_holding_lock.rs | 26 ++++++++- tests/ui/await_holding_lock.stderr | 64 +++++++++++++++++------ tests/ui/await_holding_refcell_ref.stderr | 30 ++++++----- 4 files changed, 114 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/await_holding_invalid.rs b/clippy_lints/src/await_holding_invalid.rs index 1cc3418d474..f7bc8395c82 100644 --- a/clippy_lints/src/await_holding_invalid.rs +++ b/clippy_lints/src/await_holding_invalid.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::{match_def_path, paths}; use rustc_hir::def_id::DefId; use rustc_hir::{AsyncGeneratorKind, Body, BodyId, GeneratorKind}; @@ -118,23 +118,36 @@ fn check_interior_types(cx: &LateContext<'_>, ty_causes: &[GeneratorInteriorType 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( + span_lint_and_then( 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", + "this `MutexGuard` is held across an `await` point", + |diag| { + diag.help( + "consider using an async-aware `Mutex` type or ensuring the \ + `MutexGuard` is dropped before calling await", + ); + diag.span_note( + ty_cause.scope_span.unwrap_or(span), + "these are all the `await` points this lock is held through", + ); + }, ); } if is_refcell_ref(cx, adt.did) { - span_lint_and_note( + span_lint_and_then( 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", + "this `RefCell` reference is held across an `await` point", + |diag| { + diag.help("ensure the reference is dropped before calling `await`"); + diag.span_note( + ty_cause.scope_span.unwrap_or(span), + "these are all the `await` points this reference is held through", + ); + }, ); } } diff --git a/tests/ui/await_holding_lock.rs b/tests/ui/await_holding_lock.rs index dd6640a387a..f03356d83df 100644 --- a/tests/ui/await_holding_lock.rs +++ b/tests/ui/await_holding_lock.rs @@ -1,6 +1,6 @@ #![warn(clippy::await_holding_lock)] -use std::sync::Mutex; +use std::sync::{Mutex, RwLock}; async fn bad(x: &Mutex) -> u32 { let guard = x.lock().unwrap(); @@ -17,6 +17,30 @@ async fn good(x: &Mutex) -> u32 { 47 } +pub async fn bad_rw(x: &RwLock) -> u32 { + let guard = x.read().unwrap(); + baz().await +} + +pub async fn bad_rw_write(x: &RwLock) -> u32 { + let mut guard = x.write().unwrap(); + baz().await +} + +pub async fn good_rw(x: &RwLock) -> u32 { + { + let guard = x.read().unwrap(); + let y = *guard + 1; + } + { + let mut guard = x.write().unwrap(); + *guard += 1; + } + baz().await; + let guard = x.read().unwrap(); + 47 +} + async fn baz() -> u32 { 42 } diff --git a/tests/ui/await_holding_lock.stderr b/tests/ui/await_holding_lock.stderr index ddfb104cdfb..3a501acbb67 100644 --- a/tests/ui/await_holding_lock.stderr +++ b/tests/ui/await_holding_lock.stderr @@ -1,11 +1,12 @@ -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 +error: this `MutexGuard` is held across an `await` point --> $DIR/await_holding_lock.rs:6: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 + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through --> $DIR/await_holding_lock.rs:6:5 | LL | / let guard = x.lock().unwrap(); @@ -13,14 +14,45 @@ 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:27:9 +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:21:9 + | +LL | let guard = x.read().unwrap(); + | ^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:21:5 + | +LL | / let guard = x.read().unwrap(); +LL | | baz().await +LL | | } + | |_^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:26:9 + | +LL | let mut guard = x.write().unwrap(); + | ^^^^^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:26:5 + | +LL | / let mut guard = x.write().unwrap(); +LL | | baz().await +LL | | } + | |_^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:51:9 | LL | let guard = x.lock().unwrap(); | ^^^^^ | -note: these are all the await points this lock is held through - --> $DIR/await_holding_lock.rs:27:5 + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:51:5 | LL | / let guard = x.lock().unwrap(); LL | | @@ -31,33 +63,35 @@ 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:40:13 +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:64:13 | LL | let guard = x.lock().unwrap(); | ^^^^^ | -note: these are all the await points this lock is held through - --> $DIR/await_holding_lock.rs:40:9 + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:64: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 +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:76: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 + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:76:9 | LL | / let guard = x.lock().unwrap(); LL | | baz().await LL | | } | |_____^ -error: aborting due to 4 previous errors +error: aborting due to 6 previous errors diff --git a/tests/ui/await_holding_refcell_ref.stderr b/tests/ui/await_holding_refcell_ref.stderr index 67cc0032be2..4339fca735d 100644 --- a/tests/ui/await_holding_refcell_ref.stderr +++ b/tests/ui/await_holding_refcell_ref.stderr @@ -1,11 +1,12 @@ -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await +error: this `RefCell` reference is held across an `await` point --> $DIR/await_holding_refcell_ref.rs:6: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 + = help: ensure the reference is dropped before calling `await` +note: these are all the `await` points this reference is held through --> $DIR/await_holding_refcell_ref.rs:6:5 | LL | / let b = x.borrow(); @@ -13,13 +14,14 @@ LL | | baz().await LL | | } | |_^ -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await +error: this `RefCell` reference is held across an `await` point --> $DIR/await_holding_refcell_ref.rs:11:9 | LL | let b = x.borrow_mut(); | ^ | -note: these are all the await points this ref is held through + = help: ensure the reference is dropped before calling `await` +note: these are all the `await` points this reference is held through --> $DIR/await_holding_refcell_ref.rs:11:5 | LL | / let b = x.borrow_mut(); @@ -27,13 +29,14 @@ LL | | baz().await LL | | } | |_^ -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await +error: this `RefCell` reference is held across an `await` point --> $DIR/await_holding_refcell_ref.rs:32:9 | LL | let b = x.borrow_mut(); | ^ | -note: these are all the await points this ref is held through + = help: ensure the reference is dropped before calling `await` +note: these are all the `await` points this reference is held through --> $DIR/await_holding_refcell_ref.rs:32:5 | LL | / let b = x.borrow_mut(); @@ -45,13 +48,14 @@ LL | | first + second + third LL | | } | |_^ -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await +error: this `RefCell` reference is held across an `await` point --> $DIR/await_holding_refcell_ref.rs:44:9 | LL | let b = x.borrow_mut(); | ^ | -note: these are all the await points this ref is held through + = help: ensure the reference is dropped before calling `await` +note: these are all the `await` points this reference is held through --> $DIR/await_holding_refcell_ref.rs:44:5 | LL | / let b = x.borrow_mut(); @@ -63,13 +67,14 @@ LL | | first + second + third LL | | } | |_^ -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await +error: this `RefCell` reference is held across an `await` point --> $DIR/await_holding_refcell_ref.rs:59:13 | LL | let b = x.borrow_mut(); | ^ | -note: these are all the await points this ref is held through + = help: ensure the reference is dropped before calling `await` +note: these are all the `await` points this reference is held through --> $DIR/await_holding_refcell_ref.rs:59:9 | LL | / let b = x.borrow_mut(); @@ -77,13 +82,14 @@ LL | | baz().await LL | | }; | |_____^ -error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await +error: this `RefCell` reference is held across an `await` point --> $DIR/await_holding_refcell_ref.rs:71:13 | LL | let b = x.borrow_mut(); | ^ | -note: these are all the await points this ref is held through + = help: ensure the reference is dropped before calling `await` +note: these are all the `await` points this reference is held through --> $DIR/await_holding_refcell_ref.rs:71:9 | LL | / let b = x.borrow_mut(); From c4944fb60d102a4a4e0cb5f3f86379bea2338ed0 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sat, 12 Feb 2022 10:32:44 +0100 Subject: [PATCH 2/4] Actually lint parking_lot in await_holding_lock This adapts the paths for the parking_lot mutex guards, so that parking_lot mutexes and RwLocks actually get linted. This is now also tested. --- clippy_utils/src/paths.rs | 6 +- tests/ui/await_holding_lock.rs | 214 ++++++++++++++++------- tests/ui/await_holding_lock.stderr | 262 ++++++++++++++++++++--------- 3 files changed, 333 insertions(+), 149 deletions(-) diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 48c0e89e4d2..b54bd3a4fef 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -105,9 +105,9 @@ pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString", pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"]; pub const PARKING_LOT_RAWMUTEX: [&str; 3] = ["parking_lot", "raw_mutex", "RawMutex"]; pub const PARKING_LOT_RAWRWLOCK: [&str; 3] = ["parking_lot", "raw_rwlock", "RawRwLock"]; -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 PARKING_LOT_MUTEX_GUARD: [&str; 3] = ["lock_api", "mutex", "MutexGuard"]; +pub const PARKING_LOT_RWLOCK_READ_GUARD: [&str; 3] = ["lock_api", "rwlock", "RwLockReadGuard"]; +pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 3] = ["lock_api", "rwlock", "RwLockWriteGuard"]; pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"]; pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"]; pub const PERMISSIONS: [&str; 3] = ["std", "fs", "Permissions"]; diff --git a/tests/ui/await_holding_lock.rs b/tests/ui/await_holding_lock.rs index f03356d83df..1a57db91ab5 100644 --- a/tests/ui/await_holding_lock.rs +++ b/tests/ui/await_holding_lock.rs @@ -1,88 +1,178 @@ #![warn(clippy::await_holding_lock)] -use std::sync::{Mutex, RwLock}; +// When adding or modifying a test, please do the same for parking_lot::Mutex. +mod std_mutex { + use std::sync::{Mutex, RwLock}; -async fn bad(x: &Mutex) -> u32 { - let guard = x.lock().unwrap(); - baz().await -} - -async fn good(x: &Mutex) -> u32 { - { + pub async fn bad(x: &Mutex) -> u32 { let guard = x.lock().unwrap(); - let y = *guard + 1; + baz().await } - baz().await; - let guard = x.lock().unwrap(); - 47 -} -pub async fn bad_rw(x: &RwLock) -> u32 { - let guard = x.read().unwrap(); - baz().await -} + pub async fn good(x: &Mutex) -> u32 { + { + let guard = x.lock().unwrap(); + let y = *guard + 1; + } + baz().await; + let guard = x.lock().unwrap(); + 47 + } -pub async fn bad_rw_write(x: &RwLock) -> u32 { - let mut guard = x.write().unwrap(); - baz().await -} - -pub async fn good_rw(x: &RwLock) -> u32 { - { + pub async fn bad_rw(x: &RwLock) -> u32 { let guard = x.read().unwrap(); - let y = *guard + 1; + baz().await } - { + + pub async fn bad_rw_write(x: &RwLock) -> u32 { let mut guard = x.write().unwrap(); - *guard += 1; + baz().await } - baz().await; - let guard = x.read().unwrap(); - 47 -} -async fn baz() -> u32 { - 42 -} + pub async fn good_rw(x: &RwLock) -> u32 { + { + let guard = x.read().unwrap(); + let y = *guard + 1; + } + { + let mut guard = x.write().unwrap(); + *guard += 1; + } + baz().await; + let guard = x.read().unwrap(); + 47 + } -async fn also_bad(x: &Mutex) -> u32 { - let first = baz().await; + pub async fn baz() -> u32 { + 42 + } - let guard = x.lock().unwrap(); + pub async fn also_bad(x: &Mutex) -> u32 { + let first = baz().await; - let second = baz().await; - - let third = baz().await; - - 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; + let second = baz().await; - first + second + third + let third = baz().await; + + first + second + third + } + + pub 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 + } + + #[allow(clippy::manual_async_fn)] + pub fn block_bad(x: &Mutex) -> impl std::future::Future + '_ { + async move { + let guard = x.lock().unwrap(); + baz().await + } + } } -#[allow(clippy::manual_async_fn)] -fn block_bad(x: &Mutex) -> impl std::future::Future + '_ { - async move { - let guard = x.lock().unwrap(); +// When adding or modifying a test, please do the same for std::Mutex. +mod parking_lot_mutex { + use parking_lot::{Mutex, RwLock}; + + pub async fn bad(x: &Mutex) -> u32 { + let guard = x.lock(); baz().await } + + pub async fn good(x: &Mutex) -> u32 { + { + let guard = x.lock(); + let y = *guard + 1; + } + baz().await; + let guard = x.lock(); + 47 + } + + pub async fn bad_rw(x: &RwLock) -> u32 { + let guard = x.read(); + baz().await + } + + pub async fn bad_rw_write(x: &RwLock) -> u32 { + let mut guard = x.write(); + baz().await + } + + pub async fn good_rw(x: &RwLock) -> u32 { + { + let guard = x.read(); + let y = *guard + 1; + } + { + let mut guard = x.write(); + *guard += 1; + } + baz().await; + let guard = x.read(); + 47 + } + + pub async fn baz() -> u32 { + 42 + } + + pub async fn also_bad(x: &Mutex) -> u32 { + let first = baz().await; + + let guard = x.lock(); + + let second = baz().await; + + let third = baz().await; + + first + second + third + } + + pub async fn not_good(x: &Mutex) -> u32 { + let first = baz().await; + + let second = { + let guard = x.lock(); + baz().await + }; + + let third = baz().await; + + first + second + third + } + + #[allow(clippy::manual_async_fn)] + pub fn block_bad(x: &Mutex) -> impl std::future::Future + '_ { + async move { + let guard = x.lock(); + baz().await + } + } } fn main() { - let m = Mutex::new(100); - good(&m); - bad(&m); - also_bad(&m); - not_good(&m); - block_bad(&m); + let m = std::sync::Mutex::new(100); + std_mutex::good(&m); + std_mutex::bad(&m); + std_mutex::also_bad(&m); + std_mutex::not_good(&m); + std_mutex::block_bad(&m); + + let m = parking_lot::Mutex::new(100); + parking_lot_mutex::good(&m); + parking_lot_mutex::bad(&m); + parking_lot_mutex::also_bad(&m); + parking_lot_mutex::not_good(&m); } diff --git a/tests/ui/await_holding_lock.stderr b/tests/ui/await_holding_lock.stderr index 3a501acbb67..a6c1dd228e4 100644 --- a/tests/ui/await_holding_lock.stderr +++ b/tests/ui/await_holding_lock.stderr @@ -1,97 +1,191 @@ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:6:9 + --> $DIR/await_holding_lock.rs:8:13 | -LL | let guard = x.lock().unwrap(); - | ^^^^^ +LL | let guard = x.lock().unwrap(); + | ^^^^^ | = note: `-D clippy::await-holding-lock` implied by `-D warnings` = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:6: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:21:9 - | -LL | let guard = x.read().unwrap(); - | ^^^^^ - | - = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await -note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:21:5 - | -LL | / let guard = x.read().unwrap(); -LL | | baz().await -LL | | } - | |_^ - -error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:26:9 - | -LL | let mut guard = x.write().unwrap(); - | ^^^^^^^^^ - | - = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await -note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:26:5 - | -LL | / let mut guard = x.write().unwrap(); -LL | | baz().await -LL | | } - | |_^ - -error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:51:9 - | -LL | let guard = x.lock().unwrap(); - | ^^^^^ - | - = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await -note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:51: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 - --> $DIR/await_holding_lock.rs:64:13 - | -LL | let guard = x.lock().unwrap(); - | ^^^^^ - | - = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await -note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:64:9 - | -LL | / let guard = x.lock().unwrap(); -LL | | baz().await -LL | | }; - | |_____^ - -error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:76:13 - | -LL | let guard = x.lock().unwrap(); - | ^^^^^ - | - = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await -note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:76:9 + --> $DIR/await_holding_lock.rs:8:9 | LL | / let guard = x.lock().unwrap(); LL | | baz().await LL | | } | |_____^ -error: aborting due to 6 previous errors +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:23:13 + | +LL | let guard = x.read().unwrap(); + | ^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:23:9 + | +LL | / let guard = x.read().unwrap(); +LL | | baz().await +LL | | } + | |_____^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:28:13 + | +LL | let mut guard = x.write().unwrap(); + | ^^^^^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:28:9 + | +LL | / let mut guard = x.write().unwrap(); +LL | | baz().await +LL | | } + | |_____^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:53:13 + | +LL | let guard = x.lock().unwrap(); + | ^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:53:9 + | +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 + --> $DIR/await_holding_lock.rs:66:17 + | +LL | let guard = x.lock().unwrap(); + | ^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:66:13 + | +LL | / let guard = x.lock().unwrap(); +LL | | baz().await +LL | | }; + | |_________^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:78:17 + | +LL | let guard = x.lock().unwrap(); + | ^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:78:13 + | +LL | / let guard = x.lock().unwrap(); +LL | | baz().await +LL | | } + | |_________^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:89:13 + | +LL | let guard = x.lock(); + | ^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:89:9 + | +LL | / let guard = x.lock(); +LL | | baz().await +LL | | } + | |_____^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:104:13 + | +LL | let guard = x.read(); + | ^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:104:9 + | +LL | / let guard = x.read(); +LL | | baz().await +LL | | } + | |_____^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:109:13 + | +LL | let mut guard = x.write(); + | ^^^^^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:109:9 + | +LL | / let mut guard = x.write(); +LL | | baz().await +LL | | } + | |_____^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:134:13 + | +LL | let guard = x.lock(); + | ^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:134:9 + | +LL | / let guard = x.lock(); +LL | | +LL | | let second = baz().await; +LL | | +... | +LL | | first + second + third +LL | | } + | |_____^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:147:17 + | +LL | let guard = x.lock(); + | ^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:147:13 + | +LL | / let guard = x.lock(); +LL | | baz().await +LL | | }; + | |_________^ + +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:159:17 + | +LL | let guard = x.lock(); + | ^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:159:13 + | +LL | / let guard = x.lock(); +LL | | baz().await +LL | | } + | |_________^ + +error: aborting due to 12 previous errors From c5709419b1b31c8d4c5e369c7b9adbf592c599ca Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sat, 12 Feb 2022 11:30:03 +0100 Subject: [PATCH 3/4] Add test for drop-before-await FP --- tests/ui/await_holding_lock.rs | 30 +++++++++---- tests/ui/await_holding_lock.stderr | 67 +++++++++++++++++++----------- 2 files changed, 64 insertions(+), 33 deletions(-) diff --git a/tests/ui/await_holding_lock.rs b/tests/ui/await_holding_lock.rs index 1a57db91ab5..57e5b55045b 100644 --- a/tests/ui/await_holding_lock.rs +++ b/tests/ui/await_holding_lock.rs @@ -2,6 +2,7 @@ // When adding or modifying a test, please do the same for parking_lot::Mutex. mod std_mutex { + use super::baz; use std::sync::{Mutex, RwLock}; pub async fn bad(x: &Mutex) -> u32 { @@ -43,10 +44,6 @@ mod std_mutex { 47 } - pub async fn baz() -> u32 { - 42 - } - pub async fn also_bad(x: &Mutex) -> u32 { let first = baz().await; @@ -83,6 +80,7 @@ mod std_mutex { // When adding or modifying a test, please do the same for std::Mutex. mod parking_lot_mutex { + use super::baz; use parking_lot::{Mutex, RwLock}; pub async fn bad(x: &Mutex) -> u32 { @@ -124,10 +122,6 @@ mod parking_lot_mutex { 47 } - pub async fn baz() -> u32 { - 42 - } - pub async fn also_bad(x: &Mutex) -> u32 { let first = baz().await; @@ -162,6 +156,26 @@ mod parking_lot_mutex { } } +async fn baz() -> u32 { + 42 +} + +async fn no_await(x: std::sync::Mutex) { + let mut guard = x.lock().unwrap(); + *guard += 1; +} + +// FIXME: FP, because the `MutexGuard` is dropped before crossing the await point. This is +// something the needs to be fixed in rustc. There's already drop-tracking, but this is currently +// disabled, see rust-lang/rust#93751. This case isn't picked up by drop-tracking though. If the +// `*guard += 1` is removed it is picked up. +async fn dropped_before_await(x: std::sync::Mutex) { + let mut guard = x.lock().unwrap(); + *guard += 1; + drop(guard); + baz().await; +} + fn main() { let m = std::sync::Mutex::new(100); std_mutex::good(&m); diff --git a/tests/ui/await_holding_lock.stderr b/tests/ui/await_holding_lock.stderr index a6c1dd228e4..976da8d9242 100644 --- a/tests/ui/await_holding_lock.stderr +++ b/tests/ui/await_holding_lock.stderr @@ -1,5 +1,5 @@ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:8:13 + --> $DIR/await_holding_lock.rs:9:13 | LL | let guard = x.lock().unwrap(); | ^^^^^ @@ -7,7 +7,7 @@ LL | let guard = x.lock().unwrap(); = note: `-D clippy::await-holding-lock` implied by `-D warnings` = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:8:9 + --> $DIR/await_holding_lock.rs:9:9 | LL | / let guard = x.lock().unwrap(); LL | | baz().await @@ -15,14 +15,14 @@ LL | | } | |_____^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:23:13 + --> $DIR/await_holding_lock.rs:24:13 | LL | let guard = x.read().unwrap(); | ^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:23:9 + --> $DIR/await_holding_lock.rs:24:9 | LL | / let guard = x.read().unwrap(); LL | | baz().await @@ -30,14 +30,14 @@ LL | | } | |_____^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:28:13 + --> $DIR/await_holding_lock.rs:29:13 | LL | let mut guard = x.write().unwrap(); | ^^^^^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:28:9 + --> $DIR/await_holding_lock.rs:29:9 | LL | / let mut guard = x.write().unwrap(); LL | | baz().await @@ -45,14 +45,14 @@ LL | | } | |_____^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:53:13 + --> $DIR/await_holding_lock.rs:50:13 | LL | let guard = x.lock().unwrap(); | ^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:53:9 + --> $DIR/await_holding_lock.rs:50:9 | LL | / let guard = x.lock().unwrap(); LL | | @@ -64,14 +64,14 @@ LL | | } | |_____^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:66:17 + --> $DIR/await_holding_lock.rs:63:17 | LL | let guard = x.lock().unwrap(); | ^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:66:13 + --> $DIR/await_holding_lock.rs:63:13 | LL | / let guard = x.lock().unwrap(); LL | | baz().await @@ -79,14 +79,14 @@ LL | | }; | |_________^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:78:17 + --> $DIR/await_holding_lock.rs:75:17 | LL | let guard = x.lock().unwrap(); | ^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:78:13 + --> $DIR/await_holding_lock.rs:75:13 | LL | / let guard = x.lock().unwrap(); LL | | baz().await @@ -94,14 +94,14 @@ LL | | } | |_________^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:89:13 + --> $DIR/await_holding_lock.rs:87:13 | LL | let guard = x.lock(); | ^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:89:9 + --> $DIR/await_holding_lock.rs:87:9 | LL | / let guard = x.lock(); LL | | baz().await @@ -109,14 +109,14 @@ LL | | } | |_____^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:104:13 + --> $DIR/await_holding_lock.rs:102:13 | LL | let guard = x.read(); | ^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:104:9 + --> $DIR/await_holding_lock.rs:102:9 | LL | / let guard = x.read(); LL | | baz().await @@ -124,14 +124,14 @@ LL | | } | |_____^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:109:13 + --> $DIR/await_holding_lock.rs:107:13 | LL | let mut guard = x.write(); | ^^^^^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:109:9 + --> $DIR/await_holding_lock.rs:107:9 | LL | / let mut guard = x.write(); LL | | baz().await @@ -139,14 +139,14 @@ LL | | } | |_____^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:134:13 + --> $DIR/await_holding_lock.rs:128:13 | LL | let guard = x.lock(); | ^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:134:9 + --> $DIR/await_holding_lock.rs:128:9 | LL | / let guard = x.lock(); LL | | @@ -158,14 +158,14 @@ LL | | } | |_____^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:147:17 + --> $DIR/await_holding_lock.rs:141:17 | LL | let guard = x.lock(); | ^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:147:13 + --> $DIR/await_holding_lock.rs:141:13 | LL | / let guard = x.lock(); LL | | baz().await @@ -173,19 +173,36 @@ LL | | }; | |_________^ error: this `MutexGuard` is held across an `await` point - --> $DIR/await_holding_lock.rs:159:17 + --> $DIR/await_holding_lock.rs:153:17 | LL | let guard = x.lock(); | ^^^^^ | = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await note: these are all the `await` points this lock is held through - --> $DIR/await_holding_lock.rs:159:13 + --> $DIR/await_holding_lock.rs:153:13 | LL | / let guard = x.lock(); LL | | baz().await LL | | } | |_________^ -error: aborting due to 12 previous errors +error: this `MutexGuard` is held across an `await` point + --> $DIR/await_holding_lock.rs:173:9 + | +LL | let mut guard = x.lock().unwrap(); + | ^^^^^^^^^ + | + = help: consider using an async-aware `Mutex` type or ensuring the `MutexGuard` is dropped before calling await +note: these are all the `await` points this lock is held through + --> $DIR/await_holding_lock.rs:173:5 + | +LL | / let mut guard = x.lock().unwrap(); +LL | | *guard += 1; +LL | | drop(guard); +LL | | baz().await; +LL | | } + | |_^ + +error: aborting due to 13 previous errors From ea0ce7bb76bb01741fc3ba2ca32a80c75a380d0f Mon Sep 17 00:00:00 2001 From: flip1995 Date: Thu, 17 Feb 2022 17:57:39 +0100 Subject: [PATCH 4/4] Move await_holding_* lints to suspicious and improve doc Even though the FP for that the lints were moved to pedantic isn't fixed yet, running the lintcheck tool over the most popular 279 crates didn't trigger this lint once. I would say that this lint is valuable enough, despite the known FP, to be warn-by-default. Especially since a pretty nice workaround exists. --- clippy_lints/src/await_holding_invalid.rs | 88 ++++++++++++++------- clippy_lints/src/lib.register_all.rs | 2 + clippy_lints/src/lib.register_pedantic.rs | 2 - clippy_lints/src/lib.register_suspicious.rs | 2 + 4 files changed, 64 insertions(+), 30 deletions(-) diff --git a/clippy_lints/src/await_holding_invalid.rs b/clippy_lints/src/await_holding_invalid.rs index f7bc8395c82..f0979840ff8 100644 --- a/clippy_lints/src/await_holding_invalid.rs +++ b/clippy_lints/src/await_holding_invalid.rs @@ -9,8 +9,7 @@ use rustc_span::Span; declare_clippy_lint! { /// ### What it does - /// Checks for calls to await while holding a - /// non-async-aware MutexGuard. + /// Checks for calls to await while holding a non-async-aware MutexGuard. /// /// ### Why is this bad? /// The Mutex types found in std::sync and parking_lot @@ -22,41 +21,57 @@ declare_clippy_lint! { /// either by introducing a scope or an explicit call to Drop::drop. /// /// ### Known problems - /// Will report false positive for explicitly dropped guards ([#6446](https://github.com/rust-lang/rust-clippy/issues/6446)). + /// Will report false positive for explicitly dropped guards + /// ([#6446](https://github.com/rust-lang/rust-clippy/issues/6446)). A workaround for this is + /// to wrap the `.lock()` call in a block instead of explicitly dropping the guard. /// /// ### Example - /// ```rust,ignore - /// use std::sync::Mutex; - /// + /// ```rust + /// # use std::sync::Mutex; + /// # async fn baz() {} /// async fn foo(x: &Mutex) { - /// let guard = x.lock().unwrap(); + /// let mut guard = x.lock().unwrap(); /// *guard += 1; - /// bar.await; + /// baz().await; + /// } + /// + /// async fn bar(x: &Mutex) { + /// let mut guard = x.lock().unwrap(); + /// *guard += 1; + /// drop(guard); // explicit drop + /// baz().await; /// } /// ``` /// /// Use instead: - /// ```rust,ignore - /// use std::sync::Mutex; - /// + /// ```rust + /// # use std::sync::Mutex; + /// # async fn baz() {} /// async fn foo(x: &Mutex) { /// { - /// let guard = x.lock().unwrap(); + /// let mut guard = x.lock().unwrap(); /// *guard += 1; /// } - /// bar.await; + /// baz().await; + /// } + /// + /// async fn bar(x: &Mutex) { + /// { + /// let mut guard = x.lock().unwrap(); + /// *guard += 1; + /// } // guard dropped here at end of scope + /// baz().await; /// } /// ``` #[clippy::version = "1.45.0"] pub AWAIT_HOLDING_LOCK, - pedantic, - "Inside an async function, holding a MutexGuard while calling await" + suspicious, + "inside an async function, holding a `MutexGuard` while calling `await`" } declare_clippy_lint! { /// ### What it does - /// Checks for calls to await while holding a - /// `RefCell` `Ref` or `RefMut`. + /// Checks for calls to await while holding a `RefCell` `Ref` or `RefMut`. /// /// ### Why is this bad? /// `RefCell` refs only check for exclusive mutable access @@ -64,35 +79,52 @@ declare_clippy_lint! { /// risks panics from a mutable ref shared while other refs are outstanding. /// /// ### Known problems - /// Will report false positive for explicitly dropped refs ([#6353](https://github.com/rust-lang/rust-clippy/issues/6353)). + /// Will report false positive for explicitly dropped refs + /// ([#6353](https://github.com/rust-lang/rust-clippy/issues/6353)). A workaround for this is + /// to wrap the `.borrow[_mut]()` call in a block instead of explicitly dropping the ref. /// /// ### Example - /// ```rust,ignore - /// use std::cell::RefCell; - /// + /// ```rust + /// # use std::cell::RefCell; + /// # async fn baz() {} /// async fn foo(x: &RefCell) { /// let mut y = x.borrow_mut(); /// *y += 1; - /// bar.await; + /// baz().await; + /// } + /// + /// async fn bar(x: &RefCell) { + /// let mut y = x.borrow_mut(); + /// *y += 1; + /// drop(y); // explicit drop + /// baz().await; /// } /// ``` /// /// Use instead: - /// ```rust,ignore - /// use std::cell::RefCell; - /// + /// ```rust + /// # use std::cell::RefCell; + /// # async fn baz() {} /// async fn foo(x: &RefCell) { /// { /// let mut y = x.borrow_mut(); /// *y += 1; /// } - /// bar.await; + /// baz().await; + /// } + /// + /// async fn bar(x: &RefCell) { + /// { + /// let mut y = x.borrow_mut(); + /// *y += 1; + /// } // y dropped here at end of scope + /// baz().await; /// } /// ``` #[clippy::version = "1.49.0"] pub AWAIT_HOLDING_REFCELL_REF, - pedantic, - "Inside an async function, holding a RefCell ref while calling await" + suspicious, + "inside an async function, holding a `RefCell` ref while calling `await`" } declare_lint_pass!(AwaitHolding => [AWAIT_HOLDING_LOCK, AWAIT_HOLDING_REFCELL_REF]); diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index de0ff536907..a94f6b528b4 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -14,6 +14,8 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ 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(blacklisted_name::BLACKLISTED_NAME), diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs index 1292675f4a9..00d30513181 100644 --- a/clippy_lints/src/lib.register_pedantic.rs +++ b/clippy_lints/src/lib.register_pedantic.rs @@ -4,8 +4,6 @@ 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_invalid::AWAIT_HOLDING_REFCELL_REF), LintId::of(bit_mask::VERBOSE_BIT_MASK), LintId::of(borrow_as_ptr::BORROW_AS_PTR), LintId::of(bytecount::NAIVE_BYTECOUNT), diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 10f8ae4b7f7..da56f800804 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/clippy_lints/src/lib.register_suspicious.rs @@ -5,6 +5,8 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec![ LintId::of(assign_ops::MISREFACTORED_ASSIGN_OP), LintId::of(attrs::BLANKET_CLIPPY_RESTRICTION_LINTS), + LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK), + LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF), LintId::of(eval_order_dependence::EVAL_ORDER_DEPENDENCE), LintId::of(float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS), LintId::of(formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),