From 463897fd399482bd99fa80871269139f13d740c8 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 1 Mar 2016 13:05:39 +0100 Subject: [PATCH] lint on too many single character bindings --- README.md | 1 + src/lib.rs | 6 +- src/non_expressive_names.rs | 66 ++++++++++++++++++---- tests/compile-fail/eta.rs | 2 +- tests/compile-fail/for_loop.rs | 1 + tests/compile-fail/non_expressive_names.rs | 27 +++++++++ 6 files changed, 91 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 2ee4dfc5e..65fc097af 100644 --- a/README.md +++ b/README.md @@ -74,6 +74,7 @@ name [let_unit_value](https://github.com/Manishearth/rust-clippy/wiki#let_unit_value) | warn | creating a let binding to a value of unit type, which usually can't be used afterwards [linkedlist](https://github.com/Manishearth/rust-clippy/wiki#linkedlist) | warn | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a VecDeque [manual_swap](https://github.com/Manishearth/rust-clippy/wiki#manual_swap) | warn | manual swap +[many_single_char_names](https://github.com/Manishearth/rust-clippy/wiki#many_single_char_names) | warn | too many single character bindings [map_clone](https://github.com/Manishearth/rust-clippy/wiki#map_clone) | warn | using `.map(|x| x.clone())` to clone an iterator or option's contents (recommends `.cloned()` instead) [map_entry](https://github.com/Manishearth/rust-clippy/wiki#map_entry) | warn | use of `contains_key` followed by `insert` on a `HashMap` or `BTreeMap` [match_bool](https://github.com/Manishearth/rust-clippy/wiki#match_bool) | warn | a match on boolean expression; recommends `if..else` block instead diff --git a/src/lib.rs b/src/lib.rs index ef6b1f3a2..82383e2b7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -204,7 +204,10 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_late_lint_pass(box types::CharLitAsU8); reg.register_late_lint_pass(box print::PrintLint); reg.register_late_lint_pass(box vec::UselessVec); - reg.register_early_lint_pass(box non_expressive_names::SimilarNames(1)); + reg.register_early_lint_pass(box non_expressive_names::NonExpressiveNames { + similarity_threshold: 1, + max_single_char_names: 5, + }); reg.register_late_lint_pass(box drop_ref::DropRefPass); reg.register_late_lint_pass(box types::AbsurdExtremeComparisons); reg.register_late_lint_pass(box regex::RegexPass::default()); @@ -331,6 +334,7 @@ pub fn plugin_registrar(reg: &mut Registry) { needless_update::NEEDLESS_UPDATE, new_without_default::NEW_WITHOUT_DEFAULT, no_effect::NO_EFFECT, + non_expressive_names::MANY_SINGLE_CHAR_NAMES, non_expressive_names::SIMILAR_NAMES, open_options::NONSENSICAL_OPEN_OPTIONS, overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL, diff --git a/src/non_expressive_names.rs b/src/non_expressive_names.rs index fc8bb69c5..7f2fc618e 100644 --- a/src/non_expressive_names.rs +++ b/src/non_expressive_names.rs @@ -3,7 +3,7 @@ use syntax::codemap::Span; use syntax::parse::token::InternedString; use syntax::ast::*; use syntax::visit::{self, FnKind}; -use utils::{span_lint_and_then, in_macro}; +use utils::{span_lint_and_then, in_macro, span_lint}; use strsim::levenshtein; /// **What it does:** This lint warns about names that are very similar and thus confusing @@ -19,18 +19,35 @@ declare_lint! { "similarly named items and bindings" } -pub struct SimilarNames(pub usize); +/// **What it does:** This lint warns about having too many variables whose name consists of a single character +/// +/// **Why is this bad?** It's hard to memorize what a variable means without a descriptive name. +/// +/// **Known problems:** None? +/// +/// **Example:** let (a, b, c, d, e, f, g) = (...); +declare_lint! { + pub MANY_SINGLE_CHAR_NAMES, + Warn, + "too many single character bindings" +} -impl LintPass for SimilarNames { +pub struct NonExpressiveNames { + pub similarity_threshold: usize, + pub max_single_char_names: usize, +} + +impl LintPass for NonExpressiveNames { fn get_lints(&self) -> LintArray { - lint_array!(SIMILAR_NAMES) + lint_array!(SIMILAR_NAMES, MANY_SINGLE_CHAR_NAMES) } } struct SimilarNamesLocalVisitor<'a, 'b: 'a> { names: Vec<(InternedString, Span)>, cx: &'a EarlyContext<'b>, - limit: usize, + lint: &'a NonExpressiveNames, + single_char_names: Vec, } const WHITELIST: &'static [&'static str] = &[ @@ -57,7 +74,15 @@ impl<'a, 'b, 'c> SimilarNamesNameVisitor<'a, 'b, 'c> { if interned_name.chars().any(char::is_uppercase) { return; } - if interned_name.chars().count() < 3 { + let count = interned_name.chars().count(); + if count < 3 { + if count == 1 { + let c = interned_name.chars().next().expect("already checked"); + // make sure we ignore shadowing + if !self.0.single_char_names.contains(&c) { + self.0.single_char_names.push(c); + } + } return; } for &allow in WHITELIST { @@ -85,7 +110,7 @@ impl<'a, 'b, 'c> SimilarNamesNameVisitor<'a, 'b, 'c> { continue; } // if they differ enough it's all good - if dist > self.0.limit { + if dist > self.0.lint.similarity_threshold { continue; } // are we doing stuff like `for item in items`? @@ -119,7 +144,8 @@ impl<'a, 'b, 'c> SimilarNamesNameVisitor<'a, 'b, 'c> { |diag| { diag.span_note(sp, "existing binding defined here"); if let Some(split) = split_at { - diag.span_help(span, &format!("separate the discriminating character by an underscore like: `{}_{}`", + diag.span_help(span, &format!("separate the discriminating character \ + by an underscore like: `{}_{}`", &interned_name[..split], &interned_name[split..])); } @@ -130,6 +156,19 @@ impl<'a, 'b, 'c> SimilarNamesNameVisitor<'a, 'b, 'c> { } } +impl<'a, 'b> SimilarNamesLocalVisitor<'a, 'b> { + fn check_single_char_count(&self, span: Span) { + if self.single_char_names.len() < self.lint.max_single_char_names { + return; + } + span_lint(self.cx, + MANY_SINGLE_CHAR_NAMES, + span, + &format!("scope contains {} bindings whose name are just one char", + self.single_char_names.len())); + } +} + impl<'v, 'a, 'b> visit::Visitor<'v> for SimilarNamesLocalVisitor<'a, 'b> { fn visit_local(&mut self, local: &'v Local) { SimilarNamesNameVisitor(self).visit_local(local) @@ -137,26 +176,33 @@ impl<'v, 'a, 'b> visit::Visitor<'v> for SimilarNamesLocalVisitor<'a, 'b> { fn visit_block(&mut self, blk: &'v Block) { // ensure scoping rules work let n = self.names.len(); + let single_char_count = self.single_char_names.len(); visit::walk_block(self, blk); self.names.truncate(n); + self.check_single_char_count(blk.span); + self.single_char_names.truncate(single_char_count); } fn visit_arm(&mut self, arm: &'v Arm) { let n = self.names.len(); + let single_char_count = self.single_char_names.len(); // just go through the first pattern, as either all patterns bind the same bindings or rustc would have errored much earlier SimilarNamesNameVisitor(self).visit_pat(&arm.pats[0]); self.names.truncate(n); + self.check_single_char_count(arm.body.span); + self.single_char_names.truncate(single_char_count); } fn visit_item(&mut self, _: &'v Item) { // do nothing } } -impl EarlyLintPass for SimilarNames { +impl EarlyLintPass for NonExpressiveNames { fn check_fn(&mut self, cx: &EarlyContext, _: FnKind, decl: &FnDecl, blk: &Block, _: Span, _: NodeId) { let mut visitor = SimilarNamesLocalVisitor { names: Vec::new(), cx: cx, - limit: self.0, + lint: &self, + single_char_names: Vec::new(), }; // initialize with function arguments for arg in &decl.inputs { diff --git a/tests/compile-fail/eta.rs b/tests/compile-fail/eta.rs index 0e72efe65..3fd089bf5 100644 --- a/tests/compile-fail/eta.rs +++ b/tests/compile-fail/eta.rs @@ -1,6 +1,6 @@ #![feature(plugin)] #![plugin(clippy)] -#![allow(unknown_lints, unused, no_effect, redundant_closure_call)] +#![allow(unknown_lints, unused, no_effect, redundant_closure_call, many_single_char_names)] #![deny(redundant_closure)] fn main() { diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index b111439ba..064f66537 100644 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -89,6 +89,7 @@ impl Unrelated { #[deny(needless_range_loop, explicit_iter_loop, iter_next_loop, reverse_range_loop, explicit_counter_loop)] #[deny(unused_collect)] #[allow(linkedlist, shadow_unrelated, unnecessary_mut_passed, cyclomatic_complexity, similar_names)] +#[allow(many_single_char_names)] fn main() { const MAX_LEN: usize = 42; diff --git a/tests/compile-fail/non_expressive_names.rs b/tests/compile-fail/non_expressive_names.rs index 35dba1a82..9c75b0735 100644 --- a/tests/compile-fail/non_expressive_names.rs +++ b/tests/compile-fail/non_expressive_names.rs @@ -35,3 +35,30 @@ fn main() { let cakes: i32; let coke: i32; //~ ERROR: name is too similar } + + +fn bla() { + let a: i32; + let (b, c, d): (i32, i64, i16); + { + { + let cdefg: i32; + let blar: i32; + } + { //~ ERROR: scope contains 5 bindings whose name are just one char + let e: i32; + } + { //~ ERROR: scope contains 6 bindings whose name are just one char + let e: i32; + let f: i32; + } + match 5 { + 1 => println!(""), + e => panic!(), //~ ERROR: scope contains 5 bindings whose name are just one char + } + match 5 { + 1 => println!(""), + _ => panic!(), + } + } +}