Merge pull request #813 from oli-obk/fix/non_expressive_names

Fix/non expressive names

Author: mcarton

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<ExistingName>,
     cx: &'a EarlyContext<'b>,
     lint: &'a NonExpressiveNames,
     single_char_names: Vec<char>,
 }
 
-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,83 +126,66 @@ 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 {
-                    continue;
-                }
-                if levenstein_not_1(&interned_name, &existing_name) {
+            if existing_name.len > count {
+                if existing_name.len - count != 1 || levenstein_not_1(&interned_name, &existing_name.interned) {
                     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
-                        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)
+                        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;
-                        } else if interned_chars.ne(existing_chars) {
-                            // too many chars differ
-                            continue
                         }
-                    } else {
-                        // too many chars differ
+                        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.char_indices().rev().next().map(|(i, _)| i);
+                    split_at = interned_name.chars().next().map(|c| c.len_utf8());
                 }
             }
             span_lint_and_then(self.0.cx,
                                SIMILAR_NAMES,
                                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,
+        });
     }
 }
 

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)]