From f50f102ef908a1ddfaa96cb66a35f9eea3675bf8 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 27 Sep 2023 20:27:56 +0800 Subject: [PATCH 1/3] implemented basic lint logic for [`unnecessary_blocking_ops`] --- CHANGELOG.md | 1 + clippy_config/src/conf.rs | 20 +++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 9 ++ clippy_lints/src/unnecessary_blocking_ops.rs | 162 +++++++++++++++++++ tests/ui/unnecessary_blocking_ops.rs | 66 ++++++++ tests/ui/unnecessary_blocking_ops.stderr | 71 ++++++++ 7 files changed, 330 insertions(+) create mode 100644 clippy_lints/src/unnecessary_blocking_ops.rs create mode 100644 tests/ui/unnecessary_blocking_ops.rs create mode 100644 tests/ui/unnecessary_blocking_ops.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 62fc5e2c5d42..450f7c8994ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5910,6 +5910,7 @@ Released 2018-09-13 [`unit_hash`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_hash [`unit_return_expecting_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_return_expecting_ord [`unknown_clippy_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#unknown_clippy_lints +[`unnecessary_blocking_ops`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_blocking_ops [`unnecessary_box_returns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns [`unnecessary_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast [`unnecessary_clippy_cfg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_clippy_cfg diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 7f53aad67933..ffcb1f30e62b 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -642,6 +642,26 @@ define_Conf! { /// /// Whether to also emit warnings for unsafe blocks with metavariable expansions in **private** macros. (warn_unsafe_macro_metavars_in_private_macros: bool = false), + /// Lint: UNNECESSARY_BLOCKING_OPS. + /// + /// List of additional blocking function paths to check. + /// + /// #### Example + /// + /// ```toml + /// blocking-ops = ["my_crate::some_blocking_fn"] + /// ``` + (blocking_ops: Vec = <_>::default()), + /// Lint: UNNECESSARY_BLOCKING_OPS. + /// + /// List of additional blocking function paths to check, with replacement suggestion function paths. + /// + /// #### Example + /// + /// ```toml + /// blocking-ops-with-suggestions = [["my_crate::some_blocking_fn" , "my_crate::use_this_instead"]] + /// ``` + (blocking_ops_with_suggestions: Vec<[String; 2]> = <_>::default()), } /// Search for the configuration file. diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 593f59cff423..f502b081978a 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -731,6 +731,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::unit_types::UNIT_ARG_INFO, crate::unit_types::UNIT_CMP_INFO, crate::unnamed_address::FN_ADDRESS_COMPARISONS_INFO, + crate::unnecessary_blocking_ops::UNNECESSARY_BLOCKING_OPS_INFO, crate::unnecessary_box_returns::UNNECESSARY_BOX_RETURNS_INFO, crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO, crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 54e19c1aa7f9..bd812590f4f0 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -359,6 +359,7 @@ mod uninit_vec; mod unit_return_expecting_ord; mod unit_types; mod unnamed_address; +mod unnecessary_blocking_ops; mod unnecessary_box_returns; mod unnecessary_map_on_constructor; mod unnecessary_owned_empty_strings; @@ -556,6 +557,8 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { array_size_threshold, avoid_breaking_exported_api, ref await_holding_invalid_types, + ref blocking_ops, + ref blocking_ops_with_suggestions, cargo_ignore_publish, cognitive_complexity_threshold, ref disallowed_macros, @@ -1178,6 +1181,12 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(set_contains_or_insert::HashsetInsertAfterContains)); store.register_early_pass(|| Box::new(byte_char_slices::ByteCharSlice)); store.register_early_pass(|| Box::new(cfg_not_test::CfgNotTest)); + store.register_late_pass(move |_| { + Box::new(unnecessary_blocking_ops::UnnecessaryBlockingOps::new( + blocking_ops.clone(), + blocking_ops_with_suggestions.clone(), + )) + }); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/unnecessary_blocking_ops.rs b/clippy_lints/src/unnecessary_blocking_ops.rs new file mode 100644 index 000000000000..389ccc147f37 --- /dev/null +++ b/clippy_lints/src/unnecessary_blocking_ops.rs @@ -0,0 +1,162 @@ +use std::ops::ControlFlow; + +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::visitors::for_each_expr_with_closures; +use clippy_utils::{def_path_def_ids, fn_def_id, is_lint_allowed}; +use rustc_data_structures::fx::FxHashMap; +use rustc_errors::Applicability; +use rustc_hir::def_id::DefId; +use rustc_hir::hir_id::CRATE_HIR_ID; +use rustc_hir::{Body, ExprKind, GeneratorKind, HirIdSet}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for async function or async closure with blocking operations that + /// could be replaced with their async counterpart. + /// + /// ### Why is this bad? + /// Blocking a thread prevents tasks being swapped, causing other tasks to stop running + /// until the thread is no longer blocked, which might not be as expected in an async context. + /// + /// ### Known problems + /// Not all blocking operations can be detected, as for now, this lint only detects a small + /// set of functions in standard library by default. And some of the suggestions might need + /// additional features to work properly. + /// + /// ### Example + /// ```rust + /// use std::time::Duration; + /// pub async fn foo() { + /// std::thread::sleep(Duration::from_secs(5)); + /// } + /// ``` + /// Use instead: + /// ```rust + /// use std::time::Duration; + /// pub async fn foo() { + /// tokio::time::sleep(Duration::from_secs(5)); + /// } + /// ``` + #[clippy::version = "1.74.0"] + pub UNNECESSARY_BLOCKING_OPS, + nursery, + "blocking operations in an async context" +} + +pub(crate) struct UnnecessaryBlockingOps { + blocking_ops: Vec, + blocking_ops_with_suggs: Vec<[String; 2]>, + /// Map of resolved funtion def_id with suggestion string after checking crate + id_with_suggs: FxHashMap, + /// Keep track of visited block ids to skip checking the same bodies in `check_body` calls + visited_block: HirIdSet, +} + +impl UnnecessaryBlockingOps { + pub(crate) fn new(blocking_ops: Vec, blocking_ops_with_suggs: Vec<[String; 2]>) -> Self { + Self { + blocking_ops, + blocking_ops_with_suggs, + id_with_suggs: FxHashMap::default(), + visited_block: HirIdSet::default(), + } + } +} + +impl_lint_pass!(UnnecessaryBlockingOps => [UNNECESSARY_BLOCKING_OPS]); + +// TODO: Should we throw away all suggestions and and give full control to user configurations? +// this feels like a free ad for tokio :P +static HARD_CODED_BLOCKING_OPS_WITH_SUGG: [[&str; 2]; 26] = [ + // Sleep + ["std::thread::sleep", "tokio::time::sleep"], + // IO functions + ["std::io::copy", "tokio::io::copy"], + ["std::io::empty", "tokio::io::empty"], + ["std::io::repeat", "tokio::io::repeat"], + ["std::io::sink", "tokio::io::sink"], + ["std::io::stderr", "tokio::io::stderr"], + ["std::io::stdin", "tokio::io::stdin"], + ["std::io::stdout", "tokio::io::stdout"], + // Filesystem functions + ["std::fs::try_exists", "tokio::fs::try_exists"], + ["std::fs::canonicalize", "tokio::fs::canonicalize"], + ["std::fs::copy", "tokio::fs::copy"], + ["std::fs::create_dir", "tokio::fs::create_dir"], + ["std::fs::create_dir_all", "tokio::fs::create_dir_all"], + ["std::fs::hard_link", "tokio::fs::hard_link"], + ["std::fs::metadata", "tokio::fs::metadata"], + ["std::fs::read", "tokio::fs::read"], + ["std::fs::read_dir", "tokio::fs::read_dir"], + ["std::fs::read_to_string", "tokio::fs::read_to_string"], + ["std::fs::remove_dir", "tokio::fs::remove_dir"], + ["std::fs::remove_dir_all", "tokio::fs::remove_dir_all"], + ["std::fs::remove_file", "tokio::fs::remove_file"], + ["std::fs::rename", "tokio::fs::rename"], + ["std::fs::set_permissions", "tokio::fs::set_permissions"], + ["std::fs::soft_link", "tokio::fs::soft_link"], + ["std::fs::symlink_metadata", "tokio::fs::symlink_metadata"], + ["std::fs::write", "tokio::fs::write"], +]; + +impl<'tcx> LateLintPass<'tcx> for UnnecessaryBlockingOps { + fn check_crate(&mut self, cx: &LateContext<'tcx>) { + // Avoids processing and storing a long list of paths if this lint was allowed entirely + if is_lint_allowed(cx, UNNECESSARY_BLOCKING_OPS, CRATE_HIR_ID) { + return; + } + + let full_fn_list = HARD_CODED_BLOCKING_OPS_WITH_SUGG + .into_iter() + // Chain configured functions without suggestions + .chain(self.blocking_ops.iter().map(|p| [p, ""])) + // Chain configured functions with suggestions + .chain( + self.blocking_ops_with_suggs + .iter() + .map(|[p, s]| [p.as_str(), s.as_str()]), + ); + + for [path_str, sugg_path_str] in full_fn_list { + let path = path_str.split("::").collect::>(); + for did in def_path_def_ids(cx, &path) { + self.id_with_suggs.insert(did, sugg_path_str.to_string()); + } + } + } + + fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'tcx>) { + if self.visited_block.contains(&body.value.hir_id) { + return; + } + if let Some(GeneratorKind::Async(_)) = body.generator_kind() { + for_each_expr_with_closures(cx, body, |ex| { + if let ExprKind::Block(block, _) = ex.kind { + self.visited_block.insert(block.hir_id); + } else if let Some(call_did) = fn_def_id(cx, ex) && + let Some(replace_sugg) = self.id_with_suggs.get(&call_did) + { + span_lint_and_then( + cx, + UNNECESSARY_BLOCKING_OPS, + ex.span, + "blocking function call detected in an async body", + |diag| { + if !replace_sugg.is_empty() { + diag.span_suggestion( + ex.span, + "try using an async counterpart such as", + replace_sugg, + Applicability::Unspecified, + ); + } + } + ); + } + ControlFlow::<()>::Continue(()) + }); + } + } +} diff --git a/tests/ui/unnecessary_blocking_ops.rs b/tests/ui/unnecessary_blocking_ops.rs new file mode 100644 index 000000000000..adadc87cae68 --- /dev/null +++ b/tests/ui/unnecessary_blocking_ops.rs @@ -0,0 +1,66 @@ +//@no-rustfix +#![feature(async_fn_in_trait)] +#![feature(async_closure)] +#![allow(incomplete_features)] +#![warn(clippy::unnecessary_blocking_ops)] +use std::{fs, io}; +use std::thread::sleep; +use std::time::Duration; +use tokio::io as tokio_io; + +mod totally_thread_safe { + pub async fn sleep(_dur: std::time::Duration) {} +} + +pub async fn async_fn() { + sleep(Duration::from_secs(1)); + fs::remove_dir("").unwrap(); + fs::copy("", "_").unwrap(); + let _ = fs::canonicalize(""); + + { + fs::write("", "").unwrap(); + let _ = io::stdin(); + } + let _stdout = io::stdout(); + let mut r: &[u8] = b"hello"; + let mut w: Vec = vec![]; + io::copy(&mut r, &mut w).unwrap(); +} + +pub async fn non_blocking() { + totally_thread_safe::sleep(Duration::from_secs(1)).await; // don't lint, not blocking + + + let mut r: &[u8] = b"hello"; + let mut w: Vec = vec![]; + tokio_io::copy(&mut r, &mut w).await; // don't lint, not blocking +} + +trait AsyncTrait { + async fn foo(&self); +} + +struct SomeType(u8); +impl AsyncTrait for SomeType { + async fn foo(&self) { + sleep(Duration::from_secs(self.0 as _)); + } +} + +fn do_something() {} + +fn closures() { + let _ = async || sleep(Duration::from_secs(1)); + let async_closure = async move |_a: i32| { + let _ = 1; + do_something(); + sleep(Duration::from_secs(1)); + }; + let non_async_closure = |_a: i32| { + sleep(Duration::from_secs(1)); // don't lint, not async + do_something(); + }; +} + +fn main() {} diff --git a/tests/ui/unnecessary_blocking_ops.stderr b/tests/ui/unnecessary_blocking_ops.stderr new file mode 100644 index 000000000000..4e71bf39d6d3 --- /dev/null +++ b/tests/ui/unnecessary_blocking_ops.stderr @@ -0,0 +1,71 @@ +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:16:5 + | +LL | sleep(Duration::from_secs(1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::time::sleep` + | + = note: `-D clippy::unnecessary-blocking-ops` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_blocking_ops)]` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:17:5 + | +LL | fs::remove_dir("").unwrap(); + | ^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::fs::remove_dir` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:18:5 + | +LL | fs::copy("", "_").unwrap(); + | ^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::fs::copy` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:19:13 + | +LL | let _ = fs::canonicalize(""); + | ^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::fs::canonicalize` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:22:9 + | +LL | fs::write("", "").unwrap(); + | ^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::fs::write` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:23:17 + | +LL | let _ = io::stdin(); + | ^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::io::stdin` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:25:19 + | +LL | let _stdout = io::stdout(); + | ^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::io::stdout` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:28:5 + | +LL | io::copy(&mut r, &mut w).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::io::copy` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:47:9 + | +LL | sleep(Duration::from_secs(self.0 as _)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::time::sleep` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:54:22 + | +LL | let _ = async || sleep(Duration::from_secs(1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::time::sleep` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:58:9 + | +LL | sleep(Duration::from_secs(1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::time::sleep` + +error: aborting due to 11 previous errors + From fd6120449d596be4ea937a61bf396157eea58a87 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Thu, 28 Sep 2023 17:41:49 +0800 Subject: [PATCH 2/3] remove hard-coded suggestion, add ui-toml test --- clippy_lints/src/unnecessary_blocking_ops.rs | 137 ++++++++++-------- .../unnecessary_blocking_ops/clippy.toml | 8 + .../unnecessary_blocking_ops.rs | 35 +++++ .../unnecessary_blocking_ops.stderr | 51 +++++++ tests/ui/unnecessary_blocking_ops.rs | 15 +- tests/ui/unnecessary_blocking_ops.stderr | 48 +++--- 6 files changed, 197 insertions(+), 97 deletions(-) create mode 100644 tests/ui-toml/unnecessary_blocking_ops/clippy.toml create mode 100644 tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs create mode 100644 tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.stderr diff --git a/clippy_lints/src/unnecessary_blocking_ops.rs b/clippy_lints/src/unnecessary_blocking_ops.rs index 389ccc147f37..b155e7f3ccba 100644 --- a/clippy_lints/src/unnecessary_blocking_ops.rs +++ b/clippy_lints/src/unnecessary_blocking_ops.rs @@ -1,15 +1,17 @@ use std::ops::ControlFlow; use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::snippet_with_applicability; use clippy_utils::visitors::for_each_expr_with_closures; use clippy_utils::{def_path_def_ids, fn_def_id, is_lint_allowed}; use rustc_data_structures::fx::FxHashMap; -use rustc_errors::Applicability; +use rustc_errors::{Applicability, Diagnostic}; use rustc_hir::def_id::DefId; use rustc_hir::hir_id::CRATE_HIR_ID; -use rustc_hir::{Body, ExprKind, GeneratorKind, HirIdSet}; +use rustc_hir::{Body, Expr, ExprKind, GeneratorKind, HirIdSet}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::Span; declare_clippy_lint! { /// ### What it does @@ -33,7 +35,7 @@ declare_clippy_lint! { /// } /// ``` /// Use instead: - /// ```rust + /// ```ignore /// use std::time::Duration; /// pub async fn foo() { /// tokio::time::sleep(Duration::from_secs(5)); @@ -49,7 +51,7 @@ pub(crate) struct UnnecessaryBlockingOps { blocking_ops: Vec, blocking_ops_with_suggs: Vec<[String; 2]>, /// Map of resolved funtion def_id with suggestion string after checking crate - id_with_suggs: FxHashMap, + id_with_suggs: FxHashMap>, /// Keep track of visited block ids to skip checking the same bodies in `check_body` calls visited_block: HirIdSet, } @@ -67,38 +69,30 @@ impl UnnecessaryBlockingOps { impl_lint_pass!(UnnecessaryBlockingOps => [UNNECESSARY_BLOCKING_OPS]); -// TODO: Should we throw away all suggestions and and give full control to user configurations? -// this feels like a free ad for tokio :P -static HARD_CODED_BLOCKING_OPS_WITH_SUGG: [[&str; 2]; 26] = [ - // Sleep - ["std::thread::sleep", "tokio::time::sleep"], - // IO functions - ["std::io::copy", "tokio::io::copy"], - ["std::io::empty", "tokio::io::empty"], - ["std::io::repeat", "tokio::io::repeat"], - ["std::io::sink", "tokio::io::sink"], - ["std::io::stderr", "tokio::io::stderr"], - ["std::io::stdin", "tokio::io::stdin"], - ["std::io::stdout", "tokio::io::stdout"], +static HARD_CODED_BLOCKING_OPS: [&[&str]; 21] = [ + &["std", "thread", "sleep"], // Filesystem functions - ["std::fs::try_exists", "tokio::fs::try_exists"], - ["std::fs::canonicalize", "tokio::fs::canonicalize"], - ["std::fs::copy", "tokio::fs::copy"], - ["std::fs::create_dir", "tokio::fs::create_dir"], - ["std::fs::create_dir_all", "tokio::fs::create_dir_all"], - ["std::fs::hard_link", "tokio::fs::hard_link"], - ["std::fs::metadata", "tokio::fs::metadata"], - ["std::fs::read", "tokio::fs::read"], - ["std::fs::read_dir", "tokio::fs::read_dir"], - ["std::fs::read_to_string", "tokio::fs::read_to_string"], - ["std::fs::remove_dir", "tokio::fs::remove_dir"], - ["std::fs::remove_dir_all", "tokio::fs::remove_dir_all"], - ["std::fs::remove_file", "tokio::fs::remove_file"], - ["std::fs::rename", "tokio::fs::rename"], - ["std::fs::set_permissions", "tokio::fs::set_permissions"], - ["std::fs::soft_link", "tokio::fs::soft_link"], - ["std::fs::symlink_metadata", "tokio::fs::symlink_metadata"], - ["std::fs::write", "tokio::fs::write"], + &["std", "fs", "try_exists"], + &["std", "fs", "canonicalize"], + &["std", "fs", "copy"], + &["std", "fs", "create_dir"], + &["std", "fs", "create_dir_all"], + &["std", "fs", "hard_link"], + &["std", "fs", "metadata"], + &["std", "fs", "read"], + &["std", "fs", "read_dir"], + &["std", "fs", "read_link"], + &["std", "fs", "read_to_string"], + &["std", "fs", "remove_dir"], + &["std", "fs", "remove_dir_all"], + &["std", "fs", "remove_file"], + &["std", "fs", "rename"], + &["std", "fs", "set_permissions"], + &["std", "fs", "symlink_metadata"], + &["std", "fs", "write"], + // IO functions + &["std", "io", "copy"], + &["std", "io", "read_to_string"], ]; impl<'tcx> LateLintPass<'tcx> for UnnecessaryBlockingOps { @@ -108,55 +102,72 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryBlockingOps { return; } - let full_fn_list = HARD_CODED_BLOCKING_OPS_WITH_SUGG + let full_fn_list = HARD_CODED_BLOCKING_OPS .into_iter() + .map(|p| (p.to_vec(), None)) // Chain configured functions without suggestions - .chain(self.blocking_ops.iter().map(|p| [p, ""])) + .chain( + self.blocking_ops + .iter() + .map(|p| (p.split("::").collect::>(), None)), + ) // Chain configured functions with suggestions .chain( self.blocking_ops_with_suggs .iter() - .map(|[p, s]| [p.as_str(), s.as_str()]), + .map(|[p, s]| (p.split("::").collect::>(), Some(s.as_str()))), ); - - for [path_str, sugg_path_str] in full_fn_list { - let path = path_str.split("::").collect::>(); + for (path, maybe_sugg_str) in full_fn_list { for did in def_path_def_ids(cx, &path) { - self.id_with_suggs.insert(did, sugg_path_str.to_string()); + self.id_with_suggs.insert(did, maybe_sugg_str.map(ToOwned::to_owned)); } } } fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'tcx>) { - if self.visited_block.contains(&body.value.hir_id) { + if is_lint_allowed(cx, UNNECESSARY_BLOCKING_OPS, body.value.hir_id) + || self.visited_block.contains(&body.value.hir_id) + { return; } if let Some(GeneratorKind::Async(_)) = body.generator_kind() { for_each_expr_with_closures(cx, body, |ex| { - if let ExprKind::Block(block, _) = ex.kind { - self.visited_block.insert(block.hir_id); - } else if let Some(call_did) = fn_def_id(cx, ex) && - let Some(replace_sugg) = self.id_with_suggs.get(&call_did) - { - span_lint_and_then( - cx, - UNNECESSARY_BLOCKING_OPS, - ex.span, - "blocking function call detected in an async body", - |diag| { - if !replace_sugg.is_empty() { - diag.span_suggestion( - ex.span, - "try using an async counterpart such as", - replace_sugg, - Applicability::Unspecified, - ); + match ex.kind { + ExprKind::Block(block, _) => { + self.visited_block.insert(block.hir_id); + } + ExprKind::Call(call, _) + if let Some(call_did) = fn_def_id(cx, ex) && + let Some(maybe_sugg) = self.id_with_suggs.get(&call_did) => { + span_lint_and_then( + cx, + UNNECESSARY_BLOCKING_OPS, + call.span, + "blocking function call detected in an async body", + |diag| { + if let Some(sugg_fn_path) = maybe_sugg { + make_suggestion(diag, cx, ex, call.span, sugg_fn_path); + } } - } - ); + ); + } + _ => {} } ControlFlow::<()>::Continue(()) }); } } } + +fn make_suggestion(diag: &mut Diagnostic, cx: &LateContext<'_>, expr: &Expr<'_>, fn_span: Span, sugg_fn_path: &str) { + let mut applicability = Applicability::Unspecified; + let args_span = expr.span.with_lo(fn_span.hi()); + let args_snippet = snippet_with_applicability(cx, args_span, "..", &mut applicability); + let suggestion = format!("{sugg_fn_path}{args_snippet}.await"); + diag.span_suggestion( + expr.span, + "try using its async counterpart", + suggestion, + Applicability::Unspecified, + ); +} diff --git a/tests/ui-toml/unnecessary_blocking_ops/clippy.toml b/tests/ui-toml/unnecessary_blocking_ops/clippy.toml new file mode 100644 index 000000000000..0ce53ba99ee9 --- /dev/null +++ b/tests/ui-toml/unnecessary_blocking_ops/clippy.toml @@ -0,0 +1,8 @@ +blocking-ops = ["unnecessary_blocking_ops::blocking_mod::sleep"] +blocking-ops-with-suggestions = [ + ["std::fs::remove_dir", "tokio::fs::remove_dir"], + ["std::fs::copy", "tokio::fs::copy"], + ["std::io::copy", "tokio::io::copy"], + ["std::io::read_to_string", "unnecessary_blocking_ops::async_mod::read_to_string"], + ["std::thread::sleep", "unnecessary_blocking_ops::async_mod::sleep"], +] diff --git a/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs b/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs new file mode 100644 index 000000000000..4fce92aa70ef --- /dev/null +++ b/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs @@ -0,0 +1,35 @@ +//@no-rustfix +#![warn(clippy::unnecessary_blocking_ops)] +use std::thread::sleep; +use std::time::Duration; +use std::{fs, io}; + +mod async_mod { + pub async fn sleep(_dur: std::time::Duration) {} + pub async fn read_to_string(mut reader: std::io::Stdin) -> Result { + Ok(String::new()) + } +} + +mod blocking_mod { + pub async fn sleep(_dur: std::time::Duration) {} +} + +pub async fn async_fn() { + sleep(Duration::from_secs(1)); + //~^ ERROR: blocking function call detected in an async body + fs::remove_dir("").unwrap(); + //~^ ERROR: blocking function call detected in an async body + fs::copy("", "_").unwrap(); + //~^ ERROR: blocking function call detected in an async body + let mut r: &[u8] = b"hello"; + let mut w: Vec = vec![]; + io::copy(&mut r, &mut w).unwrap(); + //~^ ERROR: blocking function call detected in an async body + let _cont = io::read_to_string(io::stdin()).unwrap(); + //~^ ERROR: blocking function call detected in an async body + fs::create_dir("").unwrap(); + //~^ ERROR: blocking function call detected in an async body +} + +fn main() {} diff --git a/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.stderr b/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.stderr new file mode 100644 index 000000000000..de2414295939 --- /dev/null +++ b/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.stderr @@ -0,0 +1,51 @@ +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:19:5 + | +LL | sleep(Duration::from_secs(1)); + | ^^^^^------------------------ + | | + | help: try using its async counterpart: `unnecessary_blocking_ops::async_mod::sleep(Duration::from_secs(1)).await` + | + = note: `-D clippy::unnecessary-blocking-ops` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_blocking_ops)]` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:21:5 + | +LL | fs::remove_dir("").unwrap(); + | ^^^^^^^^^^^^^^---- + | | + | help: try using its async counterpart: `tokio::fs::remove_dir("").await` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:23:5 + | +LL | fs::copy("", "_").unwrap(); + | ^^^^^^^^--------- + | | + | help: try using its async counterpart: `tokio::fs::copy("", "_").await` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:27:5 + | +LL | io::copy(&mut r, &mut w).unwrap(); + | ^^^^^^^^---------------- + | | + | help: try using its async counterpart: `tokio::io::copy(&mut r, &mut w).await` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:29:17 + | +LL | let _cont = io::read_to_string(io::stdin()).unwrap(); + | ^^^^^^^^^^^^^^^^^^------------- + | | + | help: try using its async counterpart: `unnecessary_blocking_ops::async_mod::read_to_string(io::stdin()).await` + +error: blocking function call detected in an async body + --> $DIR/unnecessary_blocking_ops.rs:31:5 + | +LL | fs::create_dir("").unwrap(); + | ^^^^^^^^^^^^^^ + +error: aborting due to 6 previous errors + diff --git a/tests/ui/unnecessary_blocking_ops.rs b/tests/ui/unnecessary_blocking_ops.rs index adadc87cae68..0b941d06e1fc 100644 --- a/tests/ui/unnecessary_blocking_ops.rs +++ b/tests/ui/unnecessary_blocking_ops.rs @@ -1,11 +1,10 @@ -//@no-rustfix #![feature(async_fn_in_trait)] #![feature(async_closure)] #![allow(incomplete_features)] #![warn(clippy::unnecessary_blocking_ops)] -use std::{fs, io}; use std::thread::sleep; use std::time::Duration; +use std::{fs, io}; use tokio::io as tokio_io; mod totally_thread_safe { @@ -14,24 +13,29 @@ mod totally_thread_safe { pub async fn async_fn() { sleep(Duration::from_secs(1)); + //~^ ERROR: blocking function call detected in an async body fs::remove_dir("").unwrap(); + //~^ ERROR: blocking function call detected in an async body fs::copy("", "_").unwrap(); + //~^ ERROR: blocking function call detected in an async body let _ = fs::canonicalize(""); + //~^ ERROR: blocking function call detected in an async body { fs::write("", "").unwrap(); + //~^ ERROR: blocking function call detected in an async body let _ = io::stdin(); } let _stdout = io::stdout(); let mut r: &[u8] = b"hello"; let mut w: Vec = vec![]; io::copy(&mut r, &mut w).unwrap(); + //~^ ERROR: blocking function call detected in an async body } pub async fn non_blocking() { totally_thread_safe::sleep(Duration::from_secs(1)).await; // don't lint, not blocking - - + let mut r: &[u8] = b"hello"; let mut w: Vec = vec![]; tokio_io::copy(&mut r, &mut w).await; // don't lint, not blocking @@ -45,6 +49,7 @@ struct SomeType(u8); impl AsyncTrait for SomeType { async fn foo(&self) { sleep(Duration::from_secs(self.0 as _)); + //~^ ERROR: blocking function call detected in an async body } } @@ -52,10 +57,12 @@ fn do_something() {} fn closures() { let _ = async || sleep(Duration::from_secs(1)); + //~^ ERROR: blocking function call detected in an async body let async_closure = async move |_a: i32| { let _ = 1; do_something(); sleep(Duration::from_secs(1)); + //~^ ERROR: blocking function call detected in an async body }; let non_async_closure = |_a: i32| { sleep(Duration::from_secs(1)); // don't lint, not async diff --git a/tests/ui/unnecessary_blocking_ops.stderr b/tests/ui/unnecessary_blocking_ops.stderr index 4e71bf39d6d3..2f2118a622ac 100644 --- a/tests/ui/unnecessary_blocking_ops.stderr +++ b/tests/ui/unnecessary_blocking_ops.stderr @@ -1,8 +1,8 @@ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:16:5 + --> $DIR/unnecessary_blocking_ops.rs:15:5 | LL | sleep(Duration::from_secs(1)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::time::sleep` + | ^^^^^ | = note: `-D clippy::unnecessary-blocking-ops` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::unnecessary_blocking_ops)]` @@ -11,61 +11,49 @@ error: blocking function call detected in an async body --> $DIR/unnecessary_blocking_ops.rs:17:5 | LL | fs::remove_dir("").unwrap(); - | ^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::fs::remove_dir` + | ^^^^^^^^^^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:18:5 + --> $DIR/unnecessary_blocking_ops.rs:19:5 | LL | fs::copy("", "_").unwrap(); - | ^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::fs::copy` + | ^^^^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:19:13 + --> $DIR/unnecessary_blocking_ops.rs:21:13 | LL | let _ = fs::canonicalize(""); - | ^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::fs::canonicalize` + | ^^^^^^^^^^^^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:22:9 + --> $DIR/unnecessary_blocking_ops.rs:25:9 | LL | fs::write("", "").unwrap(); - | ^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::fs::write` + | ^^^^^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:23:17 - | -LL | let _ = io::stdin(); - | ^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::io::stdin` - -error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:25:19 - | -LL | let _stdout = io::stdout(); - | ^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::io::stdout` - -error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:28:5 + --> $DIR/unnecessary_blocking_ops.rs:32:5 | LL | io::copy(&mut r, &mut w).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::io::copy` + | ^^^^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:47:9 + --> $DIR/unnecessary_blocking_ops.rs:51:9 | LL | sleep(Duration::from_secs(self.0 as _)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::time::sleep` + | ^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:54:22 + --> $DIR/unnecessary_blocking_ops.rs:59:22 | LL | let _ = async || sleep(Duration::from_secs(1)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::time::sleep` + | ^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:58:9 + --> $DIR/unnecessary_blocking_ops.rs:64:9 | LL | sleep(Duration::from_secs(1)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using an async counterpart such as: `tokio::time::sleep` + | ^^^^^ -error: aborting due to 11 previous errors +error: aborting due to 9 previous errors From 7beca2397730d7ec0678ff2b616b65b63494361d Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Mon, 1 Apr 2024 17:36:20 +0800 Subject: [PATCH 3/3] add `SuggestedPath` config type; --- clippy_config/src/conf.rs | 45 ++++- clippy_config/src/types.rs | 28 +++ clippy_lints/src/lib.rs | 2 - clippy_lints/src/unnecessary_blocking_ops.rs | 186 ++++++++---------- .../toml_unknown_key/conf_unknown_key.stderr | 3 + .../unnecessary_blocking_ops/clippy.toml | 12 +- .../unnecessary_blocking_ops.fixed | 31 +++ .../unnecessary_blocking_ops.rs | 8 +- .../unnecessary_blocking_ops.stderr | 34 +--- tests/ui/unnecessary_blocking_ops.rs | 8 +- tests/ui/unnecessary_blocking_ops.stderr | 26 ++- 11 files changed, 224 insertions(+), 159 deletions(-) create mode 100644 tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.fixed diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index ffcb1f30e62b..7513ca3667f7 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -1,5 +1,7 @@ use crate::msrvs::Msrv; -use crate::types::{DisallowedPath, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour, Rename}; +use crate::types::{ + DisallowedPath, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour, Rename, SuggestedPath, +}; use crate::ClippyConfiguration; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; @@ -44,6 +46,31 @@ const DEFAULT_ALLOWED_IDENTS_BELOW_MIN_CHARS: &[&str] = &["i", "j", "x", "y", "z const DEFAULT_ALLOWED_PREFIXES: &[&str] = &["to", "as", "into", "from", "try_into", "try_from"]; const DEFAULT_ALLOWED_TRAITS_WITH_RENAMED_PARAMS: &[&str] = &["core::convert::From", "core::convert::TryFrom", "core::str::FromStr"]; +const DEFAULT_BLOCKING_OP_PATHS: &[&str] = &[ + "std::thread::sleep", + // Filesystem functions + "std::fs::try_exists", + "std::fs::canonicalize", + "std::fs::copy", + "std::fs::create_dir", + "std::fs::create_dir_all", + "std::fs::hard_link", + "std::fs::metadata", + "std::fs::read", + "std::fs::read_dir", + "std::fs::read_link", + "std::fs::read_to_string", + "std::fs::remove_dir", + "std::fs::remove_dir_all", + "std::fs::remove_file", + "std::fs::rename", + "std::fs::set_permissions", + "std::fs::symlink_metadata", + "std::fs::write", + // IO functions + "std::io::copy", + "std::io::read_to_string", +]; /// Conf with parse errors #[derive(Default)] @@ -644,24 +671,22 @@ define_Conf! { (warn_unsafe_macro_metavars_in_private_macros: bool = false), /// Lint: UNNECESSARY_BLOCKING_OPS. /// - /// List of additional blocking function paths to check. + /// List of additional blocking function paths to check, with optional suggestions for each path. /// /// #### Example /// /// ```toml - /// blocking-ops = ["my_crate::some_blocking_fn"] + /// blocking-ops = [ "my_crate::blocking_foo" ] /// ``` - (blocking_ops: Vec = <_>::default()), - /// Lint: UNNECESSARY_BLOCKING_OPS. /// - /// List of additional blocking function paths to check, with replacement suggestion function paths. - /// - /// #### Example + /// Or, if you are sure that some functions can be replaced by a suggested one: /// /// ```toml - /// blocking-ops-with-suggestions = [["my_crate::some_blocking_fn" , "my_crate::use_this_instead"]] + /// blocking-ops = [ + /// { path = "my_crate::blocking_foo", suggestion = "my_crate::async::async_foo" } + /// ] /// ``` - (blocking_ops_with_suggestions: Vec<[String; 2]> = <_>::default()), + (blocking_ops: Vec = DEFAULT_BLOCKING_OP_PATHS.iter().map(SuggestedPath::from_path_str).collect()), } /// Search for the configuration file. diff --git a/clippy_config/src/types.rs b/clippy_config/src/types.rs index 435aa9244c52..356523d80298 100644 --- a/clippy_config/src/types.rs +++ b/clippy_config/src/types.rs @@ -32,6 +32,33 @@ impl DisallowedPath { } } +#[derive(Clone, Debug, Deserialize, PartialEq, Eq)] +#[serde(untagged)] +pub enum SuggestedPath { + Simple(String), + WithSuggestion { path: String, suggestion: Option }, +} + +impl SuggestedPath { + pub fn path(&self) -> &str { + let (Self::Simple(path) | Self::WithSuggestion { path, .. }) = self; + + path + } + + pub fn from_path_str(path: &S) -> Self { + Self::Simple(path.to_string()) + } + + pub fn suggestion(&self) -> Option<&str> { + if let Self::WithSuggestion { suggestion, .. } = self { + suggestion.as_deref() + } else { + None + } + } +} + #[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, Serialize)] pub enum MatchLintBehaviour { AllTypes, @@ -125,6 +152,7 @@ unimplemented_serialize! { DisallowedPath, Rename, MacroMatcher, + SuggestedPath, } #[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, Serialize)] diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index bd812590f4f0..34af7b02ec91 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -558,7 +558,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { avoid_breaking_exported_api, ref await_holding_invalid_types, ref blocking_ops, - ref blocking_ops_with_suggestions, cargo_ignore_publish, cognitive_complexity_threshold, ref disallowed_macros, @@ -1184,7 +1183,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| { Box::new(unnecessary_blocking_ops::UnnecessaryBlockingOps::new( blocking_ops.clone(), - blocking_ops_with_suggestions.clone(), )) }); // add lints here, do not remove this comment, it's used in `new_lint` diff --git a/clippy_lints/src/unnecessary_blocking_ops.rs b/clippy_lints/src/unnecessary_blocking_ops.rs index b155e7f3ccba..40e9ecedee1f 100644 --- a/clippy_lints/src/unnecessary_blocking_ops.rs +++ b/clippy_lints/src/unnecessary_blocking_ops.rs @@ -1,16 +1,17 @@ -use std::ops::ControlFlow; - +use clippy_config::types::SuggestedPath; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet_with_applicability; -use clippy_utils::visitors::for_each_expr_with_closures; use clippy_utils::{def_path_def_ids, fn_def_id, is_lint_allowed}; use rustc_data_structures::fx::FxHashMap; -use rustc_errors::{Applicability, Diagnostic}; +use rustc_errors::{Applicability, Diag}; use rustc_hir::def_id::DefId; -use rustc_hir::hir_id::CRATE_HIR_ID; -use rustc_hir::{Body, Expr, ExprKind, GeneratorKind, HirIdSet}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_hir::{ + Body, BodyId, Closure, ClosureKind, CoroutineDesugaring, CoroutineKind, Expr, ExprKind, ImplItem, ImplItemKind, + Item, ItemKind, Node, TraitItem, TraitItemKind, +}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_session::impl_lint_pass; use rustc_span::Span; declare_clippy_lint! { @@ -43,81 +44,35 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.74.0"] pub UNNECESSARY_BLOCKING_OPS, - nursery, + pedantic, "blocking operations in an async context" } pub(crate) struct UnnecessaryBlockingOps { - blocking_ops: Vec, - blocking_ops_with_suggs: Vec<[String; 2]>, - /// Map of resolved funtion def_id with suggestion string after checking crate + blocking_ops: Vec, + /// Map of resolved funtion `def_id` with suggestion string after checking crate id_with_suggs: FxHashMap>, - /// Keep track of visited block ids to skip checking the same bodies in `check_body` calls - visited_block: HirIdSet, + /// Tracking whether a body is async after entering it. + body_asyncness: Vec, } impl UnnecessaryBlockingOps { - pub(crate) fn new(blocking_ops: Vec, blocking_ops_with_suggs: Vec<[String; 2]>) -> Self { + pub(crate) fn new(blocking_ops: Vec) -> Self { Self { blocking_ops, - blocking_ops_with_suggs, id_with_suggs: FxHashMap::default(), - visited_block: HirIdSet::default(), + body_asyncness: vec![], } } } impl_lint_pass!(UnnecessaryBlockingOps => [UNNECESSARY_BLOCKING_OPS]); -static HARD_CODED_BLOCKING_OPS: [&[&str]; 21] = [ - &["std", "thread", "sleep"], - // Filesystem functions - &["std", "fs", "try_exists"], - &["std", "fs", "canonicalize"], - &["std", "fs", "copy"], - &["std", "fs", "create_dir"], - &["std", "fs", "create_dir_all"], - &["std", "fs", "hard_link"], - &["std", "fs", "metadata"], - &["std", "fs", "read"], - &["std", "fs", "read_dir"], - &["std", "fs", "read_link"], - &["std", "fs", "read_to_string"], - &["std", "fs", "remove_dir"], - &["std", "fs", "remove_dir_all"], - &["std", "fs", "remove_file"], - &["std", "fs", "rename"], - &["std", "fs", "set_permissions"], - &["std", "fs", "symlink_metadata"], - &["std", "fs", "write"], - // IO functions - &["std", "io", "copy"], - &["std", "io", "read_to_string"], -]; - impl<'tcx> LateLintPass<'tcx> for UnnecessaryBlockingOps { fn check_crate(&mut self, cx: &LateContext<'tcx>) { - // Avoids processing and storing a long list of paths if this lint was allowed entirely - if is_lint_allowed(cx, UNNECESSARY_BLOCKING_OPS, CRATE_HIR_ID) { - return; - } - - let full_fn_list = HARD_CODED_BLOCKING_OPS - .into_iter() - .map(|p| (p.to_vec(), None)) - // Chain configured functions without suggestions - .chain( - self.blocking_ops - .iter() - .map(|p| (p.split("::").collect::>(), None)), - ) - // Chain configured functions with suggestions - .chain( - self.blocking_ops_with_suggs - .iter() - .map(|[p, s]| (p.split("::").collect::>(), Some(s.as_str()))), - ); - for (path, maybe_sugg_str) in full_fn_list { + let full_fn_list = self.blocking_ops.iter().map(|p| (p.path(), p.suggestion())); + for (path_str, maybe_sugg_str) in full_fn_list { + let path: Vec<&str> = path_str.split("::").collect(); for did in def_path_def_ids(cx, &path) { self.id_with_suggs.insert(did, maybe_sugg_str.map(ToOwned::to_owned)); } @@ -125,49 +80,82 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryBlockingOps { } fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'tcx>) { - if is_lint_allowed(cx, UNNECESSARY_BLOCKING_OPS, body.value.hir_id) - || self.visited_block.contains(&body.value.hir_id) - { + if is_lint_allowed(cx, UNNECESSARY_BLOCKING_OPS, body.value.hir_id) { return; } - if let Some(GeneratorKind::Async(_)) = body.generator_kind() { - for_each_expr_with_closures(cx, body, |ex| { - match ex.kind { - ExprKind::Block(block, _) => { - self.visited_block.insert(block.hir_id); - } - ExprKind::Call(call, _) - if let Some(call_did) = fn_def_id(cx, ex) && - let Some(maybe_sugg) = self.id_with_suggs.get(&call_did) => { - span_lint_and_then( - cx, - UNNECESSARY_BLOCKING_OPS, - call.span, - "blocking function call detected in an async body", - |diag| { - if let Some(sugg_fn_path) = maybe_sugg { - make_suggestion(diag, cx, ex, call.span, sugg_fn_path); - } - } - ); + self.body_asyncness.push(in_async_body(cx, body.id())); + } + + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if !in_external_macro(cx.sess(), expr.span) + && matches!(self.body_asyncness.last(), Some(true)) + && let ExprKind::Call(call, _) = &expr.kind + && let Some(call_did) = fn_def_id(cx, expr) + && let Some(maybe_sugg) = self.id_with_suggs.get(&call_did) + { + span_lint_and_then( + cx, + UNNECESSARY_BLOCKING_OPS, + call.span, + "blocking function call detected in an async body", + |diag| { + if let Some(sugg_fn_path) = maybe_sugg { + make_suggestion(diag, cx, expr, call.span, sugg_fn_path); } - _ => {} - } - ControlFlow::<()>::Continue(()) - }); + }, + ); } } + + fn check_body_post(&mut self, _: &LateContext<'tcx>, _: &'tcx Body<'tcx>) { + self.body_asyncness.pop(); + } } -fn make_suggestion(diag: &mut Diagnostic, cx: &LateContext<'_>, expr: &Expr<'_>, fn_span: Span, sugg_fn_path: &str) { - let mut applicability = Applicability::Unspecified; +fn make_suggestion(diag: &mut Diag<'_, ()>, cx: &LateContext<'_>, expr: &Expr<'_>, fn_span: Span, sugg_fn_path: &str) { + // Suggestion should only be offered when user specified it in the configuration file, + // so we only assume it can be fixed here if only the path could be found. + let mut applicability = if def_path_def_ids(cx, &sugg_fn_path.split("::").collect::>()) + .next() + .is_some() + { + Applicability::MaybeIncorrect + } else { + Applicability::Unspecified + }; + let args_span = expr.span.with_lo(fn_span.hi()); let args_snippet = snippet_with_applicability(cx, args_span, "..", &mut applicability); let suggestion = format!("{sugg_fn_path}{args_snippet}.await"); - diag.span_suggestion( - expr.span, - "try using its async counterpart", - suggestion, - Applicability::Unspecified, - ); + diag.span_suggestion(expr.span, "try using its async counterpart", suggestion, applicability); +} + +/// Check whether a body is from an async function/closure. +fn in_async_body(cx: &LateContext<'_>, body_id: BodyId) -> bool { + let parent_node = cx.tcx.parent_hir_node(body_id.hir_id); + match parent_node { + Node::Expr(expr) => matches!( + expr.kind, + ExprKind::Closure(Closure { + kind: ClosureKind::Coroutine(CoroutineKind::Desugared( + CoroutineDesugaring::Async | CoroutineDesugaring::AsyncGen, + _ + )), + .. + }) + ), + Node::Item(Item { + kind: ItemKind::Fn(fn_sig, ..), + .. + }) + | Node::ImplItem(ImplItem { + kind: ImplItemKind::Fn(fn_sig, _), + .. + }) + | Node::TraitItem(TraitItem { + kind: TraitItemKind::Fn(fn_sig, _), + .. + }) => fn_sig.header.is_async(), + _ => false, + } } 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 5cf9c0fb2710..9106844c7c75 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -27,6 +27,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect avoid-breaking-exported-api await-holding-invalid-types blacklisted-names + blocking-ops cargo-ignore-publish check-private-items cognitive-complexity-threshold @@ -111,6 +112,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect avoid-breaking-exported-api await-holding-invalid-types blacklisted-names + blocking-ops cargo-ignore-publish check-private-items cognitive-complexity-threshold @@ -195,6 +197,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni avoid-breaking-exported-api await-holding-invalid-types blacklisted-names + blocking-ops cargo-ignore-publish check-private-items cognitive-complexity-threshold diff --git a/tests/ui-toml/unnecessary_blocking_ops/clippy.toml b/tests/ui-toml/unnecessary_blocking_ops/clippy.toml index 0ce53ba99ee9..b191daaac49f 100644 --- a/tests/ui-toml/unnecessary_blocking_ops/clippy.toml +++ b/tests/ui-toml/unnecessary_blocking_ops/clippy.toml @@ -1,8 +1,6 @@ -blocking-ops = ["unnecessary_blocking_ops::blocking_mod::sleep"] -blocking-ops-with-suggestions = [ - ["std::fs::remove_dir", "tokio::fs::remove_dir"], - ["std::fs::copy", "tokio::fs::copy"], - ["std::io::copy", "tokio::io::copy"], - ["std::io::read_to_string", "unnecessary_blocking_ops::async_mod::read_to_string"], - ["std::thread::sleep", "unnecessary_blocking_ops::async_mod::sleep"], +blocking-ops = [ + { path = "unnecessary_blocking_ops::blocking_mod::sleep", suggestion = "crate::async_mod::sleep" }, + { path = "std::io::copy", suggestion = "tokio::io::copy" }, + { path = "std::io::read_to_string", suggestion = "crate::async_mod::read_to_string" }, + { path = "std::thread::sleep", suggestion = "crate::async_mod::sleep" }, ] diff --git a/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.fixed b/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.fixed new file mode 100644 index 000000000000..cd2fcbf40725 --- /dev/null +++ b/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.fixed @@ -0,0 +1,31 @@ +#![warn(clippy::unnecessary_blocking_ops)] +use std::thread::sleep; +use std::time::Duration; +use std::{fs, io}; + +mod async_mod { + pub async fn sleep(_dur: std::time::Duration) {} + pub async fn read_to_string(mut reader: std::io::Stdin) -> Result { + Ok(String::new()) + } +} + +mod blocking_mod { + pub async fn sleep(_dur: std::time::Duration) {} +} + +pub async fn async_fn() { + crate::async_mod::sleep(Duration::from_secs(1)).await; + //~^ ERROR: blocking function call detected in an async body + crate::async_mod::sleep(Duration::from_secs(1)).await; + //~^ ERROR: blocking function call detected in an async body + let mut r: &[u8] = b"hello"; + let mut w: Vec = vec![]; + tokio::io::copy(&mut r, &mut w).await.unwrap(); + //~^ ERROR: blocking function call detected in an async body + let _cont = crate::async_mod::read_to_string(io::stdin()).await.unwrap(); + //~^ ERROR: blocking function call detected in an async body + fs::create_dir("").unwrap(); // Don't lint, config overrided +} + +fn main() {} diff --git a/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs b/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs index 4fce92aa70ef..bd57f5dbbe2a 100644 --- a/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs +++ b/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs @@ -1,4 +1,3 @@ -//@no-rustfix #![warn(clippy::unnecessary_blocking_ops)] use std::thread::sleep; use std::time::Duration; @@ -18,9 +17,7 @@ mod blocking_mod { pub async fn async_fn() { sleep(Duration::from_secs(1)); //~^ ERROR: blocking function call detected in an async body - fs::remove_dir("").unwrap(); - //~^ ERROR: blocking function call detected in an async body - fs::copy("", "_").unwrap(); + blocking_mod::sleep(Duration::from_secs(1)); //~^ ERROR: blocking function call detected in an async body let mut r: &[u8] = b"hello"; let mut w: Vec = vec![]; @@ -28,8 +25,7 @@ pub async fn async_fn() { //~^ ERROR: blocking function call detected in an async body let _cont = io::read_to_string(io::stdin()).unwrap(); //~^ ERROR: blocking function call detected in an async body - fs::create_dir("").unwrap(); - //~^ ERROR: blocking function call detected in an async body + fs::create_dir("").unwrap(); // Don't lint, config overrided } fn main() {} diff --git a/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.stderr b/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.stderr index de2414295939..faa1701d9dcc 100644 --- a/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.stderr +++ b/tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.stderr @@ -1,32 +1,24 @@ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:19:5 + --> tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs:18:5 | LL | sleep(Duration::from_secs(1)); | ^^^^^------------------------ | | - | help: try using its async counterpart: `unnecessary_blocking_ops::async_mod::sleep(Duration::from_secs(1)).await` + | help: try using its async counterpart: `crate::async_mod::sleep(Duration::from_secs(1)).await` | = note: `-D clippy::unnecessary-blocking-ops` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::unnecessary_blocking_ops)]` error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:21:5 + --> tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs:20:5 | -LL | fs::remove_dir("").unwrap(); - | ^^^^^^^^^^^^^^---- +LL | blocking_mod::sleep(Duration::from_secs(1)); + | ^^^^^^^^^^^^^^^^^^^------------------------ | | - | help: try using its async counterpart: `tokio::fs::remove_dir("").await` + | help: try using its async counterpart: `crate::async_mod::sleep(Duration::from_secs(1)).await` error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:23:5 - | -LL | fs::copy("", "_").unwrap(); - | ^^^^^^^^--------- - | | - | help: try using its async counterpart: `tokio::fs::copy("", "_").await` - -error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:27:5 + --> tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs:24:5 | LL | io::copy(&mut r, &mut w).unwrap(); | ^^^^^^^^---------------- @@ -34,18 +26,12 @@ LL | io::copy(&mut r, &mut w).unwrap(); | help: try using its async counterpart: `tokio::io::copy(&mut r, &mut w).await` error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:29:17 + --> tests/ui-toml/unnecessary_blocking_ops/unnecessary_blocking_ops.rs:26:17 | LL | let _cont = io::read_to_string(io::stdin()).unwrap(); | ^^^^^^^^^^^^^^^^^^------------- | | - | help: try using its async counterpart: `unnecessary_blocking_ops::async_mod::read_to_string(io::stdin()).await` - -error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:31:5 - | -LL | fs::create_dir("").unwrap(); - | ^^^^^^^^^^^^^^ + | help: try using its async counterpart: `crate::async_mod::read_to_string(io::stdin()).await` -error: aborting due to 6 previous errors +error: aborting due to 4 previous errors diff --git a/tests/ui/unnecessary_blocking_ops.rs b/tests/ui/unnecessary_blocking_ops.rs index 0b941d06e1fc..f80d977e7c5e 100644 --- a/tests/ui/unnecessary_blocking_ops.rs +++ b/tests/ui/unnecessary_blocking_ops.rs @@ -1,4 +1,3 @@ -#![feature(async_fn_in_trait)] #![feature(async_closure)] #![allow(incomplete_features)] #![warn(clippy::unnecessary_blocking_ops)] @@ -70,4 +69,11 @@ fn closures() { }; } +fn thread_spawn() { + std::thread::spawn(|| sleep(Duration::from_secs(1))); + std::thread::spawn(async || {}); + std::thread::spawn(async || sleep(Duration::from_secs(1))); + //~^ ERROR: blocking function call detected in an async body +} + fn main() {} diff --git a/tests/ui/unnecessary_blocking_ops.stderr b/tests/ui/unnecessary_blocking_ops.stderr index 2f2118a622ac..e0a5cea992ae 100644 --- a/tests/ui/unnecessary_blocking_ops.stderr +++ b/tests/ui/unnecessary_blocking_ops.stderr @@ -1,5 +1,5 @@ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:15:5 + --> tests/ui/unnecessary_blocking_ops.rs:14:5 | LL | sleep(Duration::from_secs(1)); | ^^^^^ @@ -8,52 +8,58 @@ LL | sleep(Duration::from_secs(1)); = help: to override `-D warnings` add `#[allow(clippy::unnecessary_blocking_ops)]` error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:17:5 + --> tests/ui/unnecessary_blocking_ops.rs:16:5 | LL | fs::remove_dir("").unwrap(); | ^^^^^^^^^^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:19:5 + --> tests/ui/unnecessary_blocking_ops.rs:18:5 | LL | fs::copy("", "_").unwrap(); | ^^^^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:21:13 + --> tests/ui/unnecessary_blocking_ops.rs:20:13 | LL | let _ = fs::canonicalize(""); | ^^^^^^^^^^^^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:25:9 + --> tests/ui/unnecessary_blocking_ops.rs:24:9 | LL | fs::write("", "").unwrap(); | ^^^^^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:32:5 + --> tests/ui/unnecessary_blocking_ops.rs:31:5 | LL | io::copy(&mut r, &mut w).unwrap(); | ^^^^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:51:9 + --> tests/ui/unnecessary_blocking_ops.rs:50:9 | LL | sleep(Duration::from_secs(self.0 as _)); | ^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:59:22 + --> tests/ui/unnecessary_blocking_ops.rs:58:22 | LL | let _ = async || sleep(Duration::from_secs(1)); | ^^^^^ error: blocking function call detected in an async body - --> $DIR/unnecessary_blocking_ops.rs:64:9 + --> tests/ui/unnecessary_blocking_ops.rs:63:9 | LL | sleep(Duration::from_secs(1)); | ^^^^^ -error: aborting due to 9 previous errors +error: blocking function call detected in an async body + --> tests/ui/unnecessary_blocking_ops.rs:75:33 + | +LL | std::thread::spawn(async || sleep(Duration::from_secs(1))); + | ^^^^^ + +error: aborting due to 10 previous errors