From 09129c1b416cf0101b75e72e6a3ffdfbbef78542 Mon Sep 17 00:00:00 2001 From: mcarton Date: Tue, 12 Jan 2016 20:23:28 +0100 Subject: [PATCH] Add BTreeMap to the HASHMAP_ENTRY rule Fixes #433 --- README.md | 2 +- src/{hashmap.rs => entry.rs} | 67 ++++++++++++--------- src/lib.rs | 6 +- src/utils.rs | 15 ++--- tests/compile-fail/{hashmap.rs => entry.rs} | 10 ++- 5 files changed, 58 insertions(+), 42 deletions(-) rename src/{hashmap.rs => entry.rs} (53%) rename tests/compile-fail/{hashmap.rs => entry.rs} (79%) diff --git a/README.md b/README.md index 131050877066..631c1d7f1d2b 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,6 @@ 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 @@ -43,6 +42,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 [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 [match_overlapping_arm](https://github.com/Manishearth/rust-clippy/wiki#match_overlapping_arm) | warn | a match has overlapping arms [match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats) | warn | a match or `if let` has all arms prefixed with `&`; the match expression can be dereferenced instead diff --git a/src/hashmap.rs b/src/entry.rs similarity index 53% rename from src/hashmap.rs rename to src/entry.rs index 095a00e3777b..1885bea164b8 100644 --- a/src/hashmap.rs +++ b/src/entry.rs @@ -2,11 +2,12 @@ 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::{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 = ¶ms[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,21 @@ 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, ¶ms[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!("Consider using `{}.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!("Consider using `{}.entry({})`", + snippet(cx, map.span, ".."), + snippet(cx, params[1].span, "..")) + }; + + span_help_and_lint(cx, MAP_ENTRY, span, + &format!("usage of `contains_key` followed by `insert` on `{}`", kind), + &help); } } } diff --git a/src/lib.rs b/src/lib.rs index f8db15605ffa..9bb59693795c 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 77e63fe4458a..d3ebe809ac92 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -19,17 +19,18 @@ use std::ops::{Deref, DerefMut}; pub type MethodArgs = HirVec>; // 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: /// diff --git a/tests/compile-fail/hashmap.rs b/tests/compile-fail/entry.rs similarity index 79% rename from tests/compile-fail/hashmap.rs rename to tests/compile-fail/entry.rs index a53566a794e8..7ea0e3952b08 100644 --- a/tests/compile-fail/hashmap.rs +++ b/tests/compile-fail/entry.rs @@ -2,9 +2,9 @@ #![plugin(clippy)] #![allow(unused)] -#![deny(hashmap_entry)] +#![deny(map_entry)] -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use std::hash::Hash; fn foo() {} @@ -33,6 +33,12 @@ fn insert_if_absent3(m: &mut HashMap, k: K, v: V) { //~^^HELP: Consider using `m.entry(k)` } +fn insert_in_btreemap(m: &mut BTreeMap, k: K, v: V) { + if !m.contains_key(&k) { foo(); m.insert(k, v) } else { None }; + //~^ERROR: usage of `contains_key` followed by `insert` on `BTreeMap` + //~^^HELP: Consider using `m.entry(k)` +} + fn insert_other_if_absent(m: &mut HashMap, k: K, o: K, v: V) { if !m.contains_key(&k) { m.insert(o, v); } }