From 72898355429c278ea994758805a92e611aecb430 Mon Sep 17 00:00:00 2001 From: kraktus Date: Tue, 27 Sep 2022 22:24:14 +0200 Subject: [PATCH] [`should_implement_trait`] Also lint `default` method --- clippy_lints/src/methods/mod.rs | 83 +++++++++---------- .../ui/should_impl_trait/method_list_1.stderr | 12 ++- 2 files changed, 49 insertions(+), 46 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index cdde4c54d63..c43f822e3a3 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3255,65 +3255,59 @@ impl<'tcx> LateLintPass<'tcx> for Methods { let self_ty = cx.tcx.type_of(item.def_id); let implements_trait = matches!(item.kind, hir::ItemKind::Impl(hir::Impl { of_trait: Some(_), .. })); - if_chain! { - if let hir::ImplItemKind::Fn(ref sig, id) = impl_item.kind; - if let Some(first_arg) = iter_input_pats(sig.decl, cx.tcx.hir().body(id)).next(); - + if let hir::ImplItemKind::Fn(ref sig, id) = impl_item.kind { let method_sig = cx.tcx.fn_sig(impl_item.def_id); let method_sig = cx.tcx.erase_late_bound_regions(method_sig); - - let first_arg_ty = method_sig.inputs().iter().next(); - - // check conventions w.r.t. conversion method names and predicates - if let Some(first_arg_ty) = first_arg_ty; - - then { - // if this impl block implements a trait, lint in trait definition instead - if !implements_trait && cx.access_levels.is_exported(impl_item.def_id) { - // check missing trait implementations - for method_config in &TRAIT_METHODS { - if name == method_config.method_name && - sig.decl.inputs.len() == method_config.param_count && - method_config.output_type.matches(&sig.decl.output) && - method_config.self_kind.matches(cx, self_ty, *first_arg_ty) && - fn_header_equals(method_config.fn_header, sig.header) && - method_config.lifetime_param_cond(impl_item) - { - span_lint_and_help( - cx, - SHOULD_IMPLEMENT_TRAIT, - impl_item.span, - &format!( - "method `{}` can be confused for the standard trait method `{}::{}`", - method_config.method_name, - method_config.trait_name, - method_config.method_name - ), - None, - &format!( - "consider implementing the trait `{}` or choosing a less ambiguous method name", - method_config.trait_name - ) - ); - } + let first_arg_ty_opt = method_sig.inputs().iter().next().copied(); + // if this impl block implements a trait, lint in trait definition instead + if !implements_trait && cx.access_levels.is_exported(impl_item.def_id) { + // check missing trait implementations + for method_config in &TRAIT_METHODS { + if name == method_config.method_name + && sig.decl.inputs.len() == method_config.param_count + && method_config.output_type.matches(&sig.decl.output) + // in case there is no first arg, since we already have checked the number of arguments + // it's should be always true + && first_arg_ty_opt.map_or(true, |first_arg_ty| method_config + .self_kind.matches(cx, self_ty, first_arg_ty) + ) + && fn_header_equals(method_config.fn_header, sig.header) + && method_config.lifetime_param_cond(impl_item) + { + span_lint_and_help( + cx, + SHOULD_IMPLEMENT_TRAIT, + impl_item.span, + &format!( + "method `{}` can be confused for the standard trait method `{}::{}`", + method_config.method_name, method_config.trait_name, method_config.method_name + ), + None, + &format!( + "consider implementing the trait `{}` or choosing a less ambiguous method name", + method_config.trait_name + ), + ); } } + } - if sig.decl.implicit_self.has_implicit_self() + if sig.decl.implicit_self.has_implicit_self() && !(self.avoid_breaking_exported_api - && cx.access_levels.is_exported(impl_item.def_id)) + && cx.access_levels.is_exported(impl_item.def_id)) + && let Some(first_arg) = iter_input_pats(sig.decl, cx.tcx.hir().body(id)).next() + && let Some(first_arg_ty) = first_arg_ty_opt { wrong_self_convention::check( cx, name, self_ty, - *first_arg_ty, + first_arg_ty, first_arg.pat.span, implements_trait, false ); } - } } // if this impl block implements a trait, lint in trait definition instead @@ -3799,7 +3793,6 @@ const TRAIT_METHODS: [ShouldImplTraitCase; 30] = [ ShouldImplTraitCase::new("std::borrow::BorrowMut", "borrow_mut", 1, FN_HEADER, SelfKind::RefMut, OutType::Ref, true), ShouldImplTraitCase::new("std::clone::Clone", "clone", 1, FN_HEADER, SelfKind::Ref, OutType::Any, true), ShouldImplTraitCase::new("std::cmp::Ord", "cmp", 2, FN_HEADER, SelfKind::Ref, OutType::Any, true), - // FIXME: default doesn't work ShouldImplTraitCase::new("std::default::Default", "default", 0, FN_HEADER, SelfKind::No, OutType::Any, true), ShouldImplTraitCase::new("std::ops::Deref", "deref", 1, FN_HEADER, SelfKind::Ref, OutType::Ref, true), ShouldImplTraitCase::new("std::ops::DerefMut", "deref_mut", 1, FN_HEADER, SelfKind::RefMut, OutType::Ref, true), @@ -3827,7 +3820,7 @@ enum SelfKind { Value, Ref, RefMut, - No, + No, // When we want the first argument type to be different than `Self` } impl SelfKind { diff --git a/tests/ui/should_impl_trait/method_list_1.stderr b/tests/ui/should_impl_trait/method_list_1.stderr index 2b7d4628c3f..e6daedc5654 100644 --- a/tests/ui/should_impl_trait/method_list_1.stderr +++ b/tests/ui/should_impl_trait/method_list_1.stderr @@ -99,6 +99,16 @@ LL | | } | = help: consider implementing the trait `std::cmp::Ord` or choosing a less ambiguous method name +error: method `default` can be confused for the standard trait method `std::default::Default::default` + --> $DIR/method_list_1.rs:65:5 + | +LL | / pub fn default() -> Self { +LL | | unimplemented!() +LL | | } + | |_____^ + | + = help: consider implementing the trait `std::default::Default` or choosing a less ambiguous method name + error: method `deref` can be confused for the standard trait method `std::ops::Deref::deref` --> $DIR/method_list_1.rs:69:5 | @@ -139,5 +149,5 @@ LL | | } | = help: consider implementing the trait `std::ops::Drop` or choosing a less ambiguous method name -error: aborting due to 14 previous errors +error: aborting due to 15 previous errors