Skip to content

Commit 940bbd6

Browse files
borsbors
committed
Auto merge of #5437 - rabisg0:should-impl-trait, r=flip1995
Check fn header along with decl when suggesting to implement trait When checking for functions that are potential candidates for trait implementations check the function header to make sure modifiers like asyncness, constness and safety match before triggering the lint. Fixes #5413, #4290 changelog: check fn header along with decl for should_implement_trait
2 parents 7bfdee5 + c2e5534 commit 940bbd6

File tree

3 files changed

+77
-48
lines changed

3 files changed

+77
-48
lines changed

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 49 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1453,11 +1453,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
14531453
then {
14541454
if cx.access_levels.is_exported(impl_item.hir_id) {
14551455
// check missing trait implementations
1456-
for &(method_name, n_args, self_kind, out_type, trait_name) in &TRAIT_METHODS {
1456+
for &(method_name, n_args, fn_header, self_kind, out_type, trait_name) in &TRAIT_METHODS {
14571457
if name == method_name &&
1458-
sig.decl.inputs.len() == n_args &&
1459-
out_type.matches(cx, &sig.decl.output) &&
1460-
self_kind.matches(cx, self_ty, first_arg_ty) {
1458+
sig.decl.inputs.len() == n_args &&
1459+
out_type.matches(cx, &sig.decl.output) &&
1460+
self_kind.matches(cx, self_ty, first_arg_ty) &&
1461+
fn_header_equals(*fn_header, sig.header) {
14611462
span_lint(cx, SHOULD_IMPLEMENT_TRAIT, impl_item.span, &format!(
14621463
"defining a method called `{}` on this type; consider implementing \
14631464
the `{}` trait or choosing a less ambiguous name", name, trait_name));
@@ -3352,38 +3353,45 @@ const CONVENTIONS: [(Convention, &[SelfKind]); 7] = [
33523353
(Convention::StartsWith("to_"), &[SelfKind::Ref]),
33533354
];
33543355

3356+
const FN_HEADER: hir::FnHeader = hir::FnHeader {
3357+
unsafety: hir::Unsafety::Normal,
3358+
constness: hir::Constness::NotConst,
3359+
asyncness: hir::IsAsync::NotAsync,
3360+
abi: rustc_target::spec::abi::Abi::Rust,
3361+
};
3362+
33553363
#[rustfmt::skip]
3356-
const TRAIT_METHODS: [(&str, usize, SelfKind, OutType, &str); 30] = [
3357-
("add", 2, SelfKind::Value, OutType::Any, "std::ops::Add"),
3358-
("as_mut", 1, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
3359-
("as_ref", 1, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
3360-
("bitand", 2, SelfKind::Value, OutType::Any, "std::ops::BitAnd"),
3361-
("bitor", 2, SelfKind::Value, OutType::Any, "std::ops::BitOr"),
3362-
("bitxor", 2, SelfKind::Value, OutType::Any, "std::ops::BitXor"),
3363-
("borrow", 1, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"),
3364-
("borrow_mut", 1, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"),
3365-
("clone", 1, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
3366-
("cmp", 2, SelfKind::Ref, OutType::Any, "std::cmp::Ord"),
3367-
("default", 0, SelfKind::No, OutType::Any, "std::default::Default"),
3368-
("deref", 1, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
3369-
("deref_mut", 1, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
3370-
("div", 2, SelfKind::Value, OutType::Any, "std::ops::Div"),
3371-
("drop", 1, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
3372-
("eq", 2, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
3373-
("from_iter", 1, SelfKind::No, OutType::Any, "std::iter::FromIterator"),
3374-
("from_str", 1, SelfKind::No, OutType::Any, "std::str::FromStr"),
3375-
("hash", 2, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
3376-
("index", 2, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
3377-
("index_mut", 2, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
3378-
("into_iter", 1, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
3379-
("mul", 2, SelfKind::Value, OutType::Any, "std::ops::Mul"),
3380-
("neg", 1, SelfKind::Value, OutType::Any, "std::ops::Neg"),
3381-
("next", 1, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
3382-
("not", 1, SelfKind::Value, OutType::Any, "std::ops::Not"),
3383-
("rem", 2, SelfKind::Value, OutType::Any, "std::ops::Rem"),
3384-
("shl", 2, SelfKind::Value, OutType::Any, "std::ops::Shl"),
3385-
("shr", 2, SelfKind::Value, OutType::Any, "std::ops::Shr"),
3386-
("sub", 2, SelfKind::Value, OutType::Any, "std::ops::Sub"),
3364+
const TRAIT_METHODS: [(&str, usize, &hir::FnHeader, SelfKind, OutType, &str); 30] = [
3365+
("add", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Add"),
3366+
("as_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
3367+
("as_ref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
3368+
("bitand", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitAnd"),
3369+
("bitor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitOr"),
3370+
("bitxor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitXor"),
3371+
("borrow", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"),
3372+
("borrow_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"),
3373+
("clone", 1, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
3374+
("cmp", 2, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::cmp::Ord"),
3375+
("default", 0, &FN_HEADER, SelfKind::No, OutType::Any, "std::default::Default"),
3376+
("deref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
3377+
("deref_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
3378+
("div", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Div"),
3379+
("drop", 1, &FN_HEADER, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
3380+
("eq", 2, &FN_HEADER, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
3381+
("from_iter", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::iter::FromIterator"),
3382+
("from_str", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::str::FromStr"),
3383+
("hash", 2, &FN_HEADER, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
3384+
("index", 2, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
3385+
("index_mut", 2, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
3386+
("into_iter", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
3387+
("mul", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Mul"),
3388+
("neg", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Neg"),
3389+
("next", 1, &FN_HEADER, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
3390+
("not", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Not"),
3391+
("rem", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Rem"),
3392+
("shl", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shl"),
3393+
("shr", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shr"),
3394+
("sub", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Sub"),
33873395
];
33883396

33893397
#[rustfmt::skip]
@@ -3596,3 +3604,9 @@ fn lint_filetype_is_file(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, args: &
35963604
let help_msg = format!("use `{}FileType::is_dir()` instead", help_unary);
35973605
span_lint_and_help(cx, FILETYPE_IS_FILE, span, &lint_msg, &help_msg);
35983606
}
3607+
3608+
fn fn_header_equals(expected: hir::FnHeader, actual: hir::FnHeader) -> bool {
3609+
expected.constness == actual.constness
3610+
&& expected.unsafety == actual.unsafety
3611+
&& expected.asyncness == actual.asyncness
3612+
}

‎tests/ui/methods.rs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
clippy::blacklisted_name,
77
clippy::default_trait_access,
88
clippy::missing_docs_in_private_items,
9+
clippy::missing_safety_doc,
910
clippy::non_ascii_literal,
1011
clippy::new_without_default,
1112
clippy::needless_pass_by_value,
@@ -83,6 +84,20 @@ impl T {
8384
}
8485
}
8586

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+
86101
struct Lt<'a> {
87102
foo: &'a u32,
88103
}

‎tests/ui/methods.stderr‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
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:38:5
2+
--> $DIR/methods.rs:39:5
33
|
44
LL | / pub fn add(self, other: T) -> T {
55
LL | | self
@@ -9,7 +9,7 @@ LL | | }
99
= note: `-D clippy::should-implement-trait` implied by `-D warnings`
1010

1111
error: methods called `new` usually return `Self`
12-
--> $DIR/methods.rs:154:5
12+
--> $DIR/methods.rs:169:5
1313
|
1414
LL | / fn new() -> i32 {
1515
LL | | 0
@@ -19,7 +19,7 @@ LL | | }
1919
= note: `-D clippy::new-ret-no-self` implied by `-D warnings`
2020

2121
error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead.
22-
--> $DIR/methods.rs:173:13
22+
--> $DIR/methods.rs:188:13
2323
|
2424
LL | let _ = v.iter().filter(|&x| *x < 0).next();
2525
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -28,7 +28,7 @@ LL | let _ = v.iter().filter(|&x| *x < 0).next();
2828
= note: replace `filter(|&x| *x < 0).next()` with `find(|&x| *x < 0)`
2929

3030
error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead.
31-
--> $DIR/methods.rs:176:13
31+
--> $DIR/methods.rs:191:13
3232
|
3333
LL | let _ = v.iter().filter(|&x| {
3434
| _____________^
@@ -38,33 +38,33 @@ LL | | ).next();
3838
| |___________________________^
3939

4040
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
41-
--> $DIR/methods.rs:193:22
41+
--> $DIR/methods.rs:208:22
4242
|
4343
LL | let _ = v.iter().find(|&x| *x < 0).is_some();
4444
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x < 0)`
4545
|
4646
= note: `-D clippy::search-is-some` implied by `-D warnings`
4747

4848
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
49-
--> $DIR/methods.rs:194:20
49+
--> $DIR/methods.rs:209:20
5050
|
5151
LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less
5252
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| **y == x)`
5353

5454
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
55-
--> $DIR/methods.rs:195:20
55+
--> $DIR/methods.rs:210:20
5656
|
5757
LL | let _ = (0..1).find(|x| *x == 0).is_some();
5858
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| x == 0)`
5959

6060
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
61-
--> $DIR/methods.rs:196:22
61+
--> $DIR/methods.rs:211:22
6262
|
6363
LL | let _ = v.iter().find(|x| **x == 0).is_some();
6464
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x == 0)`
6565

6666
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
67-
--> $DIR/methods.rs:199:13
67+
--> $DIR/methods.rs:214:13
6868
|
6969
LL | let _ = v.iter().find(|&x| {
7070
| _____________^
@@ -74,13 +74,13 @@ LL | | ).is_some();
7474
| |______________________________^
7575

7676
error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
77-
--> $DIR/methods.rs:205:22
77+
--> $DIR/methods.rs:220:22
7878
|
7979
LL | let _ = v.iter().position(|&x| x < 0).is_some();
8080
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)`
8181

8282
error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
83-
--> $DIR/methods.rs:208:13
83+
--> $DIR/methods.rs:223:13
8484
|
8585
LL | let _ = v.iter().position(|&x| {
8686
| _____________^
@@ -90,13 +90,13 @@ LL | | ).is_some();
9090
| |______________________________^
9191

9292
error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
93-
--> $DIR/methods.rs:214:22
93+
--> $DIR/methods.rs:229:22
9494
|
9595
LL | let _ = v.iter().rposition(|&x| x < 0).is_some();
9696
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)`
9797

9898
error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
99-
--> $DIR/methods.rs:217:13
99+
--> $DIR/methods.rs:232:13
100100
|
101101
LL | let _ = v.iter().rposition(|&x| {
102102
| _____________^

0 commit comments

Comments
 (0)