diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 47df156ab3a2..bee43c7cbdf3 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -561,17 +561,7 @@ define_Conf! { /// ```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()), + (blocking_ops: Vec = <_>::default()), } /// Search for the configuration file. diff --git a/clippy_config/src/types.rs b/clippy_config/src/types.rs index baee09629ac4..cae9cac7f14d 100644 --- a/clippy_config/src/types.rs +++ b/clippy_config/src/types.rs @@ -13,11 +13,12 @@ pub struct Rename { pub enum DisallowedPath { Simple(String), WithReason { path: String, reason: Option }, + WithSuggestion(String, String), } impl DisallowedPath { pub fn path(&self) -> &str { - let (Self::Simple(path) | Self::WithReason { path, .. }) = self; + let (Self::Simple(path) | Self::WithReason { path, .. } | Self::WithSuggestion(path, _)) = self; path } @@ -30,6 +31,14 @@ impl DisallowedPath { _ => None, } } + + pub fn suggestion(&self) -> Option<&str> { + if let Self::WithSuggestion(_, sugg) = self { + Some(sugg) + } else { + None + } + } } #[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, Serialize)] diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2b647f46f754..4d29cf90b61f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -529,7 +529,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, @@ -1098,7 +1097,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..27a96970ec38 100644 --- a/clippy_lints/src/unnecessary_blocking_ops.rs +++ b/clippy_lints/src/unnecessary_blocking_ops.rs @@ -1,16 +1,13 @@ -use std::ops::ControlFlow; - +use clippy_config::types::DisallowedPath; 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_hir::def_id::DefId; -use rustc_hir::hir_id::CRATE_HIR_ID; -use rustc_hir::{Body, Expr, ExprKind, GeneratorKind, HirIdSet}; +use rustc_hir::{Body, CoroutineKind, Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_session::impl_lint_pass; use rustc_span::Span; declare_clippy_lint! { @@ -48,76 +45,59 @@ declare_clippy_lint! { } pub(crate) struct UnnecessaryBlockingOps { - blocking_ops: Vec, - blocking_ops_with_suggs: Vec<[String; 2]>, + 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, + is_in_async: bool, } 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(), + is_in_async: false, } } } impl_lint_pass!(UnnecessaryBlockingOps => [UNNECESSARY_BLOCKING_OPS]); -static HARD_CODED_BLOCKING_OPS: [&[&str]; 21] = [ - &["std", "thread", "sleep"], +static HARD_CODED_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"], + "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"], + "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 = HARD_CODED_BLOCKING_OP_PATHS + .iter() + .map(|p| (*p, None)) + // Chain configured functions with possible suggestions + .chain(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,36 +105,38 @@ 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); - } - } - ); + + if let Some(CoroutineKind::Async(_)) = body.coroutine_kind() { + self.is_in_async = true; + } + } + + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if self.is_in_async + && 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>, body: &'tcx Body<'tcx>) { + if !matches!(body.coroutine_kind(), Some(CoroutineKind::Async(_))) { + self.is_in_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 ed4fb84df1ae..edd3a07b8545 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -20,6 +20,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 @@ -96,6 +97,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 diff --git a/tests/ui-toml/unnecessary_blocking_ops/clippy.toml b/tests/ui-toml/unnecessary_blocking_ops/clippy.toml index 0ce53ba99ee9..5ad54750e1ac 100644 --- a/tests/ui-toml/unnecessary_blocking_ops/clippy.toml +++ b/tests/ui-toml/unnecessary_blocking_ops/clippy.toml @@ -1,5 +1,5 @@ -blocking-ops = ["unnecessary_blocking_ops::blocking_mod::sleep"] -blocking-ops-with-suggestions = [ +blocking-ops = [ + "unnecessary_blocking_ops::blocking_mod::sleep", ["std::fs::remove_dir", "tokio::fs::remove_dir"], ["std::fs::copy", "tokio::fs::copy"], ["std::io::copy", "tokio::io::copy"], diff --git a/tests/ui/unnecessary_blocking_ops.rs b/tests/ui/unnecessary_blocking_ops.rs index 0b941d06e1fc..969ed4f99377 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)] diff --git a/tests/ui/unnecessary_blocking_ops.stderr b/tests/ui/unnecessary_blocking_ops.stderr index 2f2118a622ac..5dc5a9f60b2c 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 + --> $DIR/unnecessary_blocking_ops.rs:14:5 | LL | sleep(Duration::from_secs(1)); | ^^^^^ @@ -8,49 +8,49 @@ 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 + --> $DIR/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 + --> $DIR/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 + --> $DIR/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 + --> $DIR/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 + --> $DIR/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 + --> $DIR/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 + --> $DIR/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 + --> $DIR/unnecessary_blocking_ops.rs:63:9 | LL | sleep(Duration::from_secs(1)); | ^^^^^