Merge pull request #727 from oli-obk/similar_names

lint on binding-names that are too similar

README.md

@@ -14,7 +14,7 @@ Table of contents:
 * [License](#license)
 
 ##Lints
-There are 136 lints included in this crate:
+There are 138 lints included in this crate:
 
 name                                                                                                                 | default | meaning
 ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -74,6 +74,7 @@ name
 [let_unit_value](https://github.com/Manishearth/rust-clippy/wiki#let_unit_value)                                     | warn    | creating a let binding to a value of unit type, which usually can't be used afterwards
 [linkedlist](https://github.com/Manishearth/rust-clippy/wiki#linkedlist)                                             | warn    | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a VecDeque
 [manual_swap](https://github.com/Manishearth/rust-clippy/wiki#manual_swap)                                           | warn    | manual swap
+[many_single_char_names](https://github.com/Manishearth/rust-clippy/wiki#many_single_char_names)                     | warn    | too many single character bindings
 [map_clone](https://github.com/Manishearth/rust-clippy/wiki#map_clone)                                               | warn    | using `.map(|x| x.clone())` to clone an iterator or option's contents (recommends `.cloned()` instead)
 [map_entry](https://github.com/Manishearth/rust-clippy/wiki#map_entry)                                               | warn    | use of `contains_key` followed by `insert` on a `HashMap` or `BTreeMap`
 [match_bool](https://github.com/Manishearth/rust-clippy/wiki#match_bool)                                             | warn    | a match on boolean expression; recommends `if..else` block instead
@@ -119,6 +120,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

src/consts.rs

@@ -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)) => {
+            (&Constant::Str(ref ls, ref l_sty), &Constant::Str(ref rs, ref r_sty)) => {
-                if lsty == rsty {
+                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>
     {

src/lib.rs

@@ -3,6 +3,7 @@
 #![feature(rustc_private, collections)]
 #![feature(iter_arith)]
 #![feature(custom_attribute)]
+#![feature(slice_patterns)]
 #![allow(indexing_slicing, shadow_reuse, unknown_lints)]
 
 // this only exists to allow the "dogfood" integration test to work
@@ -84,6 +85,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 +202,9 @@ 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::NonExpressiveNames {
+        max_single_char_names: conf.max_single_char_names,
+    });
     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,8 @@ pub fn plugin_registrar(reg: &mut Registry) {
         needless_update::NEEDLESS_UPDATE,
         new_without_default::NEW_WITHOUT_DEFAULT,
         no_effect::NO_EFFECT,
+        non_expressive_names::MANY_SINGLE_CHAR_NAMES,
+        non_expressive_names::SIMILAR_NAMES,
         open_options::NONSENSICAL_OPEN_OPTIONS,
         overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
         panic::PANIC_PARAMS,

src/needless_bool.rs

@@ -6,7 +6,7 @@ use rustc::lint::*;
 use rustc_front::hir::*;
 use syntax::ast::LitKind;
 use syntax::codemap::Spanned;
-use utils::{span_lint, span_lint_and_then, snippet};
+use utils::{span_lint, span_lint_and_then, snippet, snippet_opt};
 
 /// **What it does:** This lint checks for expressions of the form `if c { true } else { false }` (or vice versa) and suggest using the condition directly.
 ///
@@ -47,44 +47,39 @@ impl LintPass for NeedlessBool {
 
 impl LateLintPass for NeedlessBool {
     fn check_expr(&mut self, cx: &LateContext, e: &Expr) {
+        use self::Expression::*;
         if let ExprIf(ref pred, ref then_block, Some(ref else_expr)) = e.node {
+            let reduce = |hint: &str, not| {
+                let hint = match snippet_opt(cx, pred.span) {
+                    Some(pred_snip) => format!("`{}{}`", not, pred_snip),
+                    None => hint.into(),
+                };
+                span_lint_and_then(cx,
+                                   NEEDLESS_BOOL,
+                                   e.span,
+                                   "this if-then-else expression returns a bool literal", |db| {
+                    db.span_suggestion(e.span, "you can reduce it to", hint);
+                });
+            };
             match (fetch_bool_block(then_block), fetch_bool_expr(else_expr)) {
-                (Some(true), Some(true)) => {
+                (RetBool(true), RetBool(true)) |
+                (Bool(true), Bool(true)) => {
                     span_lint(cx,
                               NEEDLESS_BOOL,
                               e.span,
                               "this if-then-else expression will always return true");
                 }
-                (Some(false), Some(false)) => {
+                (RetBool(false), RetBool(false)) |
+                (Bool(false), Bool(false)) => {
                     span_lint(cx,
                               NEEDLESS_BOOL,
                               e.span,
                               "this if-then-else expression will always return false");
                 }
-                (Some(true), Some(false)) => {
+                (RetBool(true), RetBool(false)) => reduce("its predicate", "return "),
-                    let pred_snip = snippet(cx, pred.span, "..");
+                (Bool(true), Bool(false)) => reduce("its predicate", ""),
-                    let hint = if pred_snip == ".." {
+                (RetBool(false), RetBool(true)) => reduce("`!` and its predicate", "return !"),
-                        "its predicate".into()
+                (Bool(false), Bool(true)) => reduce("`!` and its predicate", "!"),
-                    } else {
-                        format!("`{}`", pred_snip)
-                    };
-                    span_lint(cx,
-                              NEEDLESS_BOOL,
-                              e.span,
-                              &format!("you can reduce this if-then-else expression to just {}", hint));
-                }
-                (Some(false), Some(true)) => {
-                    let pred_snip = snippet(cx, pred.span, "..");
-                    let hint = if pred_snip == ".." {
-                        "`!` and its predicate".into()
-                    } else {
-                        format!("`!{}`", pred_snip)
-                    };
-                    span_lint(cx,
-                              NEEDLESS_BOOL,
-                              e.span,
-                              &format!("you can reduce this if-then-else expression to just {}", hint));
-                }
                 _ => (),
             }
         }
@@ -102,9 +97,10 @@ impl LintPass for BoolComparison {
 
 impl LateLintPass for BoolComparison {
     fn check_expr(&mut self, cx: &LateContext, e: &Expr) {
+        use self::Expression::*;
         if let ExprBinary(Spanned{ node: BiEq, .. }, ref left_side, ref right_side) = e.node {
             match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) {
-                (Some(true), None) => {
+                (Bool(true), Other) => {
                     let hint = snippet(cx, right_side.span, "..").into_owned();
                     span_lint_and_then(cx,
                                        BOOL_COMPARISON,
@@ -114,7 +110,7 @@ impl LateLintPass for BoolComparison {
                                            db.span_suggestion(e.span, "try simplifying it as shown:", hint);
                                        });
                 }
-                (None, Some(true)) => {
+                (Other, Bool(true)) => {
                     let hint = snippet(cx, left_side.span, "..").into_owned();
                     span_lint_and_then(cx,
                                        BOOL_COMPARISON,
@@ -124,7 +120,7 @@ impl LateLintPass for BoolComparison {
                                            db.span_suggestion(e.span, "try simplifying it as shown:", hint);
                                        });
                 }
-                (Some(false), None) => {
+                (Bool(false), Other) => {
                     let hint = format!("!{}", snippet(cx, right_side.span, ".."));
                     span_lint_and_then(cx,
                                        BOOL_COMPARISON,
@@ -134,7 +130,7 @@ impl LateLintPass for BoolComparison {
                                            db.span_suggestion(e.span, "try simplifying it as shown:", hint);
                                        });
                 }
-                (None, Some(false)) => {
+                (Other, Bool(false)) => {
                     let hint = format!("!{}", snippet(cx, left_side.span, ".."));
                     span_lint_and_then(cx,
                                        BOOL_COMPARISON,
@@ -150,24 +146,42 @@ impl LateLintPass for BoolComparison {
     }
 }
 
-fn fetch_bool_block(block: &Block) -> Option<bool> {
+enum Expression {
-    if block.stmts.is_empty() {
+    Bool(bool),
-        block.expr.as_ref().and_then(|e| fetch_bool_expr(e))
+    RetBool(bool),
+    Other,
+}
+
+fn fetch_bool_block(block: &Block) -> Expression {
+    match (&*block.stmts, block.expr.as_ref()) {
+        ([], Some(e)) => fetch_bool_expr(&**e),
+        ([ref e], None) => if let StmtSemi(ref e, _) = e.node {
+            if let ExprRet(_) = e.node {
+                fetch_bool_expr(&**e)
             } else {
-        None
+                Expression::Other
+            }
+        } else {
+            Expression::Other
+        },
+        _ => Expression::Other,
     }
 }
 
-fn fetch_bool_expr(expr: &Expr) -> Option<bool> {
+fn fetch_bool_expr(expr: &Expr) -> Expression {
     match expr.node {
         ExprBlock(ref block) => fetch_bool_block(block),
         ExprLit(ref lit_ptr) => {
             if let LitKind::Bool(value) = lit_ptr.node {
-                Some(value)
+                Expression::Bool(value)
             } else {
-                None
+                Expression::Other
             }
-        }
+        },
-        _ => None,
+        ExprRet(Some(ref expr)) => match fetch_bool_expr(expr) {
+            Expression::Bool(value) => Expression::RetBool(value),
+            _ => Expression::Other,
+        },
+        _ => Expression::Other,
     }
 }

src/non_expressive_names.rs

@@ -0,0 +1,274 @@
+use rustc::lint::*;
+use syntax::codemap::Span;
+use syntax::parse::token::InternedString;
+use syntax::ast::*;
+use syntax::visit::{self, FnKind};
+use utils::{span_lint_and_then, in_macro, span_lint};
+
+/// **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"
+}
+
+/// **What it does:** This lint warns about having too many variables whose name consists of a single character
+///
+/// **Why is this bad?** It's hard to memorize what a variable means without a descriptive name.
+///
+/// **Known problems:** None?
+///
+/// **Example:** let (a, b, c, d, e, f, g) = (...);
+declare_lint! {
+    pub MANY_SINGLE_CHAR_NAMES,
+    Warn,
+    "too many single character bindings"
+}
+
+pub struct NonExpressiveNames {
+    pub max_single_char_names: u64,
+}
+
+impl LintPass for NonExpressiveNames {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(SIMILAR_NAMES, MANY_SINGLE_CHAR_NAMES)
+    }
+}
+
+struct SimilarNamesLocalVisitor<'a, 'b: 'a> {
+    names: Vec<(InternedString, Span, usize)>,
+    cx: &'a EarlyContext<'b>,
+    lint: &'a NonExpressiveNames,
+    single_char_names: Vec<char>,
+}
+
+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);
+    }
+}
+
+fn whitelisted(interned_name: &str) -> bool {
+    for &allow in WHITELIST {
+        if interned_name == allow {
+            return true;
+        }
+        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 true;
+        }
+        // *_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 true;
+        }
+    }
+    false
+}
+
+impl<'a, 'b, 'c> SimilarNamesNameVisitor<'a, 'b, 'c> {
+    fn check_short_name(&mut self, c: char, span: Span) {
+        // make sure we ignore shadowing
+        if self.0.single_char_names.contains(&c) {
+            return;
+        }
+        self.0.single_char_names.push(c);
+        if self.0.single_char_names.len() as u64 >= self.0.lint.max_single_char_names {
+            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()));
+        }
+    }
+    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;
+        }
+        let count = interned_name.chars().count();
+        if count < 3 {
+            if count != 1 {
+                return;
+            }
+            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 {
+            let mut split_at = None;
+            if existing_len > count {
+                if existing_len - count != 1 {
+                    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) {
+                    continue;
+                }
+            } else {
+                let mut interned_chars = interned_name.chars();
+                let mut existing_chars = existing_name.chars();
+
+                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)
+                            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)
+                            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,
+                               SIMILAR_NAMES,
+                               span,
+                               "binding's name is too similar to existing binding",
+                               |diag| {
+                                   diag.span_note(sp, "existing binding defined here");
+                                   if let Some(split) = split_at {
+                                       diag.span_help(span, &format!("separate the discriminating character \
+                                                                      by an underscore like: `{}_{}`",
+                                                                     &interned_name[..split],
+                                                                     &interned_name[split..]));
+                                   }
+                               });
+            return;
+        }
+        self.0.names.push((interned_name, span, count));
+    }
+}
+
+impl<'a, 'b> SimilarNamesLocalVisitor<'a, 'b> {
+    /// ensure scoping rules work
+    fn apply<F: for<'c> Fn(&'c mut Self)>(&mut self, f: F) {
+        let n = self.names.len();
+        let single_char_count = self.single_char_names.len();
+        f(self);
+        self.names.truncate(n);
+        self.single_char_names.truncate(single_char_count);
+    }
+}
+
+impl<'v, 'a, 'b> visit::Visitor<'v> for SimilarNamesLocalVisitor<'a, 'b> {
+    fn visit_local(&mut self, local: &'v Local) {
+        if let Some(ref init) = local.init {
+            self.apply(|this| visit::walk_expr(this, &**init));
+        }
+        // add the pattern after the expression because the bindings aren't available yet in the init expression
+        SimilarNamesNameVisitor(self).visit_pat(&*local.pat);
+    }
+    fn visit_block(&mut self, blk: &'v Block) {
+        self.apply(|this| visit::walk_block(this, blk));
+    }
+    fn visit_arm(&mut self, arm: &'v Arm) {
+        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| visit::walk_expr(this, &arm.body));
+        });
+    }
+    fn visit_item(&mut self, _: &'v Item) {
+        // do nothing
+    }
+}
+
+impl EarlyLintPass for NonExpressiveNames {
+    fn check_fn(&mut self, cx: &EarlyContext, _: FnKind, decl: &FnDecl, blk: &Block, _: Span, _: NodeId) {
+        let mut visitor = SimilarNamesLocalVisitor {
+            names: Vec::new(),
+            cx: cx,
+            lint: &self,
+            single_char_names: Vec::new(),
+        };
+        // 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);
+    }
+}
+
+/// precondition: a_name.chars().count() < b_name.chars().count()
+fn levenstein_not_1(a_name: &str, b_name: &str) -> bool {
+    debug_assert!(a_name.chars().count() < b_name.chars().count());
+    let mut a_chars = a_name.chars();
+    let mut b_chars = b_name.chars();
+    while let (Some(a), Some(b)) = (a_chars.next(), b_chars.next()) {
+        if a == b {
+            continue;
+        }
+        if let Some(b2) = b_chars.next() {
+            // check if there's just one character inserted
+            return !(a == b2 && a_chars.eq(b_chars));
+        } else {
+            // tuple
+            // ntuple
+            return true;
+        }
+    }
+    // for item in items
+    true
+}

src/types.rs

@@ -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 lx = detect_extreme_expr(cx, normalized_lhs);
-    let rx = detect_extreme_expr(cx, rhs2);
+    let rx = detect_extreme_expr(cx, normalized_rhs);
 
     Some(match rel {
         Rel::Lt => {

src/utils/conf.rs

@@ -153,6 +153,8 @@ define_Conf! {
     ("too-many-arguments-threshold", too_many_arguments_threshold, 7 => u64),
     /// Lint: TYPE_COMPLEXITY. The maximum complexity a type can have
     ("type-complexity-threshold", type_complexity_threshold, 250 => u64),
+    /// Lint: MANY_SINGLE_CHAR_NAMES. The maximum number of single char bindings a scope may have
+    ("single-char-binding-names-threshold", max_single_char_names, 5 => u64),
 }
 
 /// Read the `toml` configuration file. The function will ignore “File not found” errors iif

src/utils/hir.rs

@@ -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)) => {
+            (&ExprBinary(l_op, ref ll, ref lr), &ExprBinary(r_op, ref rl, ref rr)) => {
-                lop.node == rop.node && self.eq_expr(ll, rl) && self.eq_expr(lr, 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)) => {
+            (&ExprCall(ref l_fun, ref l_args), &ExprCall(ref r_fun, ref r_args)) => {
-                !self.ignore_fn && self.eq_expr(lfun, rfun) && self.eq_exprs(largs, rargs)
+                !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)) => {
+            (&ExprField(ref l_f_exp, ref l_f_ident), &ExprField(ref r_f_exp, ref r_f_ident)) => {
-                lfident.node == rfident.node && self.eq_expr(lfexp, rfexp)
+                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 l_name, ref l_tys, ref l_args),
-             &ExprMethodCall(ref rname, ref rtys, ref rargs)) => {
+             &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.ignore_fn && l_name.node == r_name.node && l_tys.is_empty() && r_tys.is_empty() &&
-                self.eq_exprs(largs, rargs)
+                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)) => {
+            (&ExprPath(ref l_qself, ref l_subpath), &ExprPath(ref r_qself, ref r_subpath)) => {
-                both(lqself, rqself, |l, r| self.eq_qself(l, r)) && self.eq_path(lsubpath, rsubpath)
+                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)) => {
+            (&ExprStruct(ref l_path, ref lf, ref lo), &ExprStruct(ref r_path, ref rf, ref ro)) => {
-                self.eq_path(lpath, rpath) &&
+                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),
+            (&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 lrmut), &TyRptr(_, ref rrmut)) => {
+            (&TyRptr(_, ref l_rmut), &TyRptr(_, ref r_rmut)) => {
-                lrmut.mutbl == rrmut.mutbl && self.eq_ty(&*lrmut.ty, &*rrmut.ty)
+                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)) => {
+            (&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(lpath, rpath)
+                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,

src/utils/mod.rs

@@ -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 {
+pub fn differing_macro_contexts(lhs: Span, rhs: Span) -> bool {
-    sp1.expn_id != sp2.expn_id
+    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 {

tests/compile-fail/approx_const.rs

@@ -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

tests/compile-fail/blacklisted_name.rs

@@ -3,7 +3,7 @@
 
 #![allow(dead_code)]
 #![allow(single_match)]
-#![allow(unused_variables)]
+#![allow(unused_variables, similar_names)]
 #![deny(blacklisted_name)]
 
 fn test(foo: ()) {} //~ERROR use of a blacklisted/placeholder name `foo`

tests/compile-fail/drop_ref.rs

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

tests/compile-fail/eta.rs

@@ -1,6 +1,6 @@
 #![feature(plugin)]
 #![plugin(clippy)]
-#![allow(unknown_lints, unused, no_effect, redundant_closure_call)]
+#![allow(unknown_lints, unused, no_effect, redundant_closure_call, many_single_char_names)]
 #![deny(redundant_closure)]
 
 fn main() {

tests/compile-fail/for_loop.rs

@@ -88,7 +88,8 @@ 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)]
+#[allow(many_single_char_names)]
 fn main() {
     const MAX_LEN: usize = 42;
 

tests/compile-fail/len_zero.rs

@@ -92,38 +92,38 @@ fn main() {
         println!("Nor should this!");
     }
 
-    let hie = HasIsEmpty;
+    let has_is_empty = HasIsEmpty;
-    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!");
     }
-    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;
+    let with_is_empty: &WithIsEmpty = &Wither;
-    if wie.len() == 0 {
+    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;
+    let has_wrong_is_empty = HasWrongIsEmpty;
-    if hwie.len() == 0 { //no error as HasWrongIsEmpty does not have .is_empty()
+    if has_wrong_is_empty.len() == 0 { //no error as HasWrongIsEmpty does not have .is_empty()
         println!("Or this!");
     }
 }

tests/compile-fail/methods.rs

@@ -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;
 

tests/compile-fail/needless_bool.rs

@@ -7,7 +7,45 @@ fn main() {
     let x = true;
     if x { true } else { true }; //~ERROR this if-then-else expression will always return true
     if x { false } else { false }; //~ERROR this if-then-else expression will always return false
-    if x { true } else { false }; //~ERROR you can reduce this if-then-else expression to just `x`
+    if x { true } else { false };
-    if x { false } else { true }; //~ERROR you can reduce this if-then-else expression to just `!x`
+    //~^ ERROR this if-then-else expression returns a bool literal
+    //~| HELP you can reduce it to
+    //~| SUGGESTION `x`
+    if x { false } else { true };
+    //~^ ERROR this if-then-else expression returns a bool literal
+    //~| HELP you can reduce it to
+    //~| SUGGESTION `!x`
     if x { x } else { false }; // would also be questionable, but we don't catch this yet
+    bool_ret(x);
+    bool_ret2(x);
+    bool_ret3(x);
+    bool_ret4(x);
+}
+
+#[deny(needless_bool)]
+#[allow(if_same_then_else)]
+fn bool_ret(x: bool) -> bool {
+    if x { return true } else { return true }; //~ERROR this if-then-else expression will always return true
+}
+
+#[deny(needless_bool)]
+#[allow(if_same_then_else)]
+fn bool_ret2(x: bool) -> bool {
+    if x { return false } else { return false }; //~ERROR this if-then-else expression will always return false
+}
+
+#[deny(needless_bool)]
+fn bool_ret3(x: bool) -> bool {
+    if x { return true } else { return false };
+    //~^ ERROR this if-then-else expression returns a bool literal
+    //~| HELP you can reduce it to
+    //~| SUGGESTION `return x`
+}
+
+#[deny(needless_bool)]
+fn bool_ret4(x: bool) -> bool {
+    if x { return false } else { return true };
+    //~^ ERROR this if-then-else expression returns a bool literal
+    //~| HELP you can reduce it to
+    //~| SUGGESTION `return !x`
 }

tests/compile-fail/non_expressive_names.rs

@@ -0,0 +1,122 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+#![deny(clippy)]
+//~^ 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
+//~| NOTE: lint level defined here
+//~| NOTE: lint level defined here
+//~| NOTE: lint level defined here
+//~| NOTE: lint level defined here
+#![allow(unused)]
+
+fn main() {
+    let specter: i32;
+    let spectre: i32;
+
+    let apple: i32; //~ NOTE: existing binding defined here
+    //~^ NOTE: existing binding defined here
+    let bpple: i32; //~ ERROR: name is too similar
+    //~| HELP: separate the discriminating character by an underscore like: `b_pple`
+    //~| HELP: for further information visit
+    let cpple: i32; //~ ERROR: name is too similar
+    //~| HELP: separate the discriminating character by an underscore like: `c_pple`
+    //~| HELP: for further information visit
+
+    let a_bar: i32;
+    let b_bar: i32;
+    let c_bar: i32;
+
+    let items = [5];
+    for item in &items {
+        loop {}
+    }
+
+    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
+    //~| HELP: for further information visit
+
+    let blubx: i32; //~ NOTE: existing binding defined here
+    let bluby: i32; //~ ERROR: name is too similar
+    //~| HELP: for further information visit
+    //~| HELP: separate the discriminating character by an underscore like: `blub_y`
+
+    let cake: i32; //~ NOTE: existing binding defined here
+    let cakes: i32;
+    let coke: i32; //~ ERROR: name is too similar
+    //~| HELP: for further information visit
+
+    match 5 {
+        cheese @ 1 => {},
+        rabbit => panic!(),
+    }
+    let cheese: i32;
+    match (42, 43) {
+        (cheese1, 1) => {},
+        (cheese2, 2) => panic!(),
+        _ => println!(""),
+    }
+}
+
+#[derive(Clone, Debug)]
+enum MaybeInst {
+    Split,
+    Split1(usize),
+    Split2(usize),
+}
+
+struct InstSplit {
+    uiae: usize,
+}
+
+impl MaybeInst {
+    fn fill(&mut self) {
+        let filled = match *self {
+            MaybeInst::Split1(goto1) => panic!(1),
+            MaybeInst::Split2(goto2) => panic!(2),
+            _ => unimplemented!(),
+        };
+        unimplemented!()
+    }
+}
+
+fn bla() {
+    let a: i32;
+    let (b, c, d): (i32, i64, i16);
+    {
+        {
+            let cdefg: i32;
+            let blar: i32;
+        }
+        {
+            let e: i32; //~ ERROR: 5th binding whose name is just one char
+            //~| HELP: for further information visit
+        }
+        {
+            let e: i32; //~ ERROR: 5th binding whose name is just one char
+            //~| HELP: for further information visit
+            let f: i32; //~ ERROR: 6th binding whose name is just one char
+            //~| HELP: for further information visit
+        }
+        match 5 {
+            1 => println!(""),
+            e => panic!(), //~ ERROR: 5th binding whose name is just one char
+            //~| HELP: for further information visit
+        }
+        match 5 {
+            1 => println!(""),
+            _ => panic!(),
+        }
+    }
+}

tests/compile-fail/overflow_check_conditional.rs

@@ -1,6 +1,7 @@
 #![feature(plugin)]
 #![plugin(clippy)]
 
+#![allow(many_single_char_names)]
 #![deny(overflow_check_conditional)]
 
 fn main() {