]>
Commit | Line | Data |
---|---|---|
1 | use clippy_utils::diagnostics::{span_lint, span_lint_and_then}; | |
2 | use rustc_ast::ast::{ | |
3 | self, Arm, AssocItem, AssocItemKind, Attribute, Block, FnDecl, Item, ItemKind, Local, Pat, PatKind, | |
4 | }; | |
5 | use rustc_ast::visit::{walk_block, walk_expr, walk_pat, Visitor}; | |
6 | use rustc_lint::{EarlyContext, EarlyLintPass}; | |
7 | use rustc_middle::lint::in_external_macro; | |
8 | use rustc_session::{declare_tool_lint, impl_lint_pass}; | |
9 | use rustc_span::source_map::Span; | |
10 | use rustc_span::sym; | |
11 | use rustc_span::symbol::{Ident, Symbol}; | |
12 | use std::cmp::Ordering; | |
13 | ||
14 | declare_clippy_lint! { | |
15 | /// ### What it does | |
16 | /// Checks for names that are very similar and thus confusing. | |
17 | /// | |
18 | /// ### Why is this bad? | |
19 | /// It's hard to distinguish between names that differ only | |
20 | /// by a single character. | |
21 | /// | |
22 | /// ### Example | |
23 | /// ```ignore | |
24 | /// let checked_exp = something; | |
25 | /// let checked_expr = something_else; | |
26 | /// ``` | |
27 | pub SIMILAR_NAMES, | |
28 | pedantic, | |
29 | "similarly named items and bindings" | |
30 | } | |
31 | ||
32 | declare_clippy_lint! { | |
33 | /// ### What it does | |
34 | /// Checks for too many variables whose name consists of a | |
35 | /// single character. | |
36 | /// | |
37 | /// ### Why is this bad? | |
38 | /// It's hard to memorize what a variable means without a | |
39 | /// descriptive name. | |
40 | /// | |
41 | /// ### Example | |
42 | /// ```ignore | |
43 | /// let (a, b, c, d, e, f, g) = (...); | |
44 | /// ``` | |
45 | pub MANY_SINGLE_CHAR_NAMES, | |
46 | pedantic, | |
47 | "too many single character bindings" | |
48 | } | |
49 | ||
50 | declare_clippy_lint! { | |
51 | /// ### What it does | |
52 | /// Checks if you have variables whose name consists of just | |
53 | /// underscores and digits. | |
54 | /// | |
55 | /// ### Why is this bad? | |
56 | /// It's hard to memorize what a variable means without a | |
57 | /// descriptive name. | |
58 | /// | |
59 | /// ### Example | |
60 | /// ```rust | |
61 | /// let _1 = 1; | |
62 | /// let ___1 = 1; | |
63 | /// let __1___2 = 11; | |
64 | /// ``` | |
65 | pub JUST_UNDERSCORES_AND_DIGITS, | |
66 | style, | |
67 | "unclear name" | |
68 | } | |
69 | ||
70 | #[derive(Copy, Clone)] | |
71 | pub struct NonExpressiveNames { | |
72 | pub single_char_binding_names_threshold: u64, | |
73 | } | |
74 | ||
75 | impl_lint_pass!(NonExpressiveNames => [SIMILAR_NAMES, MANY_SINGLE_CHAR_NAMES, JUST_UNDERSCORES_AND_DIGITS]); | |
76 | ||
77 | struct ExistingName { | |
78 | interned: Symbol, | |
79 | span: Span, | |
80 | len: usize, | |
81 | exemptions: &'static [&'static str], | |
82 | } | |
83 | ||
84 | struct SimilarNamesLocalVisitor<'a, 'tcx> { | |
85 | names: Vec<ExistingName>, | |
86 | cx: &'a EarlyContext<'tcx>, | |
87 | lint: &'a NonExpressiveNames, | |
88 | ||
89 | /// A stack of scopes containing the single-character bindings in each scope. | |
90 | single_char_names: Vec<Vec<Ident>>, | |
91 | } | |
92 | ||
93 | impl<'a, 'tcx> SimilarNamesLocalVisitor<'a, 'tcx> { | |
94 | fn check_single_char_names(&self) { | |
95 | let num_single_char_names = self.single_char_names.iter().flatten().count(); | |
96 | let threshold = self.lint.single_char_binding_names_threshold; | |
97 | if num_single_char_names as u64 > threshold { | |
98 | let span = self | |
99 | .single_char_names | |
100 | .iter() | |
101 | .flatten() | |
102 | .map(|ident| ident.span) | |
103 | .collect::<Vec<_>>(); | |
104 | span_lint( | |
105 | self.cx, | |
106 | MANY_SINGLE_CHAR_NAMES, | |
107 | span, | |
108 | &format!( | |
109 | "{} bindings with single-character names in scope", | |
110 | num_single_char_names | |
111 | ), | |
112 | ); | |
113 | } | |
114 | } | |
115 | } | |
116 | ||
117 | // this list contains lists of names that are allowed to be similar | |
118 | // the assumption is that no name is ever contained in multiple lists. | |
119 | #[rustfmt::skip] | |
120 | const ALLOWED_TO_BE_SIMILAR: &[&[&str]] = &[ | |
121 | &["parsed", "parser"], | |
122 | &["lhs", "rhs"], | |
123 | &["tx", "rx"], | |
124 | &["set", "get"], | |
125 | &["args", "arms"], | |
126 | &["qpath", "path"], | |
127 | &["lit", "lint"], | |
128 | &["wparam", "lparam"], | |
129 | &["iter", "item"], | |
130 | ]; | |
131 | ||
132 | struct SimilarNamesNameVisitor<'a, 'tcx, 'b>(&'b mut SimilarNamesLocalVisitor<'a, 'tcx>); | |
133 | ||
134 | impl<'a, 'tcx, 'b> Visitor<'tcx> for SimilarNamesNameVisitor<'a, 'tcx, 'b> { | |
135 | fn visit_pat(&mut self, pat: &'tcx Pat) { | |
136 | match pat.kind { | |
137 | PatKind::Ident(_, ident, _) => { | |
138 | if !pat.span.from_expansion() { | |
139 | self.check_ident(ident); | |
140 | } | |
141 | }, | |
142 | PatKind::Struct(_, _, ref fields, _) => { | |
143 | for field in fields { | |
144 | if !field.is_shorthand { | |
145 | self.visit_pat(&field.pat); | |
146 | } | |
147 | } | |
148 | }, | |
149 | // just go through the first pattern, as either all patterns | |
150 | // bind the same bindings or rustc would have errored much earlier | |
151 | PatKind::Or(ref pats) => self.visit_pat(&pats[0]), | |
152 | _ => walk_pat(self, pat), | |
153 | } | |
154 | } | |
155 | } | |
156 | ||
157 | #[must_use] | |
158 | fn get_exemptions(interned_name: &str) -> Option<&'static [&'static str]> { | |
159 | for &list in ALLOWED_TO_BE_SIMILAR { | |
160 | if allowed_to_be_similar(interned_name, list) { | |
161 | return Some(list); | |
162 | } | |
163 | } | |
164 | None | |
165 | } | |
166 | ||
167 | #[must_use] | |
168 | fn allowed_to_be_similar(interned_name: &str, list: &[&str]) -> bool { | |
169 | list.iter() | |
170 | .any(|&name| interned_name.starts_with(name) || interned_name.ends_with(name)) | |
171 | } | |
172 | ||
173 | impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> { | |
174 | fn check_short_ident(&mut self, ident: Ident) { | |
175 | // Ignore shadowing | |
176 | if self | |
177 | .0 | |
178 | .single_char_names | |
179 | .iter() | |
180 | .flatten() | |
181 | .any(|id| id.name == ident.name) | |
182 | { | |
183 | return; | |
184 | } | |
185 | ||
186 | if let Some(scope) = &mut self.0.single_char_names.last_mut() { | |
187 | scope.push(ident); | |
188 | } | |
189 | } | |
190 | ||
191 | #[allow(clippy::too_many_lines)] | |
192 | fn check_ident(&mut self, ident: Ident) { | |
193 | let interned_name = ident.name.as_str(); | |
194 | if interned_name.chars().any(char::is_uppercase) { | |
195 | return; | |
196 | } | |
197 | if interned_name.chars().all(|c| c.is_digit(10) || c == '_') { | |
198 | span_lint( | |
199 | self.0.cx, | |
200 | JUST_UNDERSCORES_AND_DIGITS, | |
201 | ident.span, | |
202 | "consider choosing a more descriptive name", | |
203 | ); | |
204 | return; | |
205 | } | |
206 | if interned_name.starts_with('_') { | |
207 | // these bindings are typically unused or represent an ignored portion of a destructuring pattern | |
208 | return; | |
209 | } | |
210 | let count = interned_name.chars().count(); | |
211 | if count < 3 { | |
212 | if count == 1 { | |
213 | self.check_short_ident(ident); | |
214 | } | |
215 | return; | |
216 | } | |
217 | for existing_name in &self.0.names { | |
218 | if allowed_to_be_similar(&interned_name, existing_name.exemptions) { | |
219 | continue; | |
220 | } | |
221 | match existing_name.len.cmp(&count) { | |
222 | Ordering::Greater => { | |
223 | if existing_name.len - count != 1 | |
224 | || levenstein_not_1(&interned_name, &existing_name.interned.as_str()) | |
225 | { | |
226 | continue; | |
227 | } | |
228 | }, | |
229 | Ordering::Less => { | |
230 | if count - existing_name.len != 1 | |
231 | || levenstein_not_1(&existing_name.interned.as_str(), &interned_name) | |
232 | { | |
233 | continue; | |
234 | } | |
235 | }, | |
236 | Ordering::Equal => { | |
237 | let mut interned_chars = interned_name.chars(); | |
238 | let interned_str = existing_name.interned.as_str(); | |
239 | let mut existing_chars = interned_str.chars(); | |
240 | let first_i = interned_chars.next().expect("we know we have at least one char"); | |
241 | let first_e = existing_chars.next().expect("we know we have at least one char"); | |
242 | let eq_or_numeric = |(a, b): (char, char)| a == b || a.is_numeric() && b.is_numeric(); | |
243 | ||
244 | if eq_or_numeric((first_i, first_e)) { | |
245 | let last_i = interned_chars.next_back().expect("we know we have at least two chars"); | |
246 | let last_e = existing_chars.next_back().expect("we know we have at least two chars"); | |
247 | if eq_or_numeric((last_i, last_e)) { | |
248 | if interned_chars | |
249 | .zip(existing_chars) | |
250 | .filter(|&ie| !eq_or_numeric(ie)) | |
251 | .count() | |
252 | != 1 | |
253 | { | |
254 | continue; | |
255 | } | |
256 | } else { | |
257 | let second_last_i = interned_chars | |
258 | .next_back() | |
259 | .expect("we know we have at least three chars"); | |
260 | let second_last_e = existing_chars | |
261 | .next_back() | |
262 | .expect("we know we have at least three chars"); | |
263 | if !eq_or_numeric((second_last_i, second_last_e)) | |
264 | || second_last_i == '_' | |
265 | || !interned_chars.zip(existing_chars).all(eq_or_numeric) | |
266 | { | |
267 | // allowed similarity foo_x, foo_y | |
268 | // or too many chars differ (foo_x, boo_y) or (foox, booy) | |
269 | continue; | |
270 | } | |
271 | } | |
272 | } else { | |
273 | let second_i = interned_chars.next().expect("we know we have at least two chars"); | |
274 | let second_e = existing_chars.next().expect("we know we have at least two chars"); | |
275 | if !eq_or_numeric((second_i, second_e)) | |
276 | || second_i == '_' | |
277 | || !interned_chars.zip(existing_chars).all(eq_or_numeric) | |
278 | { | |
279 | // allowed similarity x_foo, y_foo | |
280 | // or too many chars differ (x_foo, y_boo) or (xfoo, yboo) | |
281 | continue; | |
282 | } | |
283 | } | |
284 | }, | |
285 | } | |
286 | span_lint_and_then( | |
287 | self.0.cx, | |
288 | SIMILAR_NAMES, | |
289 | ident.span, | |
290 | "binding's name is too similar to existing binding", | |
291 | |diag| { | |
292 | diag.span_note(existing_name.span, "existing binding defined here"); | |
293 | }, | |
294 | ); | |
295 | return; | |
296 | } | |
297 | self.0.names.push(ExistingName { | |
298 | exemptions: get_exemptions(&interned_name).unwrap_or(&[]), | |
299 | interned: ident.name, | |
300 | span: ident.span, | |
301 | len: count, | |
302 | }); | |
303 | } | |
304 | } | |
305 | ||
306 | impl<'a, 'b> SimilarNamesLocalVisitor<'a, 'b> { | |
307 | /// ensure scoping rules work | |
308 | fn apply<F: for<'c> Fn(&'c mut Self)>(&mut self, f: F) { | |
309 | let n = self.names.len(); | |
310 | let single_char_count = self.single_char_names.len(); | |
311 | f(self); | |
312 | self.names.truncate(n); | |
313 | self.single_char_names.truncate(single_char_count); | |
314 | } | |
315 | } | |
316 | ||
317 | impl<'a, 'tcx> Visitor<'tcx> for SimilarNamesLocalVisitor<'a, 'tcx> { | |
318 | fn visit_local(&mut self, local: &'tcx Local) { | |
319 | if let Some((init, els)) = &local.kind.init_else_opt() { | |
320 | self.apply(|this| walk_expr(this, init)); | |
321 | if let Some(els) = els { | |
322 | self.apply(|this| walk_block(this, els)); | |
323 | } | |
324 | } | |
325 | // add the pattern after the expression because the bindings aren't available | |
326 | // yet in the init | |
327 | // expression | |
328 | SimilarNamesNameVisitor(self).visit_pat(&*local.pat); | |
329 | } | |
330 | fn visit_block(&mut self, blk: &'tcx Block) { | |
331 | self.single_char_names.push(vec![]); | |
332 | ||
333 | self.apply(|this| walk_block(this, blk)); | |
334 | ||
335 | self.check_single_char_names(); | |
336 | self.single_char_names.pop(); | |
337 | } | |
338 | fn visit_arm(&mut self, arm: &'tcx Arm) { | |
339 | self.single_char_names.push(vec![]); | |
340 | ||
341 | self.apply(|this| { | |
342 | SimilarNamesNameVisitor(this).visit_pat(&arm.pat); | |
343 | this.apply(|this| walk_expr(this, &arm.body)); | |
344 | }); | |
345 | ||
346 | self.check_single_char_names(); | |
347 | self.single_char_names.pop(); | |
348 | } | |
349 | fn visit_item(&mut self, _: &Item) { | |
350 | // do not recurse into inner items | |
351 | } | |
352 | } | |
353 | ||
354 | impl EarlyLintPass for NonExpressiveNames { | |
355 | fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { | |
356 | if in_external_macro(cx.sess, item.span) { | |
357 | return; | |
358 | } | |
359 | ||
360 | if let ItemKind::Fn(box ast::Fn { ref sig, body: Some(ref blk), .. }) = item.kind { | |
361 | do_check(self, cx, &item.attrs, &sig.decl, blk); | |
362 | } | |
363 | } | |
364 | ||
365 | fn check_impl_item(&mut self, cx: &EarlyContext<'_>, item: &AssocItem) { | |
366 | if in_external_macro(cx.sess, item.span) { | |
367 | return; | |
368 | } | |
369 | ||
370 | if let AssocItemKind::Fn(box ast::Fn { ref sig, body: Some(ref blk), .. }) = item.kind { | |
371 | do_check(self, cx, &item.attrs, &sig.decl, blk); | |
372 | } | |
373 | } | |
374 | } | |
375 | ||
376 | fn do_check(lint: &mut NonExpressiveNames, cx: &EarlyContext<'_>, attrs: &[Attribute], decl: &FnDecl, blk: &Block) { | |
377 | if !attrs.iter().any(|attr| attr.has_name(sym::test)) { | |
378 | let mut visitor = SimilarNamesLocalVisitor { | |
379 | names: Vec::new(), | |
380 | cx, | |
381 | lint, | |
382 | single_char_names: vec![vec![]], | |
383 | }; | |
384 | ||
385 | // initialize with function arguments | |
386 | for arg in &decl.inputs { | |
387 | SimilarNamesNameVisitor(&mut visitor).visit_pat(&arg.pat); | |
388 | } | |
389 | // walk all other bindings | |
390 | walk_block(&mut visitor, blk); | |
391 | ||
392 | visitor.check_single_char_names(); | |
393 | } | |
394 | } | |
395 | ||
396 | /// Precondition: `a_name.chars().count() < b_name.chars().count()`. | |
397 | #[must_use] | |
398 | fn levenstein_not_1(a_name: &str, b_name: &str) -> bool { | |
399 | debug_assert!(a_name.chars().count() < b_name.chars().count()); | |
400 | let mut a_chars = a_name.chars(); | |
401 | let mut b_chars = b_name.chars(); | |
402 | while let (Some(a), Some(b)) = (a_chars.next(), b_chars.next()) { | |
403 | if a == b { | |
404 | continue; | |
405 | } | |
406 | if let Some(b2) = b_chars.next() { | |
407 | // check if there's just one character inserted | |
408 | return a != b2 || a_chars.ne(b_chars); | |
409 | } | |
410 | // tuple | |
411 | // ntuple | |
412 | return true; | |
413 | } | |
414 | // for item in items | |
415 | true | |
416 | } |