lint on too many single character bindings

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

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,

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<char>,
 }
 
 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 {

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() {

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;
 

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!(),
+        }
+    }
+}