Skip to content

Commit

Permalink
Check fn header along with decl when suggesting to implement trait
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@rabisg0
rabisg0 committed Apr 8, 2020
1 parent d342cee commit 44916a8
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 47 deletions.
84 changes: 49 additions & 35 deletions clippy_lints/src/methods/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1426,11 +1426,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));
Expand Down Expand Up @@ -3266,38 +3267,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]
Expand Down Expand Up @@ -3510,3 +3518,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
}
15 changes: 15 additions & 0 deletions tests/ui/methods.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,21 @@ impl T {
}
}

pub struct T1;

impl T1 {
#![allow(clippy::missing_safety_doc)]
// 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,
}
Expand Down
24 changes: 12 additions & 12 deletions tests/ui/methods.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -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| {
| _____________^
Expand All @@ -38,33 +38,33 @@ 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)`
|
= 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| {
| _____________^
Expand All @@ -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| {
| _____________^
Expand All @@ -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| {
| _____________^
Expand Down

0 comments on commit 44916a8

Please sign in to comment.