reduce [unnecessary_blocking_ops]'s configuration to one

J-ZhengLi · J-ZhengLi · commit 0bf62399b3d6 · 2023-10-25T09:55:04.000+08:00
by re-using `DisallowedPath` from configuration mod
diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs
@@ -552,17 +552,7 @@ define_Conf! {
     /// ```toml
     /// blocking-ops = ["my_crate::some_blocking_fn"]
     /// ```
-    (blocking_ops: Vec<String> = <_>::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<DisallowedPath> = <_>::default()),
 }
 
 /// Search for the configuration file.
diff --git a/clippy_config/src/types.rs b/clippy_config/src/types.rs
@@ -14,11 +14,12 @@ pub struct Rename {
 pub enum DisallowedPath {
     Simple(String),
     WithReason { path: String, reason: Option<String> },
+    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
     }
@@ -31,6 +32,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, Hash, Deserialize, Serialize)]
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -1066,11 +1066,9 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(msrv())));
     store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter));
     let blocking_ops = conf.blocking_ops.clone();
-    let blocking_ops_with_suggs = conf.blocking_ops_with_suggestions.clone();
     store.register_late_pass(move |_| {
         Box::new(unnecessary_blocking_ops::UnnecessaryBlockingOps::new(
             blocking_ops.clone(),
-            blocking_ops_with_suggs.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
@@ -1,14 +1,11 @@
-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_span::Span;
@@ -48,113 +45,98 @@ declare_clippy_lint! {
 }
 
 pub(crate) struct UnnecessaryBlockingOps {
-    blocking_ops: Vec<String>,
-    blocking_ops_with_suggs: Vec<[String; 2]>,
+    blocking_ops: Vec<DisallowedPath>,
     /// Map of resolved funtion def_id with suggestion string after checking crate
     id_with_suggs: FxHashMap<DefId, Option<String>>,
-    /// 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<String>, blocking_ops_with_suggs: Vec<[String; 2]>) -> Self {
+    pub(crate) fn new(blocking_ops: Vec<DisallowedPath>) -> 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::<Vec<_>>(), None)),
-            )
-            // Chain configured functions with suggestions
-            .chain(
-                self.blocking_ops_with_suggs
-                    .iter()
-                    .map(|[p, s]| (p.split("::").collect::<Vec<_>>(), 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));
             }
         }
     }
 
     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
@@ -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
            cognitive-complexity-threshold
            cyclomatic-complexity-threshold
@@ -94,6 +95,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
            cognitive-complexity-threshold
            cyclomatic-complexity-threshold
diff --git 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
@@ -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
@@ -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));
    |         ^^^^^

Original file line number	Diff line number	Diff line change
`@@ -14,11 +14,12 @@ pub struct Rename {`
`14`	`14`	`pub enum DisallowedPath {`
`15`	`15`	`Simple(String),`
`16`	`16`	`WithReason { path: String, reason: Option<String> },`
	`17`	`+ WithSuggestion(String, String),`
`17`	`18`	`}`
`18`	`19`
`19`	`20`	`impl DisallowedPath {`
`20`	`21`	`pub fn path(&self) -> &str {`
`21`		`- let (Self::Simple(path) \| Self::WithReason { path, .. }) = self;`
	`22`	`+ let (Self::Simple(path) \| Self::WithReason { path, .. } \| Self::WithSuggestion(path, _)) = self;`
`22`	`23`
`23`	`24`	`path`
`24`	`25`	`}`
`@@ -31,6 +32,14 @@ impl DisallowedPath {`
`31`	`32`	`_ => None,`
`32`	`33`	`}`
`33`	`34`	`}`
	`35`	`+`
	`36`	`+ pub fn suggestion(&self) -> Option<&str> {`
	`37`	`+ if let Self::WithSuggestion(_, sugg) = self {`
	`38`	`+ Some(sugg)`
	`39`	`+ } else {`
	`40`	`+ None`
	`41`	`+ }`
	`42`	`+ }`
`34`	`43`	`}`
`35`	`44`
`36`	`45`	`#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Deserialize, Serialize)]`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,3 @@`
`1`		`-#![feature(async_fn_in_trait)]`
`2`	`1`	`#![feature(async_closure)]`
`3`	`2`	`#![allow(incomplete_features)]`
`4`	`3`	`#![warn(clippy::unnecessary_blocking_ops)]`