Add await_holding_invalid_type lint

changelog: [`await_holding_invalid_type`] This lint allows users to create a denylist of types which are not allowed to be held across await points. This is essentially a re-implementation of the language-level [`must_not_suspend` lint](rust-lang/rust#83310). That lint has a lot of work still to be done before it will reach Rust stable, and in the meantime there are a lot of types which can trip up developers if they are used improperly.
OneSignal · Apr 15, 2022 · 4844325 · 4844325
1 parent aade96f
commit 4844325
Show file tree

Hide file tree

Showing 13 changed files with 215 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -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

diff --git a/Cargo.toml b/Cargo.toml
@@ -50,6 +50,7 @@ futures = "0.3"
 parking_lot = "0.11.2"
 tokio = { version = "1", features = ["io-util"] }
 rustc-semver = "1.1"
+tracing = "0.1"
 
 [build-dependencies]
 rustc_tools_util = { version = "0.2", path = "rustc_tools_util" }

diff --git a/clippy_lints/src/await_holding_invalid.rs b/clippy_lints/src/await_holding_invalid.rs
@@ -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.
@@ -127,17 +130,81 @@ 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
+    ///
+    /// The `tracing` library has types which should not be held across `await`
+    /// points.
+    ///
+    /// ```toml
+    /// await-holding-invalid-types = [
+    ///   "tracing::span::Entered",
+    ///   "tracing::span::EnteredSpan",
+    /// ]
+    /// ```
+    ///
+    /// ```rust
+    /// # async fn baz() {}
+    /// async fn foo() {
+    ///   let _entered = tracing::info_span!("baz").entered();
+    ///   baz().await;
+    /// }
+    /// ```
+    #[clippy::version = "1.49.0"]
+    pub AWAIT_HOLDING_INVALID_TYPE,
+    suspicious,
+    "inside an async function, holding a type across an await point which is not safe to be held across an await point"
+}
+
+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,
@@ -146,46 +213,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 according to config",),
+        |diag| {
+            if let Some(reason) = reason {
+                diag.note(format!("{reason} (according to clippy.toml)"));
+            }
+        },
+    );
+}
+
 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)

diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs
@@ -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),

diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
@@ -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,

diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs
@@ -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),

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -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(),
+        ))
+    });
     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;

diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs
@@ -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.

diff --git a/tests/compile-test.rs b/tests/compile-test.rs
@@ -35,6 +35,7 @@ static TEST_DEPENDENCIES: &[&str] = &[
     "tokio",
     "parking_lot",
     "rustc_semver",
+    "tracing",
 ];
 
 // Test dependencies may need an `extern crate` here to ensure that they show up
@@ -59,6 +60,9 @@ extern crate rustc_semver;
 extern crate syn;
 #[allow(unused_extern_crates)]
 extern crate tokio;
+#[allow(unused_extern_crates, unused_imports)]
+#[macro_use]
+extern crate tracing;
 
 /// Produces a string with an `--extern` flag for all UI test crate
 /// dependencies.

diff --git a/tests/ui-toml/await_holding_invalid_type/await_holding_invalid_type.rs b/tests/ui-toml/await_holding_invalid_type/await_holding_invalid_type.rs
@@ -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();
+}
diff --git a/tests/ui-toml/await_holding_invalid_type/await_holding_invalid_type.stderr b/tests/ui-toml/await_holding_invalid_type/await_holding_invalid_type.stderr
@@ -0,0 +1,25 @@
+error: `std::string::String` may not be held across an `await` point according to config
+  --> $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 (according to clippy.toml)
+
+error: `std::net::Ipv4Addr` may not be held across an `await` point according to config
+  --> $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 according to config
+  --> $DIR/await_holding_invalid_type.rs:31:13
+   |
+LL |         let _x = String::from("hi!");
+   |             ^^
+   |
+   = note: strings are bad (according to clippy.toml)
+
+error: aborting due to 3 previous errors
+
diff --git a/tests/ui-toml/await_holding_invalid_type/clippy.toml b/tests/ui-toml/await_holding_invalid_type/clippy.toml
@@ -0,0 +1,4 @@
+await-holding-invalid-types = [
+    { path = "std::string::String", reason = "strings are bad" },
+    "std::net::Ipv4Addr",
+]
diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
@@ -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