Skip to content

Commit 26a9a37

Browse files
ada4aada4a
committed
clean-up: extract should_implement_trait
Also put `SelfKind` into `lib.rs`, since it's shared with `wrong_self_convention` -- not `utils.rs`, because it's not really a utility function
1 parent f6e223c commit 26a9a37

File tree

4 files changed

+247
-219
lines changed

4 files changed

+247
-219
lines changed

‎clippy_lints/src/methods/lib.rs‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
use clippy_utils::sym;
2+
use clippy_utils::ty::{implements_trait, is_copy};
3+
use rustc_hir::Mutability;
4+
use rustc_lint::LateContext;
5+
use rustc_middle::ty::{self, Ty};
6+
7+
#[derive(Clone, Copy, PartialEq, Eq, Debug)]
8+
pub(super) enum SelfKind {
9+
Value,
10+
Ref,
11+
RefMut,
12+
No, // When we want the first argument type to be different than `Self`
13+
}
14+
15+
impl SelfKind {
16+
pub(super) fn matches<'a>(self, cx: &LateContext<'a>, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool {
17+
fn matches_value<'a>(cx: &LateContext<'a>, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool {
18+
if ty == parent_ty {
19+
true
20+
} else if let Some(boxed_ty) = ty.boxed_ty() {
21+
boxed_ty == parent_ty
22+
} else if let ty::Adt(adt_def, args) = ty.kind()
23+
&& matches!(cx.tcx.get_diagnostic_name(adt_def.did()), Some(sym::Rc | sym::Arc))
24+
{
25+
args.types().next() == Some(parent_ty)
26+
} else {
27+
false
28+
}
29+
}
30+
31+
fn matches_ref<'a>(cx: &LateContext<'a>, mutability: Mutability, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool {
32+
if let ty::Ref(_, t, m) = *ty.kind() {
33+
return m == mutability && t == parent_ty;
34+
}
35+
36+
let trait_sym = match mutability {
37+
Mutability::Not => sym::AsRef,
38+
Mutability::Mut => sym::AsMut,
39+
};
40+
41+
let Some(trait_def_id) = cx.tcx.get_diagnostic_item(trait_sym) else {
42+
return false;
43+
};
44+
implements_trait(cx, ty, trait_def_id, &[parent_ty.into()])
45+
}
46+
47+
fn matches_none<'a>(cx: &LateContext<'a>, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool {
48+
!matches_value(cx, parent_ty, ty)
49+
&& !matches_ref(cx, Mutability::Not, parent_ty, ty)
50+
&& !matches_ref(cx, Mutability::Mut, parent_ty, ty)
51+
}
52+
53+
match self {
54+
Self::Value => matches_value(cx, parent_ty, ty),
55+
Self::Ref => matches_ref(cx, Mutability::Not, parent_ty, ty) || ty == parent_ty && is_copy(cx, ty),
56+
Self::RefMut => matches_ref(cx, Mutability::Mut, parent_ty, ty),
57+
Self::No => matches_none(cx, parent_ty, ty),
58+
}
59+
}
60+
61+
#[must_use]
62+
pub(super) fn description(self) -> &'static str {
63+
match self {
64+
Self::Value => "`self` by value",
65+
Self::Ref => "`self` by reference",
66+
Self::RefMut => "`self` by mutable reference",
67+
Self::No => "no `self`",
68+
}
69+
}
70+
}

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 11 additions & 218 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ mod iter_skip_zero;
5555
mod iter_with_drain;
5656
mod iterator_step_by_zero;
5757
mod join_absolute_paths;
58+
mod lib;
5859
mod manual_c_str_literals;
5960
mod manual_contains;
6061
mod manual_inspect;
@@ -102,6 +103,7 @@ mod return_and_then;
102103
mod search_is_some;
103104
mod seek_from_current;
104105
mod seek_to_start_instead_of_rewind;
106+
mod should_implement_trait;
105107
mod single_char_add_str;
106108
mod skip_while_next;
107109
mod sliced_string_as_bytes;
@@ -146,19 +148,18 @@ mod zst_offset;
146148

147149
use clippy_config::Conf;
148150
use clippy_utils::consts::{ConstEvalCtxt, Constant};
149-
use clippy_utils::diagnostics::{span_lint, span_lint_and_help};
151+
use clippy_utils::diagnostics::span_lint;
150152
use clippy_utils::macros::FormatArgsStorage;
151153
use clippy_utils::msrvs::{self, Msrv};
152-
use clippy_utils::ty::{contains_ty_adt_constructor_opaque, implements_trait, is_copy};
153-
use clippy_utils::{contains_return, is_bool, is_trait_method, iter_input_pats, peel_blocks, return_ty, sym};
154+
use clippy_utils::ty::contains_ty_adt_constructor_opaque;
155+
use clippy_utils::{contains_return, is_trait_method, iter_input_pats, peel_blocks, return_ty, sym};
154156
pub use path_ends_with_ext::DEFAULT_ALLOWED_DOTFILES;
155-
use rustc_abi::ExternAbi;
156157
use rustc_data_structures::fx::FxHashSet;
157158
use rustc_hir::{self as hir, Expr, ExprKind, Node, Stmt, StmtKind, TraitItem, TraitItemKind};
158159
use rustc_lint::{LateContext, LateLintPass, LintContext};
159-
use rustc_middle::ty::{self, TraitRef, Ty};
160+
use rustc_middle::ty::TraitRef;
160161
use rustc_session::impl_lint_pass;
161-
use rustc_span::{Span, Symbol, kw};
162+
use rustc_span::{Span, Symbol};
162163

163164
declare_clippy_lint! {
164165
/// ### What it does
@@ -4888,47 +4889,17 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
48884889
if impl_item.span.in_external_macro(cx.sess().source_map()) {
48894890
return;
48904891
}
4892+
48914893
if let hir::ImplItemKind::Fn(ref sig, id) = impl_item.kind {
4892-
let name = impl_item.ident.name;
48934894
let parent = cx.tcx.hir_get_parent_item(impl_item.hir_id()).def_id;
48944895
let item = cx.tcx.hir_expect_item(parent);
48954896
let self_ty = cx.tcx.type_of(item.owner_id).instantiate_identity();
4896-
48974897
let implements_trait = matches!(item.kind, hir::ItemKind::Impl(hir::Impl { of_trait: Some(_), .. }));
4898+
48984899
let method_sig = cx.tcx.fn_sig(impl_item.owner_id).instantiate_identity();
48994900
let method_sig = cx.tcx.instantiate_bound_regions_with_erased(method_sig);
49004901
let first_arg_ty_opt = method_sig.inputs().iter().next().copied();
4901-
// if this impl block implements a trait, lint in trait definition instead
4902-
if !implements_trait && cx.effective_visibilities.is_exported(impl_item.owner_id.def_id) {
4903-
// check missing trait implementations
4904-
for method_config in &TRAIT_METHODS {
4905-
if name == method_config.method_name
4906-
&& sig.decl.inputs.len() == method_config.param_count
4907-
&& method_config.output_type.matches(&sig.decl.output)
4908-
// in case there is no first arg, since we already have checked the number of arguments
4909-
// it's should be always true
4910-
&& first_arg_ty_opt
4911-
.is_none_or(|first_arg_ty| method_config.self_kind.matches(cx, self_ty, first_arg_ty))
4912-
&& fn_header_equals(method_config.fn_header, sig.header)
4913-
&& method_config.lifetime_param_cond(impl_item)
4914-
{
4915-
span_lint_and_help(
4916-
cx,
4917-
SHOULD_IMPLEMENT_TRAIT,
4918-
impl_item.span,
4919-
format!(
4920-
"method `{}` can be confused for the standard trait method `{}::{}`",
4921-
method_config.method_name, method_config.trait_name, method_config.method_name
4922-
),
4923-
None,
4924-
format!(
4925-
"consider implementing the trait `{}` or choosing a less ambiguous method name",
4926-
method_config.trait_name
4927-
),
4928-
);
4929-
}
4930-
}
4931-
}
4902+
should_implement_trait::check_impl_item(cx, impl_item, self_ty, implements_trait, first_arg_ty_opt, sig);
49324903

49334904
if sig.decl.implicit_self.has_implicit_self()
49344905
&& !(self.avoid_breaking_exported_api
@@ -4938,7 +4909,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
49384909
{
49394910
wrong_self_convention::check(
49404911
cx,
4941-
name,
4912+
impl_item.ident.name,
49424913
self_ty,
49434914
first_arg_ty,
49444915
first_arg.pat.span,
@@ -5711,181 +5682,3 @@ fn lint_binary_expr_with_method_call(cx: &LateContext<'_>, info: &mut BinaryExpr
57115682
lint_with_both_lhs_and_rhs!(chars_next_cmp_with_unwrap::check, cx, info);
57125683
lint_with_both_lhs_and_rhs!(chars_last_cmp_with_unwrap::check, cx, info);
57135684
}
5714-
5715-
const FN_HEADER: hir::FnHeader = hir::FnHeader {
5716-
safety: hir::HeaderSafety::Normal(hir::Safety::Safe),
5717-
constness: hir::Constness::NotConst,
5718-
asyncness: hir::IsAsync::NotAsync,
5719-
abi: ExternAbi::Rust,
5720-
};
5721-
5722-
struct ShouldImplTraitCase {
5723-
trait_name: &'static str,
5724-
method_name: Symbol,
5725-
param_count: usize,
5726-
fn_header: hir::FnHeader,
5727-
// implicit self kind expected (none, self, &self, ...)
5728-
self_kind: SelfKind,
5729-
// checks against the output type
5730-
output_type: OutType,
5731-
// certain methods with explicit lifetimes can't implement the equivalent trait method
5732-
lint_explicit_lifetime: bool,
5733-
}
5734-
impl ShouldImplTraitCase {
5735-
const fn new(
5736-
trait_name: &'static str,
5737-
method_name: Symbol,
5738-
param_count: usize,
5739-
fn_header: hir::FnHeader,
5740-
self_kind: SelfKind,
5741-
output_type: OutType,
5742-
lint_explicit_lifetime: bool,
5743-
) -> ShouldImplTraitCase {
5744-
ShouldImplTraitCase {
5745-
trait_name,
5746-
method_name,
5747-
param_count,
5748-
fn_header,
5749-
self_kind,
5750-
output_type,
5751-
lint_explicit_lifetime,
5752-
}
5753-
}
5754-
5755-
fn lifetime_param_cond(&self, impl_item: &hir::ImplItem<'_>) -> bool {
5756-
self.lint_explicit_lifetime
5757-
|| !impl_item.generics.params.iter().any(|p| {
5758-
matches!(
5759-
p.kind,
5760-
hir::GenericParamKind::Lifetime {
5761-
kind: hir::LifetimeParamKind::Explicit
5762-
}
5763-
)
5764-
})
5765-
}
5766-
}
5767-
5768-
#[rustfmt::skip]
5769-
const TRAIT_METHODS: [ShouldImplTraitCase; 30] = [
5770-
ShouldImplTraitCase::new("std::ops::Add", sym::add, 2, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5771-
ShouldImplTraitCase::new("std::convert::AsMut", sym::as_mut, 1, FN_HEADER, SelfKind::RefMut, OutType::Ref, true ),
5772-
ShouldImplTraitCase::new("std::convert::AsRef", sym::as_ref, 1, FN_HEADER, SelfKind::Ref, OutType::Ref, true ),
5773-
ShouldImplTraitCase::new("std::ops::BitAnd", sym::bitand, 2, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5774-
ShouldImplTraitCase::new("std::ops::BitOr", sym::bitor, 2, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5775-
ShouldImplTraitCase::new("std::ops::BitXor", sym::bitxor, 2, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5776-
ShouldImplTraitCase::new("std::borrow::Borrow", sym::borrow, 1, FN_HEADER, SelfKind::Ref, OutType::Ref, true ),
5777-
ShouldImplTraitCase::new("std::borrow::BorrowMut", sym::borrow_mut, 1, FN_HEADER, SelfKind::RefMut, OutType::Ref, true ),
5778-
ShouldImplTraitCase::new("std::clone::Clone", sym::clone, 1, FN_HEADER, SelfKind::Ref, OutType::Any, true ),
5779-
ShouldImplTraitCase::new("std::cmp::Ord", sym::cmp, 2, FN_HEADER, SelfKind::Ref, OutType::Any, true ),
5780-
ShouldImplTraitCase::new("std::default::Default", kw::Default, 0, FN_HEADER, SelfKind::No, OutType::Any, true ),
5781-
ShouldImplTraitCase::new("std::ops::Deref", sym::deref, 1, FN_HEADER, SelfKind::Ref, OutType::Ref, true ),
5782-
ShouldImplTraitCase::new("std::ops::DerefMut", sym::deref_mut, 1, FN_HEADER, SelfKind::RefMut, OutType::Ref, true ),
5783-
ShouldImplTraitCase::new("std::ops::Div", sym::div, 2, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5784-
ShouldImplTraitCase::new("std::ops::Drop", sym::drop, 1, FN_HEADER, SelfKind::RefMut, OutType::Unit, true ),
5785-
ShouldImplTraitCase::new("std::cmp::PartialEq", sym::eq, 2, FN_HEADER, SelfKind::Ref, OutType::Bool, true ),
5786-
ShouldImplTraitCase::new("std::iter::FromIterator", sym::from_iter, 1, FN_HEADER, SelfKind::No, OutType::Any, true ),
5787-
ShouldImplTraitCase::new("std::str::FromStr", sym::from_str, 1, FN_HEADER, SelfKind::No, OutType::Any, true ),
5788-
ShouldImplTraitCase::new("std::hash::Hash", sym::hash, 2, FN_HEADER, SelfKind::Ref, OutType::Unit, true ),
5789-
ShouldImplTraitCase::new("std::ops::Index", sym::index, 2, FN_HEADER, SelfKind::Ref, OutType::Ref, true ),
5790-
ShouldImplTraitCase::new("std::ops::IndexMut", sym::index_mut, 2, FN_HEADER, SelfKind::RefMut, OutType::Ref, true ),
5791-
ShouldImplTraitCase::new("std::iter::IntoIterator", sym::into_iter, 1, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5792-
ShouldImplTraitCase::new("std::ops::Mul", sym::mul, 2, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5793-
ShouldImplTraitCase::new("std::ops::Neg", sym::neg, 1, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5794-
ShouldImplTraitCase::new("std::iter::Iterator", sym::next, 1, FN_HEADER, SelfKind::RefMut, OutType::Any, false),
5795-
ShouldImplTraitCase::new("std::ops::Not", sym::not, 1, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5796-
ShouldImplTraitCase::new("std::ops::Rem", sym::rem, 2, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5797-
ShouldImplTraitCase::new("std::ops::Shl", sym::shl, 2, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5798-
ShouldImplTraitCase::new("std::ops::Shr", sym::shr, 2, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5799-
ShouldImplTraitCase::new("std::ops::Sub", sym::sub, 2, FN_HEADER, SelfKind::Value, OutType::Any, true ),
5800-
];
5801-
5802-
#[derive(Clone, Copy, PartialEq, Eq, Debug)]
5803-
enum SelfKind {
5804-
Value,
5805-
Ref,
5806-
RefMut,
5807-
No, // When we want the first argument type to be different than `Self`
5808-
}
5809-
5810-
impl SelfKind {
5811-
fn matches<'a>(self, cx: &LateContext<'a>, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool {
5812-
fn matches_value<'a>(cx: &LateContext<'a>, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool {
5813-
if ty == parent_ty {
5814-
true
5815-
} else if let Some(boxed_ty) = ty.boxed_ty() {
5816-
boxed_ty == parent_ty
5817-
} else if let ty::Adt(adt, args) = ty.kind()
5818-
&& matches!(cx.tcx.get_diagnostic_name(adt.did()), Some(sym::Rc | sym::Arc))
5819-
{
5820-
args.types().next() == Some(parent_ty)
5821-
} else {
5822-
false
5823-
}
5824-
}
5825-
5826-
fn matches_ref<'a>(cx: &LateContext<'a>, mutability: hir::Mutability, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool {
5827-
if let ty::Ref(_, t, m) = *ty.kind() {
5828-
return m == mutability && t == parent_ty;
5829-
}
5830-
5831-
let trait_sym = match mutability {
5832-
hir::Mutability::Not => sym::AsRef,
5833-
hir::Mutability::Mut => sym::AsMut,
5834-
};
5835-
5836-
let Some(trait_def_id) = cx.tcx.get_diagnostic_item(trait_sym) else {
5837-
return false;
5838-
};
5839-
implements_trait(cx, ty, trait_def_id, &[parent_ty.into()])
5840-
}
5841-
5842-
fn matches_none<'a>(cx: &LateContext<'a>, parent_ty: Ty<'a>, ty: Ty<'a>) -> bool {
5843-
!matches_value(cx, parent_ty, ty)
5844-
&& !matches_ref(cx, hir::Mutability::Not, parent_ty, ty)
5845-
&& !matches_ref(cx, hir::Mutability::Mut, parent_ty, ty)
5846-
}
5847-
5848-
match self {
5849-
Self::Value => matches_value(cx, parent_ty, ty),
5850-
Self::Ref => matches_ref(cx, hir::Mutability::Not, parent_ty, ty) || ty == parent_ty && is_copy(cx, ty),
5851-
Self::RefMut => matches_ref(cx, hir::Mutability::Mut, parent_ty, ty),
5852-
Self::No => matches_none(cx, parent_ty, ty),
5853-
}
5854-
}
5855-
5856-
#[must_use]
5857-
fn description(self) -> &'static str {
5858-
match self {
5859-
Self::Value => "`self` by value",
5860-
Self::Ref => "`self` by reference",
5861-
Self::RefMut => "`self` by mutable reference",
5862-
Self::No => "no `self`",
5863-
}
5864-
}
5865-
}
5866-
5867-
#[derive(Clone, Copy)]
5868-
enum OutType {
5869-
Unit,
5870-
Bool,
5871-
Any,
5872-
Ref,
5873-
}
5874-
5875-
impl OutType {
5876-
fn matches(self, ty: &hir::FnRetTy<'_>) -> bool {
5877-
let is_unit = |ty: &hir::Ty<'_>| matches!(ty.kind, hir::TyKind::Tup(&[]));
5878-
match (self, ty) {
5879-
(Self::Unit, &hir::FnRetTy::DefaultReturn(_)) => true,
5880-
(Self::Unit, &hir::FnRetTy::Return(ty)) if is_unit(ty) => true,
5881-
(Self::Bool, &hir::FnRetTy::Return(ty)) if is_bool(ty) => true,
5882-
(Self::Any, &hir::FnRetTy::Return(ty)) if !is_unit(ty) => true,
5883-
(Self::Ref, &hir::FnRetTy::Return(ty)) => matches!(ty.kind, hir::TyKind::Ref(_, _)),
5884-
_ => false,
5885-
}
5886-
}
5887-
}
5888-
5889-
fn fn_header_equals(expected: hir::FnHeader, actual: hir::FnHeader) -> bool {
5890-
expected.constness == actual.constness && expected.safety == actual.safety && expected.asyncness == actual.asyncness
5891-
}

0 commit comments

Comments
 (0)