Skip to content

Commit

Permalink
reduce [unnecessary_blocking_ops]'s configuration to one
Browse files Browse the repository at this point in the history
by re-using `DisallowedPath` from configuration mod
  • Loading branch information
J-ZhengLi committed Oct 25, 2023
1 parent 184f30c commit 95ff7c8
Show file tree
Hide file tree
Showing 8 changed files with 87 additions and 107 deletions.
12 changes: 1 addition & 11 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 10 additions & 1 deletion clippy_config/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)]
Expand Down
2 changes: 0 additions & 2 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
144 changes: 63 additions & 81 deletions clippy_lints/src/unnecessary_blocking_ops.rs
Original file line number Diff line number Diff line change
@@ -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, Expr, ExprKind, CoroutineKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::Span;
Expand Down Expand Up @@ -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;
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions tests/ui-toml/unnecessary_blocking_ops/clippy.toml
Original file line number Diff line number Diff line change
@@ -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"],
Expand Down
1 change: 0 additions & 1 deletion tests/ui/unnecessary_blocking_ops.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#![feature(async_fn_in_trait)]
#![feature(async_closure)]
#![allow(incomplete_features)]
#![warn(clippy::unnecessary_blocking_ops)]
Expand Down
18 changes: 9 additions & 9 deletions tests/ui/unnecessary_blocking_ops.stderr
Original file line number Diff line number Diff line change
@@ -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));
| ^^^^^
Expand All @@ -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));
| ^^^^^
Expand Down

0 comments on commit 95ff7c8

Please sign in to comment.