From 5df3cdd583ef4e0be0aa38f074c6d3dab015a392 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Thu, 10 Jul 2025 10:25:26 -0700 Subject: [PATCH 1/2] disallowed_methods: handle full paths as replacement for method call This can't do the "correct" thing, and check if the method is defined, because `FnCtxt::lookup_method` isn't accessible in Clippy. To work around this problem, we convert method calls to FQP syntax if the user specifies a full path as a replacement. If they specify only a name, then we only replace the name. Since clippy.toml resolves paths from the top level of the crate, this is basically guaranteed to produce a correct result, if we ignore method call adjustments like autoderef. --- clippy_config/src/types.rs | 17 ++-- clippy_lints/src/await_holding_invalid.rs | 3 +- clippy_lints/src/disallowed_macros.rs | 3 +- clippy_lints/src/disallowed_methods.rs | 81 ++++++++++++++++++- clippy_lints/src/disallowed_types.rs | 3 +- .../clippy.toml | 5 ++ .../replaceable_disallowed_methods.fixed | 34 ++++++++ .../replaceable_disallowed_methods.rs | 34 ++++++++ .../replaceable_disallowed_methods.stderr | 62 +++++++++++++- 9 files changed, 226 insertions(+), 16 deletions(-) diff --git a/clippy_config/src/types.rs b/clippy_config/src/types.rs index f64eefa0c232..8218c8bf25ca 100644 --- a/clippy_config/src/types.rs +++ b/clippy_config/src/types.rs @@ -77,15 +77,18 @@ impl DisallowedPath { &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_ref().map_or("use", String::as_str) + } + + 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()); } diff --git a/clippy_lints/src/await_holding_invalid.rs b/clippy_lints/src/await_holding_invalid.rs index 31cc004f6855..793cbaf90c4e 100644 --- a/clippy_lints/src/await_holding_invalid.rs +++ b/clippy_lints/src/await_holding_invalid.rs @@ -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}; @@ -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), ); } diff --git a/clippy_lints/src/disallowed_macros.rs b/clippy_lints/src/disallowed_macros.rs index 37a12119731e..2c930ab7e653 100644 --- a/clippy_lints/src/disallowed_macros.rs +++ b/clippy_lints/src/disallowed_macros.rs @@ -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::{ @@ -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 { diff --git a/clippy_lints/src/disallowed_methods.rs b/clippy_lints/src/disallowed_methods.rs index 8c067432cb4e..45d7094b79b9 100644 --- a/clippy_lints/src/disallowed_methods.rs +++ b/clippy_lints/src/disallowed_methods.rs @@ -2,6 +2,8 @@ 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}; @@ -9,6 +11,8 @@ 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 @@ -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 }, @@ -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, @@ -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), ); } } diff --git a/clippy_lints/src/disallowed_types.rs b/clippy_lints/src/disallowed_types.rs index 9a82327a0d7b..1f9173916b2a 100644 --- a/clippy_lints/src/disallowed_types.rs +++ b/clippy_lints/src/disallowed_types.rs @@ -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}; @@ -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), ); } } diff --git a/tests/ui-toml/toml_replaceable_disallowed_methods/clippy.toml b/tests/ui-toml/toml_replaceable_disallowed_methods/clippy.toml index dc393f1355b5..bf83b870522e 100644 --- a/tests/ui-toml/toml_replaceable_disallowed_methods/clippy.toml +++ b/tests/ui-toml/toml_replaceable_disallowed_methods/clippy.toml @@ -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" }, ] diff --git a/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.fixed b/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.fixed index df28e0bc94be..aa844168a59a 100644 --- a/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.fixed +++ b/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.fixed @@ -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); } diff --git a/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs b/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs index 9257f0e3e1a7..cca5e367c406 100644 --- a/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs +++ b/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.rs @@ -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); } diff --git a/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.stderr b/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.stderr index 6f70b4ae6a9e..a099c9981589 100644 --- a/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.stderr +++ b/tests/ui-toml/toml_replaceable_disallowed_methods/replaceable_disallowed_methods.stderr @@ -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` @@ -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 From 58a3a4c2be7ae436fd49cdf14d9de62e2dc0a025 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Fri, 11 Jul 2025 12:43:45 -0700 Subject: [PATCH 2/2] Update clippy_config/src/types.rs Co-authored-by: Ada Alakbarova <58857108+ada4a@users.noreply.github.com> --- clippy_config/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_config/src/types.rs b/clippy_config/src/types.rs index 8218c8bf25ca..7704d3cd62d8 100644 --- a/clippy_config/src/types.rs +++ b/clippy_config/src/types.rs @@ -82,7 +82,7 @@ impl DisallowedPath { } pub fn reason(&self) -> &str { - self.reason.as_ref().map_or("use", String::as_str) + self.reason.as_deref().unwrap_or("use") } pub fn diag_amendment(&self, span: Span, applicability: Applicability) -> impl FnOnce(&mut Diag<'_, ()>) {