Skip to content

Commit 0929a24

Browse files
borsbors
committed
Auto merge of #6828 - mgacek8:issue_6758_enhance_wrong_self_convention, r=flip1995
wrong_self_convention: fix lint in case of `to_*_mut` method fixes #6758 changelog: wrong_self_convention: fix lint in case of `to_*_mut` method. When a method starts with `to_` and ends with `_mut`, clippy expects a `&mut self` parameter, otherwise `&self`. Any feedback is welcome. I was also thinking if shouldn't we treat `to_` the same way as `as_`. Namely to accept `self` taken: `&self` or `&mut self`.
2 parents 9cd0f50 + 2547edb commit 0929a24

6 files changed

+189
-45
lines changed

‎clippy_lints/src/methods/mod.rs

+8-7
Original file line numberDiff line numberDiff line change
@@ -192,13 +192,14 @@ declare_clippy_lint! {
192192
/// **What it does:** Checks for methods with certain name prefixes and which
193193
/// doesn't match how self is taken. The actual rules are:
194194
///
195-
/// |Prefix |`self` taken |
196-
/// |-------|----------------------|
197-
/// |`as_` |`&self` or `&mut self`|
198-
/// |`from_`| none |
199-
/// |`into_`|`self` |
200-
/// |`is_` |`&self` or none |
201-
/// |`to_` |`&self` |
195+
/// |Prefix |Postfix |`self` taken |
196+
/// |-------|------------|----------------------|
197+
/// |`as_` | none |`&self` or `&mut self`|
198+
/// |`from_`| none | none |
199+
/// |`into_`| none |`self` |
200+
/// |`is_` | none |`&self` or none |
201+
/// |`to_` | `_mut` |`&mut &self` |
202+
/// |`to_` | not `_mut` |`&self` |
202203
///
203204
/// **Why is this bad?** Consistency breeds readability. If you follow the
204205
/// conventions, your users won't be surprised that they, e.g., need to supply a

‎clippy_lints/src/methods/wrong_self_convention.rs

+59-13
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use crate::methods::SelfKind;
2-
use crate::utils::span_lint;
2+
use crate::utils::span_lint_and_help;
33
use rustc_lint::LateContext;
44
use rustc_middle::ty::TyS;
55
use rustc_span::source_map::Span;
@@ -9,18 +9,22 @@ use super::WRONG_PUB_SELF_CONVENTION;
99
use super::WRONG_SELF_CONVENTION;
1010

1111
#[rustfmt::skip]
12-
const CONVENTIONS: [(Convention, &[SelfKind]); 7] = [
13-
(Convention::Eq("new"), &[SelfKind::No]),
14-
(Convention::StartsWith("as_"), &[SelfKind::Ref, SelfKind::RefMut]),
15-
(Convention::StartsWith("from_"), &[SelfKind::No]),
16-
(Convention::StartsWith("into_"), &[SelfKind::Value]),
17-
(Convention::StartsWith("is_"), &[SelfKind::Ref, SelfKind::No]),
18-
(Convention::Eq("to_mut"), &[SelfKind::RefMut]),
19-
(Convention::StartsWith("to_"), &[SelfKind::Ref]),
12+
const CONVENTIONS: [(&[Convention], &[SelfKind]); 8] = [
13+
(&[Convention::Eq("new")], &[SelfKind::No]),
14+
(&[Convention::StartsWith("as_")], &[SelfKind::Ref, SelfKind::RefMut]),
15+
(&[Convention::StartsWith("from_")], &[SelfKind::No]),
16+
(&[Convention::StartsWith("into_")], &[SelfKind::Value]),
17+
(&[Convention::StartsWith("is_")], &[SelfKind::Ref, SelfKind::No]),
18+
(&[Convention::Eq("to_mut")], &[SelfKind::RefMut]),
19+
(&[Convention::StartsWith("to_"), Convention::EndsWith("_mut")], &[SelfKind::RefMut]),
20+
(&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut")], &[SelfKind::Ref]),
2021
];
22+
2123
enum Convention {
2224
Eq(&'static str),
2325
StartsWith(&'static str),
26+
EndsWith(&'static str),
27+
NotEndsWith(&'static str),
2428
}
2529

2630
impl Convention {
@@ -29,6 +33,8 @@ impl Convention {
2933
match *self {
3034
Self::Eq(this) => this == other,
3135
Self::StartsWith(this) => other.starts_with(this) && this != other,
36+
Self::EndsWith(this) => other.ends_with(this) && this != other,
37+
Self::NotEndsWith(this) => !Self::EndsWith(this).check(other),
3238
}
3339
}
3440
}
@@ -38,6 +44,8 @@ impl fmt::Display for Convention {
3844
match *self {
3945
Self::Eq(this) => this.fmt(f),
4046
Self::StartsWith(this) => this.fmt(f).and_then(|_| '*'.fmt(f)),
47+
Self::EndsWith(this) => '*'.fmt(f).and_then(|_| this.fmt(f)),
48+
Self::NotEndsWith(this) => '~'.fmt(f).and_then(|_| this.fmt(f)),
4149
}
4250
}
4351
}
@@ -55,21 +63,59 @@ pub(super) fn check<'tcx>(
5563
} else {
5664
WRONG_SELF_CONVENTION
5765
};
58-
if let Some((ref conv, self_kinds)) = &CONVENTIONS.iter().find(|(ref conv, _)| conv.check(item_name)) {
66+
if let Some((conventions, self_kinds)) = &CONVENTIONS
67+
.iter()
68+
.find(|(convs, _)| convs.iter().all(|conv| conv.check(item_name)))
69+
{
5970
if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
60-
span_lint(
71+
let suggestion = {
72+
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
92+
.iter()
93+
.map(|c| format!("`{}`", &c.to_string()))
94+
.collect::<Vec<_>>()
95+
.join(" and ");
96+
97+
format!("methods called like this: ({})", &s)
98+
}
99+
} else {
100+
format!("methods called `{}`", &conventions[0])
101+
}
102+
};
103+
104+
span_lint_and_help(
61105
cx,
62106
lint,
63107
first_arg_span,
64108
&format!(
65-
"methods called `{}` usually take {}; consider choosing a less ambiguous name",
66-
conv,
109+
"{} usually take {}",
110+
suggestion,
67111
&self_kinds
68112
.iter()
69113
.map(|k| k.description())
70114
.collect::<Vec<_>>()
71115
.join(" or ")
72116
),
117+
None,
118+
"consider choosing a less ambiguous name",
73119
);
74120
}
75121
}

‎tests/ui/def_id_nocore.stderr

+2-1
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
1-
error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
1+
error: methods called `as_*` usually take self by reference or self by mutable reference
22
--> $DIR/def_id_nocore.rs:26:19
33
|
44
LL | pub fn as_ref(self) -> &'static str {
55
| ^^^^
66
|
77
= note: `-D clippy::wrong-self-convention` implied by `-D warnings`
8+
= help: consider choosing a less ambiguous name
89

910
error: aborting due to previous error
1011

‎tests/ui/wrong_self_convention.stderr

+71-24
Original file line numberDiff line numberDiff line change
@@ -1,148 +1,195 @@
1-
error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
1+
error: methods called `from_*` usually take no self
22
--> $DIR/wrong_self_convention.rs:18:17
33
|
44
LL | fn from_i32(self) {}
55
| ^^^^
66
|
77
= note: `-D clippy::wrong-self-convention` implied by `-D warnings`
8+
= help: consider choosing a less ambiguous name
89

9-
error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
10+
error: methods called `from_*` usually take no self
1011
--> $DIR/wrong_self_convention.rs:24:21
1112
|
1213
LL | pub fn from_i64(self) {}
1314
| ^^^^
15+
|
16+
= help: consider choosing a less ambiguous name
1417

15-
error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
18+
error: methods called `as_*` usually take self by reference or self by mutable reference
1619
--> $DIR/wrong_self_convention.rs:36:15
1720
|
1821
LL | fn as_i32(self) {}
1922
| ^^^^
23+
|
24+
= help: consider choosing a less ambiguous name
2025

21-
error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
26+
error: methods called `into_*` usually take self by value
2227
--> $DIR/wrong_self_convention.rs:38:17
2328
|
2429
LL | fn into_i32(&self) {}
2530
| ^^^^^
31+
|
32+
= help: consider choosing a less ambiguous name
2633

27-
error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name
34+
error: methods called `is_*` usually take self by reference or no self
2835
--> $DIR/wrong_self_convention.rs:40:15
2936
|
3037
LL | fn is_i32(self) {}
3138
| ^^^^
39+
|
40+
= help: consider choosing a less ambiguous name
3241

33-
error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
42+
error: methods called `to_*` usually take self by reference
3443
--> $DIR/wrong_self_convention.rs:42:15
3544
|
3645
LL | fn to_i32(self) {}
3746
| ^^^^
47+
|
48+
= help: consider choosing a less ambiguous name
3849

39-
error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
50+
error: methods called `from_*` usually take no self
4051
--> $DIR/wrong_self_convention.rs:44:17
4152
|
4253
LL | fn from_i32(self) {}
4354
| ^^^^
55+
|
56+
= help: consider choosing a less ambiguous name
4457

45-
error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
58+
error: methods called `as_*` usually take self by reference or self by mutable reference
4659
--> $DIR/wrong_self_convention.rs:46:19
4760
|
4861
LL | pub fn as_i64(self) {}
4962
| ^^^^
63+
|
64+
= help: consider choosing a less ambiguous name
5065

51-
error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
66+
error: methods called `into_*` usually take self by value
5267
--> $DIR/wrong_self_convention.rs:47:21
5368
|
5469
LL | pub fn into_i64(&self) {}
5570
| ^^^^^
71+
|
72+
= help: consider choosing a less ambiguous name
5673

57-
error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name
74+
error: methods called `is_*` usually take self by reference or no self
5875
--> $DIR/wrong_self_convention.rs:48:19
5976
|
6077
LL | pub fn is_i64(self) {}
6178
| ^^^^
79+
|
80+
= help: consider choosing a less ambiguous name
6281

63-
error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
82+
error: methods called `to_*` usually take self by reference
6483
--> $DIR/wrong_self_convention.rs:49:19
6584
|
6685
LL | pub fn to_i64(self) {}
6786
| ^^^^
87+
|
88+
= help: consider choosing a less ambiguous name
6889

69-
error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
90+
error: methods called `from_*` usually take no self
7091
--> $DIR/wrong_self_convention.rs:50:21
7192
|
7293
LL | pub fn from_i64(self) {}
7394
| ^^^^
95+
|
96+
= help: consider choosing a less ambiguous name
7497

75-
error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
98+
error: methods called `as_*` usually take self by reference or self by mutable reference
7699
--> $DIR/wrong_self_convention.rs:95:19
77100
|
78101
LL | fn as_i32(self) {}
79102
| ^^^^
103+
|
104+
= help: consider choosing a less ambiguous name
80105

81-
error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
106+
error: methods called `into_*` usually take self by value
82107
--> $DIR/wrong_self_convention.rs:98:25
83108
|
84109
LL | fn into_i32_ref(&self) {}
85110
| ^^^^^
111+
|
112+
= help: consider choosing a less ambiguous name
86113

87-
error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name
114+
error: methods called `is_*` usually take self by reference or no self
88115
--> $DIR/wrong_self_convention.rs:100:19
89116
|
90117
LL | fn is_i32(self) {}
91118
| ^^^^
119+
|
120+
= help: consider choosing a less ambiguous name
92121

93-
error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
122+
error: methods called `to_*` usually take self by reference
94123
--> $DIR/wrong_self_convention.rs:102:19
95124
|
96125
LL | fn to_i32(self) {}
97126
| ^^^^
127+
|
128+
= help: consider choosing a less ambiguous name
98129

99-
error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
130+
error: methods called `from_*` usually take no self
100131
--> $DIR/wrong_self_convention.rs:104:21
101132
|
102133
LL | fn from_i32(self) {}
103134
| ^^^^
135+
|
136+
= help: consider choosing a less ambiguous name
104137

105-
error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
138+
error: methods called `as_*` usually take self by reference or self by mutable reference
106139
--> $DIR/wrong_self_convention.rs:119:19
107140
|
108141
LL | fn as_i32(self);
109142
| ^^^^
143+
|
144+
= help: consider choosing a less ambiguous name
110145

111-
error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
146+
error: methods called `into_*` usually take self by value
112147
--> $DIR/wrong_self_convention.rs:122:25
113148
|
114149
LL | fn into_i32_ref(&self);
115150
| ^^^^^
151+
|
152+
= help: consider choosing a less ambiguous name
116153

117-
error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name
154+
error: methods called `is_*` usually take self by reference or no self
118155
--> $DIR/wrong_self_convention.rs:124:19
119156
|
120157
LL | fn is_i32(self);
121158
| ^^^^
159+
|
160+
= help: consider choosing a less ambiguous name
122161

123-
error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
162+
error: methods called `to_*` usually take self by reference
124163
--> $DIR/wrong_self_convention.rs:126:19
125164
|
126165
LL | fn to_i32(self);
127166
| ^^^^
167+
|
168+
= help: consider choosing a less ambiguous name
128169

129-
error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
170+
error: methods called `from_*` usually take no self
130171
--> $DIR/wrong_self_convention.rs:128:21
131172
|
132173
LL | fn from_i32(self);
133174
| ^^^^
175+
|
176+
= help: consider choosing a less ambiguous name
134177

135-
error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
178+
error: methods called `into_*` usually take self by value
136179
--> $DIR/wrong_self_convention.rs:146:25
137180
|
138181
LL | fn into_i32_ref(&self);
139182
| ^^^^^
183+
|
184+
= help: consider choosing a less ambiguous name
140185

141-
error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
186+
error: methods called `from_*` usually take no self
142187
--> $DIR/wrong_self_convention.rs:152:21
143188
|
144189
LL | fn from_i32(self);
145190
| ^^^^
191+
|
192+
= help: consider choosing a less ambiguous name
146193

147194
error: aborting due to 24 previous errors
148195

0 commit comments

Comments
 (0)