Skip to content

Commit 10d9622

Browse files
mgacek8mgacek8
committed
wrong_self_convention: fix FP inside trait impl for to_* method
When the `to_*` method takes `&self` and it is a trait implementation, we don't trigger the lint.
1 parent 0e06b3c commit 10d9622

6 files changed

+81
-17
lines changed

‎clippy_lints/src/methods/mod.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,8 +205,16 @@ declare_clippy_lint! {
205205
/// |`to_` | not `_mut` |`self` | `Copy` |
206206
/// |`to_` | not `_mut` |`&self` | not `Copy` |
207207
///
208+
/// Note: Clippy doesn't trigger methods with `to_` prefix in:
209+
/// - Traits definition.
210+
/// Clippy can not tell if a type that implements a trait is `Copy` or not.
211+
/// - Traits implementation, when `&self` is taken.
212+
/// The method signature is controlled by the trait and often `&self` is required for all types that implement the trait
213+
/// (see e.g. the `std::string::ToString` trait).
214+
///
208215
/// Please find more info here:
209216
/// https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
217+
//
210218
///
211219
/// **Why is this bad?** Consistency breeds readability. If you follow the
212220
/// conventions, your users won't be surprised that they, e.g., need to supply a
@@ -1850,7 +1858,6 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
18501858
let self_ty = cx.tcx.type_of(item.def_id);
18511859

18521860
let implements_trait = matches!(item.kind, hir::ItemKind::Impl(hir::Impl { of_trait: Some(_), .. }));
1853-
18541861
if_chain! {
18551862
if let hir::ImplItemKind::Fn(ref sig, id) = impl_item.kind;
18561863
if let Some(first_arg) = iter_input_pats(&sig.decl, cx.tcx.hir().body(id)).next();
@@ -1902,6 +1909,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
19021909
self_ty,
19031910
first_arg_ty,
19041911
first_arg.pat.span,
1912+
implements_trait,
19051913
false
19061914
);
19071915
}
@@ -1972,6 +1980,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
19721980
self_ty,
19731981
first_arg_ty,
19741982
first_arg_span,
1983+
false,
19751984
true
19761985
);
19771986
}

‎clippy_lints/src/methods/wrong_self_convention.rs

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@ const CONVENTIONS: [(&[Convention], &[SelfKind]); 9] = [
2121

2222
// Conversion using `to_` can use borrowed (non-Copy types) or owned (Copy types).
2323
// Source: https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
24-
(&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(false), Convention::ImplementsTrait(false)], &[SelfKind::Ref]),
25-
(&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(true), Convention::ImplementsTrait(false)], &[SelfKind::Value]),
24+
(&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(false),
25+
Convention::IsTraitItem(false)], &[SelfKind::Ref]),
26+
(&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(true),
27+
Convention::IsTraitItem(false), Convention::ImplementsTrait(false)], &[SelfKind::Value]),
2628
];
2729

2830
enum Convention {
@@ -32,18 +34,27 @@ enum Convention {
3234
NotEndsWith(&'static str),
3335
IsSelfTypeCopy(bool),
3436
ImplementsTrait(bool),
37+
IsTraitItem(bool),
3538
}
3639

3740
impl Convention {
3841
#[must_use]
39-
fn check<'tcx>(&self, cx: &LateContext<'tcx>, self_ty: &'tcx TyS<'tcx>, other: &str, is_trait_def: bool) -> bool {
42+
fn check<'tcx>(
43+
&self,
44+
cx: &LateContext<'tcx>,
45+
self_ty: &'tcx TyS<'tcx>,
46+
other: &str,
47+
implements_trait: bool,
48+
is_trait_item: bool,
49+
) -> bool {
4050
match *self {
4151
Self::Eq(this) => this == other,
4252
Self::StartsWith(this) => other.starts_with(this) && this != other,
4353
Self::EndsWith(this) => other.ends_with(this) && this != other,
44-
Self::NotEndsWith(this) => !Self::EndsWith(this).check(cx, self_ty, other, is_trait_def),
54+
Self::NotEndsWith(this) => !Self::EndsWith(this).check(cx, self_ty, other, implements_trait, is_trait_item),
4555
Self::IsSelfTypeCopy(is_true) => is_true == is_copy(cx, self_ty),
46-
Self::ImplementsTrait(is_true) => is_true == is_trait_def,
56+
Self::ImplementsTrait(is_true) => is_true == implements_trait,
57+
Self::IsTraitItem(is_true) => is_true == is_trait_item,
4758
}
4859
}
4960
}
@@ -60,7 +71,11 @@ impl fmt::Display for Convention {
6071
},
6172
Self::ImplementsTrait(is_true) => {
6273
let (negation, s_suffix) = if is_true { ("", "s") } else { (" does not", "") };
63-
format!("Method{} implement{} a trait", negation, s_suffix).fmt(f)
74+
format!("method{} implement{} a trait", negation, s_suffix).fmt(f)
75+
},
76+
Self::IsTraitItem(is_true) => {
77+
let suffix = if is_true { " is" } else { " is not" };
78+
format!("method{} a trait item", suffix).fmt(f)
6479
},
6580
}
6681
}
@@ -73,6 +88,7 @@ pub(super) fn check<'tcx>(
7388
self_ty: &'tcx TyS<'tcx>,
7489
first_arg_ty: &'tcx TyS<'tcx>,
7590
first_arg_span: Span,
91+
implements_trait: bool,
7692
is_trait_item: bool,
7793
) {
7894
let lint = if is_pub {
@@ -83,7 +99,7 @@ pub(super) fn check<'tcx>(
8399
if let Some((conventions, self_kinds)) = &CONVENTIONS.iter().find(|(convs, _)| {
84100
convs
85101
.iter()
86-
.all(|conv| conv.check(cx, self_ty, item_name, is_trait_item))
102+
.all(|conv| conv.check(cx, self_ty, item_name, implements_trait, is_trait_item))
87103
}) {
88104
if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
89105
let suggestion = {
@@ -99,6 +115,7 @@ pub(super) fn check<'tcx>(
99115
.filter_map(|conv| {
100116
if (cut_ends_with_conv && matches!(conv, Convention::NotEndsWith(_)))
101117
|| matches!(conv, Convention::ImplementsTrait(_))
118+
|| matches!(conv, Convention::IsTraitItem(_))
102119
{
103120
None
104121
} else {

‎tests/ui/wrong_self_convention.rs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,15 +165,10 @@ mod issue6307 {
165165
}
166166

167167
mod issue6727 {
168-
trait ToU64 {
169-
fn to_u64(self) -> u64;
170-
fn to_u64_v2(&self) -> u64;
171-
}
172-
173168
#[derive(Clone, Copy)]
174169
struct FooCopy;
175170

176-
impl ToU64 for FooCopy {
171+
impl FooCopy {
177172
fn to_u64(self) -> u64 {
178173
1
179174
}
@@ -185,7 +180,7 @@ mod issue6727 {
185180

186181
struct FooNoCopy;
187182

188-
impl ToU64 for FooNoCopy {
183+
impl FooNoCopy {
189184
// trigger lint
190185
fn to_u64(self) -> u64 {
191186
2

‎tests/ui/wrong_self_convention.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,15 +176,15 @@ LL | fn from_i32(self);
176176
= help: consider choosing a less ambiguous name
177177

178178
error: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value
179-
--> $DIR/wrong_self_convention.rs:181:22
179+
--> $DIR/wrong_self_convention.rs:176:22
180180
|
181181
LL | fn to_u64_v2(&self) -> u64 {
182182
| ^^^^^
183183
|
184184
= help: consider choosing a less ambiguous name
185185

186186
error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference
187-
--> $DIR/wrong_self_convention.rs:190:19
187+
--> $DIR/wrong_self_convention.rs:185:19
188188
|
189189
LL | fn to_u64(self) -> u64 {
190190
| ^^^^

‎tests/ui/wrong_self_convention2.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// edition:2018
2+
#![warn(clippy::wrong_self_convention)]
3+
#![warn(clippy::wrong_pub_self_convention)]
4+
#![allow(dead_code)]
5+
6+
fn main() {}
7+
8+
mod issue6983 {
9+
pub struct Thing;
10+
pub trait Trait {
11+
fn to_thing(&self) -> Thing;
12+
}
13+
14+
impl Trait for u8 {
15+
// don't trigger, e.g. `ToString` from `std` requires `&self`
16+
fn to_thing(&self) -> Thing {
17+
Thing
18+
}
19+
}
20+
21+
trait ToU64 {
22+
fn to_u64(self) -> u64;
23+
}
24+
25+
struct FooNoCopy;
26+
// trigger lint
27+
impl ToU64 for FooNoCopy {
28+
fn to_u64(self) -> u64 {
29+
2
30+
}
31+
}
32+
}

‎tests/ui/wrong_self_convention2.stderr

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference
2+
--> $DIR/wrong_self_convention2.rs:28:19
3+
|
4+
LL | fn to_u64(self) -> u64 {
5+
| ^^^^
6+
|
7+
= note: `-D clippy::wrong-self-convention` implied by `-D warnings`
8+
= help: consider choosing a less ambiguous name
9+
10+
error: aborting due to previous error
11+

0 commit comments

Comments
 (0)