Skip to content

Fix/non expressive names #813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 30, 2016
Merged

Fix/non expressive names #813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7765224
minor code readability improvements
oli-obk Mar 30, 2016
d3362a2
don't lint on binding names where only a numeric char changes to anot…
oli-obk Mar 30, 2016
f03d93e
better whitelisting of "confusable" binding names
oli-obk Mar 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 70 additions & 66 deletions src/non_expressive_names.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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: `{}_{}`",
Expand All @@ -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,
});
}
}

Expand Down
16 changes: 16 additions & 0 deletions tests/compile-fail/non_expressive_names.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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)]
Expand Down