diff --git a/src/non_expressive_names.rs b/src/non_expressive_names.rs index 87373c7a0..32118b11c 100644 --- a/src/non_expressive_names.rs +++ b/src/non_expressive_names.rs @@ -41,15 +41,25 @@ impl LintPass for NonExpressiveNames { } } +struct ExistingName { + interned: InternedString, + span: Span, + len: usize, + whitelist: &'static[&'static str], +} + struct SimilarNamesLocalVisitor<'a, 'b: 'a> { - names: Vec<(InternedString, Span, usize)>, + names: Vec, cx: &'a EarlyContext<'b>, lint: &'a NonExpressiveNames, single_char_names: Vec, } -const WHITELIST: &'static [&'static str] = &[ - "lhs", "rhs", +// this list contains lists of names that are allowed to be similar +// the assumption is that no name is ever contained in multiple lists. +const WHITELIST: &'static [&'static [&'static str]] = &[ + &["parsed", "parser"], + &["lhs", "rhs"], ]; struct SimilarNamesNameVisitor<'a, 'b: 'a, 'c: 'b>(&'a mut SimilarNamesLocalVisitor<'b, 'c>); @@ -63,21 +73,27 @@ impl<'v, 'a, 'b, 'c> visit::Visitor<'v> for SimilarNamesNameVisitor<'a, 'b, 'c> } } -fn whitelisted(interned_name: &str) -> bool { +fn get_whitelist(interned_name: &str) -> Option<&'static[&'static str]> { for &allow in WHITELIST { - if interned_name == allow { - return true; + if whitelisted(interned_name, allow) { + return Some(allow); } - if interned_name.len() <= allow.len() { - continue; - } - // allow_* - let allow_start = allow.chars().chain(Some('_')); + } + None +} + +fn whitelisted(interned_name: &str, list: &[&str]) -> bool { + if list.iter().any(|&name| interned_name == name) { + return true; + } + for name in list { + // name_* + let allow_start = name.chars().chain(Some('_')); if interned_name.chars().zip(allow_start).all(|(l, r)| l == r) { return true; } - // *_allow - let allow_end = Some('_').into_iter().chain(allow.chars()); + // *_name + let allow_end = Some('_').into_iter().chain(name.chars()); if interned_name.chars().rev().zip(allow_end.rev()).all(|(l, r)| l == r) { return true; } @@ -110,75 +126,58 @@ impl<'a, 'b, 'c> SimilarNamesNameVisitor<'a, 'b, 'c> { } let count = interned_name.chars().count(); if count < 3 { - if count != 1 { - return; + if count == 1 { + let c = interned_name.chars().next().expect("already checked"); + self.check_short_name(c, span); } - let c = interned_name.chars().next().expect("already checked"); - self.check_short_name(c, span); return; } - if whitelisted(&interned_name) { - return; - } - for &(ref existing_name, sp, existing_len) in &self.0.names { + for existing_name in &self.0.names { + if whitelisted(&interned_name, existing_name.whitelist) { + continue; + } let mut split_at = None; - if existing_len > count { - if existing_len - count != 1 { + if existing_name.len > count { + if existing_name.len - count != 1 || levenstein_not_1(&interned_name, &existing_name.interned) { continue; } - if levenstein_not_1(&interned_name, &existing_name) { - continue; - } - } else if existing_len < count { - if count - existing_len != 1 { - continue; - } - if levenstein_not_1(&existing_name, &interned_name) { + } else if existing_name.len < count { + if count - existing_name.len != 1 || levenstein_not_1(&existing_name.interned, &interned_name) { continue; } } else { let mut interned_chars = interned_name.chars(); - let mut existing_chars = existing_name.chars(); + let mut existing_chars = existing_name.interned.chars(); + let first_i = interned_chars.next().expect("we know we have at least one char"); + let first_e = existing_chars.next().expect("we know we have at least one char"); + let eq_or_numeric = |a: char, b: char| a == b || a.is_numeric() && b.is_numeric(); - if interned_chars.next() != existing_chars.next() { - let i = interned_chars.next().expect("we know we have more than 1 char"); - let e = existing_chars.next().expect("we know we have more than 1 char"); - if i == e { - if i == '_' { - // allowed similarity x_foo, y_foo - // or too many chars differ (x_foo, y_boo) + if eq_or_numeric(first_i, first_e) { + let last_i = interned_chars.next_back().expect("we know we have at least two chars"); + let last_e = existing_chars.next_back().expect("we know we have at least two chars"); + if eq_or_numeric(last_i, last_e) { + if interned_chars.zip(existing_chars).filter(|&(i, e)| !eq_or_numeric(i, e)).count() != 1 { continue; - } else if interned_chars.ne(existing_chars) { - // too many chars differ - continue } } else { - // too many chars differ + let second_last_i = interned_chars.next_back().expect("we know we have at least three chars"); + let second_last_e = existing_chars.next_back().expect("we know we have at least three chars"); + if !eq_or_numeric(second_last_i, second_last_e) || second_last_i == '_' || !interned_chars.zip(existing_chars).all(|(i, e)| eq_or_numeric(i, e)) { + // allowed similarity foo_x, foo_y + // or too many chars differ (foo_x, boo_y) or (foox, booy) + continue; + } + split_at = interned_name.char_indices().rev().next().map(|(i, _)| i); + } + } else { + let second_i = interned_chars.next().expect("we know we have at least two chars"); + let second_e = existing_chars.next().expect("we know we have at least two chars"); + if !eq_or_numeric(second_i, second_e) || second_i == '_' || !interned_chars.zip(existing_chars).all(|(i, e)| eq_or_numeric(i, e)) { + // allowed similarity x_foo, y_foo + // or too many chars differ (x_foo, y_boo) or (xfoo, yboo) continue; } split_at = interned_name.chars().next().map(|c| c.len_utf8()); - } else if interned_chars.next_back() == existing_chars.next_back() { - if interned_chars.zip(existing_chars).filter(|&(i, e)| i != e).count() != 1 { - // too many chars differ, or none differ (aka shadowing) - continue; - } - } else { - let i = interned_chars.next_back().expect("we know we have more than 2 chars"); - let e = existing_chars.next_back().expect("we know we have more than 2 chars"); - if i == e { - if i == '_' { - // allowed similarity foo_x, foo_x - // or too many chars differ (foo_x, boo_x) - continue; - } else if interned_chars.ne(existing_chars) { - // too many chars differ - continue - } - } else { - // too many chars differ - continue; - } - split_at = interned_name.char_indices().rev().next().map(|(i, _)| i); } } span_lint_and_then(self.0.cx, @@ -186,7 +185,7 @@ impl<'a, 'b, 'c> SimilarNamesNameVisitor<'a, 'b, 'c> { span, "binding's name is too similar to existing binding", |diag| { - diag.span_note(sp, "existing binding defined here"); + diag.span_note(existing_name.span, "existing binding defined here"); if let Some(split) = split_at { diag.span_help(span, &format!("separate the discriminating character \ by an underscore like: `{}_{}`", @@ -196,7 +195,12 @@ impl<'a, 'b, 'c> SimilarNamesNameVisitor<'a, 'b, 'c> { }); return; } - self.0.names.push((interned_name, span, count)); + self.0.names.push(ExistingName { + whitelist: get_whitelist(&interned_name).unwrap_or(&[]), + interned: interned_name, + span: span, + len: count, + }); } } diff --git a/tests/compile-fail/non_expressive_names.rs b/tests/compile-fail/non_expressive_names.rs index ac412fb44..aab88f742 100644 --- a/tests/compile-fail/non_expressive_names.rs +++ b/tests/compile-fail/non_expressive_names.rs @@ -10,6 +10,8 @@ //~| NOTE: lint level defined here //~| NOTE: lint level defined here //~| NOTE: lint level defined here +//~| NOTE: lint level defined here +//~| NOTE: lint level defined here #![allow(unused)] fn main() { @@ -67,6 +69,20 @@ fn main() { (cheese2, 2) => panic!(), _ => println!(""), } + let ipv4: i32; + let ipv6: i32; + let abcd1: i32; + let abdc2: i32; + let xyz1abc: i32; //~ NOTE: existing binding defined here + let xyz2abc: i32; + let xyzeabc: i32; //~ ERROR: name is too similar + //~| HELP: for further information visit + + let parser: i32; //~ NOTE: existing binding defined here + let parsed: i32; + let parsee: i32; //~ ERROR: name is too similar + //~| HELP: for further information visit + //~| HELP: separate the discriminating character by an underscore like: `parse_e` } #[derive(Clone, Debug)]