lint on binding-names that are too similar

This commit is contained in:
Oliver Schneider 2016-03-01 10:13:54 +01:00
parent 075040b034
commit 06ca1fc0a6
14 changed files with 245 additions and 48 deletions

View file

@ -23,6 +23,7 @@ regex_macros = { version = "0.1.33", optional = true }
semver = "0.2.1"
toml = "0.1"
unicode-normalization = "0.1"
strsim = "0.4.0"
[dev-dependencies]
compiletest_rs = "0.1.0"

View file

@ -119,6 +119,7 @@ name
[shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same) | allow | rebinding a name to itself, e.g. `let mut x = &mut x`
[shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | allow | The name is re-bound without even using the original value
[should_implement_trait](https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait) | warn | defining a method that should be implementing a std trait
[similar_names](https://github.com/Manishearth/rust-clippy/wiki#similar_names) | warn | similarly named items and bindings
[single_char_pattern](https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern) | warn | using a single-character str where a char could be used, e.g. `_.split("x")`
[single_match](https://github.com/Manishearth/rust-clippy/wiki#single_match) | warn | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead
[single_match_else](https://github.com/Manishearth/rust-clippy/wiki#single_match_else) | allow | a match statement with a two arms where the second arm's pattern is a wildcard; recommends `if let` instead

View file

@ -82,7 +82,7 @@ impl Constant {
impl PartialEq for Constant {
fn eq(&self, other: &Constant) -> bool {
match (self, other) {
(&Constant::Str(ref ls, ref lsty), &Constant::Str(ref rs, ref rsty)) => ls == rs && lsty == rsty,
(&Constant::Str(ref ls, ref l_sty), &Constant::Str(ref rs, ref r_sty)) => ls == rs && l_sty == r_sty,
(&Constant::Binary(ref l), &Constant::Binary(ref r)) => l == r,
(&Constant::Char(l), &Constant::Char(r)) => l == r,
(&Constant::Int(l), &Constant::Int(r)) => l.is_negative() == r.is_negative() && l.to_u64_unchecked() == r.to_u64_unchecked(),
@ -145,8 +145,8 @@ impl Hash for Constant {
impl PartialOrd for Constant {
fn partial_cmp(&self, other: &Constant) -> Option<Ordering> {
match (self, other) {
(&Constant::Str(ref ls, ref lsty), &Constant::Str(ref rs, ref rsty)) => {
if lsty == rsty {
(&Constant::Str(ref ls, ref l_sty), &Constant::Str(ref rs, ref r_sty)) => {
if l_sty == r_sty {
Some(ls.cmp(rs))
} else {
None
@ -354,6 +354,7 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> {
}
}
fn binop_apply<F>(&mut self, left: &Expr, right: &Expr, op: F) -> Option<Constant>
where F: Fn(Constant, Constant) -> Option<Constant>
{

View file

@ -31,6 +31,9 @@ extern crate unicode_normalization;
// for semver check in attrs.rs
extern crate semver;
// for levensthein distance
extern crate strsim;
// for regex checking
extern crate regex_syntax;
@ -84,6 +87,7 @@ pub mod needless_features;
pub mod needless_update;
pub mod new_without_default;
pub mod no_effect;
pub mod non_expressive_names;
pub mod open_options;
pub mod overflow_check_conditional;
pub mod panic;
@ -200,6 +204,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
reg.register_late_lint_pass(box types::CharLitAsU8);
reg.register_late_lint_pass(box print::PrintLint);
reg.register_late_lint_pass(box vec::UselessVec);
reg.register_early_lint_pass(box non_expressive_names::SimilarNames(1));
reg.register_late_lint_pass(box drop_ref::DropRefPass);
reg.register_late_lint_pass(box types::AbsurdExtremeComparisons);
reg.register_late_lint_pass(box regex::RegexPass::default());
@ -326,6 +331,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
needless_update::NEEDLESS_UPDATE,
new_without_default::NEW_WITHOUT_DEFAULT,
no_effect::NO_EFFECT,
non_expressive_names::SIMILAR_NAMES,
open_options::NONSENSICAL_OPEN_OPTIONS,
overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
panic::PANIC_PARAMS,

150
src/non_expressive_names.rs Normal file
View file

@ -0,0 +1,150 @@
use rustc::lint::*;
use syntax::codemap::Span;
use syntax::parse::token::InternedString;
use syntax::ast::*;
use syntax::visit::{self, FnKind};
use utils::{span_note_and_lint, in_macro};
use strsim::levenshtein;
/// **What it does:** This lint warns about names that are very similar and thus confusing
///
/// **Why is this bad?** It's hard to distinguish between names that differ only by a single character
///
/// **Known problems:** None?
///
/// **Example:** `checked_exp` and `checked_expr`
declare_lint! {
pub SIMILAR_NAMES,
Warn,
"similarly named items and bindings"
}
pub struct SimilarNames(pub usize);
impl LintPass for SimilarNames {
fn get_lints(&self) -> LintArray {
lint_array!(SIMILAR_NAMES)
}
}
struct SimilarNamesLocalVisitor<'a, 'b: 'a> {
names: Vec<(InternedString, Span)>,
cx: &'a EarlyContext<'b>,
limit: usize,
}
const WHITELIST: &'static [&'static str] = &[
"lhs", "rhs",
];
struct SimilarNamesNameVisitor<'a, 'b: 'a, 'c: 'b>(&'a mut SimilarNamesLocalVisitor<'b, 'c>);
impl<'v, 'a, 'b, 'c> visit::Visitor<'v> for SimilarNamesNameVisitor<'a, 'b, 'c> {
fn visit_pat(&mut self, pat: &'v Pat) {
if let PatKind::Ident(_, id, _) = pat.node {
self.check_name(id.span, id.node.name);
}
visit::walk_pat(self, pat);
}
}
impl<'a, 'b, 'c> SimilarNamesNameVisitor<'a, 'b, 'c> {
fn check_name(&mut self, span: Span, name: Name) {
if in_macro(self.0.cx, span) {
return;
}
let interned_name = name.as_str();
if interned_name.chars().any(char::is_uppercase) {
return;
}
if interned_name.chars().count() < 3 {
return;
}
for &allow in WHITELIST {
if interned_name == allow {
return;
}
if interned_name.len() <= allow.len() {
continue;
}
// allow_*
let allow_start = allow.chars().chain(Some('_'));
if interned_name.chars().zip(allow_start).all(|(l, r)| l == r) {
return;
}
// *_allow
let allow_end = Some('_').into_iter().chain(allow.chars());
if interned_name.chars().rev().zip(allow_end.rev()).all(|(l, r)| l == r) {
return;
}
}
for &(ref existing_name, sp) in &self.0.names {
let dist = levenshtein(&interned_name, &existing_name);
// equality is caught by shadow lints
if dist == 0 {
continue;
}
// if they differ enough it's all good
if dist > self.0.limit {
continue;
}
// are we doing stuff like `for item in items`?
if interned_name.starts_with(&**existing_name) ||
existing_name.starts_with(&*interned_name) ||
interned_name.ends_with(&**existing_name) ||
existing_name.ends_with(&*interned_name) {
continue;
}
if dist == 1 {
// are we doing stuff like a_bar, b_bar, c_bar?
if interned_name.chars().next() != existing_name.chars().next() && interned_name.chars().nth(1) == Some('_') {
continue;
}
// are we doing stuff like foo_x, foo_y, foo_z?
if interned_name.chars().rev().next() != existing_name.chars().rev().next() && interned_name.chars().rev().nth(1) == Some('_') {
continue;
}
}
span_note_and_lint(self.0.cx, SIMILAR_NAMES, span, "binding's name is too similar to existing binding", sp, "existing binding defined here");
return;
}
self.0.names.push((interned_name, span));
}
}
impl<'v, 'a, 'b> visit::Visitor<'v> for SimilarNamesLocalVisitor<'a, 'b> {
fn visit_local(&mut self, local: &'v Local) {
SimilarNamesNameVisitor(self).visit_local(local)
}
fn visit_block(&mut self, blk: &'v Block) {
// ensure scoping rules work
let n = self.names.len();
visit::walk_block(self, blk);
self.names.truncate(n);
}
fn visit_arm(&mut self, arm: &'v Arm) {
let n = self.names.len();
// just go through the first pattern, as either all patterns bind the same bindings or rustc would have errored much earlier
SimilarNamesNameVisitor(self).visit_pat(&arm.pats[0]);
self.names.truncate(n);
}
fn visit_item(&mut self, _: &'v Item) {
// do nothing
}
}
impl EarlyLintPass for SimilarNames {
fn check_fn(&mut self, cx: &EarlyContext, _: FnKind, decl: &FnDecl, blk: &Block, _: Span, _: NodeId) {
let mut visitor = SimilarNamesLocalVisitor {
names: Vec::new(),
cx: cx,
limit: self.0,
};
// initialize with function arguments
for arg in &decl.inputs {
visit::walk_pat(&mut SimilarNamesNameVisitor(&mut visitor), &arg.pat);
}
// walk all other bindings
visit::walk_block(&mut visitor, blk);
}
}

View file

@ -638,7 +638,7 @@ fn detect_absurd_comparison<'a>(cx: &LateContext, op: BinOp_, lhs: &'a Expr, rhs
Lt,
Le,
};
let (rel, lhs2, rhs2) = match op {
let (rel, normalized_lhs, normalized_rhs) = match op {
BiLt => (Rel::Lt, lhs, rhs),
BiLe => (Rel::Le, lhs, rhs),
BiGt => (Rel::Lt, rhs, lhs),
@ -646,8 +646,8 @@ fn detect_absurd_comparison<'a>(cx: &LateContext, op: BinOp_, lhs: &'a Expr, rhs
_ => return None,
};
let lx = detect_extreme_expr(cx, lhs2);
let rx = detect_extreme_expr(cx, rhs2);
let lx = detect_extreme_expr(cx, normalized_lhs);
let rx = detect_extreme_expr(cx, normalized_rhs);
Some(match rel {
Rel::Lt => {

View file

@ -67,24 +67,24 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> {
}
match (&left.node, &right.node) {
(&ExprAddrOf(lmut, ref le), &ExprAddrOf(rmut, ref re)) => lmut == rmut && self.eq_expr(le, re),
(&ExprAddrOf(l_mut, ref le), &ExprAddrOf(r_mut, ref re)) => l_mut == r_mut && self.eq_expr(le, re),
(&ExprAgain(li), &ExprAgain(ri)) => both(&li, &ri, |l, r| l.node.name.as_str() == r.node.name.as_str()),
(&ExprAssign(ref ll, ref lr), &ExprAssign(ref rl, ref rr)) => self.eq_expr(ll, rl) && self.eq_expr(lr, rr),
(&ExprAssignOp(ref lo, ref ll, ref lr), &ExprAssignOp(ref ro, ref rl, ref rr)) => {
lo.node == ro.node && self.eq_expr(ll, rl) && self.eq_expr(lr, rr)
}
(&ExprBlock(ref l), &ExprBlock(ref r)) => self.eq_block(l, r),
(&ExprBinary(lop, ref ll, ref lr), &ExprBinary(rop, ref rl, ref rr)) => {
lop.node == rop.node && self.eq_expr(ll, rl) && self.eq_expr(lr, rr)
(&ExprBinary(l_op, ref ll, ref lr), &ExprBinary(r_op, ref rl, ref rr)) => {
l_op.node == r_op.node && self.eq_expr(ll, rl) && self.eq_expr(lr, rr)
}
(&ExprBreak(li), &ExprBreak(ri)) => both(&li, &ri, |l, r| l.node.name.as_str() == r.node.name.as_str()),
(&ExprBox(ref l), &ExprBox(ref r)) => self.eq_expr(l, r),
(&ExprCall(ref lfun, ref largs), &ExprCall(ref rfun, ref rargs)) => {
!self.ignore_fn && self.eq_expr(lfun, rfun) && self.eq_exprs(largs, rargs)
(&ExprCall(ref l_fun, ref l_args), &ExprCall(ref r_fun, ref r_args)) => {
!self.ignore_fn && self.eq_expr(l_fun, r_fun) && self.eq_exprs(l_args, r_args)
}
(&ExprCast(ref lx, ref lt), &ExprCast(ref rx, ref rt)) => self.eq_expr(lx, rx) && self.eq_ty(lt, rt),
(&ExprField(ref lfexp, ref lfident), &ExprField(ref rfexp, ref rfident)) => {
lfident.node == rfident.node && self.eq_expr(lfexp, rfexp)
(&ExprField(ref l_f_exp, ref l_f_ident), &ExprField(ref r_f_exp, ref r_f_ident)) => {
l_f_ident.node == r_f_ident.node && self.eq_expr(l_f_exp, r_f_exp)
}
(&ExprIndex(ref la, ref li), &ExprIndex(ref ra, ref ri)) => self.eq_expr(la, ra) && self.eq_expr(li, ri),
(&ExprIf(ref lc, ref lt, ref le), &ExprIf(ref rc, ref rt, ref re)) => {
@ -101,25 +101,25 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> {
over(&l.pats, &r.pats, |l, r| self.eq_pat(l, r))
})
}
(&ExprMethodCall(ref lname, ref ltys, ref largs),
&ExprMethodCall(ref rname, ref rtys, ref rargs)) => {
(&ExprMethodCall(ref l_name, ref l_tys, ref l_args),
&ExprMethodCall(ref r_name, ref r_tys, ref r_args)) => {
// TODO: tys
!self.ignore_fn && lname.node == rname.node && ltys.is_empty() && rtys.is_empty() &&
self.eq_exprs(largs, rargs)
!self.ignore_fn && l_name.node == r_name.node && l_tys.is_empty() && r_tys.is_empty() &&
self.eq_exprs(l_args, r_args)
}
(&ExprRepeat(ref le, ref ll), &ExprRepeat(ref re, ref rl)) => self.eq_expr(le, re) && self.eq_expr(ll, rl),
(&ExprRet(ref l), &ExprRet(ref r)) => both(l, r, |l, r| self.eq_expr(l, r)),
(&ExprPath(ref lqself, ref lsubpath), &ExprPath(ref rqself, ref rsubpath)) => {
both(lqself, rqself, |l, r| self.eq_qself(l, r)) && self.eq_path(lsubpath, rsubpath)
(&ExprPath(ref l_qself, ref l_subpath), &ExprPath(ref r_qself, ref r_subpath)) => {
both(l_qself, r_qself, |l, r| self.eq_qself(l, r)) && self.eq_path(l_subpath, r_subpath)
}
(&ExprStruct(ref lpath, ref lf, ref lo), &ExprStruct(ref rpath, ref rf, ref ro)) => {
self.eq_path(lpath, rpath) &&
(&ExprStruct(ref l_path, ref lf, ref lo), &ExprStruct(ref r_path, ref rf, ref ro)) => {
self.eq_path(l_path, r_path) &&
both(lo, ro, |l, r| self.eq_expr(l, r)) &&
over(lf, rf, |l, r| self.eq_field(l, r))
}
(&ExprTup(ref ltup), &ExprTup(ref rtup)) => self.eq_exprs(ltup, rtup),
(&ExprTup(ref l_tup), &ExprTup(ref r_tup)) => self.eq_exprs(l_tup, r_tup),
(&ExprTupField(ref le, li), &ExprTupField(ref re, ri)) => li.node == ri.node && self.eq_expr(le, re),
(&ExprUnary(lop, ref le), &ExprUnary(rop, ref re)) => lop == rop && self.eq_expr(le, re),
(&ExprUnary(l_op, ref le), &ExprUnary(r_op, ref re)) => l_op == r_op && self.eq_expr(le, re),
(&ExprVec(ref l), &ExprVec(ref r)) => self.eq_exprs(l, r),
(&ExprWhile(ref lc, ref lb, ref ll), &ExprWhile(ref rc, ref rb, ref rl)) => {
self.eq_expr(lc, rc) && self.eq_block(lb, rb) && both(ll, rl, |l, r| l.name.as_str() == r.name.as_str())
@ -179,16 +179,16 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> {
fn eq_ty(&self, left: &Ty, right: &Ty) -> bool {
match (&left.node, &right.node) {
(&TyVec(ref lvec), &TyVec(ref rvec)) => self.eq_ty(lvec, rvec),
(&TyVec(ref l_vec), &TyVec(ref r_vec)) => self.eq_ty(l_vec, r_vec),
(&TyFixedLengthVec(ref lt, ref ll), &TyFixedLengthVec(ref rt, ref rl)) => {
self.eq_ty(lt, rt) && self.eq_expr(ll, rl)
}
(&TyPtr(ref lmut), &TyPtr(ref rmut)) => lmut.mutbl == rmut.mutbl && self.eq_ty(&*lmut.ty, &*rmut.ty),
(&TyRptr(_, ref lrmut), &TyRptr(_, ref rrmut)) => {
lrmut.mutbl == rrmut.mutbl && self.eq_ty(&*lrmut.ty, &*rrmut.ty)
(&TyPtr(ref l_mut), &TyPtr(ref r_mut)) => l_mut.mutbl == r_mut.mutbl && self.eq_ty(&*l_mut.ty, &*r_mut.ty),
(&TyRptr(_, ref l_rmut), &TyRptr(_, ref r_rmut)) => {
l_rmut.mutbl == r_rmut.mutbl && self.eq_ty(&*l_rmut.ty, &*r_rmut.ty)
}
(&TyPath(ref lq, ref lpath), &TyPath(ref rq, ref rpath)) => {
both(lq, rq, |l, r| self.eq_qself(l, r)) && self.eq_path(lpath, rpath)
(&TyPath(ref lq, ref l_path), &TyPath(ref rq, ref r_path)) => {
both(lq, rq, |l, r| self.eq_qself(l, r)) && self.eq_path(l_path, r_path)
}
(&TyTup(ref l), &TyTup(ref r)) => over(l, r, |l, r| self.eq_ty(l, r)),
(&TyInfer, &TyInfer) => true,

View file

@ -102,8 +102,8 @@ macro_rules! if_let_chain {
/// Returns true if the two spans come from differing expansions (i.e. one is from a macro and one
/// isn't).
pub fn differing_macro_contexts(sp1: Span, sp2: Span) -> bool {
sp1.expn_id != sp2.expn_id
pub fn differing_macro_contexts(lhs: Span, rhs: Span) -> bool {
rhs.expn_id != lhs.expn_id
}
/// Returns true if this `expn_info` was expanded by any macro.
pub fn in_macro<T: LintContext>(cx: &T, span: Span) -> bool {

View file

@ -2,7 +2,7 @@
#![plugin(clippy)]
#[deny(approx_constant)]
#[allow(unused, shadow_unrelated)]
#[allow(unused, shadow_unrelated, similar_names)]
fn main() {
let my_e = 2.7182; //~ERROR approximate value of `f{32, 64}::E` found
let almost_e = 2.718; //~ERROR approximate value of `f{32, 64}::E` found

View file

@ -2,7 +2,7 @@
#![plugin(clippy)]
#![deny(drop_ref)]
#![allow(toplevel_ref_arg)]
#![allow(toplevel_ref_arg, similar_names)]
use std::mem::drop;

View file

@ -88,7 +88,7 @@ impl Unrelated {
#[deny(needless_range_loop, explicit_iter_loop, iter_next_loop, reverse_range_loop, explicit_counter_loop)]
#[deny(unused_collect)]
#[allow(linkedlist, shadow_unrelated, unnecessary_mut_passed, cyclomatic_complexity)]
#[allow(linkedlist, shadow_unrelated, unnecessary_mut_passed, cyclomatic_complexity, similar_names)]
fn main() {
const MAX_LEN: usize = 42;

View file

@ -92,38 +92,38 @@ fn main() {
println!("Nor should this!");
}
let hie = HasIsEmpty;
if hie.len() == 0 {
let has_is_empty = HasIsEmpty;
if has_is_empty.len() == 0 {
//~^ERROR length comparison to zero
//~|HELP consider using `is_empty`
//~|SUGGESTION hie.is_empty()
//~|SUGGESTION has_is_empty.is_empty()
println!("Or this!");
}
if hie.len() != 0 {
if has_is_empty.len() != 0 {
//~^ERROR length comparison to zero
//~|HELP consider using `is_empty`
//~|SUGGESTION !hie.is_empty()
//~|SUGGESTION !has_is_empty.is_empty()
println!("Or this!");
}
if hie.len() > 0 {
if has_is_empty.len() > 0 {
//~^ERROR length comparison to zero
//~|HELP consider using `is_empty`
//~|SUGGESTION !hie.is_empty()
//~|SUGGESTION !has_is_empty.is_empty()
println!("Or this!");
}
assert!(!hie.is_empty());
assert!(!has_is_empty.is_empty());
let wie : &WithIsEmpty = &Wither;
if wie.len() == 0 {
let with_is_empty: &WithIsEmpty = &Wither;
if with_is_empty.len() == 0 {
//~^ERROR length comparison to zero
//~|HELP consider using `is_empty`
//~|SUGGESTION wie.is_empty()
//~|SUGGESTION with_is_empty.is_empty()
println!("Or this!");
}
assert!(!wie.is_empty());
assert!(!with_is_empty.is_empty());
let hwie = HasWrongIsEmpty;
if hwie.len() == 0 { //no error as HasWrongIsEmpty does not have .is_empty()
let has_wrong_is_empty = HasWrongIsEmpty;
if has_wrong_is_empty.len() == 0 { //no error as HasWrongIsEmpty does not have .is_empty()
println!("Or this!");
}
}

View file

@ -286,6 +286,7 @@ fn or_fun_call() {
//~|SUGGESTION btree.entry(42).or_insert_with(String::new);
}
#[allow(similar_names)]
fn main() {
use std::io;

View file

@ -0,0 +1,37 @@
#![feature(plugin)]
#![plugin(clippy)]
#![deny(clippy)]
#![allow(unused)]
fn main() {
let specter: i32;
let spectre: i32;
let apple: i32; //~ NOTE: existing binding defined here
let bpple: i32; //~ ERROR: name is too similar
let cpple: i32; //~ ERROR: name is too similar
let a_bar: i32;
let b_bar: i32;
let c_bar: i32;
let foo_x: i32;
let foo_y: i32;
let rhs: i32;
let lhs: i32;
let bla_rhs: i32;
let bla_lhs: i32;
let blubrhs: i32; //~ NOTE: existing binding defined here
let blublhs: i32; //~ ERROR: name is too similar
let blubx: i32; //~ NOTE: existing binding defined here
let bluby: i32; //~ ERROR: name is too similar
let cake: i32; //~ NOTE: existing binding defined here
let caked: i32; //~ NOTE: existing binding defined here
let cakes: i32; //~ ERROR: name is too similar
let coke: i32; //~ ERROR: name is too similar
}