Skip to content

disallowed_methods: handle full paths as replacement for method call #15242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions clippy_config/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,18 @@ impl<const REPLACEMENT_ALLOWED: bool> DisallowedPath<REPLACEMENT_ALLOWED> {
&self.path
}

pub fn diag_amendment(&self, span: Span) -> impl FnOnce(&mut Diag<'_, ()>) {
pub fn replacement(&self) -> Option<&str> {
self.replacement.as_deref()
}

pub fn reason(&self) -> &str {
self.reason.as_deref().unwrap_or("use")
}

pub fn diag_amendment(&self, span: Span, applicability: Applicability) -> impl FnOnce(&mut Diag<'_, ()>) {
move |diag| {
if let Some(replacement) = &self.replacement {
diag.span_suggestion(
span,
self.reason.as_ref().map_or_else(|| String::from("use"), Clone::clone),
replacement,
Applicability::MachineApplicable,
);
diag.span_suggestion(span, self.reason().to_string(), replacement, applicability);
} else if let Some(reason) = &self.reason {
diag.note(reason.clone());
}
Expand Down
3 changes: 2 additions & 1 deletion clippy_lints/src/await_holding_invalid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use clippy_config::Conf;
use clippy_config::types::{DisallowedPathWithoutReplacement, create_disallowed_map};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::paths::{self, PathNS};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, DefIdMap};
use rustc_lint::{LateContext, LateLintPass};
Expand Down Expand Up @@ -271,7 +272,7 @@ fn emit_invalid_type(
AWAIT_HOLDING_INVALID_TYPE,
span,
format!("holding a disallowed type across an await point `{path}`"),
disallowed_path.diag_amendment(span),
disallowed_path.diag_amendment(span, Applicability::MachineApplicable),
);
}

Expand Down
3 changes: 2 additions & 1 deletion clippy_lints/src/disallowed_macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then};
use clippy_utils::macros::macro_backtrace;
use clippy_utils::paths::PathNS;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::Applicability;
use rustc_hir::def::DefKind;
use rustc_hir::def_id::DefIdMap;
use rustc_hir::{
Expand Down Expand Up @@ -104,7 +105,7 @@ impl DisallowedMacros {

if let Some(&(path, disallowed_path)) = self.disallowed.get(&mac.def_id) {
let msg = format!("use of a disallowed macro `{path}`");
let add_note = disallowed_path.diag_amendment(mac.span);
let add_note = disallowed_path.diag_amendment(mac.span, Applicability::MachineApplicable);
if matches!(mac.kind, MacroKind::Derive)
&& let Some(derive_src) = derive_src
{
Expand Down
81 changes: 78 additions & 3 deletions clippy_lints/src/disallowed_methods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@ use clippy_config::Conf;
use clippy_config::types::{DisallowedPath, create_disallowed_map};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::paths::PathNS;
use clippy_utils::source::snippet_with_applicability;
use rustc_errors::Applicability;
use rustc_hir::def::{CtorKind, DefKind, Res};
use rustc_hir::def_id::DefIdMap;
use rustc_hir::{Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::TyCtxt;
use rustc_session::impl_lint_pass;

use std::borrow::Cow;

declare_clippy_lint! {
/// ### What it does
/// Denies the configured methods and functions in clippy.toml
Expand All @@ -34,6 +38,15 @@ declare_clippy_lint! {
/// { path = "std::vec::Vec::leak", reason = "no leaking memory" },
/// # Can also add a `replacement` that will be offered as a suggestion.
/// { path = "std::sync::Mutex::new", reason = "prefer faster & simpler non-poisonable mutex", replacement = "parking_lot::Mutex::new" },
/// # Replacement can be specified as a bare name. If you do this, the method name will be
/// # replaced without altering the rest of the call.
/// # self.bad_method() becomes self.good_method()
/// # Type::bad_method() becomes Type::good_method()
/// { path = "crate::Type::bad_method", replacement = "good_method" }
/// # If replacement is specifed as a full path, the call will be converted to a fully-qualified call.
/// # self.evil_method() becomes crate::free_function(self)
/// # Type::evil_method() becomes crate::free_function()
/// { path = "crate::Type::evil_method", replacement = "crate::free_function" }
/// # This would normally error if the path is incorrect, but with `allow-invalid` = `true`,
/// # it will be silently ignored
/// { path = "std::fs::InvalidPath", reason = "use alternative instead", allow-invalid = true },
Expand Down Expand Up @@ -89,8 +102,70 @@ impl_lint_pass!(DisallowedMethods => [DISALLOWED_METHODS]);
impl<'tcx> LateLintPass<'tcx> for DisallowedMethods {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
let (id, span) = match &expr.kind {
ExprKind::Path(path) if let Res::Def(_, id) = cx.qpath_res(path, expr.hir_id) => (id, expr.span),
ExprKind::MethodCall(name, ..) if let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) => {
ExprKind::Path(call_path) if let Res::Def(_, id) = cx.qpath_res(call_path, expr.hir_id) => {
if let Some(&(path, disallowed_path)) = self.disallowed.get(&id)
&& let Some(replacement) = disallowed_path.replacement()
&& !replacement.contains("::")
&& let &rustc_hir::QPath::Resolved(
_,
&rustc_hir::Path {
segments: &[.., method_name_in_call],
..
},
)
| &rustc_hir::QPath::TypeRelative(_, &method_name_in_call) = call_path
{
// FQP method call with non-fully-qualified replacement.
// To support this, we need to only replace the last node in the path, not the whole thing.
span_lint_and_then(
cx,
DISALLOWED_METHODS,
expr.span,
format!("use of a disallowed method `{path}`"),
disallowed_path.diag_amendment(method_name_in_call.ident.span, Applicability::MaybeIncorrect),
);
return;
}
(id, expr.span)
},
ExprKind::MethodCall(name, self_expr, params_exprs, call_span)
if let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) =>
{
if let Some(&(path, disallowed_path)) = self.disallowed.get(&id)
&& let Some(replacement) = disallowed_path.replacement()
&& replacement.contains("::")
{
span_lint_and_then(
cx,
DISALLOWED_METHODS,
name.ident.span,
format!("use of a disallowed method `{path}`"),
|diag| {
// FnCtxt is not exported, so we cannot use its `lookup_method` method.
// Instead, we convert to a FQP if the user supplies a `::` path here.
// Not MachineApplicable because adjustments (auto deref etc) aren't handled.
let mut applicability = Applicability::MaybeIncorrect;
let self_snippet = snippet_with_applicability(cx, self_expr.span, "..", &mut applicability);
let (comma, params_snippet) = if let (Some(first), Some(last)) =
(params_exprs.first(), params_exprs.last())
{
(
", ",
snippet_with_applicability(cx, first.span.to(last.span), "..", &mut applicability),
)
} else {
("", Cow::default())
};
diag.span_suggestion(
self_expr.span.to(*call_span),
disallowed_path.reason(),
format!("{replacement}({self_snippet}{comma}{params_snippet})"),
applicability,
);
},
);
return;
}
(id, name.ident.span)
},
_ => return,
Expand All @@ -101,7 +176,7 @@ impl<'tcx> LateLintPass<'tcx> for DisallowedMethods {
DISALLOWED_METHODS,
span,
format!("use of a disallowed method `{path}`"),
disallowed_path.diag_amendment(span),
disallowed_path.diag_amendment(span, Applicability::MaybeIncorrect),
);
}
}
Expand Down
3 changes: 2 additions & 1 deletion clippy_lints/src/disallowed_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use clippy_config::types::{DisallowedPath, create_disallowed_map};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::paths::PathNS;
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::DefIdMap;
use rustc_hir::{AmbigArg, Item, ItemKind, PolyTraitRef, PrimTy, Ty, TyKind, UseKind};
Expand Down Expand Up @@ -86,7 +87,7 @@ impl DisallowedTypes {
DISALLOWED_TYPES,
span,
format!("use of a disallowed type `{path}`"),
disallowed_path.diag_amendment(span),
disallowed_path.diag_amendment(span, Applicability::MachineApplicable),
);
}
}
Expand Down
5 changes: 5 additions & 0 deletions tests/ui-toml/toml_replaceable_disallowed_methods/clippy.toml
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
disallowed-methods = [
{ path = "replaceable_disallowed_methods::bad", replacement = "good" },
{ path = "replaceable_disallowed_methods::questionable", replacement = "good", reason = "a better function exists" },
{ path = "f64::round", replacement = "f64::round_ties_even", reason = "https://github.com/rust-lang/rust-clippy/issues/15067" },
{ path = "replaceable_disallowed_methods::WithMethod::evil", replacement = "crate::free", reason = "replace method with free function" },
{ path = "replaceable_disallowed_methods::WithMethod::mean", replacement = "nice", reason = "replace method with other method" },
{ path = "replaceable_disallowed_methods::WithMethod::ugly", replacement = "WithMethod::nice", reason = "replace method with qualified method" },
{ path = "replaceable_disallowed_methods::WithMethod::cruel", replacement = "WithMethod::kind", reason = "replace method with qualified method with param" },
]
Original file line number Diff line number Diff line change
@@ -1,10 +1,44 @@
fn bad() {}
fn questionable() {}
fn good() {}
fn free(_: WithMethod) {}

pub struct WithMethod;

impl WithMethod {
pub fn evil(self) {}
pub fn mean(self) {}
pub fn ugly(self) {}
pub fn nice(self) {}
pub fn cruel(self, _: i32) {}
pub fn kind(self, _: i32) {}
}

fn main() {
good();
//~^ disallowed_methods
good();
//~^ disallowed_methods
f64::round_ties_even(3f64);
//~^ disallowed_methods
crate::free(WithMethod);
//~^ disallowed_methods
crate::free(WithMethod);
//~^ disallowed_methods
WithMethod.nice();
//~^ disallowed_methods
WithMethod::nice(WithMethod);
//~^ disallowed_methods
WithMethod::nice(WithMethod);
//~^ disallowed_methods
WithMethod::nice(WithMethod);
//~^ disallowed_methods
WithMethod::kind(WithMethod, 1);
//~^ disallowed_methods
WithMethod::kind(WithMethod, 1);
//~^ disallowed_methods

WithMethod.nice();
good();
free(WithMethod);
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,44 @@
fn bad() {}
fn questionable() {}
fn good() {}
fn free(_: WithMethod) {}

pub struct WithMethod;

impl WithMethod {
pub fn evil(self) {}
pub fn mean(self) {}
pub fn ugly(self) {}
pub fn nice(self) {}
pub fn cruel(self, _: i32) {}
pub fn kind(self, _: i32) {}
}

fn main() {
bad();
//~^ disallowed_methods
questionable();
//~^ disallowed_methods
3f64.round();
//~^ disallowed_methods
WithMethod.evil();
//~^ disallowed_methods
WithMethod::evil(WithMethod);
//~^ disallowed_methods
WithMethod.mean();
//~^ disallowed_methods
WithMethod::mean(WithMethod);
//~^ disallowed_methods
WithMethod.ugly();
//~^ disallowed_methods
WithMethod::ugly(WithMethod);
//~^ disallowed_methods
WithMethod.cruel(1);
//~^ disallowed_methods
WithMethod::cruel(WithMethod, 1);
//~^ disallowed_methods

WithMethod.nice();
good();
free(WithMethod);
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: use of a disallowed method `replaceable_disallowed_methods::bad`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:6:5
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:18:5
|
LL | bad();
| ^^^ help: use: `good`
Expand All @@ -8,10 +8,66 @@ LL | bad();
= help: to override `-D warnings` add `#[allow(clippy::disallowed_methods)]`

error: use of a disallowed method `replaceable_disallowed_methods::questionable`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:8:5
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:20:5
|
LL | questionable();
| ^^^^^^^^^^^^ help: a better function exists: `good`

error: aborting due to 2 previous errors
error: use of a disallowed method `f64::round`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:22:10
|
LL | 3f64.round();
| -----^^^^^-- help: https://github.com/rust-lang/rust-clippy/issues/15067: `f64::round_ties_even(3f64)`

error: use of a disallowed method `replaceable_disallowed_methods::WithMethod::evil`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:24:16
|
LL | WithMethod.evil();
| -----------^^^^-- help: replace method with free function: `crate::free(WithMethod)`

error: use of a disallowed method `replaceable_disallowed_methods::WithMethod::evil`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:26:5
|
LL | WithMethod::evil(WithMethod);
| ^^^^^^^^^^^^^^^^ help: replace method with free function: `crate::free`

error: use of a disallowed method `replaceable_disallowed_methods::WithMethod::mean`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:28:16
|
LL | WithMethod.mean();
| ^^^^ help: replace method with other method: `nice`

error: use of a disallowed method `replaceable_disallowed_methods::WithMethod::mean`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:30:5
|
LL | WithMethod::mean(WithMethod);
| ^^^^^^^^^^^^----
| |
| help: replace method with other method: `nice`

error: use of a disallowed method `replaceable_disallowed_methods::WithMethod::ugly`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:32:16
|
LL | WithMethod.ugly();
| -----------^^^^-- help: replace method with qualified method: `WithMethod::nice(WithMethod)`

error: use of a disallowed method `replaceable_disallowed_methods::WithMethod::ugly`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:34:5
|
LL | WithMethod::ugly(WithMethod);
| ^^^^^^^^^^^^^^^^ help: replace method with qualified method: `WithMethod::nice`

error: use of a disallowed method `replaceable_disallowed_methods::WithMethod::cruel`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:36:16
|
LL | WithMethod.cruel(1);
| -----------^^^^^--- help: replace method with qualified method with param: `WithMethod::kind(WithMethod, 1)`

error: use of a disallowed method `replaceable_disallowed_methods::WithMethod::cruel`
--> tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs:38:5
|
LL | WithMethod::cruel(WithMethod, 1);
| ^^^^^^^^^^^^^^^^^ help: replace method with qualified method with param: `WithMethod::kind`

error: aborting due to 11 previous errors