Skip to content

Commit c2e5534

Browse files
rabisg0rabisg0
committed
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
1 parent d342cee commit c2e5534

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
@@ -1426,11 +1426,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
14261426
then {
14271427
if cx.access_levels.is_exported(impl_item.hir_id) {
14281428
// check missing trait implementations
1429-
for &(method_name, n_args, self_kind, out_type, trait_name) in &TRAIT_METHODS {
1429+
for &(method_name, n_args, fn_header, self_kind, out_type, trait_name) in &TRAIT_METHODS {
14301430
if name == method_name &&
1431-
sig.decl.inputs.len() == n_args &&
1432-
out_type.matches(cx, &sig.decl.output) &&
1433-
self_kind.matches(cx, self_ty, first_arg_ty) {
1431+
sig.decl.inputs.len() == n_args &&
1432+
out_type.matches(cx, &sig.decl.output) &&
1433+
self_kind.matches(cx, self_ty, first_arg_ty) &&
1434+
fn_header_equals(*fn_header, sig.header) {
14341435
span_lint(cx, SHOULD_IMPLEMENT_TRAIT, impl_item.span, &format!(
14351436
"defining a method called `{}` on this type; consider implementing \
14361437
the `{}` trait or choosing a less ambiguous name", name, trait_name));
@@ -3266,38 +3267,45 @@ const CONVENTIONS: [(Convention, &[SelfKind]); 7] = [
32663267
(Convention::StartsWith("to_"), &[SelfKind::Ref]),
32673268
];
32683269

3270+
const FN_HEADER: hir::FnHeader = hir::FnHeader {
3271+
unsafety: hir::Unsafety::Normal,
3272+
constness: hir::Constness::NotConst,
3273+
asyncness: hir::IsAsync::NotAsync,
3274+
abi: rustc_target::spec::abi::Abi::Rust,
3275+
};
3276+
32693277
#[rustfmt::skip]
3270-
const TRAIT_METHODS: [(&str, usize, SelfKind, OutType, &str); 30] = [
3271-
("add", 2, SelfKind::Value, OutType::Any, "std::ops::Add"),
3272-
("as_mut", 1, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
3273-
("as_ref", 1, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
3274-
("bitand", 2, SelfKind::Value, OutType::Any, "std::ops::BitAnd"),
3275-
("bitor", 2, SelfKind::Value, OutType::Any, "std::ops::BitOr"),
3276-
("bitxor", 2, SelfKind::Value, OutType::Any, "std::ops::BitXor"),
3277-
("borrow", 1, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"),
3278-
("borrow_mut", 1, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"),
3279-
("clone", 1, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
3280-
("cmp", 2, SelfKind::Ref, OutType::Any, "std::cmp::Ord"),
3281-
("default", 0, SelfKind::No, OutType::Any, "std::default::Default"),
3282-
("deref", 1, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
3283-
("deref_mut", 1, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
3284-
("div", 2, SelfKind::Value, OutType::Any, "std::ops::Div"),
3285-
("drop", 1, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
3286-
("eq", 2, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
3287-
("from_iter", 1, SelfKind::No, OutType::Any, "std::iter::FromIterator"),
3288-
("from_str", 1, SelfKind::No, OutType::Any, "std::str::FromStr"),
3289-
("hash", 2, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
3290-
("index", 2, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
3291-
("index_mut", 2, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
3292-
("into_iter", 1, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
3293-
("mul", 2, SelfKind::Value, OutType::Any, "std::ops::Mul"),
3294-
("neg", 1, SelfKind::Value, OutType::Any, "std::ops::Neg"),
3295-
("next", 1, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
3296-
("not", 1, SelfKind::Value, OutType::Any, "std::ops::Not"),
3297-
("rem", 2, SelfKind::Value, OutType::Any, "std::ops::Rem"),
3298-
("shl", 2, SelfKind::Value, OutType::Any, "std::ops::Shl"),
3299-
("shr", 2, SelfKind::Value, OutType::Any, "std::ops::Shr"),
3300-
("sub", 2, SelfKind::Value, OutType::Any, "std::ops::Sub"),
3278+
const TRAIT_METHODS: [(&str, usize, &hir::FnHeader, SelfKind, OutType, &str); 30] = [
3279+
("add", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Add"),
3280+
("as_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
3281+
("as_ref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
3282+
("bitand", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitAnd"),
3283+
("bitor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitOr"),
3284+
("bitxor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitXor"),
3285+
("borrow", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"),
3286+
("borrow_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"),
3287+
("clone", 1, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
3288+
("cmp", 2, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::cmp::Ord"),
3289+
("default", 0, &FN_HEADER, SelfKind::No, OutType::Any, "std::default::Default"),
3290+
("deref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
3291+
("deref_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
3292+
("div", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Div"),
3293+
("drop", 1, &FN_HEADER, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
3294+
("eq", 2, &FN_HEADER, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
3295+
("from_iter", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::iter::FromIterator"),
3296+
("from_str", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::str::FromStr"),
3297+
("hash", 2, &FN_HEADER, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
3298+
("index", 2, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
3299+
("index_mut", 2, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
3300+
("into_iter", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
3301+
("mul", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Mul"),
3302+
("neg", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Neg"),
3303+
("next", 1, &FN_HEADER, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
3304+
("not", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Not"),
3305+
("rem", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Rem"),
3306+
("shl", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shl"),
3307+
("shr", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shr"),
3308+
("sub", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Sub"),
33013309
];
33023310

33033311
#[rustfmt::skip]
@@ -3510,3 +3518,9 @@ fn lint_filetype_is_file(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, args: &
35103518
let help_msg = format!("use `{}FileType::is_dir()` instead", help_unary);
35113519
span_lint_and_help(cx, FILETYPE_IS_FILE, span, &lint_msg, &help_msg);
35123520
}
3521+
3522+
fn fn_header_equals(expected: hir::FnHeader, actual: hir::FnHeader) -> bool {
3523+
expected.constness == actual.constness
3524+
&& expected.unsafety == actual.unsafety
3525+
&& expected.asyncness == actual.asyncness
3526+
}

‎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)