From 0d50d44ea6519f52706db59a6110ddbe82736b35 Mon Sep 17 00:00:00 2001 From: Andy Russell Date: Wed, 3 Apr 2019 15:06:39 -0400 Subject: [PATCH] use a multispan for MANY_SINGLE_CHAR_NAMES --- clippy_lints/src/non_expressive_names.rs | 87 +++++++++++++++++------- clippy_lints/src/utils/diagnostics.rs | 4 +- tests/ui/non_expressive_names.rs | 39 +++++++++++ tests/ui/non_expressive_names.stderr | 57 +++++++++++----- tests/ui/non_expressive_names.stdout | 0 5 files changed, 142 insertions(+), 45 deletions(-) create mode 100644 tests/ui/non_expressive_names.stdout diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs index 7bdeec0a56f..39d69d0bd51 100644 --- 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, cx: &'a EarlyContext<'tcx>, lint: &'a NonExpressiveNames, - single_char_names: Vec, + + /// A stack of scopes containing the single-character bindings in each scope. + single_char_names: Vec>, +} + +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::>(); + 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,27 +166,24 @@ 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; } @@ -168,7 +191,7 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> { span_lint( self.0.cx, JUST_UNDERSCORES_AND_DIGITS, - span, + ident.span, "consider choosing a more descriptive name", ); return; @@ -176,8 +199,7 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> { 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 index fa9a7dac9a0..5be0a67c64d 100644 --- 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, 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 index cd158287ff7..301ff276120 100644 --- 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 index a81ecdfe4de..b848c07667c 100644 --- a/tests/ui/non_expressive_names.stderr +++ b/tests/ui/non_expressive_names.stderr @@ -1,31 +1,54 @@ -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 | ^^ @@ -33,34 +56,34 @@ 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 new file mode 100644 index 00000000000..e69de29bb2d