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

3 files changed, 77 insertions, 48 deletions

diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index 3f49849ba32..c539e0360fb 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -1453,11 +1453,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
             then {
                 if cx.access_levels.is_exported(impl_item.hir_id) {
                 // check missing trait implementations
-                    for &(method_name, n_args, self_kind, out_type, trait_name) in &TRAIT_METHODS {
+                    for &(method_name, n_args, fn_header, self_kind, out_type, trait_name) in &TRAIT_METHODS {
                         if name == method_name &&
-                        sig.decl.inputs.len() == n_args &&
-                        out_type.matches(cx, &sig.decl.output) &&
-                        self_kind.matches(cx, self_ty, first_arg_ty) {
+                            sig.decl.inputs.len() == n_args &&
+                            out_type.matches(cx, &sig.decl.output) &&
+                            self_kind.matches(cx, self_ty, first_arg_ty) &&
+                            fn_header_equals(*fn_header, sig.header) {
                             span_lint(cx, SHOULD_IMPLEMENT_TRAIT, impl_item.span, &format!(
                                 "defining a method called `{}` on this type; consider implementing \
                                 the `{}` trait or choosing a less ambiguous name", name, trait_name));
@@ -3352,38 +3353,45 @@ const CONVENTIONS: [(Convention, &[SelfKind]); 7] = [
     (Convention::StartsWith("to_"), &[SelfKind::Ref]),
 ];
 
+const FN_HEADER: hir::FnHeader = hir::FnHeader {
+    unsafety: hir::Unsafety::Normal,
+    constness: hir::Constness::NotConst,
+    asyncness: hir::IsAsync::NotAsync,
+    abi: rustc_target::spec::abi::Abi::Rust,
+};
+
 #[rustfmt::skip]
-const TRAIT_METHODS: [(&str, usize, SelfKind, OutType, &str); 30] = [
-    ("add", 2, SelfKind::Value, OutType::Any, "std::ops::Add"),
-    ("as_mut", 1, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
-    ("as_ref", 1, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
-    ("bitand", 2, SelfKind::Value, OutType::Any, "std::ops::BitAnd"),
-    ("bitor", 2, SelfKind::Value, OutType::Any, "std::ops::BitOr"),
-    ("bitxor", 2, SelfKind::Value, OutType::Any, "std::ops::BitXor"),
-    ("borrow", 1, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"),
-    ("borrow_mut", 1, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"),
-    ("clone", 1, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
-    ("cmp", 2, SelfKind::Ref, OutType::Any, "std::cmp::Ord"),
-    ("default", 0, SelfKind::No, OutType::Any, "std::default::Default"),
-    ("deref", 1, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
-    ("deref_mut", 1, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
-    ("div", 2, SelfKind::Value, OutType::Any, "std::ops::Div"),
-    ("drop", 1, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
-    ("eq", 2, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
-    ("from_iter", 1, SelfKind::No, OutType::Any, "std::iter::FromIterator"),
-    ("from_str", 1, SelfKind::No, OutType::Any, "std::str::FromStr"),
-    ("hash", 2, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
-    ("index", 2, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
-    ("index_mut", 2, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
-    ("into_iter", 1, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
-    ("mul", 2, SelfKind::Value, OutType::Any, "std::ops::Mul"),
-    ("neg", 1, SelfKind::Value, OutType::Any, "std::ops::Neg"),
-    ("next", 1, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
-    ("not", 1, SelfKind::Value, OutType::Any, "std::ops::Not"),
-    ("rem", 2, SelfKind::Value, OutType::Any, "std::ops::Rem"),
-    ("shl", 2, SelfKind::Value, OutType::Any, "std::ops::Shl"),
-    ("shr", 2, SelfKind::Value, OutType::Any, "std::ops::Shr"),
-    ("sub", 2, SelfKind::Value, OutType::Any, "std::ops::Sub"),
+const TRAIT_METHODS: [(&str, usize, &hir::FnHeader, SelfKind, OutType, &str); 30] = [
+    ("add", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Add"),
+    ("as_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
+    ("as_ref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
+    ("bitand", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitAnd"),
+    ("bitor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitOr"),
+    ("bitxor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitXor"),
+    ("borrow", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"),
+    ("borrow_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"),
+    ("clone", 1, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
+    ("cmp", 2, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::cmp::Ord"),
+    ("default", 0, &FN_HEADER, SelfKind::No, OutType::Any, "std::default::Default"),
+    ("deref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
+    ("deref_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
+    ("div", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Div"),
+    ("drop", 1, &FN_HEADER, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
+    ("eq", 2, &FN_HEADER, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
+    ("from_iter", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::iter::FromIterator"),
+    ("from_str", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::str::FromStr"),
+    ("hash", 2, &FN_HEADER, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
+    ("index", 2, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
+    ("index_mut", 2, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
+    ("into_iter", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
+    ("mul", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Mul"),
+    ("neg", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Neg"),
+    ("next", 1, &FN_HEADER, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
+    ("not", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Not"),
+    ("rem", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Rem"),
+    ("shl", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shl"),
+    ("shr", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shr"),
+    ("sub", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Sub"),
 ];
 
 #[rustfmt::skip]
@@ -3596,3 +3604,9 @@ fn lint_filetype_is_file(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, args: &
     let help_msg = format!("use `{}FileType::is_dir()` instead", help_unary);
     span_lint_and_help(cx, FILETYPE_IS_FILE, span, &lint_msg, &help_msg);
 }
+
+fn fn_header_equals(expected: hir::FnHeader, actual: hir::FnHeader) -> bool {
+    expected.constness == actual.constness
+        && expected.unsafety == actual.unsafety
+        && expected.asyncness == actual.asyncness
+}
diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs
index 2af33b26340..7880cf36415 100644
--- a/tests/ui/methods.rs
+++ b/tests/ui/methods.rs
@@ -6,6 +6,7 @@
     clippy::blacklisted_name,
     clippy::default_trait_access,
     clippy::missing_docs_in_private_items,
+    clippy::missing_safety_doc,
     clippy::non_ascii_literal,
     clippy::new_without_default,
     clippy::needless_pass_by_value,
@@ -83,6 +84,20 @@ impl T {
     }
 }
 
+pub struct T1;
+
+impl T1 {
+    // Shouldn't trigger lint as it is unsafe.
+    pub unsafe fn add(self, rhs: T1) -> T1 {
+        self
+    }
+
+    // Should not trigger lint since this is an async function.
+    pub async fn next(&mut self) -> Option<T1> {
+        None
+    }
+}
+
 struct Lt<'a> {
     foo: &'a u32,
 }
diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr
index 878e78fdcc5..01cf487ac14 100644
--- a/tests/ui/methods.stderr
+++ b/tests/ui/methods.stderr
@@ -1,5 +1,5 @@
 error: defining a method called `add` on this type; consider implementing the `std::ops::Add` trait or choosing a less ambiguous name
-  --> $DIR/methods.rs:38:5
+  --> $DIR/methods.rs:39:5
    |
 LL | /     pub fn add(self, other: T) -> T {
 LL | |         self
@@ -9,7 +9,7 @@ LL | |     }
    = note: `-D clippy::should-implement-trait` implied by `-D warnings`
 
 error: methods called `new` usually return `Self`
-  --> $DIR/methods.rs:154:5
+  --> $DIR/methods.rs:169:5
    |
 LL | /     fn new() -> i32 {
 LL | |         0
@@ -19,7 +19,7 @@ LL | |     }
    = note: `-D clippy::new-ret-no-self` implied by `-D warnings`
 
 error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead.
-  --> $DIR/methods.rs:173:13
+  --> $DIR/methods.rs:188:13
    |
 LL |     let _ = v.iter().filter(|&x| *x < 0).next();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -28,7 +28,7 @@ LL |     let _ = v.iter().filter(|&x| *x < 0).next();
    = note: replace `filter(|&x| *x < 0).next()` with `find(|&x| *x < 0)`
 
 error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead.
-  --> $DIR/methods.rs:176:13
+  --> $DIR/methods.rs:191:13
    |
 LL |       let _ = v.iter().filter(|&x| {
    |  _____________^
@@ -38,7 +38,7 @@ LL | |                    ).next();
    | |___________________________^
 
 error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:193:22
+  --> $DIR/methods.rs:208:22
    |
 LL |     let _ = v.iter().find(|&x| *x < 0).is_some();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x < 0)`
@@ -46,25 +46,25 @@ LL |     let _ = v.iter().find(|&x| *x < 0).is_some();
    = note: `-D clippy::search-is-some` implied by `-D warnings`
 
 error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:194:20
+  --> $DIR/methods.rs:209:20
    |
 LL |     let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| **y == x)`
 
 error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:195:20
+  --> $DIR/methods.rs:210:20
    |
 LL |     let _ = (0..1).find(|x| *x == 0).is_some();
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| x == 0)`
 
 error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:196:22
+  --> $DIR/methods.rs:211:22
    |
 LL |     let _ = v.iter().find(|x| **x == 0).is_some();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x == 0)`
 
 error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:199:13
+  --> $DIR/methods.rs:214:13
    |
 LL |       let _ = v.iter().find(|&x| {
    |  _____________^
@@ -74,13 +74,13 @@ LL | |                    ).is_some();
    | |______________________________^
 
 error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:205:22
+  --> $DIR/methods.rs:220:22
    |
 LL |     let _ = v.iter().position(|&x| x < 0).is_some();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)`
 
 error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:208:13
+  --> $DIR/methods.rs:223:13
    |
 LL |       let _ = v.iter().position(|&x| {
    |  _____________^
@@ -90,13 +90,13 @@ LL | |                    ).is_some();
    | |______________________________^
 
 error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:214:22
+  --> $DIR/methods.rs:229:22
    |
 LL |     let _ = v.iter().rposition(|&x| x < 0).is_some();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)`
 
 error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:217:13
+  --> $DIR/methods.rs:232:13
    |
 LL |       let _ = v.iter().rposition(|&x| {
    |  _____________^