Merge pull request #548 from mcarton/fixes

Fix #433 and doc

3 files changed, 109 insertions, 91 deletions

diff --git a/src/hashmap.rs b/src/entry.rs
index 095a00e3777..d9fb7269be6 100644
--- a/src/hashmap.rs
+++ b/src/entry.rs
@@ -1,12 +1,13 @@
 use rustc::lint::*;
 use rustc_front::hir::*;
 use syntax::codemap::Span;
-use utils::{get_item_name, is_exp_equal, match_type, snippet, span_help_and_lint, walk_ptrs_ty};
-use utils::HASHMAP_PATH;
+use utils::{get_item_name, is_exp_equal, match_type, snippet, span_lint_and_then, walk_ptrs_ty};
+use utils::{BTREEMAP_PATH, HASHMAP_PATH};
 
-/// **What it does:** This lint checks for uses of `contains_key` + `insert` on `HashMap`.
+/// **What it does:** This lint checks for uses of `contains_key` + `insert` on `HashMap` or
+/// `BTreeMap`.
 ///
-/// **Why is this bad?** Using `HashMap::entry` is more efficient.
+/// **Why is this bad?** Using `entry` is more efficient.
 ///
 /// **Known problems:** Some false negatives, eg.:
 /// ```
@@ -23,9 +24,9 @@ use utils::HASHMAP_PATH;
 /// m.entry(k).or_insert(v);
 /// ```
 declare_lint! {
-    pub HASHMAP_ENTRY,
+    pub MAP_ENTRY,
     Warn,
-    "use of `contains_key` followed by `insert` on a `HashMap`"
+    "use of `contains_key` followed by `insert` on a `HashMap` or `BTreeMap`"
 }
 
 #[derive(Copy,Clone)]
@@ -33,7 +34,7 @@ pub struct HashMapLint;
 
 impl LintPass for HashMapLint {
     fn get_lints(&self) -> LintArray {
-        lint_array!(HASHMAP_ENTRY)
+        lint_array!(MAP_ENTRY)
     }
 }
 
@@ -55,17 +56,25 @@ impl LateLintPass for HashMapLint {
                 let map = &params[0];
                 let obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(map));
 
-                if match_type(cx, obj_ty, &HASHMAP_PATH) {
-                    let sole_expr = if then.expr.is_some() { 1 } else { 0 } + then.stmts.len() == 1;
+                let kind = if match_type(cx, obj_ty, &BTREEMAP_PATH) {
+                    "BTreeMap"
+                }
+                else if match_type(cx, obj_ty, &HASHMAP_PATH) {
+                    "HashMap"
+                }
+                else {
+                    return
+                };
 
-                    if let Some(ref then) = then.expr {
-                        check_for_insert(cx, expr.span, map, key, then, sole_expr);
-                    }
+                let sole_expr = if then.expr.is_some() { 1 } else { 0 } + then.stmts.len() == 1;
 
-                    for stmt in &then.stmts {
-                        if let StmtSemi(ref stmt, _) = stmt.node {
-                            check_for_insert(cx, expr.span, map, key, stmt, sole_expr);
-                        }
+                if let Some(ref then) = then.expr {
+                    check_for_insert(cx, expr.span, map, key, then, sole_expr, kind);
+                }
+
+                for stmt in &then.stmts {
+                    if let StmtSemi(ref stmt, _) = stmt.node {
+                        check_for_insert(cx, expr.span, map, key, stmt, sole_expr, kind);
                     }
                 }
             }
@@ -73,7 +82,7 @@ impl LateLintPass for HashMapLint {
     }
 }
 
-fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, key: &Expr, expr: &Expr, sole_expr: bool) {
+fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, key: &Expr, expr: &Expr, sole_expr: bool, kind: &str) {
     if_let_chain! {
         [
             let ExprMethodCall(ref name, _, ref params) = expr.node,
@@ -82,21 +91,22 @@ fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, key: &Expr, expr:
             get_item_name(cx, map) == get_item_name(cx, &*params[0]),
             is_exp_equal(cx, key, &params[1])
         ], {
-            if sole_expr {
-                span_help_and_lint(cx, HASHMAP_ENTRY, span,
-                                   "usage of `contains_key` followed by `insert` on `HashMap`",
-                                   &format!("Consider using `{}.entry({}).or_insert({})`",
-                                            snippet(cx, map.span, ".."),
-                                            snippet(cx, params[1].span, ".."),
-                                            snippet(cx, params[2].span, ".."))); 
+            let help = if sole_expr {
+                format!("{}.entry({}).or_insert({})",
+                        snippet(cx, map.span, ".."),
+                        snippet(cx, params[1].span, ".."),
+                        snippet(cx, params[2].span, ".."))
             }
             else {
-                span_help_and_lint(cx, HASHMAP_ENTRY, span,
-                                   "usage of `contains_key` followed by `insert` on `HashMap`",
-                                   &format!("Consider using `{}.entry({})`",
-                                            snippet(cx, map.span, ".."),
-                                            snippet(cx, params[1].span, "..")));
-            }
+                format!("{}.entry({})",
+                        snippet(cx, map.span, ".."),
+                        snippet(cx, params[1].span, ".."))
+            };
+
+            span_lint_and_then(cx, MAP_ENTRY, span,
+                               &format!("usage of `contains_key` followed by `insert` on `{}`", kind), |db| {
+                db.span_suggestion(span, "Consider using", help.clone());
+            });
         }
     }
 }
diff --git a/src/lib.rs b/src/lib.rs
index f8db15605ff..9bb59693795 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -71,7 +71,7 @@ pub mod temporary_assignment;
 pub mod transmute;
 pub mod cyclomatic_complexity;
 pub mod escape;
-pub mod hashmap;
+pub mod entry;
 pub mod misc_early;
 pub mod array_indexing;
 pub mod panic;
@@ -113,7 +113,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_late_lint_pass(box types::UnitCmp);
     reg.register_late_lint_pass(box loops::LoopsPass);
     reg.register_late_lint_pass(box lifetimes::LifetimePass);
-    reg.register_late_lint_pass(box hashmap::HashMapLint);
+    reg.register_late_lint_pass(box entry::HashMapLint);
     reg.register_late_lint_pass(box ranges::StepByZero);
     reg.register_late_lint_pass(box types::CastPass);
     reg.register_late_lint_pass(box types::TypeComplexityPass);
@@ -167,10 +167,10 @@ pub fn plugin_registrar(reg: &mut Registry) {
         block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT,
         collapsible_if::COLLAPSIBLE_IF,
         cyclomatic_complexity::CYCLOMATIC_COMPLEXITY,
+        entry::MAP_ENTRY,
         eq_op::EQ_OP,
         escape::BOXED_LOCAL,
         eta_reduction::REDUNDANT_CLOSURE,
-        hashmap::HASHMAP_ENTRY,
         identity_op::IDENTITY_OP,
         len_zero::LEN_WITHOUT_IS_EMPTY,
         len_zero::LEN_ZERO,
diff --git a/src/utils.rs b/src/utils.rs
index 77e63fe4458..53f83d6921a 100644
--- a/src/utils.rs
+++ b/src/utils.rs
@@ -19,17 +19,18 @@ use std::ops::{Deref, DerefMut};
 pub type MethodArgs = HirVec<P<Expr>>;
 
 // module DefPaths for certain structs/enums we check for
+pub const BEGIN_UNWIND: [&'static str; 3] = ["std", "rt", "begin_unwind"];
+pub const BTREEMAP_PATH: [&'static str; 4] = ["collections", "btree", "map", "BTreeMap"];
+pub const CLONE_PATH: [&'static str; 2] = ["Clone", "clone"];
+pub const COW_PATH: [&'static str; 3] = ["collections", "borrow", "Cow"];
+pub const HASHMAP_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "HashMap"];
+pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "LinkedList"];
+pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"];
+pub const OPEN_OPTIONS_PATH: [&'static str; 3] = ["std", "fs", "OpenOptions"];
 pub const OPTION_PATH: [&'static str; 3] = ["core", "option", "Option"];
 pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"];
 pub const STRING_PATH: [&'static str; 3] = ["collections", "string", "String"];
 pub const VEC_PATH: [&'static str; 3] = ["collections", "vec", "Vec"];
-pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "LinkedList"];
-pub const HASHMAP_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "HashMap"];
-pub const OPEN_OPTIONS_PATH: [&'static str; 3] = ["std", "fs", "OpenOptions"];
-pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"];
-pub const CLONE_PATH: [&'static str; 2] = ["Clone", "clone"];
-pub const BEGIN_UNWIND: [&'static str; 3] = ["std", "rt", "begin_unwind"];
-pub const COW_PATH: [&'static str; 3] = ["collections", "borrow", "Cow"];
 
 /// Produce a nested chain of if-lets and ifs from the patterns:
 ///
@@ -77,20 +78,21 @@ 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)
+/// 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
 }
-/// returns true if this expn_info was expanded by any macro
+/// Returns true if this `expn_info` was expanded by any macro.
 pub fn in_macro<T: LintContext>(cx: &T, span: Span) -> bool {
     cx.sess().codemap().with_expn_info(span.expn_id, |info| info.is_some())
 }
 
-/// returns true if the macro that expanded the crate was outside of
-/// the current crate or was a compiler plugin
+/// Returns true if the macro that expanded the crate was outside of the current crate or was a
+/// compiler plugin.
 pub fn in_external_macro<T: LintContext>(cx: &T, span: Span) -> bool {
-    /// invokes in_macro with the expansion info of the given span
-    /// slightly heavy, try to use this after other checks have already happened
+    /// Invokes in_macro with the expansion info of the given span slightly heavy, try to use this
+    /// after other checks have already happened.
     fn in_macro_ext<T: LintContext>(cx: &T, opt_info: Option<&ExpnInfo>) -> bool {
         // no ExpnInfo = no macro
         opt_info.map_or(false, |info| {
@@ -109,9 +111,12 @@ pub fn in_external_macro<T: LintContext>(cx: &T, span: Span) -> bool {
     cx.sess().codemap().with_expn_info(span.expn_id, |info| in_macro_ext(cx, info))
 }
 
-/// check if a DefId's path matches the given absolute type path
-/// usage e.g. with
-/// `match_def_path(cx, id, &["core", "option", "Option"])`
+/// Check if a `DefId`'s path matches the given absolute type path usage.
+///
+/// # Examples
+/// ```
+/// match_def_path(cx, id, &["core", "option", "Option"])
+/// ```
 pub fn match_def_path(cx: &LateContext, def_id: DefId, path: &[&str]) -> bool {
     cx.tcx.with_path(def_id, |iter| {
         iter.zip(path)
@@ -119,7 +124,7 @@ pub fn match_def_path(cx: &LateContext, def_id: DefId, path: &[&str]) -> bool {
     })
 }
 
-/// check if type is struct or enum type with given def path
+/// Check if type is struct or enum type with given def path.
 pub fn match_type(cx: &LateContext, ty: ty::Ty, path: &[&str]) -> bool {
     match ty.sty {
         ty::TyEnum(ref adt, _) | ty::TyStruct(ref adt, _) => match_def_path(cx, adt.did, path),
@@ -127,7 +132,7 @@ pub fn match_type(cx: &LateContext, ty: ty::Ty, path: &[&str]) -> bool {
     }
 }
 
-/// check if method call given in "expr" belongs to given trait
+/// Check if the method call given in `expr` belongs to given trait.
 pub fn match_impl_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool {
     let method_call = ty::MethodCall::expr(expr.id);
 
@@ -144,9 +149,10 @@ pub fn match_impl_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool {
     }
 }
 
-/// check if method call given in "expr" belongs to given trait
+/// Check if the method call given in `expr` belongs to given trait.
 pub fn match_trait_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool {
     let method_call = ty::MethodCall::expr(expr.id);
+
     let trt_id = cx.tcx
                    .tables
                    .borrow()
@@ -160,21 +166,31 @@ pub fn match_trait_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool
     }
 }
 
-/// match a Path against a slice of segment string literals, e.g.
-/// `match_path(path, &["std", "rt", "begin_unwind"])`
+/// Match a `Path` against a slice of segment string literals.
+///
+/// # Examples
+/// ```
+/// match_path(path, &["std", "rt", "begin_unwind"])
+/// ```
 pub fn match_path(path: &Path, segments: &[&str]) -> bool {
     path.segments.iter().rev().zip(segments.iter().rev()).all(|(a, b)| a.identifier.name.as_str() == *b)
 }
 
-/// match a Path against a slice of segment string literals, e.g.
-/// `match_path(path, &["std", "rt", "begin_unwind"])`
+/// Match a `Path` against a slice of segment string literals, e.g.
+///
+/// # Examples
+/// ```
+/// match_path(path, &["std", "rt", "begin_unwind"])
+/// ```
 pub fn match_path_ast(path: &ast::Path, segments: &[&str]) -> bool {
     path.segments.iter().rev().zip(segments.iter().rev()).all(|(a, b)| a.identifier.name.as_str() == *b)
 }
 
-/// match an Expr against a chain of methods, and return the matched Exprs. For example, if `expr`
-/// represents the `.baz()` in `foo.bar().baz()`, `matched_method_chain(expr, &["bar", "baz"])`
-/// will return a Vec containing the Exprs for `.bar()` and `.baz()`
+/// Match an `Expr` against a chain of methods, and return the matched `Expr`s.
+///
+/// For example, if `expr` represents the `.baz()` in `foo.bar().baz()`,
+/// `matched_method_chain(expr, &["bar", "baz"])` will return a `Vec` containing the `Expr`s for
+/// `.bar()` and `.baz()`
 pub fn method_chain_args<'a>(expr: &'a Expr, methods: &[&str]) -> Option<Vec<&'a MethodArgs>> {
     let mut current = expr;
     let mut matched = Vec::with_capacity(methods.len());
@@ -196,7 +212,7 @@ pub fn method_chain_args<'a>(expr: &'a Expr, methods: &[&str]) -> Option<Vec<&'a
 }
 
 
-/// get the name of the item the expression is in, if available
+/// Get the name of the item the expression is in, if available.
 pub fn get_item_name(cx: &LateContext, expr: &Expr) -> Option<Name> {
     let parent_id = cx.tcx.map.get_parent(expr.id);
     match cx.tcx.map.find(parent_id) {
@@ -207,7 +223,7 @@ pub fn get_item_name(cx: &LateContext, expr: &Expr) -> Option<Name> {
     }
 }
 
-/// checks if a `let` decl is from a for loop desugaring
+/// Checks if a `let` decl is from a `for` loop desugaring.
 pub fn is_from_for_desugar(decl: &Decl) -> bool {
     if_let_chain! {
         [
@@ -221,31 +237,39 @@ pub fn is_from_for_desugar(decl: &Decl) -> bool {
 }
 
 
-/// convert a span to a code snippet if available, otherwise use default, e.g.
-/// `snippet(cx, expr.span, "..")`
+/// Convert a span to a code snippet if available, otherwise use default.
+///
+/// # Example
+/// ```
+/// snippet(cx, expr.span, "..")
+/// ```
 pub fn snippet<'a, T: LintContext>(cx: &T, span: Span, default: &'a str) -> Cow<'a, str> {
     cx.sess().codemap().span_to_snippet(span).map(From::from).unwrap_or(Cow::Borrowed(default))
 }
 
-/// Converts a span to a code snippet. Returns None if not available.
+/// Convert a span to a code snippet. Returns `None` if not available.
 pub fn snippet_opt<T: LintContext>(cx: &T, span: Span) -> Option<String> {
     cx.sess().codemap().span_to_snippet(span).ok()
 }
 
-/// convert a span (from a block) to a code snippet if available, otherwise use default, e.g.
-/// `snippet(cx, expr.span, "..")`
-/// This trims the code of indentation, except for the first line
-/// Use it for blocks or block-like things which need to be printed as such
+/// Convert a span (from a block) to a code snippet if available, otherwise use default.
+/// This trims the code of indentation, except for the first line. Use it for blocks or block-like
+/// things which need to be printed as such.
+///
+/// # Example
+/// ```
+/// snippet(cx, expr.span, "..")
+/// ```
 pub fn snippet_block<'a, T: LintContext>(cx: &T, span: Span, default: &'a str) -> Cow<'a, str> {
     let snip = snippet(cx, span, default);
     trim_multiline(snip, true)
 }
 
-/// Like snippet_block, but add braces if the expr is not an ExprBlock
-/// Also takes an Option<String> which can be put inside the braces
+/// Like `snippet_block`, but add braces if the expr is not an `ExprBlock`.
+/// Also takes an `Option<String>` which can be put inside the braces.
 pub fn expr_block<'a, T: LintContext>(cx: &T, expr: &Expr, option: Option<String>, default: &'a str) -> Cow<'a, str> {
     let code = snippet_block(cx, expr.span, default);
-    let string = option.map_or("".to_owned(), |s| s);
+    let string = option.unwrap_or_default();
     if let ExprBlock(_) = expr.node {
         Cow::Owned(format!("{}{}", code, string))
     } else if string.is_empty() {
@@ -255,8 +279,7 @@ pub fn expr_block<'a, T: LintContext>(cx: &T, expr: &Expr, option: Option<String
     }
 }
 
-/// Trim indentation from a multiline string
-/// with possibility of ignoring the first line
+/// Trim indentation from a multiline string with possibility of ignoring the first line.
 pub fn trim_multiline(s: Cow<str>, ignore_first: bool) -> Cow<str> {
     let s_space = trim_multiline_inner(s, ignore_first, ' ');
     let s_tab = trim_multiline_inner(s_space, ignore_first, '\t');
@@ -296,7 +319,7 @@ fn trim_multiline_inner(s: Cow<str>, ignore_first: bool, ch: char) -> Cow<str> {
     }
 }
 
-/// get a parent expr if any – this is useful to constrain a lint
+/// Get a parent expressions if any – this is useful to constrain a lint.
 pub fn get_parent_expr<'c>(cx: &'c LateContext, e: &Expr) -> Option<&'c Expr> {
     let map = &cx.tcx.map;
     let node_id: NodeId = e.id;
@@ -349,22 +372,7 @@ impl<'a> Deref for DiagnosticWrapper<'a> {
     }
 }
 
-#[cfg(not(feature="structured_logging"))]
-pub fn span_lint<'a, T: LintContext>(cx: &'a T, lint: &'static Lint, sp: Span, msg: &str) -> DiagnosticWrapper<'a> {
-    let mut db = cx.struct_span_lint(lint, sp, msg);
-    if cx.current_level(lint) != Level::Allow {
-        db.fileline_help(sp,
-                         &format!("for further information visit https://github.com/Manishearth/rust-clippy/wiki#{}",
-                                  lint.name_lower()));
-    }
-    DiagnosticWrapper(db)
-}
-
-#[cfg(feature="structured_logging")]
 pub fn span_lint<'a, T: LintContext>(cx: &'a T, lint: &'static Lint, sp: Span, msg: &str) -> DiagnosticWrapper<'a> {
-    // lint.name / lint.desc is can give details of the lint
-    // cx.sess().codemap() has all these nice functions for line/column/snippet details
-    // http://doc.rust-lang.org/syntax/codemap/struct.CodeMap.html#method.span_to_string
     let mut db = cx.struct_span_lint(lint, sp, msg);
     if cx.current_level(lint) != Level::Allow {
         db.fileline_help(sp,
@@ -418,7 +426,7 @@ pub fn span_lint_and_then<'a, T: LintContext, F>(cx: &'a T, lint: &'static Lint,
     db
 }
 
-/// return the base type for references and raw pointers
+/// Return the base type for references and raw pointers.
 pub fn walk_ptrs_ty(ty: ty::Ty) -> ty::Ty {
     match ty.sty {
         ty::TyRef(_, ref tm) | ty::TyRawPtr(ref tm) => walk_ptrs_ty(tm.ty),
@@ -426,7 +434,7 @@ pub fn walk_ptrs_ty(ty: ty::Ty) -> ty::Ty {
     }
 }
 
-/// return the base type for references and raw pointers, and count reference depth
+/// Return the base type for references and raw pointers, and count reference depth.
 pub fn walk_ptrs_ty_depth(ty: ty::Ty) -> (ty::Ty, usize) {
     fn inner(ty: ty::Ty, depth: usize) -> (ty::Ty, usize) {
         match ty.sty {