Auto merge of #3921 - euclio:single-char-names-multispan, r=flip1995

bors · bors · commit 4fdd113bedb4 · 2019-04-08T08:42:13.000Z
use a multispan for MANY_SINGLE_CHAR_NAMES
diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs
@@ -88,7 +88,33 @@ struct SimilarNamesLocalVisitor<'a, 'tcx: 'a> {
     names: Vec<ExistingName>,
     cx: &'a EarlyContext<'tcx>,
     lint: &'a NonExpressiveNames,
-    single_char_names: Vec<char>,
+
+    /// A stack of scopes containing the single-character bindings in each scope.
+    single_char_names: Vec<Vec<Ident>>,
+}
+
+impl<'a, 'tcx: 'a> SimilarNamesLocalVisitor<'a, 'tcx> {
+    fn check_single_char_names(&self) {
+        let num_single_char_names = self.single_char_names.iter().flatten().count();
+        let threshold = self.lint.single_char_binding_names_threshold;
+        if num_single_char_names as u64 >= threshold {
+            let span = self
+                .single_char_names
+                .iter()
+                .flatten()
+                .map(|ident| ident.span)
+                .collect::<Vec<_>>();
+            span_lint(
+                self.cx,
+                MANY_SINGLE_CHAR_NAMES,
+                span,
+                &format!(
+                    "{} bindings with single-character names in scope",
+                    num_single_char_names
+                ),
+            );
+        }
+    }
 }
 
 // this list contains lists of names that are allowed to be similar
@@ -109,7 +135,7 @@ struct SimilarNamesNameVisitor<'a: 'b, 'tcx: 'a, 'b>(&'b mut SimilarNamesLocalVi
 impl<'a, 'tcx: 'a, 'b> Visitor<'tcx> for SimilarNamesNameVisitor<'a, 'tcx, 'b> {
     fn visit_pat(&mut self, pat: &'tcx Pat) {
         match pat.node {
-            PatKind::Ident(_, ident, _) => self.check_name(ident.span, ident.name),
+            PatKind::Ident(_, ident, _) => self.check_ident(ident),
             PatKind::Struct(_, ref fields, _) => {
                 for field in fields {
                     if !field.node.is_shorthand {
@@ -140,44 +166,40 @@ fn whitelisted(interned_name: &str, list: &[&str]) -> bool {
 }
 
 impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> {
-    fn check_short_name(&mut self, c: char, span: Span) {
-        // make sure we ignore shadowing
-        if self.0.single_char_names.contains(&c) {
+    fn check_short_ident(&mut self, ident: Ident) {
+        // Ignore shadowing
+        if self
+            .0
+            .single_char_names
+            .iter()
+            .flatten()
+            .any(|id| id.name == ident.name)
+        {
             return;
-        }
-        self.0.single_char_names.push(c);
-        if self.0.single_char_names.len() as u64 >= self.0.lint.single_char_binding_names_threshold {
-            span_lint(
-                self.0.cx,
-                MANY_SINGLE_CHAR_NAMES,
-                span,
-                &format!(
-                    "{}th binding whose name is just one char",
-                    self.0.single_char_names.len()
-                ),
-            );
+        } else if let Some(scope) = &mut self.0.single_char_names.last_mut() {
+            scope.push(ident);
         }
     }
+
     #[allow(clippy::too_many_lines)]
-    fn check_name(&mut self, span: Span, name: Name) {
-        let interned_name = name.as_str();
+    fn check_ident(&mut self, ident: Ident) {
+        let interned_name = ident.name.as_str();
         if interned_name.chars().any(char::is_uppercase) {
             return;
         }
         if interned_name.chars().all(|c| c.is_digit(10) || c == '_') {
             span_lint(
                 self.0.cx,
                 JUST_UNDERSCORES_AND_DIGITS,
-                span,
+                ident.span,
                 "consider choosing a more descriptive name",
             );
             return;
         }
         let count = interned_name.chars().count();
         if count < 3 {
             if count == 1 {
-                let c = interned_name.chars().next().expect("already checked");
-                self.check_short_name(c, span);
+                self.check_short_ident(ident);
             }
             return;
         }
@@ -247,13 +269,13 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> {
             span_lint_and_then(
                 self.0.cx,
                 SIMILAR_NAMES,
-                span,
+                ident.span,
                 "binding's name is too similar to existing binding",
                 |diag| {
                     diag.span_note(existing_name.span, "existing binding defined here");
                     if let Some(split) = split_at {
                         diag.span_help(
-                            span,
+                            ident.span,
                             &format!(
                                 "separate the discriminating character by an \
                                  underscore like: `{}_{}`",
@@ -269,7 +291,7 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> {
         self.0.names.push(ExistingName {
             whitelist: get_whitelist(&interned_name).unwrap_or(&[]),
             interned: interned_name,
-            span,
+            span: ident.span,
             len: count,
         });
     }
@@ -297,15 +319,25 @@ impl<'a, 'tcx> Visitor<'tcx> for SimilarNamesLocalVisitor<'a, 'tcx> {
         SimilarNamesNameVisitor(self).visit_pat(&*local.pat);
     }
     fn visit_block(&mut self, blk: &'tcx Block) {
+        self.single_char_names.push(vec![]);
+
         self.apply(|this| walk_block(this, blk));
+
+        self.check_single_char_names();
+        self.single_char_names.pop();
     }
     fn visit_arm(&mut self, arm: &'tcx Arm) {
+        self.single_char_names.push(vec![]);
+
         self.apply(|this| {
             // just go through the first pattern, as either all patterns
             // bind the same bindings or rustc would have errored much earlier
             SimilarNamesNameVisitor(this).visit_pat(&arm.pats[0]);
             this.apply(|this| walk_expr(this, &arm.body));
         });
+
+        self.check_single_char_names();
+        self.single_char_names.pop();
     }
     fn visit_item(&mut self, _: &Item) {
         // do not recurse into inner items
@@ -335,14 +367,17 @@ fn do_check(lint: &mut NonExpressiveNames, cx: &EarlyContext<'_>, attrs: &[Attri
             names: Vec::new(),
             cx,
             lint,
-            single_char_names: Vec::new(),
+            single_char_names: vec![vec![]],
         };
+
         // initialize with function arguments
         for arg in &decl.inputs {
             SimilarNamesNameVisitor(&mut visitor).visit_pat(&arg.pat);
         }
         // walk all other bindings
         walk_block(&mut visitor, blk);
+
+        visitor.check_single_char_names();
     }
 }
 
diff --git a/clippy_lints/src/utils/diagnostics.rs b/clippy_lints/src/utils/diagnostics.rs
@@ -5,7 +5,7 @@ use rustc::lint::{LateContext, Lint, LintContext};
 use rustc_errors::{Applicability, CodeSuggestion, Substitution, SubstitutionPart, SuggestionStyle};
 use std::env;
 use syntax::errors::DiagnosticBuilder;
-use syntax::source_map::Span;
+use syntax::source_map::{MultiSpan, Span};
 
 /// Wrapper around `DiagnosticBuilder` that adds a link to Clippy documentation for the emitted lint
 struct DiagnosticWrapper<'a>(DiagnosticBuilder<'a>);
@@ -48,7 +48,7 @@ impl<'a> DiagnosticWrapper<'a> {
 /// 17 |     std::mem::forget(seven);
 ///    |     ^^^^^^^^^^^^^^^^^^^^^^^
 /// ```
-pub fn span_lint<'a, T: LintContext<'a>>(cx: &T, lint: &'static Lint, sp: Span, msg: &str) {
+pub fn span_lint<'a, T: LintContext<'a>>(cx: &T, lint: &'static Lint, sp: impl Into<MultiSpan>, msg: &str) {
     DiagnosticWrapper(cx.struct_span_lint(lint, sp, msg)).docs_link(lint);
 }
 
diff --git a/tests/ui/non_expressive_names.rs b/tests/ui/non_expressive_names.rs
@@ -49,6 +49,45 @@ fn bla() {
     }
 }
 
+fn bindings(a: i32, b: i32, c: i32, d: i32, e: i32, f: i32, g: i32, h: i32) {}
+
+fn bindings2() {
+    let (a, b, c, d, e, f, g, h) = unimplemented!();
+}
+
+fn shadowing() {
+    let a = 0i32;
+    let a = 0i32;
+    let a = 0i32;
+    let a = 0i32;
+    let a = 0i32;
+    let a = 0i32;
+    {
+        let a = 0i32;
+    }
+}
+
+fn patterns() {
+    enum Z {
+        A(i32),
+        B(i32),
+        C(i32),
+        D(i32),
+        E(i32),
+        F(i32),
+    }
+
+    // These should not trigger a warning, since the pattern bindings are a new scope.
+    match Z::A(0) {
+        Z::A(a) => {},
+        Z::B(b) => {},
+        Z::C(c) => {},
+        Z::D(d) => {},
+        Z::E(e) => {},
+        Z::F(f) => {},
+    }
+}
+
 fn underscores_and_numbers() {
     let _1 = 1; //~ERROR Consider a more descriptive name
     let ____1 = 1; //~ERROR Consider a more descriptive name
diff --git a/tests/ui/non_expressive_names.stderr b/tests/ui/non_expressive_names.stderr
@@ -1,66 +1,89 @@
-error: 5th binding whose name is just one char
-  --> $DIR/non_expressive_names.rs:35:17
+error: 5 bindings with single-character names in scope
+  --> $DIR/non_expressive_names.rs:27:9
    |
+LL |     let a: i32;
+   |         ^
+LL |     let (b, c, d): (i32, i64, i16);
+   |          ^  ^  ^
+...
 LL |             let e: i32;
    |                 ^
    |
    = note: `-D clippy::many-single-char-names` implied by `-D warnings`
 
-error: 5th binding whose name is just one char
-  --> $DIR/non_expressive_names.rs:38:17
+error: 6 bindings with single-character names in scope
+  --> $DIR/non_expressive_names.rs:27:9
    |
+LL |     let a: i32;
+   |         ^
+LL |     let (b, c, d): (i32, i64, i16);
+   |          ^  ^  ^
+...
 LL |             let e: i32;
    |                 ^
-
-error: 6th binding whose name is just one char
-  --> $DIR/non_expressive_names.rs:39:17
-   |
 LL |             let f: i32;
    |                 ^
 
-error: 5th binding whose name is just one char
-  --> $DIR/non_expressive_names.rs:43:13
+error: 5 bindings with single-character names in scope
+  --> $DIR/non_expressive_names.rs:27:9
    |
+LL |     let a: i32;
+   |         ^
+LL |     let (b, c, d): (i32, i64, i16);
+   |          ^  ^  ^
+...
 LL |             e => panic!(),
    |             ^
 
+error: 8 bindings with single-character names in scope
+  --> $DIR/non_expressive_names.rs:52:13
+   |
+LL | fn bindings(a: i32, b: i32, c: i32, d: i32, e: i32, f: i32, g: i32, h: i32) {}
+   |             ^       ^       ^       ^       ^       ^       ^       ^
+
+error: 8 bindings with single-character names in scope
+  --> $DIR/non_expressive_names.rs:55:10
+   |
+LL |     let (a, b, c, d, e, f, g, h) = unimplemented!();
+   |          ^  ^  ^  ^  ^  ^  ^  ^
+
 error: consider choosing a more descriptive name
-  --> $DIR/non_expressive_names.rs:53:9
+  --> $DIR/non_expressive_names.rs:92:9
    |
 LL |     let _1 = 1; //~ERROR Consider a more descriptive name
    |         ^^
    |
    = note: `-D clippy::just-underscores-and-digits` implied by `-D warnings`
 
 error: consider choosing a more descriptive name
-  --> $DIR/non_expressive_names.rs:54:9
+  --> $DIR/non_expressive_names.rs:93:9
    |
 LL |     let ____1 = 1; //~ERROR Consider a more descriptive name
    |         ^^^^^
 
 error: consider choosing a more descriptive name
-  --> $DIR/non_expressive_names.rs:55:9
+  --> $DIR/non_expressive_names.rs:94:9
    |
 LL |     let __1___2 = 12; //~ERROR Consider a more descriptive name
    |         ^^^^^^^
 
 error: consider choosing a more descriptive name
-  --> $DIR/non_expressive_names.rs:75:13
+  --> $DIR/non_expressive_names.rs:114:13
    |
 LL |         let _1 = 1;
    |             ^^
 
 error: consider choosing a more descriptive name
-  --> $DIR/non_expressive_names.rs:76:13
+  --> $DIR/non_expressive_names.rs:115:13
    |
 LL |         let ____1 = 1;
    |             ^^^^^
 
 error: consider choosing a more descriptive name
-  --> $DIR/non_expressive_names.rs:77:13
+  --> $DIR/non_expressive_names.rs:116:13
    |
 LL |         let __1___2 = 12;
    |             ^^^^^^^
 
-error: aborting due to 10 previous errors
+error: aborting due to 11 previous errors
 
diff --git a/tests/ui/non_expressive_names.stdout b/tests/ui/non_expressive_names.stdout
