Skip to content

Commit 166c520

Browse files
tnielenstnielens
committed
should_impl_trait - pr comments
1 parent 7cc1a2e commit 166c520

File tree

8 files changed

+670
-501
lines changed

8 files changed

+670
-501
lines changed

‎clippy_lints/src/methods/mod.rs

Lines changed: 101 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1495,25 +1495,31 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
14951495

14961496
then {
14971497
if cx.access_levels.is_exported(impl_item.hir_id) {
1498-
// check missing trait implementations
1499-
for &(method_name, n_args, fn_header, self_kind, out_type, trait_name) in &TRAIT_METHODS {
1500-
let no_lifetime_params = || {
1501-
!impl_item.generics.params.iter()
1502-
.any(|p| matches!(
1503-
p.kind,
1504-
hir::GenericParamKind::Lifetime { kind: hir::LifetimeParamKind::Explicit }))
1505-
};
1506-
if name == method_name &&
1507-
sig.decl.inputs.len() == n_args &&
1508-
out_type.matches(cx, &sig.decl.output) &&
1509-
self_kind.matches(cx, self_ty, first_arg_ty) &&
1510-
fn_header_equals(*fn_header, sig.header) &&
1511-
// ignore methods with lifetime params, risk of false positive
1512-
no_lifetime_params()
1498+
// check missing trait implementations
1499+
for method_config in &TRAIT_METHODS {
1500+
if name == method_config.method_name &&
1501+
sig.decl.inputs.len() == method_config.param_count &&
1502+
method_config.output_type.matches(cx, &sig.decl.output) &&
1503+
method_config.self_kind.matches(cx, self_ty, first_arg_ty) &&
1504+
fn_header_equals(*method_config.fn_header, sig.header) &&
1505+
method_config.lifetime_param_cond(&impl_item)
15131506
{
1514-
span_lint(cx, SHOULD_IMPLEMENT_TRAIT, impl_item.span, &format!(
1515-
"defining a method called `{}` on this type; consider implementing \
1516-
the `{}` trait or choosing a less ambiguous name", name, trait_name));
1507+
span_lint_and_help(
1508+
cx,
1509+
SHOULD_IMPLEMENT_TRAIT,
1510+
impl_item.span,
1511+
&format!(
1512+
"method `{}` can be confused for the standard trait method `{}::{}`",
1513+
method_config.method_name,
1514+
method_config.trait_name,
1515+
method_config.method_name
1516+
),
1517+
None,
1518+
&format!(
1519+
"consider implementing the trait `{}` or choosing a less ambiguous method name",
1520+
method_config.trait_name
1521+
)
1522+
);
15171523
}
15181524
}
15191525
}
@@ -3412,39 +3418,85 @@ const FN_HEADER: hir::FnHeader = hir::FnHeader {
34123418
abi: rustc_target::spec::abi::Abi::Rust,
34133419
};
34143420

3421+
struct ShouldImplTraitCase {
3422+
trait_name: &'static str,
3423+
method_name: &'static str,
3424+
param_count: usize,
3425+
fn_header: &'static hir::FnHeader,
3426+
// implicit self kind expected (none, self, &self, ...)
3427+
self_kind: SelfKind,
3428+
// checks against the output type
3429+
output_type: OutType,
3430+
// certain methods with explicit lifetimes can't implement the equivalent trait method
3431+
lint_explicit_lifetime: bool,
3432+
}
3433+
impl ShouldImplTraitCase {
3434+
const fn new(
3435+
trait_name: &'static str,
3436+
method_name: &'static str,
3437+
param_count: usize,
3438+
fn_header: &'static hir::FnHeader,
3439+
self_kind: SelfKind,
3440+
output_type: OutType,
3441+
lint_explicit_lifetime: bool,
3442+
) -> ShouldImplTraitCase {
3443+
ShouldImplTraitCase {
3444+
trait_name,
3445+
method_name,
3446+
param_count,
3447+
fn_header,
3448+
self_kind,
3449+
output_type,
3450+
lint_explicit_lifetime,
3451+
}
3452+
}
3453+
3454+
fn lifetime_param_cond(&self, impl_item: &hir::ImplItem<'_>) -> bool {
3455+
self.lint_explicit_lifetime
3456+
|| !impl_item.generics.params.iter().any(|p| {
3457+
matches!(
3458+
p.kind,
3459+
hir::GenericParamKind::Lifetime {
3460+
kind: hir::LifetimeParamKind::Explicit
3461+
}
3462+
)
3463+
})
3464+
}
3465+
}
3466+
34153467
#[rustfmt::skip]
3416-
const TRAIT_METHODS: [(&str, usize, &hir::FnHeader, SelfKind, OutType, &str); 30] = [
3417-
("add", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Add"),
3418-
("as_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
3419-
("as_ref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
3420-
("bitand", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitAnd"),
3421-
("bitor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitOr"),
3422-
("bitxor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::BitXor"),
3423-
("borrow", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"),
3424-
("borrow_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"),
3425-
("clone", 1, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
3426-
("cmp", 2, &FN_HEADER, SelfKind::Ref, OutType::Any, "std::cmp::Ord"),
3468+
const TRAIT_METHODS: [ShouldImplTraitCase; 30] = [
3469+
ShouldImplTraitCase::new("std::ops::Add", "add", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3470+
ShouldImplTraitCase::new("std::convert::AsMut", "as_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3471+
ShouldImplTraitCase::new("std::convert::AsRef", "as_ref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3472+
ShouldImplTraitCase::new("std::ops::BitAnd", "bitand", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3473+
ShouldImplTraitCase::new("std::ops::BitOr", "bitor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3474+
ShouldImplTraitCase::new("std::ops::BitXor", "bitxor", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3475+
ShouldImplTraitCase::new("std::borrow::Borrow", "borrow", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3476+
ShouldImplTraitCase::new("std::borrow::BorrowMut", "borrow_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3477+
ShouldImplTraitCase::new("std::clone::Clone", "clone", 1, &FN_HEADER, SelfKind::Ref, OutType::Any, true),
3478+
ShouldImplTraitCase::new("std::cmp::Ord", "cmp", 2, &FN_HEADER, SelfKind::Ref, OutType::Any, true),
34273479
// FIXME: default doesn't work
3428-
("default", 0, &FN_HEADER, SelfKind::No, OutType::Any, "std::default::Default"),
3429-
("deref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
3430-
("deref_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
3431-
("div", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Div"),
3432-
("drop", 1, &FN_HEADER, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
3433-
("eq", 2, &FN_HEADER, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
3434-
("from_iter", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::iter::FromIterator"),
3435-
("from_str", 1, &FN_HEADER, SelfKind::No, OutType::Any, "std::str::FromStr"),
3436-
("hash", 2, &FN_HEADER, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
3437-
("index", 2, &FN_HEADER, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
3438-
("index_mut", 2, &FN_HEADER, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
3439-
("into_iter", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
3440-
("mul", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Mul"),
3441-
("neg", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Neg"),
3442-
("next", 1, &FN_HEADER, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
3443-
("not", 1, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Not"),
3444-
("rem", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Rem"),
3445-
("shl", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shl"),
3446-
("shr", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Shr"),
3447-
("sub", 2, &FN_HEADER, SelfKind::Value, OutType::Any, "std::ops::Sub"),
3480+
ShouldImplTraitCase::new("std::default::Default", "default", 0, &FN_HEADER, SelfKind::No, OutType::Any, true),
3481+
ShouldImplTraitCase::new("std::ops::Deref", "deref", 1, &FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3482+
ShouldImplTraitCase::new("std::ops::DerefMut", "deref_mut", 1, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3483+
ShouldImplTraitCase::new("std::ops::Div", "div", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3484+
ShouldImplTraitCase::new("std::ops::Drop", "drop", 1, &FN_HEADER, SelfKind::RefMut, OutType::Unit, true),
3485+
ShouldImplTraitCase::new("std::cmp::PartialEq", "eq", 2, &FN_HEADER, SelfKind::Ref, OutType::Bool, true),
3486+
ShouldImplTraitCase::new("std::iter::FromIterator", "from_iter", 1, &FN_HEADER, SelfKind::No, OutType::Any, true),
3487+
ShouldImplTraitCase::new("std::str::FromStr", "from_str", 1, &FN_HEADER, SelfKind::No, OutType::Any, true),
3488+
ShouldImplTraitCase::new("std::hash::Hash", "hash", 2, &FN_HEADER, SelfKind::Ref, OutType::Unit, true),
3489+
ShouldImplTraitCase::new("std::ops::Index", "index", 2, &FN_HEADER, SelfKind::Ref, OutType::Ref, true),
3490+
ShouldImplTraitCase::new("std::ops::IndexMut", "index_mut", 2, &FN_HEADER, SelfKind::RefMut, OutType::Ref, true),
3491+
ShouldImplTraitCase::new("std::iter::IntoIterator", "into_iter", 1, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3492+
ShouldImplTraitCase::new("std::ops::Mul", "mul", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3493+
ShouldImplTraitCase::new("std::ops::Neg", "neg", 1, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3494+
ShouldImplTraitCase::new("std::iter::Iterator", "next", 1, &FN_HEADER, SelfKind::RefMut, OutType::Any, false),
3495+
ShouldImplTraitCase::new("std::ops::Not", "not", 1, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3496+
ShouldImplTraitCase::new("std::ops::Rem", "rem", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3497+
ShouldImplTraitCase::new("std::ops::Shl", "shl", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3498+
ShouldImplTraitCase::new("std::ops::Shr", "shr", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
3499+
ShouldImplTraitCase::new("std::ops::Sub", "sub", 2, &FN_HEADER, SelfKind::Value, OutType::Any, true),
34483500
];
34493501

34503502
#[rustfmt::skip]

‎tests/ui/methods.rs

Lines changed: 2 additions & 195 deletions
Original file line numberDiff line numberDiff line change
@@ -34,201 +34,6 @@ use std::sync::{self, Arc};
3434

3535
use option_helpers::IteratorFalsePositives;
3636

37-
pub struct T;
38-
39-
impl T {
40-
// *******************************************
41-
// complete trait method list, should lint all
42-
// *******************************************
43-
pub fn add(self, other: T) -> T {
44-
unimplemented!()
45-
}
46-
47-
pub fn as_mut(&mut self) -> &mut T {
48-
unimplemented!()
49-
}
50-
51-
pub fn as_ref(&self) -> &T {
52-
unimplemented!()
53-
}
54-
55-
pub fn bitand(self, rhs: T) -> T {
56-
unimplemented!()
57-
}
58-
59-
pub fn bitor(self, rhs: Self) -> Self {
60-
unimplemented!()
61-
}
62-
63-
pub fn bitxor(self, rhs: Self) -> Self {
64-
unimplemented!()
65-
}
66-
67-
pub fn borrow(&self) -> &str {
68-
unimplemented!()
69-
}
70-
71-
pub fn borrow_mut(&mut self) -> &mut str {
72-
unimplemented!()
73-
}
74-
75-
pub fn clone(&self) -> Self {
76-
unimplemented!()
77-
}
78-
79-
pub fn cmp(&self, other: &Self) -> Self {
80-
unimplemented!()
81-
}
82-
83-
pub fn default() -> Self {
84-
unimplemented!()
85-
}
86-
87-
pub fn deref(&self) -> &Self {
88-
unimplemented!()
89-
}
90-
91-
pub fn deref_mut(&mut self) -> &mut Self {
92-
unimplemented!()
93-
}
94-
95-
pub fn div(self, rhs: Self) -> Self {
96-
unimplemented!()
97-
}
98-
99-
pub fn drop(&mut self) {
100-
unimplemented!()
101-
}
102-
103-
pub fn eq(&self, other: &Self) -> bool {
104-
unimplemented!()
105-
}
106-
107-
pub fn from_iter<T>(iter: T) -> Self {
108-
unimplemented!()
109-
}
110-
111-
pub fn from_str(s: &str) -> Result<Self, Self> {
112-
unimplemented!()
113-
}
114-
115-
pub fn hash(&self, state: &mut T) {
116-
unimplemented!()
117-
}
118-
119-
pub fn index(&self, index: usize) -> &Self {
120-
unimplemented!()
121-
}
122-
123-
pub fn index_mut(&mut self, index: usize) -> &mut Self {
124-
unimplemented!()
125-
}
126-
127-
pub fn into_iter(self) -> Self {
128-
unimplemented!()
129-
}
130-
131-
pub fn mul(self, rhs: Self) -> Self {
132-
unimplemented!()
133-
}
134-
135-
pub fn neg(self) -> Self {
136-
unimplemented!()
137-
}
138-
139-
pub fn next(&mut self) -> Option<Self> {
140-
unimplemented!()
141-
}
142-
143-
pub fn not(self) -> Self {
144-
unimplemented!()
145-
}
146-
147-
pub fn rem(self, rhs: Self) -> Self {
148-
unimplemented!()
149-
}
150-
151-
pub fn shl(self, rhs: Self) -> Self {
152-
unimplemented!()
153-
}
154-
155-
pub fn shr(self, rhs: Self) -> Self {
156-
unimplemented!()
157-
}
158-
159-
pub fn sub(self, rhs: Self) -> Self {
160-
unimplemented!()
161-
}
162-
// *****************
163-
// complete list end
164-
// *****************
165-
}
166-
167-
pub struct T1;
168-
impl T1 {
169-
// corner cases: should not lint
170-
171-
// no error, not public interface
172-
pub(crate) fn drop(&mut self) {}
173-
174-
// no error, private function
175-
fn neg(self) -> Self {
176-
self
177-
}
178-
179-
// no error, private function
180-
fn eq(&self, other: Self) -> bool {
181-
true
182-
}
183-
184-
// No error; self is a ref.
185-
fn sub(&self, other: Self) -> &Self {
186-
self
187-
}
188-
189-
// No error; different number of arguments.
190-
fn div(self) -> Self {
191-
self
192-
}
193-
194-
// No error; wrong return type.
195-
fn rem(self, other: Self) {}
196-
197-
// Fine
198-
fn into_u32(self) -> u32 {
199-
0
200-
}
201-
202-
fn into_u16(&self) -> u16 {
203-
0
204-
}
205-
206-
fn to_something(self) -> u32 {
207-
0
208-
}
209-
210-
fn new(self) -> Self {
211-
unimplemented!();
212-
}
213-
214-
pub fn next<'b>(&'b mut self) -> Option<&'b mut T> {
215-
unimplemented!();
216-
}
217-
}
218-
219-
pub struct T2;
220-
impl T2 {
221-
// Shouldn't trigger lint as it is unsafe.
222-
pub unsafe fn add(self, rhs: Self) -> Self {
223-
self
224-
}
225-
226-
// Should not trigger lint since this is an async function.
227-
pub async fn next(&mut self) -> Option<Self> {
228-
None
229-
}
230-
}
231-
23237
struct Lt<'a> {
23338
foo: &'a u32,
23439
}
@@ -302,6 +107,8 @@ impl BadNew {
302107
}
303108
}
304109

110+
struct T;
111+
305112
impl Mul<T> for T {
306113
type Output = T;
307114
// No error, obviously.

0 commit comments

Comments
 (0)