Skip to content

Commit 9df7e19

Browse files
borsbors
committed
Auto merge of #5725 - montrivo:should-impl-trait, r=flip1995,yaahc
should_impl_trait - ignore methods with lifetime params Fixes: #5617 changelog: don't lint should_implement_trait when an `Iterator::next` case has explicit parameters
2 parents bbbc973 + 108032e commit 9df7e19

File tree

8 files changed

+673
-129
lines changed

8 files changed

+673
-129
lines changed

‎clippy_lints/src/methods/mod.rs

+103-41
Original file line numberDiff line numberDiff line change
@@ -1496,16 +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-
if name == method_name &&
1502-
sig.decl.inputs.len() == n_args &&
1503-
out_type.matches(cx, &sig.decl.output) &&
1504-
self_kind.matches(cx, self_ty, first_arg_ty) &&
1505-
fn_header_equals(*fn_header, sig.header) {
1506-
span_lint(cx, SHOULD_IMPLEMENT_TRAIT, impl_item.span, &format!(
1507-
"defining a method called `{}` on this type; consider implementing \
1508-
the `{}` trait or choosing a less ambiguous name", name, trait_name));
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)
1507+
{
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+
);
15091524
}
15101525
}
15111526
}
@@ -3390,38 +3405,85 @@ const FN_HEADER: hir::FnHeader = hir::FnHeader {
33903405
abi: rustc_target::spec::abi::Abi::Rust,
33913406
};
33923407

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

34273489
#[rustfmt::skip]

‎tests/ui/methods.rs

+3-65
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
clippy::non_ascii_literal,
1111
clippy::new_without_default,
1212
clippy::needless_pass_by_value,
13+
clippy::needless_lifetimes,
1314
clippy::print_stdout,
1415
clippy::must_use_candidate,
1516
clippy::use_self,
@@ -33,71 +34,6 @@ use std::sync::{self, Arc};
3334

3435
use option_helpers::IteratorFalsePositives;
3536

36-
pub struct T;
37-
38-
impl T {
39-
pub fn add(self, other: T) -> T {
40-
self
41-
}
42-
43-
// no error, not public interface
44-
pub(crate) fn drop(&mut self) {}
45-
46-
// no error, private function
47-
fn neg(self) -> Self {
48-
self
49-
}
50-
51-
// no error, private function
52-
fn eq(&self, other: T) -> bool {
53-
true
54-
}
55-
56-
// No error; self is a ref.
57-
fn sub(&self, other: T) -> &T {
58-
self
59-
}
60-
61-
// No error; different number of arguments.
62-
fn div(self) -> T {
63-
self
64-
}
65-
66-
// No error; wrong return type.
67-
fn rem(self, other: T) {}
68-
69-
// Fine
70-
fn into_u32(self) -> u32 {
71-
0
72-
}
73-
74-
fn into_u16(&self) -> u16 {
75-
0
76-
}
77-
78-
fn to_something(self) -> u32 {
79-
0
80-
}
81-
82-
fn new(self) -> Self {
83-
unimplemented!();
84-
}
85-
}
86-
87-
pub struct T1;
88-
89-
impl T1 {
90-
// Shouldn't trigger lint as it is unsafe.
91-
pub unsafe fn add(self, rhs: T1) -> T1 {
92-
self
93-
}
94-
95-
// Should not trigger lint since this is an async function.
96-
pub async fn next(&mut self) -> Option<T1> {
97-
None
98-
}
99-
}
100-
10137
struct Lt<'a> {
10238
foo: &'a u32,
10339
}
@@ -171,6 +107,8 @@ impl BadNew {
171107
}
172108
}
173109

110+
struct T;
111+
174112
impl Mul<T> for T {
175113
type Output = T;
176114
// No error, obviously.

‎tests/ui/methods.stderr

+13-23
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,5 @@
1-
error: defining a method called `add` on this type; consider implementing the `std::ops::Add` trait or choosing a less ambiguous name
2-
--> $DIR/methods.rs:39:5
3-
|
4-
LL | / pub fn add(self, other: T) -> T {
5-
LL | | self
6-
LL | | }
7-
| |_____^
8-
|
9-
= note: `-D clippy::should-implement-trait` implied by `-D warnings`
10-
111
error: methods called `new` usually return `Self`
12-
--> $DIR/methods.rs:169:5
2+
--> $DIR/methods.rs:105:5
133
|
144
LL | / fn new() -> i32 {
155
LL | | 0
@@ -19,7 +9,7 @@ LL | | }
199
= note: `-D clippy::new-ret-no-self` implied by `-D warnings`
2010

2111
error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead.
22-
--> $DIR/methods.rs:188:13
12+
--> $DIR/methods.rs:126:13
2313
|
2414
LL | let _ = v.iter().filter(|&x| *x < 0).next();
2515
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -28,7 +18,7 @@ LL | let _ = v.iter().filter(|&x| *x < 0).next();
2818
= note: replace `filter(|&x| *x < 0).next()` with `find(|&x| *x < 0)`
2919

3020
error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead.
31-
--> $DIR/methods.rs:191:13
21+
--> $DIR/methods.rs:129:13
3222
|
3323
LL | let _ = v.iter().filter(|&x| {
3424
| _____________^
@@ -38,33 +28,33 @@ LL | | ).next();
3828
| |___________________________^
3929

4030
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
41-
--> $DIR/methods.rs:208:22
31+
--> $DIR/methods.rs:146:22
4232
|
4333
LL | let _ = v.iter().find(|&x| *x < 0).is_some();
4434
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x < 0)`
4535
|
4636
= note: `-D clippy::search-is-some` implied by `-D warnings`
4737

4838
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
49-
--> $DIR/methods.rs:209:20
39+
--> $DIR/methods.rs:147:20
5040
|
5141
LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less
5242
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| **y == x)`
5343

5444
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
55-
--> $DIR/methods.rs:210:20
45+
--> $DIR/methods.rs:148:20
5646
|
5747
LL | let _ = (0..1).find(|x| *x == 0).is_some();
5848
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| x == 0)`
5949

6050
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
61-
--> $DIR/methods.rs:211:22
51+
--> $DIR/methods.rs:149:22
6252
|
6353
LL | let _ = v.iter().find(|x| **x == 0).is_some();
6454
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x == 0)`
6555

6656
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
67-
--> $DIR/methods.rs:214:13
57+
--> $DIR/methods.rs:152:13
6858
|
6959
LL | let _ = v.iter().find(|&x| {
7060
| _____________^
@@ -74,13 +64,13 @@ LL | | ).is_some();
7464
| |______________________________^
7565

7666
error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
77-
--> $DIR/methods.rs:220:22
67+
--> $DIR/methods.rs:158:22
7868
|
7969
LL | let _ = v.iter().position(|&x| x < 0).is_some();
8070
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)`
8171

8272
error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
83-
--> $DIR/methods.rs:223:13
73+
--> $DIR/methods.rs:161:13
8474
|
8575
LL | let _ = v.iter().position(|&x| {
8676
| _____________^
@@ -90,13 +80,13 @@ LL | | ).is_some();
9080
| |______________________________^
9181

9282
error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
93-
--> $DIR/methods.rs:229:22
83+
--> $DIR/methods.rs:167:22
9484
|
9585
LL | let _ = v.iter().rposition(|&x| x < 0).is_some();
9686
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)`
9787

9888
error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
99-
--> $DIR/methods.rs:232:13
89+
--> $DIR/methods.rs:170:13
10090
|
10191
LL | let _ = v.iter().rposition(|&x| {
10292
| _____________^
@@ -105,5 +95,5 @@ LL | | }
10595
LL | | ).is_some();
10696
| |______________________________^
10797

108-
error: aborting due to 13 previous errors
98+
error: aborting due to 12 previous errors
10999

0 commit comments

Comments
 (0)