Auto merge of #8707 - OneSignal:await-invalid-types, r=llogiq

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. I originally implemented this specifically for `tracing::span::Entered`, until I discovered #8434 and read the commentary on that PR. Given this implementation is fully user configurable, doesn't tie clippy to any one particular crate, and introduces no additional dependencies, it seems more appropriate.
rust-lang · Apr 18, 2022 · cbdf17c · cbdf17c
2 parents 95de4dc + 0508685
commit cbdf17c
Show file tree

Hide file tree

Showing 11 changed files with 212 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3283,6 +3283,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/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,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,
@@ -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)

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::dump_hir::DumpHir));
     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/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 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
+
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