Implement a HashMapLint

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 90 lints included in this crate:
+There are 91 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -31,6 +31,7 @@ name
 [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop)                       | warn    | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do
 [filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next)                                     | warn    | using `filter(p).next()`, which is more succinctly expressed as `.find(p)`
 [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp)                                         | warn    | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds)
+[hashmap_entry](https://github.com/Manishearth/rust-clippy/wiki#hashmap_entry)                                 | warn    | use of `contains_key` followed by `insert` on a `HashMap`
 [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op)                                     | warn    | using identity operations, e.g. `x + 0` or `y / 1`
 [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask)                   | warn    | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2`
 [inline_always](https://github.com/Manishearth/rust-clippy/wiki#inline_always)                                 | warn    | `#[inline(always)]` is a bad idea in most cases

src/hashmap.rs

@@ -0,0 +1,73 @@
+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::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.
+///
+/// **Example:** `if !m.contains_key(&k) { m.insert(k, v) }`
+declare_lint! {
+    pub HASHMAP_ENTRY,
+    Warn,
+    "use of `contains_key` followed by `insert` on a `HashMap`"
+}
+
+#[derive(Copy,Clone)]
+pub struct HashMapLint;
+
+impl LintPass for HashMapLint {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(HASHMAP_ENTRY)
+    }
+}
+
+impl LateLintPass for HashMapLint {
+    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+        if_let_chain! {
+            [
+                let ExprIf(ref check, ref then, _) = expr.node,
+                let ExprUnary(UnOp::UnNot, ref check) = check.node,
+                let ExprMethodCall(ref name, _, ref params) = check.node,
+                params.len() >= 2,
+                name.node.as_str() == "contains_key"
+            ], {
+                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);
+                    }
+                    else if then.stmts.len() == 1 {
+                        if let StmtSemi(ref stmt, _) = then.stmts[0].node {
+                            check_for_insert(cx, expr.span, map, stmt);
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
+
+fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, expr: &Expr) {
+    if_let_chain! {
+        [
+            let ExprMethodCall(ref name, _, ref params) = expr.node,
+            params.len() >= 3,
+            name.node.as_str() == "insert",
+            get_item_name(cx, map) == get_item_name(cx, &*params[0])
+        ], {
+            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, "..")));
+        }
+    }
+}

src/lib.rs

@@ -65,6 +65,7 @@ pub mod temporary_assignment;
 pub mod transmute;
 pub mod cyclomatic_complexity;
 pub mod escape;
+pub mod hashmap;
 pub mod misc_early;
 pub mod array_indexing;
 pub mod panic;
@@ -104,6 +105,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 ranges::StepByZero);
     reg.register_late_lint_pass(box types::CastPass);
     reg.register_late_lint_pass(box types::TypeComplexityPass);
@@ -158,6 +160,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         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,

src/loops.rs

@@ -13,7 +13,7 @@ use syntax::ast::Lit_::*;
 use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type,
             in_external_macro, expr_block, span_help_and_lint, is_integer_literal,
             get_enclosing_block};
-use utils::{VEC_PATH, LL_PATH};
+use utils::{HASHMAP_PATH, VEC_PATH, LL_PATH};
 
 /// **What it does:** This lint checks for looping over the range of `0..len` of some collection just to get the values by index. It is `Warn` by default.
 ///
@@ -457,7 +457,7 @@ fn is_ref_iterable_type(cx: &LateContext, e: &Expr) -> bool {
     is_iterable_array(ty) ||
         match_type(cx, ty, &VEC_PATH) ||
         match_type(cx, ty, &LL_PATH) ||
-        match_type(cx, ty, &["std", "collections", "hash", "map", "HashMap"]) ||
+        match_type(cx, ty, &HASHMAP_PATH) ||
         match_type(cx, ty, &["std", "collections", "hash", "set", "HashSet"]) ||
         match_type(cx, ty, &["collections", "vec_deque", "VecDeque"]) ||
         match_type(cx, ty, &["collections", "binary_heap", "BinaryHeap"]) ||

src/utils.rs

@@ -18,15 +18,16 @@ use std::ops::{Deref, DerefMut};
 pub type MethodArgs = HirVec<P<Expr>>;
 
 // module DefPaths for certain structs/enums we check for
-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 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 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"];
 
 /// Produce a nested chain of if-lets and ifs from the patterns:
 ///

tests/compile-fail/hashmap.rs

@@ -0,0 +1,25 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+#![allow(unused)]
+
+#![deny(hashmap_entry)]
+
+use std::collections::HashMap;
+use std::hash::Hash;
+
+fn insert_if_absent<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
+    if !m.contains_key(&k) { m.insert(k, v); } //~ERROR: usage of `contains_key` followed by `insert` on `HashMap`
+}
+
+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 };
+}
+*/
+
+fn main() {
+}