Finish the HashMapLint

src/hashmap.rs

@@ -1,14 +1,18 @@
 use rustc::lint::*;
 use rustc_front::hir::*;
 use syntax::codemap::Span;
-use utils::{get_item_name, match_type, snippet, span_help_and_lint, walk_ptrs_ty};
+use utils::{get_item_name, is_exp_equal, match_type, snippet, span_help_and_lint, walk_ptrs_ty};
 use utils::HASHMAP_PATH;
 
 /// **What it does:** This lint checks for uses of `contains_key` + `insert` on `HashMap`.
 ///
 /// **Why is this bad?** Using `HashMap::entry` is more efficient.
 ///
-/// **Known problems:** Hopefully none.
+/// **Known problems:** Some false negatives, eg.:
+/// ```
+/// let k = &key;
+/// if !m.contains_key(k) { m.insert(k.clone(), v); }
+/// ```
 ///
 /// **Example:** `if !m.contains_key(&k) { m.insert(k, v) }`
 declare_lint! {
@@ -36,16 +40,22 @@ impl LateLintPass for HashMapLint {
                 params.len() >= 2,
                 name.node.as_str() == "contains_key"
             ], {
+                let key = match params[1].node {
+                    ExprAddrOf(_, ref key) => key,
+                    _ => return
+                };
+
                 let map = &params[0];
                 let obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(map));
 
                 if match_type(cx, obj_ty, &HASHMAP_PATH) {
                     if let Some(ref then) = then.expr {
-                        check_for_insert(cx, expr.span, map, then);
+                        check_for_insert(cx, expr.span, map, key, then);
                     }
-                    else if then.stmts.len() == 1 {
-                        if let StmtSemi(ref stmt, _) = then.stmts[0].node {
-                            check_for_insert(cx, expr.span, map, stmt);
+
+                    for stmt in &then.stmts {
+                        if let StmtSemi(ref stmt, _) = stmt.node {
+                            check_for_insert(cx, expr.span, map, key, stmt);
                         }
                     }
                 }
@@ -54,20 +64,20 @@ impl LateLintPass for HashMapLint {
     }
 }
 
-fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, expr: &Expr) {
+fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, key: &Expr, expr: &Expr) {
     if_let_chain! {
         [
             let ExprMethodCall(ref name, _, ref params) = expr.node,
-            params.len() >= 3,
+            params.len() == 3,
             name.node.as_str() == "insert",
-            get_item_name(cx, map) == get_item_name(cx, &*params[0])
+            get_item_name(cx, map) == get_item_name(cx, &*params[0]),
+            is_exp_equal(cx, key, &params[1])
         ], {
             span_help_and_lint(cx, HASHMAP_ENTRY, span,
                                "usage of `contains_key` followed by `insert` on `HashMap`",
-                               &format!("Consider using `{}.entry({}).or_insert({})`",
+                               &format!("Consider using `{}.entry({})`",
                                         snippet(cx, map.span, ".."),
-                                        snippet(cx, params[1].span, ".."),
-                                        snippet(cx, params[2].span, "..")));
+                                        snippet(cx, params[1].span, "..")));
         }
     }
 }

tests/compile-fail/hashmap.rs

@@ -15,11 +15,9 @@ fn insert_if_absent2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
     if !m.contains_key(&k) { m.insert(k, v) } else { None }; //~ERROR: usage of `contains_key` followed by `insert` on `HashMap`
 }
 
-/* TODO
 fn insert_other_if_absent<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, o: K, v: V) {
-    if !m.contains_key(&k) { m.insert(o, v) } else { None };
+    if !m.contains_key(&k) { m.insert(o, v); }
 }
-*/
 
 fn main() {
 }