forked from rust-lang/rust-clippy
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add lint for holding RefCell Ref across an await
- Loading branch information
Daniel Smith
committed
Oct 21, 2020
1 parent
74530ad
commit 57bf80f
Showing
5 changed files
with
237 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
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 | ||
/// `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, | ||
pedantic, | ||
"Inside an async function, holding a RefCell ref while calling await" | ||
} | ||
|
||
declare_lint_pass!(AwaitHoldingRefCellRef => [AWAIT_HOLDING_REFCELL_REF]); | ||
|
||
impl LateLintPass<'_> for AwaitHoldingRefCellRef { | ||
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 { | ||
let body_id = BodyId { | ||
hir_id: body.value.hir_id, | ||
}; | ||
let def_id = cx.tcx.hir().body_owner_def_id(body_id); | ||
let typeck_results = cx.tcx.typeck(def_id); | ||
check_interior_types(cx, &typeck_results.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_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", | ||
); | ||
} | ||
} | ||
} | ||
} | ||
|
||
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) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
// 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 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 m = RefCell::new(100); | ||
good(&m); | ||
bad(&m); | ||
bad_mut(&m); | ||
also_bad(&m); | ||
not_good(&m); | ||
block_bad(&m); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
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:46: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:46: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:58: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:58:9 | ||
| | ||
LL | / let b = x.borrow_mut(); | ||
LL | | baz().await | ||
LL | | } | ||
| |_____^ | ||
|
||
error: aborting due to 5 previous errors | ||
|