Skip to content

Commit

Permalink
add SuggestedPath config type;
Browse files Browse the repository at this point in the history
  • Loading branch information
J-ZhengLi committed Apr 1, 2024
1 parent 66c1371 commit f90251e
Show file tree
Hide file tree
Showing 10 changed files with 194 additions and 137 deletions.
48 changes: 38 additions & 10 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -39,6 +41,31 @@ const DEFAULT_DOC_VALID_IDENTS: &[&str] = &[
];
const DEFAULT_DISALLOWED_NAMES: &[&str] = &["foo", "baz", "quux"];
const DEFAULT_ALLOWED_IDENTS_BELOW_MIN_CHARS: &[&str] = &["i", "j", "x", "y", "z", "w", "n"];
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)]
Expand Down Expand Up @@ -591,24 +618,25 @@ define_Conf! {
(allowed_wildcard_imports: FxHashSet<String> = FxHashSet::default()),
/// Lint: UNNECESSARY_BLOCKING_OPS.
///
/// List of additional blocking function paths to check.
/// List of additional blocking function paths to check.\
/// Note: Because this configuration supports different syntax (simple path or path with suggestion),
/// to prevent suggestion conflicts between default and user specified values,
/// adding a value here will override the default configuration.
///
/// #### Example
///
/// ```toml
/// blocking-ops = ["my_crate::some_blocking_fn"]
/// blocking-ops = [ "my_crate::blocking_foo" ]
/// ```
(blocking_ops: Vec<String> = <_>::default()),
/// Lint: UNNECESSARY_BLOCKING_OPS.
///
/// List of additional blocking function paths to check, with replacement suggestion function paths.
///
/// #### Example
/// Or, if you want these functions to be auto 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<SuggestedPath> = DEFAULT_BLOCKING_OP_PATHS.iter().map(SuggestedPath::from_path_str).collect()),
}

/// Search for the configuration file.
Expand Down
28 changes: 28 additions & 0 deletions clippy_config/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,33 @@ impl DisallowedPath {
}
}

#[derive(Clone, Debug, Deserialize, PartialEq, Eq)]
#[serde(untagged)]
pub enum SuggestedPath {
Simple(String),
WithSuggestion { path: String, suggestion: Option<String> },
}

impl SuggestedPath {
pub fn path(&self) -> &str {
let (Self::Simple(path) | Self::WithSuggestion { path, .. }) = self;

path
}

pub fn from_path_str<S: ToString>(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,
Expand Down Expand Up @@ -125,6 +152,7 @@ unimplemented_serialize! {
DisallowedPath,
Rename,
MacroMatcher,
SuggestedPath,
}

#[derive(Clone, Copy, Debug, PartialEq, Eq, 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 @@ -547,7 +547,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,
Expand Down Expand Up @@ -1137,7 +1136,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`
Expand Down
183 changes: 85 additions & 98 deletions clippy_lints/src/unnecessary_blocking_ops.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
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_hir::{
Body, BodyId, Closure, ClosureKind, CoroutineDesugaring, CoroutineKind, Expr, ExprKind, ImplItem, ImplItemKind,
Item, ItemKind, Node, TraitItem, TraitItemKind,
};
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! {
Expand Down Expand Up @@ -43,131 +43,118 @@ 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<String>,
blocking_ops_with_suggs: Vec<[String; 2]>,
/// Map of resolved funtion def_id with suggestion string after checking crate
blocking_ops: Vec<SuggestedPath>,
/// 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,
/// Tracking whether a body is async after entering it.
body_asyncness: Vec<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<SuggestedPath>) -> 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::<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 = 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);
}
}
);
self.body_asyncness.push(in_async_body(cx, body.id()));
}

fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
if 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 can be found
// (but it may behind feature gate or the arg is different), otherwise don't try to fix it.
let mut applicability = if def_path_def_ids(cx, &sugg_fn_path.split("::").collect::<Vec<_>>())
.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,
}
}
3 changes: 3 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 @@ -23,6 +23,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
Expand Down Expand Up @@ -102,6 +103,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
Expand Down Expand Up @@ -181,6 +183,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
Expand Down
Loading

0 comments on commit f90251e

Please sign in to comment.