Skip to content

Commit 3bd9889

Browse files
borsbors
committed
Auto merge of rust-lang#5725 - montrivo:should-impl-trait, r=flip1995
should_impl_trait - ignore methods with lifetime params Fixes: rust-lang#5617 changelog: don't lint should_implement_trait when an `Iterator::next` case has explicit parameters
2 parents 78857cc + f9ba829 commit 3bd9889

File tree

8 files changed

+674
-129
lines changed

8 files changed

+674
-129
lines changed

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 104 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1500,6 +1500,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
15001500
}
15011501
}
15021502

1503+
#[allow(clippy::too_many_lines)]
15031504
fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx hir::ImplItem<'_>) {
15041505
if in_external_macro(cx.sess(), impl_item.span) {
15051506
return;
@@ -1525,16 +1526,31 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
15251526

15261527
then {
15271528
if cx.access_levels.is_exported(impl_item.hir_id) {
1528-
// check missing trait implementations
1529-
for &(method_name, n_args, fn_header, self_kind, out_type, trait_name) in &TRAIT_METHODS {
1530-
if name == method_name &&
1531-
sig.decl.inputs.len() == n_args &&
1532-
out_type.matches(cx, &sig.decl.output) &&
1533-
self_kind.matches(cx, self_ty, first_arg_ty) &&
1534-
fn_header_equals(*fn_header, sig.header) {
1535-
span_lint(cx, SHOULD_IMPLEMENT_TRAIT, impl_item.span, &format!(
1536-
"defining a method called `{}` on this type; consider implementing \
1537-
the `{}` trait or choosing a less ambiguous name", name, trait_name));
1529+
// check missing trait implementations
1530+
for method_config in &TRAIT_METHODS {
1531+
if name == method_config.method_name &&
1532+
sig.decl.inputs.len() == method_config.param_count &&
1533+
method_config.output_type.matches(cx, &sig.decl.output) &&
1534+
method_config.self_kind.matches(cx, self_ty, first_arg_ty) &&
1535+
fn_header_equals(method_config.fn_header, sig.header) &&
1536+
method_config.lifetime_param_cond(&impl_item)
1537+
{
1538+
span_lint_and_help(
1539+
cx,
1540+
SHOULD_IMPLEMENT_TRAIT,
1541+
impl_item.span,
1542+
&format!(
1543+
"method `{}` can be confused for the standard trait method `{}::{}`",
1544+
method_config.method_name,
1545+
method_config.trait_name,
1546+
method_config.method_name
1547+
),
1548+
None,
1549+
&format!(
1550+
"consider implementing the trait `{}` or choosing a less ambiguous method name",
1551+
method_config.trait_name
1552+
)
1553+
);
15381554
}
15391555
}
15401556
}
@@ -3464,38 +3480,85 @@ const FN_HEADER: hir::FnHeader = hir::FnHeader {
34643480
abi: rustc_target::spec::abi::Abi::Rust,
34653481
};
34663482

3483+
struct ShouldImplTraitCase {
3484+
trait_name: &'static str,
3485+
method_name: &'static str,
3486+
param_count: usize,
3487+
fn_header: hir::FnHeader,
3488+
// implicit self kind expected (none, self, &self, ...)
3489+
self_kind: SelfKind,
3490+
// checks against the output type
3491+
output_type: OutType,
3492+
// certain methods with explicit lifetimes can't implement the equivalent trait method
3493+
lint_explicit_lifetime: bool,
3494+
}
3495+
impl ShouldImplTraitCase {
3496+
const fn new(
3497+
trait_name: &'static str,
3498+
method_name: &'static str,
3499+
param_count: usize,
3500+
fn_header: hir::FnHeader,
3501+
self_kind: SelfKind,
3502+
output_type: OutType,
3503+
lint_explicit_lifetime: bool,
3504+
) -> ShouldImplTraitCase {
3505+
ShouldImplTraitCase {
3506+
trait_name,
3507+
method_name,
3508+
param_count,
3509+
fn_header,
3510+
self_kind,
3511+
output_type,
3512+
lint_explicit_lifetime,
3513+
}
3514+
}
3515+
3516+
fn lifetime_param_cond(&self, impl_item: &hir::ImplItem<'_>) -> bool {
3517+
self.lint_explicit_lifetime
3518+
|| !impl_item.generics.params.iter().any(|p| {
3519+
matches!(
3520+
p.kind,
3521+
hir::GenericParamKind::Lifetime {
3522+
kind: hir::LifetimeParamKind::Explicit
3523+
}
3524+
)
3525+
})
3526+
}
3527+
}
3528+
34673529
#[rustfmt::skip]
3468-
const TRAIT_METHODS: [(&str, usize, &hir::FnHeader, SelfKind, OutType, &str); 30] = [
3469-
("add", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Add"),
3470-
("as_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
3471-
("as_ref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
3472-
("bitand", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitAnd"),
3473-
("bitor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitOr"),
3474-
("bitxor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitXor"),
3475-
("borrow", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"),
3476-
("borrow_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"),
3477-
("clone", 1, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
3478-
("cmp", 2, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::cmp::Ord"),
3479-
("default", 0, &FN_HEADER, SelfKind::No, OutType::Any, "std::default::Default"),
3480-
("deref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
3481-
("deref_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
3482-
("div", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Div"),
3483-
("drop", 1, &FN_HEADER, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
3484-
("eq", 2, &FN_HEADER, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
3485-
("from_iter", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::iter::FromIterator"),
3486-
("from_str", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::str::FromStr"),
3487-
("hash", 2, &FN_HEADER, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
3488-
("index", 2, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
3489-
("index_mut", 2, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
3490-
("into_iter", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
3491-
("mul", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Mul"),
3492-
("neg", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Neg"),
3493-
("next", 1, &FN_HEADER, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
3494-
("not", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Not"),
3495-
("rem", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Rem"),
3496-
("shl", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shl"),
3497-
("shr", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shr"),
3498-
("sub", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Sub"),
3530+
const TRAIT_METHODS: [ShouldImplTraitCase; 30] = [
3531+
ShouldImplTraitCase::new("std::ops::Add", "add", 2, FN_HEADER, SelfKind::Value, OutType::Any, true),
3532+
ShouldImplTraitCase::new("std::convert::AsMut", "as_mut", 1, FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3533+
ShouldImplTraitCase::new("std::convert::AsRef", "as_ref", 1, FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3534+
ShouldImplTraitCase::new("std::ops::BitAnd", "bitand", 2, FN_HEADER, SelfKind::Value, OutType::Any, true),
3535+
ShouldImplTraitCase::new("std::ops::BitOr", "bitor", 2, FN_HEADER, SelfKind::Value, OutType::Any, true),
3536+
ShouldImplTraitCase::new("std::ops::BitXor", "bitxor", 2, FN_HEADER, SelfKind::Value, OutType::Any, true),
3537+
ShouldImplTraitCase::new("std::borrow::Borrow", "borrow", 1, FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3538+
ShouldImplTraitCase::new("std::borrow::BorrowMut", "borrow_mut", 1, FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3539+
ShouldImplTraitCase::new("std::clone::Clone", "clone", 1, FN_HEADER, SelfKind::Ref, OutType::Any, true),
3540+
ShouldImplTraitCase::new("std::cmp::Ord", "cmp", 2, FN_HEADER, SelfKind::Ref, OutType::Any, true),
3541+
// FIXME: default doesn't work
3542+
ShouldImplTraitCase::new("std::default::Default", "default", 0, FN_HEADER, SelfKind::No, OutType::Any, true),
3543+
ShouldImplTraitCase::new("std::ops::Deref", "deref", 1, FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3544+
ShouldImplTraitCase::new("std::ops::DerefMut", "deref_mut", 1, FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3545+
ShouldImplTraitCase::new("std::ops::Div", "div", 2, FN_HEADER, SelfKind::Value, OutType::Any, true),
3546+
ShouldImplTraitCase::new("std::ops::Drop", "drop", 1, FN_HEADER, SelfKind::RefMut, OutType::Unit, true),
3547+
ShouldImplTraitCase::new("std::cmp::PartialEq", "eq", 2, FN_HEADER, SelfKind::Ref, OutType::Bool, true),
3548+
ShouldImplTraitCase::new("std::iter::FromIterator", "from_iter", 1, FN_HEADER, SelfKind::No, OutType::Any, true),
3549+
ShouldImplTraitCase::new("std::str::FromStr", "from_str", 1, FN_HEADER, SelfKind::No, OutType::Any, true),
3550+
ShouldImplTraitCase::new("std::hash::Hash", "hash", 2, FN_HEADER, SelfKind::Ref, OutType::Unit, true),
3551+
ShouldImplTraitCase::new("std::ops::Index", "index", 2, FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3552+
ShouldImplTraitCase::new("std::ops::IndexMut", "index_mut", 2, FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3553+
ShouldImplTraitCase::new("std::iter::IntoIterator", "into_iter", 1, FN_HEADER, SelfKind::Value, OutType::Any, true),
3554+
ShouldImplTraitCase::new("std::ops::Mul", "mul", 2, FN_HEADER, SelfKind::Value, OutType::Any, true),
3555+
ShouldImplTraitCase::new("std::ops::Neg", "neg", 1, FN_HEADER, SelfKind::Value, OutType::Any, true),
3556+
ShouldImplTraitCase::new("std::iter::Iterator", "next", 1, FN_HEADER, SelfKind::RefMut, OutType::Any, false),
3557+
ShouldImplTraitCase::new("std::ops::Not", "not", 1, FN_HEADER, SelfKind::Value, OutType::Any, true),
3558+
ShouldImplTraitCase::new("std::ops::Rem", "rem", 2, FN_HEADER, SelfKind::Value, OutType::Any, true),
3559+
ShouldImplTraitCase::new("std::ops::Shl", "shl", 2, FN_HEADER, SelfKind::Value, OutType::Any, true),
3560+
ShouldImplTraitCase::new("std::ops::Shr", "shr", 2, FN_HEADER, SelfKind::Value, OutType::Any, true),
3561+
ShouldImplTraitCase::new("std::ops::Sub", "sub", 2, FN_HEADER, SelfKind::Value, OutType::Any, true),
34993562
];
35003563

35013564
#[rustfmt::skip]

‎tests/ui/methods.rs‎

Lines changed: 3 additions & 65 deletions
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‎

Lines changed: 13 additions & 23 deletions
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)