diff --git a/CHANGELOG.md b/CHANGELOG.md index 39786c08156d..7aa4884d1039 100644 --- 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 index dd6518d5241b..1fb532deb9be 100644 --- 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 index 4592ca727488..36b4f7faa19f 100644 --- 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,9 +130,73 @@ 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, + def_ids: FxHashMap, +} + +impl AwaitHolding { + pub(crate) fn new(conf_invalid_types: Vec) -> 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 { @@ -137,7 +204,7 @@ impl LateLintPass<'_> for AwaitHolding { 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 index 2fa597333659..be57e1038fdf 100644 --- 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 index c608f634291f..8a97233dc5d5 100644 --- 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 index 82f45b5fd58b..7a881bfe3991 100644 --- 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 index 88e8a0cc2af0..e31f71fe5da9 100644 --- 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 index 271c3a3dd181..e6abe499b430 100644 --- 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 = Vec::new()), } /// Search for the configuration file. diff --git a/tests/compile-test.rs b/tests/compile-test.rs index c9710e3db8e8..f437553bca18 100644 --- 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 new file mode 100644 index 000000000000..fbef5c4564b1 --- /dev/null +++ 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 { + 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 new file mode 100644 index 000000000000..eebdd624d243 --- /dev/null +++ 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 new file mode 100644 index 000000000000..79990096b84e --- /dev/null +++ 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 index 00ddbd608a7c..f60afb8d4e38 100644 --- 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