Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add await_holding_invalid_type lint #8707

Merged
merged 10 commits into from
Apr 18, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3182,6 +3182,7 @@ Released 2018-09-13
[`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
[`async_yields_async`]: https://rust-lang.github.io/rust-clippy/master/index.html#async_yields_async
[`await_holding_invalid_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_invalid_type
[`await_holding_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock
[`await_holding_refcell_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_refcell_ref
[`bad_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#bad_bit_mask
Expand Down
167 changes: 129 additions & 38 deletions clippy_lints/src/await_holding_invalid.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::{match_def_path, paths};
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::def_id::DefId;
use rustc_hir::{AsyncGeneratorKind, Body, BodyId, GeneratorKind};
use rustc_hir::{def::Res, 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_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::Span;

use crate::utils::conf::DisallowedType;

declare_clippy_lint! {
/// ### What it does
/// Checks for calls to await while holding a non-async-aware MutexGuard.
Expand Down Expand Up @@ -127,17 +130,83 @@ declare_clippy_lint! {
"inside an async function, holding a `RefCell` ref while calling `await`"
}

declare_lint_pass!(AwaitHolding => [AWAIT_HOLDING_LOCK, AWAIT_HOLDING_REFCELL_REF]);
declare_clippy_lint! {
/// ### What it does
/// Allows users to configure types which should not be held across `await`
/// suspension points.
///
/// ### Why is this bad?
/// There are some types which are perfectly "safe" to be used concurrently
/// from a memory access perspective but will cause bugs at runtime if they
/// are held in such a way.
///
/// ### Known problems
///
/// ### Example
///
/// ```toml
/// await-holding-invalid-types = [
/// # You can specify a type name
/// "CustomLockType",
/// # You can (optionally) specify a reason
/// { path = "OtherCustomLockType", reason = "Relies on a thread local" }
/// ]
/// ```
///
/// ```rust
/// # async fn baz() {}
/// struct CustomLockType;
/// struct OtherCustomLockType;
/// async fn foo() {
/// let _x = CustomLockType;
/// let _y = OtherCustomLockType;
/// baz().await; // Lint violation
/// }
/// ```
#[clippy::version = "1.49.0"]
pub AWAIT_HOLDING_INVALID_TYPE,
suspicious,
"holding a type across an await point which is not allowed to be held as per the configuration"
}

impl_lint_pass!(AwaitHolding => [AWAIT_HOLDING_LOCK, AWAIT_HOLDING_REFCELL_REF, AWAIT_HOLDING_INVALID_TYPE]);

#[derive(Debug)]
pub struct AwaitHolding {
conf_invalid_types: Vec<DisallowedType>,
def_ids: FxHashMap<DefId, DisallowedType>,
}

impl AwaitHolding {
pub(crate) fn new(conf_invalid_types: Vec<DisallowedType>) -> Self {
Self {
conf_invalid_types,
def_ids: FxHashMap::default(),
}
}
}

impl LateLintPass<'_> for AwaitHolding {
fn check_crate(&mut self, cx: &LateContext<'_>) {
for conf in &self.conf_invalid_types {
let path = match conf {
DisallowedType::Simple(path) | DisallowedType::WithReason { path, .. } => path,
};
let segs: Vec<_> = path.split("::").collect();
if let Res::Def(_, id) = clippy_utils::def_path_res(cx, &segs) {
self.def_ids.insert(id, conf.clone());
}
}
}

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 typeck_results = cx.tcx.typeck_body(body_id);
check_interior_types(
self.check_interior_types(
cx,
typeck_results.generator_interior_types.as_ref().skip_binder(),
body.value.span,
Expand All @@ -146,46 +215,68 @@ impl LateLintPass<'_> for AwaitHolding {
}
}

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_then(
cx,
AWAIT_HOLDING_LOCK,
ty_cause.span,
"this `MutexGuard` is held across an `await` point",
|diag| {
diag.help(
"consider using an async-aware `Mutex` type or ensuring the \
impl AwaitHolding {
fn check_interior_types(&self, 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_then(
cx,
AWAIT_HOLDING_LOCK,
ty_cause.span,
"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_then(
cx,
AWAIT_HOLDING_REFCELL_REF,
ty_cause.span,
"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",
);
},
);
);
diag.span_note(
ty_cause.scope_span.unwrap_or(span),
"these are all the `await` points this lock is held through",
);
},
);
} else if is_refcell_ref(cx, adt.did()) {
span_lint_and_then(
cx,
AWAIT_HOLDING_REFCELL_REF,
ty_cause.span,
"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",
);
},
);
} else if let Some(disallowed) = self.def_ids.get(&adt.did()) {
emit_invalid_type(cx, ty_cause.span, disallowed);
}
}
}
}
}

fn emit_invalid_type(cx: &LateContext<'_>, span: Span, disallowed: &DisallowedType) {
let (type_name, reason) = match disallowed {
DisallowedType::Simple(path) => (path, &None),
DisallowedType::WithReason { path, reason } => (path, reason),
};

span_lint_and_then(
cx,
AWAIT_HOLDING_INVALID_TYPE,
span,
&format!("`{type_name}` may not be held across an `await` point per `clippy.toml`",),
|diag| {
if let Some(reason) = reason {
diag.note(reason.clone());
}
},
);
}

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)
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ 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_INVALID_TYPE),
LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
LintId::of(bit_mask::BAD_BIT_MASK),
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ store.register_lints(&[
attrs::INLINE_ALWAYS,
attrs::MISMATCHED_TARGET_OS,
attrs::USELESS_ATTRIBUTE,
await_holding_invalid::AWAIT_HOLDING_INVALID_TYPE,
await_holding_invalid::AWAIT_HOLDING_LOCK,
await_holding_invalid::AWAIT_HOLDING_REFCELL_REF,
bit_mask::BAD_BIT_MASK,
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_suspicious.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
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_INVALID_TYPE),
LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
LintId::of(casts::CAST_ABS_TO_UNSIGNED),
Expand Down
7 changes: 6 additions & 1 deletion clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,12 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
}

store.register_late_pass(|| Box::new(utils::author::Author));
store.register_late_pass(|| Box::new(await_holding_invalid::AwaitHolding));
let await_holding_invalid_types = conf.await_holding_invalid_types.clone();
store.register_late_pass(move || {
Box::new(await_holding_invalid::AwaitHolding::new(
await_holding_invalid_types.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await_holding_invalid_types.clone(),
await_holding_invalid_types,

No need for double clone, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this initially but borrowck doesn't like it

error[E0507]: cannot move out of `await_holding_invalid_types`, a captured variable in an `Fn` closure
   --> clippy_lints/src/lib.rs:519:88
    |
518 |     let await_holding_invalid_types = conf.await_holding_invalid_types.clone();
    |         --------------------------- captured outer variable
519 |     store.register_late_pass(move || Box::new(await_holding_invalid::AwaitHolding::new(await_holding_invalid_types)));
    |                              ----------------------------------------------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^--
    |                              |                                                         |
    |                              |                                                         move occurs because `await_holding_invalid_types` has type `std::vec::Vec<DisallowedType>`, which does not implement the `Copy` trait
    |                              captured by this `Fn` closure

))
});
store.register_late_pass(|| Box::new(serde_api::SerdeApi));
let vec_box_size_threshold = conf.vec_box_size_threshold;
let type_complexity_threshold = conf.type_complexity_threshold;
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,8 @@ define_Conf! {
/// the slice pattern that is suggested. If more elements would be necessary, the lint is suppressed.
/// For example, `[_, _, _, e, ..]` is a slice pattern with 4 elements.
(max_suggested_slice_pattern_length: u64 = 3),
/// Lint: AWAIT_HOLDING_INVALID_TYPE
(await_holding_invalid_types: Vec<crate::utils::conf::DisallowedType> = Vec::new()),
}

/// Search for the configuration file.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#![warn(clippy::await_holding_invalid_type)]
use std::net::Ipv4Addr;

async fn bad() -> u32 {
let _x = String::from("hello");
baz().await
}

async fn bad_reason() -> u32 {
let _x = Ipv4Addr::new(127, 0, 0, 1);
baz().await
}

async fn good() -> u32 {
{
let _x = String::from("hi!");
let _y = Ipv4Addr::new(127, 0, 0, 1);
}
baz().await;
let _x = String::from("hi!");
47
}

async fn baz() -> u32 {
42
}

#[allow(clippy::manual_async_fn)]
fn block_bad() -> impl std::future::Future<Output = u32> {
async move {
let _x = String::from("hi!");
baz().await
}
}

fn main() {
good();
bad();
bad_reason();
block_bad();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
error: `std::string::String` may not be held across an `await` point per `clippy.toml`
--> $DIR/await_holding_invalid_type.rs:5:9
|
LL | let _x = String::from("hello");
| ^^
|
= note: `-D clippy::await-holding-invalid-type` implied by `-D warnings`
= note: strings are bad

error: `std::net::Ipv4Addr` may not be held across an `await` point per `clippy.toml`
--> $DIR/await_holding_invalid_type.rs:10:9
|
LL | let _x = Ipv4Addr::new(127, 0, 0, 1);
| ^^

error: `std::string::String` may not be held across an `await` point per `clippy.toml`
--> $DIR/await_holding_invalid_type.rs:31:13
|
LL | let _x = String::from("hi!");
| ^^
|
= note: strings are bad

error: aborting due to 3 previous errors

4 changes: 4 additions & 0 deletions tests/ui-toml/await_holding_invalid_type/clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
await-holding-invalid-types = [
{ path = "std::string::String", reason = "strings are bad" },
"std::net::Ipv4Addr",
]
2 changes: 1 addition & 1 deletion tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `avoid-breaking-exported-api`, `msrv`, `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `pass-by-value-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-trait-bounds`, `max-struct-bools`, `max-fn-params-bools`, `warn-on-all-wildcard-imports`, `disallowed-methods`, `disallowed-types`, `unreadable-literal-lint-fractions`, `upper-case-acronyms-aggressive`, `cargo-ignore-publish`, `standard-macro-braces`, `enforced-import-renames`, `allowed-scripts`, `enable-raw-pointer-heuristic-for-send`, `max-suggested-slice-pattern-length`, `third-party` at line 5 column 1
error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `avoid-breaking-exported-api`, `msrv`, `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `pass-by-value-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-trait-bounds`, `max-struct-bools`, `max-fn-params-bools`, `warn-on-all-wildcard-imports`, `disallowed-methods`, `disallowed-types`, `unreadable-literal-lint-fractions`, `upper-case-acronyms-aggressive`, `cargo-ignore-publish`, `standard-macro-braces`, `enforced-import-renames`, `allowed-scripts`, `enable-raw-pointer-heuristic-for-send`, `max-suggested-slice-pattern-length`, `await-holding-invalid-types`, `third-party` at line 5 column 1

error: aborting due to previous error