Skip to content

Commit 0bc7ac1

Browse files
author
mgacek
committed
wrong_self_convention: to_ respects Copy types
More details: https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
1 parent 56161b2 commit 0bc7ac1

8 files changed

+147
-90
lines changed

‎clippy_lints/src/methods/mod.rs

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -193,14 +193,18 @@ declare_clippy_lint! {
193193
/// **What it does:** Checks for methods with certain name prefixes and which
194194
/// doesn't match how self is taken. The actual rules are:
195195
///
196-
/// |Prefix |Postfix |`self` taken |
197-
/// |-------|------------|----------------------|
198-
/// |`as_` | none |`&self` or `&mut self`|
199-
/// |`from_`| none | none |
200-
/// |`into_`| none |`self` |
201-
/// |`is_` | none |`&self` or none |
202-
/// |`to_` | `_mut` |`&mut &self` |
203-
/// |`to_` | not `_mut` |`&self` |
196+
/// |Prefix |Postfix |`self` taken | `self` type |
197+
/// |-------|------------|-----------------------|--------------|
198+
/// |`as_` | none |`&self` or `&mut self` | any |
199+
/// |`from_`| none | none | any |
200+
/// |`into_`| none |`self` | any |
201+
/// |`is_` | none |`&self` or none | any |
202+
/// |`to_` | `_mut` |`&mut self` | any |
203+
/// |`to_` | not `_mut` |`self` | `Copy` |
204+
/// |`to_` | not `_mut` |`&self` | not `Copy` |
205+
///
206+
/// Please find more info here:
207+
/// https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
204208
///
205209
/// **Why is this bad?** Consistency breeds readability. If you follow the
206210
/// conventions, your users won't be surprised that they, e.g., need to supply a
@@ -1837,10 +1841,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
18371841
let item = cx.tcx.hir().expect_item(parent);
18381842
let self_ty = cx.tcx.type_of(item.def_id);
18391843

1840-
// if this impl block implements a trait, lint in trait definition instead
1841-
if let hir::ItemKind::Impl(hir::Impl { of_trait: Some(_), .. }) = item.kind {
1842-
return;
1843-
}
1844+
let implements_trait = matches!(item.kind, hir::ItemKind::Impl(hir::Impl { of_trait: Some(_), .. }));
18441845

18451846
if_chain! {
18461847
if let hir::ImplItemKind::Fn(ref sig, id) = impl_item.kind;
@@ -1855,7 +1856,8 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
18551856
if let Some(first_arg_ty) = first_arg_ty;
18561857

18571858
then {
1858-
if cx.access_levels.is_exported(impl_item.hir_id()) {
1859+
// if this impl block implements a trait, lint in trait definition instead
1860+
if !implements_trait && cx.access_levels.is_exported(impl_item.hir_id()) {
18591861
// check missing trait implementations
18601862
for method_config in &TRAIT_METHODS {
18611863
if name == method_config.method_name &&
@@ -1891,11 +1893,17 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
18911893
item.vis.node.is_pub(),
18921894
self_ty,
18931895
first_arg_ty,
1894-
first_arg.pat.span
1896+
first_arg.pat.span,
1897+
false
18951898
);
18961899
}
18971900
}
18981901

1902+
// if this impl block implements a trait, lint in trait definition instead
1903+
if implements_trait {
1904+
return;
1905+
}
1906+
18991907
if let hir::ImplItemKind::Fn(_, _) = impl_item.kind {
19001908
let ret_ty = return_ty(cx, impl_item.hir_id());
19011909

@@ -1947,7 +1955,8 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
19471955
false,
19481956
self_ty,
19491957
first_arg_ty,
1950-
first_arg_span
1958+
first_arg_span,
1959+
true
19511960
);
19521961
}
19531962
}

‎clippy_lints/src/methods/wrong_self_convention.rs

Lines changed: 46 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use crate::methods::SelfKind;
22
use clippy_utils::diagnostics::span_lint_and_help;
3+
use clippy_utils::ty::is_copy;
34
use rustc_lint::LateContext;
45
use rustc_middle::ty::TyS;
56
use rustc_span::source_map::Span;
@@ -9,32 +10,40 @@ use super::WRONG_PUB_SELF_CONVENTION;
910
use super::WRONG_SELF_CONVENTION;
1011

1112
#[rustfmt::skip]
12-
const CONVENTIONS: [(&[Convention], &[SelfKind]); 8] = [
13+
const CONVENTIONS: [(&[Convention], &[SelfKind]); 9] = [
1314
(&[Convention::Eq("new")], &[SelfKind::No]),
1415
(&[Convention::StartsWith("as_")], &[SelfKind::Ref, SelfKind::RefMut]),
1516
(&[Convention::StartsWith("from_")], &[SelfKind::No]),
1617
(&[Convention::StartsWith("into_")], &[SelfKind::Value]),
1718
(&[Convention::StartsWith("is_")], &[SelfKind::Ref, SelfKind::No]),
1819
(&[Convention::Eq("to_mut")], &[SelfKind::RefMut]),
1920
(&[Convention::StartsWith("to_"), Convention::EndsWith("_mut")], &[SelfKind::RefMut]),
20-
(&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut")], &[SelfKind::Ref]),
21+
22+
// Conversion using `to_` can use borrowed (non-Copy types) or owned (Copy types).
23+
// 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]),
2126
];
2227

2328
enum Convention {
2429
Eq(&'static str),
2530
StartsWith(&'static str),
2631
EndsWith(&'static str),
2732
NotEndsWith(&'static str),
33+
IsSelfTypeCopy(bool),
34+
ImplementsTrait(bool),
2835
}
2936

3037
impl Convention {
3138
#[must_use]
32-
fn check(&self, other: &str) -> bool {
39+
fn check<'tcx>(&self, cx: &LateContext<'tcx>, self_ty: &'tcx TyS<'tcx>, other: &str, is_trait_def: bool) -> bool {
3340
match *self {
3441
Self::Eq(this) => this == other,
3542
Self::StartsWith(this) => other.starts_with(this) && this != other,
3643
Self::EndsWith(this) => other.ends_with(this) && this != other,
37-
Self::NotEndsWith(this) => !Self::EndsWith(this).check(other),
44+
Self::NotEndsWith(this) => !Self::EndsWith(this).check(cx, self_ty, other, is_trait_def),
45+
Self::IsSelfTypeCopy(is_true) => is_true == is_copy(cx, self_ty),
46+
Self::ImplementsTrait(is_true) => is_true == is_trait_def,
3847
}
3948
}
4049
}
@@ -46,6 +55,10 @@ impl fmt::Display for Convention {
4655
Self::StartsWith(this) => this.fmt(f).and_then(|_| '*'.fmt(f)),
4756
Self::EndsWith(this) => '*'.fmt(f).and_then(|_| this.fmt(f)),
4857
Self::NotEndsWith(this) => '~'.fmt(f).and_then(|_| this.fmt(f)),
58+
Self::IsSelfTypeCopy(is_true) => format!("self type is {} Copy", if is_true { "" } else { "not" }).fmt(f),
59+
Self::ImplementsTrait(is_true) => {
60+
format!("Method {} implement a trait", if is_true { "" } else { "do not" }).fmt(f)
61+
},
4962
}
5063
}
5164
}
@@ -57,45 +70,46 @@ pub(super) fn check<'tcx>(
5770
self_ty: &'tcx TyS<'tcx>,
5871
first_arg_ty: &'tcx TyS<'tcx>,
5972
first_arg_span: Span,
73+
is_trait_item: bool,
6074
) {
6175
let lint = if is_pub {
6276
WRONG_PUB_SELF_CONVENTION
6377
} else {
6478
WRONG_SELF_CONVENTION
6579
};
66-
if let Some((conventions, self_kinds)) = &CONVENTIONS
67-
.iter()
68-
.find(|(convs, _)| convs.iter().all(|conv| conv.check(item_name)))
69-
{
80+
if let Some((conventions, self_kinds)) = &CONVENTIONS.iter().find(|(convs, _)| {
81+
convs
82+
.iter()
83+
.all(|conv| conv.check(cx, self_ty, item_name, is_trait_item))
84+
}) {
7085
if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
7186
let suggestion = {
7287
if conventions.len() > 1 {
73-
let special_case = {
74-
// Don't mention `NotEndsWith` when there is also `StartsWith` convention present
75-
if conventions.len() == 2 {
76-
match conventions {
77-
[Convention::StartsWith(starts_with), Convention::NotEndsWith(_)]
78-
| [Convention::NotEndsWith(_), Convention::StartsWith(starts_with)] => {
79-
Some(format!("methods called `{}`", Convention::StartsWith(starts_with)))
80-
},
81-
_ => None,
82-
}
83-
} else {
84-
None
85-
}
86-
};
87-
88-
if let Some(suggestion) = special_case {
89-
suggestion
90-
} else {
91-
let s = conventions
88+
// Don't mention `NotEndsWith` when there is also `StartsWith` convention present
89+
let cut_ends_with_conv = conventions
90+
.iter()
91+
.find(|conv| matches!(conv, Convention::StartsWith(_)))
92+
.is_some()
93+
&& conventions
9294
.iter()
93-
.map(|c| format!("`{}`", &c.to_string()))
94-
.collect::<Vec<_>>()
95-
.join(" and ");
95+
.find(|conv| matches!(conv, Convention::NotEndsWith(_)))
96+
.is_some();
97+
98+
let s = conventions
99+
.iter()
100+
.filter_map(|conv| {
101+
if (cut_ends_with_conv && matches!(conv, Convention::NotEndsWith(_)))
102+
|| matches!(conv, Convention::ImplementsTrait(_))
103+
{
104+
None
105+
} else {
106+
Some(format!("`{}`", &conv.to_string()))
107+
}
108+
})
109+
.collect::<Vec<_>>()
110+
.join(" and ");
96111

97-
format!("methods called like this: ({})", &s)
98-
}
112+
format!("methods with the following characteristics: ({})", &s)
99113
} else {
100114
format!("methods called `{}`", &conventions[0])
101115
}

‎tests/ui/use_self.fixed

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ mod issue2894 {
7979
}
8080

8181
// This should not be linted
82+
#[allow(clippy::wrong_self_convention)]
8283
impl IntoBytes for u8 {
8384
fn to_bytes(&self) -> Vec<u8> {
8485
vec![*self]

‎tests/ui/use_self.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ mod issue2894 {
7979
}
8080

8181
// This should not be linted
82+
#[allow(clippy::wrong_self_convention)]
8283
impl IntoBytes for u8 {
8384
fn to_bytes(&self) -> Vec<u8> {
8485
vec![*self]

‎tests/ui/use_self.stderr

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -37,139 +37,139 @@ LL | Foo::new()
3737
| ^^^ help: use the applicable keyword: `Self`
3838

3939
error: unnecessary structure name repetition
40-
--> $DIR/use_self.rs:93:24
40+
--> $DIR/use_self.rs:94:24
4141
|
4242
LL | fn bad(foos: &[Foo]) -> impl Iterator<Item = &Foo> {
4343
| ^^^ help: use the applicable keyword: `Self`
4444

4545
error: unnecessary structure name repetition
46-
--> $DIR/use_self.rs:93:55
46+
--> $DIR/use_self.rs:94:55
4747
|
4848
LL | fn bad(foos: &[Foo]) -> impl Iterator<Item = &Foo> {
4949
| ^^^ help: use the applicable keyword: `Self`
5050

5151
error: unnecessary structure name repetition
52-
--> $DIR/use_self.rs:108:13
52+
--> $DIR/use_self.rs:109:13
5353
|
5454
LL | TS(0)
5555
| ^^ help: use the applicable keyword: `Self`
5656

5757
error: unnecessary structure name repetition
58-
--> $DIR/use_self.rs:143:29
58+
--> $DIR/use_self.rs:144:29
5959
|
6060
LL | fn bar() -> Bar {
6161
| ^^^ help: use the applicable keyword: `Self`
6262

6363
error: unnecessary structure name repetition
64-
--> $DIR/use_self.rs:144:21
64+
--> $DIR/use_self.rs:145:21
6565
|
6666
LL | Bar { foo: Foo {} }
6767
| ^^^ help: use the applicable keyword: `Self`
6868

6969
error: unnecessary structure name repetition
70-
--> $DIR/use_self.rs:155:21
70+
--> $DIR/use_self.rs:156:21
7171
|
7272
LL | fn baz() -> Foo {
7373
| ^^^ help: use the applicable keyword: `Self`
7474

7575
error: unnecessary structure name repetition
76-
--> $DIR/use_self.rs:156:13
76+
--> $DIR/use_self.rs:157:13
7777
|
7878
LL | Foo {}
7979
| ^^^ help: use the applicable keyword: `Self`
8080

8181
error: unnecessary structure name repetition
82-
--> $DIR/use_self.rs:173:21
82+
--> $DIR/use_self.rs:174:21
8383
|
8484
LL | let _ = Enum::B(42);
8585
| ^^^^ help: use the applicable keyword: `Self`
8686

8787
error: unnecessary structure name repetition
88-
--> $DIR/use_self.rs:174:21
88+
--> $DIR/use_self.rs:175:21
8989
|
9090
LL | let _ = Enum::C { field: true };
9191
| ^^^^ help: use the applicable keyword: `Self`
9292

9393
error: unnecessary structure name repetition
94-
--> $DIR/use_self.rs:175:21
94+
--> $DIR/use_self.rs:176:21
9595
|
9696
LL | let _ = Enum::A;
9797
| ^^^^ help: use the applicable keyword: `Self`
9898

9999
error: unnecessary structure name repetition
100-
--> $DIR/use_self.rs:217:13
100+
--> $DIR/use_self.rs:218:13
101101
|
102102
LL | nested::A::fun_1();
103103
| ^^^^^^^^^ help: use the applicable keyword: `Self`
104104

105105
error: unnecessary structure name repetition
106-
--> $DIR/use_self.rs:218:13
106+
--> $DIR/use_self.rs:219:13
107107
|
108108
LL | nested::A::A;
109109
| ^^^^^^^^^ help: use the applicable keyword: `Self`
110110

111111
error: unnecessary structure name repetition
112-
--> $DIR/use_self.rs:220:13
112+
--> $DIR/use_self.rs:221:13
113113
|
114114
LL | nested::A {};
115115
| ^^^^^^^^^ help: use the applicable keyword: `Self`
116116

117117
error: unnecessary structure name repetition
118-
--> $DIR/use_self.rs:239:13
118+
--> $DIR/use_self.rs:240:13
119119
|
120120
LL | TestStruct::from_something()
121121
| ^^^^^^^^^^ help: use the applicable keyword: `Self`
122122

123123
error: unnecessary structure name repetition
124-
--> $DIR/use_self.rs:253:25
124+
--> $DIR/use_self.rs:254:25
125125
|
126126
LL | async fn g() -> S {
127127
| ^ help: use the applicable keyword: `Self`
128128

129129
error: unnecessary structure name repetition
130-
--> $DIR/use_self.rs:254:13
130+
--> $DIR/use_self.rs:255:13
131131
|
132132
LL | S {}
133133
| ^ help: use the applicable keyword: `Self`
134134

135135
error: unnecessary structure name repetition
136-
--> $DIR/use_self.rs:258:16
136+
--> $DIR/use_self.rs:259:16
137137
|
138138
LL | &p[S::A..S::B]
139139
| ^ help: use the applicable keyword: `Self`
140140

141141
error: unnecessary structure name repetition
142-
--> $DIR/use_self.rs:258:22
142+
--> $DIR/use_self.rs:259:22
143143
|
144144
LL | &p[S::A..S::B]
145145
| ^ help: use the applicable keyword: `Self`
146146

147147
error: unnecessary structure name repetition
148-
--> $DIR/use_self.rs:281:29
148+
--> $DIR/use_self.rs:282:29
149149
|
150150
LL | fn foo(value: T) -> Foo<T> {
151151
| ^^^^^^ help: use the applicable keyword: `Self`
152152

153153
error: unnecessary structure name repetition
154-
--> $DIR/use_self.rs:282:13
154+
--> $DIR/use_self.rs:283:13
155155
|
156156
LL | Foo { value }
157157
| ^^^ help: use the applicable keyword: `Self`
158158

159159
error: unnecessary structure name repetition
160-
--> $DIR/use_self.rs:319:21
160+
--> $DIR/use_self.rs:320:21
161161
|
162162
LL | type From = T::From;
163163
| ^^^^^^^ help: use the applicable keyword: `Self`
164164

165165
error: unnecessary structure name repetition
166-
--> $DIR/use_self.rs:320:19
166+
--> $DIR/use_self.rs:321:19
167167
|
168168
LL | type To = T::To;
169169
| ^^^^^ help: use the applicable keyword: `Self`
170170

171171
error: unnecessary structure name repetition
172-
--> $DIR/use_self.rs:453:13
172+
--> $DIR/use_self.rs:454:13
173173
|
174174
LL | A::new::<submod::B>(submod::B {})
175175
| ^ help: use the applicable keyword: `Self`

0 commit comments

Comments
 (0)