Skip to content

Commit 108032e

Browse files
tnielenstnielens
committed
should_impl_trait - pr comments
1 parent d7e3f34 commit 108032e

File tree

8 files changed

+670
-501
lines changed

8 files changed

+670
-501
lines changed

‎clippy_lints/src/methods/mod.rs

Lines changed: 101 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1496,25 +1496,31 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
14961496

14971497
then {
14981498
if cx.access_levels.is_exported(impl_item.hir_id) {
1499-
// check missing trait implementations
1500-
for &(method_name, n_args, fn_header, self_kind, out_type, trait_name) in &TRAIT_METHODS {
1501-
let no_lifetime_params = || {
1502-
!impl_item.generics.params.iter()
1503-
.any(|p| matches!(
1504-
p.kind,
1505-
hir::GenericParamKind::Lifetime { kind: hir::LifetimeParamKind::Explicit }))
1506-
};
1507-
if name == method_name &&
1508-
sig.decl.inputs.len() == n_args &&
1509-
out_type.matches(cx, &sig.decl.output) &&
1510-
self_kind.matches(cx, self_ty, first_arg_ty) &&
1511-
fn_header_equals(*fn_header, sig.header) &&
1512-
// ignore methods with lifetime params, risk of false positive
1513-
no_lifetime_params()
1499+
// check missing trait implementations
1500+
for method_config in &TRAIT_METHODS {
1501+
if name == method_config.method_name &&
1502+
sig.decl.inputs.len() == method_config.param_count &&
1503+
method_config.output_type.matches(cx, &sig.decl.output) &&
1504+
method_config.self_kind.matches(cx, self_ty, first_arg_ty) &&
1505+
fn_header_equals(*method_config.fn_header, sig.header) &&
1506+
method_config.lifetime_param_cond(&impl_item)
15141507
{
1515-
span_lint(cx, SHOULD_IMPLEMENT_TRAIT, impl_item.span, &format!(
1516-
"defining a method called `{}` on this type; consider implementing \
1517-
the `{}` trait or choosing a less ambiguous name", name, trait_name));
1508+
span_lint_and_help(
1509+
cx,
1510+
SHOULD_IMPLEMENT_TRAIT,
1511+
impl_item.span,
1512+
&format!(
1513+
"method `{}` can be confused for the standard trait method `{}::{}`",
1514+
method_config.method_name,
1515+
method_config.trait_name,
1516+
method_config.method_name
1517+
),
1518+
None,
1519+
&format!(
1520+
"consider implementing the trait `{}` or choosing a less ambiguous method name",
1521+
method_config.trait_name
1522+
)
1523+
);
15181524
}
15191525
}
15201526
}
@@ -3398,39 +3404,85 @@ const FN_HEADER: hir::FnHeader = hir::FnHeader {
33983404
abi: rustc_target::spec::abi::Abi::Rust,
33993405
};
34003406

3407+
struct ShouldImplTraitCase {
3408+
trait_name: &'static str,
3409+
method_name: &'static str,
3410+
param_count: usize,
3411+
fn_header: &'static hir::FnHeader,
3412+
// implicit self kind expected (none, self, &self, ...)
3413+
self_kind: SelfKind,
3414+
// checks against the output type
3415+
output_type: OutType,
3416+
// certain methods with explicit lifetimes can't implement the equivalent trait method
3417+
lint_explicit_lifetime: bool,
3418+
}
3419+
impl ShouldImplTraitCase {
3420+
const fn new(
3421+
trait_name: &'static str,
3422+
method_name: &'static str,
3423+
param_count: usize,
3424+
fn_header: &'static hir::FnHeader,
3425+
self_kind: SelfKind,
3426+
output_type: OutType,
3427+
lint_explicit_lifetime: bool,
3428+
) -> ShouldImplTraitCase {
3429+
ShouldImplTraitCase {
3430+
trait_name,
3431+
method_name,
3432+
param_count,
3433+
fn_header,
3434+
self_kind,
3435+
output_type,
3436+
lint_explicit_lifetime,
3437+
}
3438+
}
3439+
3440+
fn lifetime_param_cond(&self, impl_item: &hir::ImplItem<'_>) -> bool {
3441+
self.lint_explicit_lifetime
3442+
|| !impl_item.generics.params.iter().any(|p| {
3443+
matches!(
3444+
p.kind,
3445+
hir::GenericParamKind::Lifetime {
3446+
kind: hir::LifetimeParamKind::Explicit
3447+
}
3448+
)
3449+
})
3450+
}
3451+
}
3452+
34013453
#[rustfmt::skip]
3402-
const TRAIT_METHODS: [(&str, usize, &hir::FnHeader, SelfKind, OutType, &str); 30] = [
3403-
("add", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Add"),
3404-
("as_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
3405-
("as_ref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
3406-
("bitand", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitAnd"),
3407-
("bitor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitOr"),
3408-
("bitxor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitXor"),
3409-
("borrow", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"),
3410-
("borrow_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"),
3411-
("clone", 1, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
3412-
("cmp", 2, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::cmp::Ord"),
3454+
const TRAIT_METHODS: [ShouldImplTraitCase; 30] = [
3455+
ShouldImplTraitCase::new("std::ops::Add", "add", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3456+
ShouldImplTraitCase::new("std::convert::AsMut", "as_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3457+
ShouldImplTraitCase::new("std::convert::AsRef", "as_ref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3458+
ShouldImplTraitCase::new("std::ops::BitAnd", "bitand", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3459+
ShouldImplTraitCase::new("std::ops::BitOr", "bitor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3460+
ShouldImplTraitCase::new("std::ops::BitXor", "bitxor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3461+
ShouldImplTraitCase::new("std::borrow::Borrow", "borrow", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3462+
ShouldImplTraitCase::new("std::borrow::BorrowMut", "borrow_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3463+
ShouldImplTraitCase::new("std::clone::Clone", "clone", 1, &FN_HEADER, SelfKind::Ref, OutType::Any, true),
3464+
ShouldImplTraitCase::new("std::cmp::Ord", "cmp", 2, &FN_HEADER, SelfKind::Ref, OutType::Any, true),
34133465
// FIXME: default doesn't work
3414-
("default", 0, &FN_HEADER, SelfKind::No, OutType::Any, "std::default::Default"),
3415-
("deref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
3416-
("deref_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
3417-
("div", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Div"),
3418-
("drop", 1, &FN_HEADER, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
3419-
("eq", 2, &FN_HEADER, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
3420-
("from_iter", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::iter::FromIterator"),
3421-
("from_str", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::str::FromStr"),
3422-
("hash", 2, &FN_HEADER, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
3423-
("index", 2, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
3424-
("index_mut", 2, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
3425-
("into_iter", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
3426-
("mul", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Mul"),
3427-
("neg", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Neg"),
3428-
("next", 1, &FN_HEADER, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
3429-
("not", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Not"),
3430-
("rem", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Rem"),
3431-
("shl", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shl"),
3432-
("shr", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shr"),
3433-
("sub", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Sub"),
3466+
ShouldImplTraitCase::new("std::default::Default", "default", 0, &FN_HEADER, SelfKind::No, OutType::Any, true),
3467+
ShouldImplTraitCase::new("std::ops::Deref", "deref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3468+
ShouldImplTraitCase::new("std::ops::DerefMut", "deref_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3469+
ShouldImplTraitCase::new("std::ops::Div", "div", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3470+
ShouldImplTraitCase::new("std::ops::Drop", "drop", 1, &FN_HEADER, SelfKind::RefMut, OutType::Unit, true),
3471+
ShouldImplTraitCase::new("std::cmp::PartialEq", "eq", 2, &FN_HEADER, SelfKind::Ref, OutType::Bool, true),
3472+
ShouldImplTraitCase::new("std::iter::FromIterator", "from_iter", 1, &FN_HEADER, SelfKind::No, OutType::Any, true),
3473+
ShouldImplTraitCase::new("std::str::FromStr", "from_str", 1, &FN_HEADER, SelfKind::No, OutType::Any, true),
3474+
ShouldImplTraitCase::new("std::hash::Hash", "hash", 2, &FN_HEADER, SelfKind::Ref, OutType::Unit, true),
3475+
ShouldImplTraitCase::new("std::ops::Index", "index", 2, &FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3476+
ShouldImplTraitCase::new("std::ops::IndexMut", "index_mut", 2, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3477+
ShouldImplTraitCase::new("std::iter::IntoIterator", "into_iter", 1, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3478+
ShouldImplTraitCase::new("std::ops::Mul", "mul", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3479+
ShouldImplTraitCase::new("std::ops::Neg", "neg", 1, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3480+
ShouldImplTraitCase::new("std::iter::Iterator", "next", 1, &FN_HEADER, SelfKind::RefMut, OutType::Any, false),
3481+
ShouldImplTraitCase::new("std::ops::Not", "not", 1, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3482+
ShouldImplTraitCase::new("std::ops::Rem", "rem", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3483+
ShouldImplTraitCase::new("std::ops::Shl", "shl", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3484+
ShouldImplTraitCase::new("std::ops::Shr", "shr", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3485+
ShouldImplTraitCase::new("std::ops::Sub", "sub", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
34343486
];
34353487

34363488
#[rustfmt::skip]

‎tests/ui/methods.rs

Lines changed: 2 additions & 195 deletions
Original file line numberDiff line numberDiff line change
@@ -34,201 +34,6 @@ use std::sync::{self, Arc};
3434

3535
use option_helpers::IteratorFalsePositives;
3636

37-
pub struct T;
38-
39-
impl T {
40-
// *******************************************
41-
// complete trait method list, should lint all
42-
// *******************************************
43-
pub fn add(self, other: T) -> T {
44-
unimplemented!()
45-
}
46-
47-
pub fn as_mut(&mut self) -> &mut T {
48-
unimplemented!()
49-
}
50-
51-
pub fn as_ref(&self) -> &T {
52-
unimplemented!()
53-
}
54-
55-
pub fn bitand(self, rhs: T) -> T {
56-
unimplemented!()
57-
}
58-
59-
pub fn bitor(self, rhs: Self) -> Self {
60-
unimplemented!()
61-
}
62-
63-
pub fn bitxor(self, rhs: Self) -> Self {
64-
unimplemented!()
65-
}
66-
67-
pub fn borrow(&self) -> &str {
68-
unimplemented!()
69-
}
70-
71-
pub fn borrow_mut(&mut self) -> &mut str {
72-
unimplemented!()
73-
}
74-
75-
pub fn clone(&self) -> Self {
76-
unimplemented!()
77-
}
78-
79-
pub fn cmp(&self, other: &Self) -> Self {
80-
unimplemented!()
81-
}
82-
83-
pub fn default() -> Self {
84-
unimplemented!()
85-
}
86-
87-
pub fn deref(&self) -> &Self {
88-
unimplemented!()
89-
}
90-
91-
pub fn deref_mut(&mut self) -> &mut Self {
92-
unimplemented!()
93-
}
94-
95-
pub fn div(self, rhs: Self) -> Self {
96-
unimplemented!()
97-
}
98-
99-
pub fn drop(&mut self) {
100-
unimplemented!()
101-
}
102-
103-
pub fn eq(&self, other: &Self) -> bool {
104-
unimplemented!()
105-
}
106-
107-
pub fn from_iter<T>(iter: T) -> Self {
108-
unimplemented!()
109-
}
110-
111-
pub fn from_str(s: &str) -> Result<Self, Self> {
112-
unimplemented!()
113-
}
114-
115-
pub fn hash(&self, state: &mut T) {
116-
unimplemented!()
117-
}
118-
119-
pub fn index(&self, index: usize) -> &Self {
120-
unimplemented!()
121-
}
122-
123-
pub fn index_mut(&mut self, index: usize) -> &mut Self {
124-
unimplemented!()
125-
}
126-
127-
pub fn into_iter(self) -> Self {
128-
unimplemented!()
129-
}
130-
131-
pub fn mul(self, rhs: Self) -> Self {
132-
unimplemented!()
133-
}
134-
135-
pub fn neg(self) -> Self {
136-
unimplemented!()
137-
}
138-
139-
pub fn next(&mut self) -> Option<Self> {
140-
unimplemented!()
141-
}
142-
143-
pub fn not(self) -> Self {
144-
unimplemented!()
145-
}
146-
147-
pub fn rem(self, rhs: Self) -> Self {
148-
unimplemented!()
149-
}
150-
151-
pub fn shl(self, rhs: Self) -> Self {
152-
unimplemented!()
153-
}
154-
155-
pub fn shr(self, rhs: Self) -> Self {
156-
unimplemented!()
157-
}
158-
159-
pub fn sub(self, rhs: Self) -> Self {
160-
unimplemented!()
161-
}
162-
// *****************
163-
// complete list end
164-
// *****************
165-
}
166-
167-
pub struct T1;
168-
impl T1 {
169-
// corner cases: should not lint
170-
171-
// no error, not public interface
172-
pub(crate) fn drop(&mut self) {}
173-
174-
// no error, private function
175-
fn neg(self) -> Self {
176-
self
177-
}
178-
179-
// no error, private function
180-
fn eq(&self, other: Self) -> bool {
181-
true
182-
}
183-
184-
// No error; self is a ref.
185-
fn sub(&self, other: Self) -> &Self {
186-
self
187-
}
188-
189-
// No error; different number of arguments.
190-
fn div(self) -> Self {
191-
self
192-
}
193-
194-
// No error; wrong return type.
195-
fn rem(self, other: Self) {}
196-
197-
// Fine
198-
fn into_u32(self) -> u32 {
199-
0
200-
}
201-
202-
fn into_u16(&self) -> u16 {
203-
0
204-
}
205-
206-
fn to_something(self) -> u32 {
207-
0
208-
}
209-
210-
fn new(self) -> Self {
211-
unimplemented!();
212-
}
213-
214-
pub fn next<'b>(&'b mut self) -> Option<&'b mut T> {
215-
unimplemented!();
216-
}
217-
}
218-
219-
pub struct T2;
220-
impl T2 {
221-
// Shouldn't trigger lint as it is unsafe.
222-
pub unsafe fn add(self, rhs: Self) -> Self {
223-
self
224-
}
225-
226-
// Should not trigger lint since this is an async function.
227-
pub async fn next(&mut self) -> Option<Self> {
228-
None
229-
}
230-
}
231-
23237
struct Lt<'a> {
23338
foo: &'a u32,
23439
}
@@ -302,6 +107,8 @@ impl BadNew {
302107
}
303108
}
304109

110+
struct T;
111+
305112
impl Mul<T> for T {
306113
type Output = T;
307114
// No error, obviously.

0 commit comments

Comments
 (0)