From 0c6e385493407815068b9ca618a0baa09ad20d08 Mon Sep 17 00:00:00 2001 From: mcarton Date: Tue, 22 Dec 2015 00:35:56 +0100 Subject: [PATCH 1/4] Implement a HashMapLint --- README.md | 3 +- src/hashmap.rs | 73 +++++++++++++++++++++++++++++++++++ src/lib.rs | 3 ++ src/loops.rs | 4 +- src/utils.rs | 17 ++++---- tests/compile-fail/hashmap.rs | 25 ++++++++++++ 6 files changed, 114 insertions(+), 11 deletions(-) create mode 100644 src/hashmap.rs create mode 100644 tests/compile-fail/hashmap.rs diff --git a/README.md b/README.md index 0d854b384fec..f1c288886364 100644 --- a/README.md +++ b/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 diff --git a/src/hashmap.rs b/src/hashmap.rs new file mode 100644 index 000000000000..6448847ee2f3 --- /dev/null +++ b/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 = ¶ms[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, ".."))); + } + } +} diff --git a/src/lib.rs b/src/lib.rs index e79f0f6ca228..76c04d53a0b0 100644 --- a/src/lib.rs +++ b/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, diff --git a/src/loops.rs b/src/loops.rs index 9f103f4e7a80..a0454a325e4d 100644 --- a/src/loops.rs +++ b/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"]) || diff --git a/src/utils.rs b/src/utils.rs index 90e8e27b4f05..76b57317c311 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -18,15 +18,16 @@ use std::ops::{Deref, DerefMut}; pub type MethodArgs = HirVec>; // 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: /// diff --git a/tests/compile-fail/hashmap.rs b/tests/compile-fail/hashmap.rs new file mode 100644 index 000000000000..9b15acf40727 --- /dev/null +++ b/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(m: &mut HashMap, 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(m: &mut HashMap, 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(m: &mut HashMap, k: K, o: K, v: V) { + if !m.contains_key(&k) { m.insert(o, v) } else { None }; +} +*/ + +fn main() { +} From 54b70ed8e1ead333c0e45d21ff3daa89061a8b05 Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 3 Jan 2016 15:49:25 +0100 Subject: [PATCH 2/4] Move eq_op::is_exp_equal to utils --- src/eq_op.rs | 71 +------------------------------------------------- src/strings.rs | 3 +-- src/utils.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 72 deletions(-) diff --git a/src/eq_op.rs b/src/eq_op.rs index 4865596b6302..3d2f32e6c7fb 100644 --- a/src/eq_op.rs +++ b/src/eq_op.rs @@ -1,10 +1,8 @@ use rustc::lint::*; use rustc_front::hir::*; use rustc_front::util as ast_util; -use syntax::ptr::P; -use consts::constant; -use utils::span_lint; +use utils::{is_exp_equal, span_lint}; /// **What it does:** This lint checks for equal operands to comparisons and bitwise binary operators (`&`, `|` and `^`). It is `Warn` by default. /// @@ -40,57 +38,6 @@ impl LateLintPass for EqOp { } } -pub fn is_exp_equal(cx: &LateContext, left : &Expr, right : &Expr) -> bool { - if let (Some(l), Some(r)) = (constant(cx, left), constant(cx, right)) { - if l == r { - return true; - } - } - match (&left.node, &right.node) { - (&ExprField(ref lfexp, ref lfident), - &ExprField(ref rfexp, ref rfident)) => - lfident.node == rfident.node && is_exp_equal(cx, lfexp, rfexp), - (&ExprLit(ref l), &ExprLit(ref r)) => l.node == r.node, - (&ExprPath(ref lqself, ref lsubpath), - &ExprPath(ref rqself, ref rsubpath)) => - both(lqself, rqself, is_qself_equal) && - is_path_equal(lsubpath, rsubpath), - (&ExprTup(ref ltup), &ExprTup(ref rtup)) => - is_exps_equal(cx, ltup, rtup), - (&ExprVec(ref l), &ExprVec(ref r)) => is_exps_equal(cx, l, r), - (&ExprCast(ref lx, ref lt), &ExprCast(ref rx, ref rt)) => - is_exp_equal(cx, lx, rx) && is_cast_ty_equal(lt, rt), - _ => false - } -} - -fn is_exps_equal(cx: &LateContext, left : &[P], right : &[P]) -> bool { - over(left, right, |l, r| is_exp_equal(cx, l, r)) -} - -fn is_path_equal(left : &Path, right : &Path) -> bool { - // The == of idents doesn't work with different contexts, - // we have to be explicit about hygiene - left.global == right.global && over(&left.segments, &right.segments, - |l, r| l.identifier.name == r.identifier.name - && l.parameters == r.parameters) -} - -fn is_qself_equal(left : &QSelf, right : &QSelf) -> bool { - left.ty.node == right.ty.node && left.position == right.position -} - -fn over(left: &[X], right: &[X], mut eq_fn: F) -> bool - where F: FnMut(&X, &X) -> bool { - left.len() == right.len() && left.iter().zip(right).all(|(x, y)| - eq_fn(x, y)) -} - -fn both(l: &Option, r: &Option, mut eq_fn : F) -> bool - where F: FnMut(&X, &X) -> bool { - l.as_ref().map_or_else(|| r.is_none(), |x| r.as_ref().map_or(false, - |y| eq_fn(x, y))) -} fn is_cmp_or_bit(op : &BinOp) -> bool { match op.node { @@ -99,19 +46,3 @@ fn is_cmp_or_bit(op : &BinOp) -> bool { _ => false } } - -fn is_cast_ty_equal(left: &Ty, right: &Ty) -> bool { - match (&left.node, &right.node) { - (&TyVec(ref lvec), &TyVec(ref rvec)) => is_cast_ty_equal(lvec, rvec), - (&TyPtr(ref lmut), &TyPtr(ref rmut)) => - lmut.mutbl == rmut.mutbl && - is_cast_ty_equal(&*lmut.ty, &*rmut.ty), - (&TyRptr(_, ref lrmut), &TyRptr(_, ref rrmut)) => - lrmut.mutbl == rrmut.mutbl && - is_cast_ty_equal(&*lrmut.ty, &*rrmut.ty), - (&TyPath(ref lq, ref lpath), &TyPath(ref rq, ref rpath)) => - both(lq, rq, is_qself_equal) && is_path_equal(lpath, rpath), - (&TyInfer, &TyInfer) => true, - _ => false - } -} diff --git a/src/strings.rs b/src/strings.rs index 861ae0bb0129..2baf26095b79 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -7,8 +7,7 @@ use rustc::lint::*; use rustc_front::hir::*; use syntax::codemap::Spanned; -use eq_op::is_exp_equal; -use utils::{match_type, span_lint, walk_ptrs_ty, get_parent_expr}; +use utils::{is_exp_equal, match_type, span_lint, walk_ptrs_ty, get_parent_expr}; use utils::STRING_PATH; /// **What it does:** This lint matches code of the form `x = x + y` (without `let`!). It is `Allow` by default. diff --git a/src/utils.rs b/src/utils.rs index 76b57317c311..1b6c75a3b78a 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -10,6 +10,7 @@ use syntax::ast::Lit_::*; use syntax::ast; use syntax::errors::DiagnosticBuilder; use syntax::ptr::P; +use consts::constant; use rustc::session::Session; use std::str::FromStr; @@ -493,3 +494,71 @@ fn parse_attrs(sess: &Session, attrs: &[ast::Attribute], name: &' } } } + +pub fn is_exp_equal(cx: &LateContext, left : &Expr, right : &Expr) -> bool { + if let (Some(l), Some(r)) = (constant(cx, left), constant(cx, right)) { + if l == r { + return true; + } + } + match (&left.node, &right.node) { + (&ExprField(ref lfexp, ref lfident), + &ExprField(ref rfexp, ref rfident)) => + lfident.node == rfident.node && is_exp_equal(cx, lfexp, rfexp), + (&ExprLit(ref l), &ExprLit(ref r)) => l.node == r.node, + (&ExprPath(ref lqself, ref lsubpath), + &ExprPath(ref rqself, ref rsubpath)) => + both(lqself, rqself, is_qself_equal) && + is_path_equal(lsubpath, rsubpath), + (&ExprTup(ref ltup), &ExprTup(ref rtup)) => + is_exps_equal(cx, ltup, rtup), + (&ExprVec(ref l), &ExprVec(ref r)) => is_exps_equal(cx, l, r), + (&ExprCast(ref lx, ref lt), &ExprCast(ref rx, ref rt)) => + is_exp_equal(cx, lx, rx) && is_cast_ty_equal(lt, rt), + _ => false + } +} + +fn is_exps_equal(cx: &LateContext, left : &[P], right : &[P]) -> bool { + over(left, right, |l, r| is_exp_equal(cx, l, r)) +} + +fn is_path_equal(left : &Path, right : &Path) -> bool { + // The == of idents doesn't work with different contexts, + // we have to be explicit about hygiene + left.global == right.global && over(&left.segments, &right.segments, + |l, r| l.identifier.name == r.identifier.name + && l.parameters == r.parameters) +} + +fn is_qself_equal(left : &QSelf, right : &QSelf) -> bool { + left.ty.node == right.ty.node && left.position == right.position +} + +fn over(left: &[X], right: &[X], mut eq_fn: F) -> bool + where F: FnMut(&X, &X) -> bool { + left.len() == right.len() && left.iter().zip(right).all(|(x, y)| + eq_fn(x, y)) +} + +fn both(l: &Option, r: &Option, mut eq_fn : F) -> bool + where F: FnMut(&X, &X) -> bool { + l.as_ref().map_or_else(|| r.is_none(), |x| r.as_ref().map_or(false, + |y| eq_fn(x, y))) +} + +fn is_cast_ty_equal(left: &Ty, right: &Ty) -> bool { + match (&left.node, &right.node) { + (&TyVec(ref lvec), &TyVec(ref rvec)) => is_cast_ty_equal(lvec, rvec), + (&TyPtr(ref lmut), &TyPtr(ref rmut)) => + lmut.mutbl == rmut.mutbl && + is_cast_ty_equal(&*lmut.ty, &*rmut.ty), + (&TyRptr(_, ref lrmut), &TyRptr(_, ref rrmut)) => + lrmut.mutbl == rrmut.mutbl && + is_cast_ty_equal(&*lrmut.ty, &*rrmut.ty), + (&TyPath(ref lq, ref lpath), &TyPath(ref rq, ref rpath)) => + both(lq, rq, is_qself_equal) && is_path_equal(lpath, rpath), + (&TyInfer, &TyInfer) => true, + _ => false + } +} From d0bb71e6a23d45b4e367007da1099df9db424b8d Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 3 Jan 2016 16:31:28 +0100 Subject: [PATCH 3/4] Finish the HashMapLint --- src/hashmap.rs | 34 ++++++++++++++++++++++------------ tests/compile-fail/hashmap.rs | 4 +--- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/hashmap.rs b/src/hashmap.rs index 6448847ee2f3..14f535b38207 100644 --- a/src/hashmap.rs +++ b/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 = ¶ms[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, ¶ms[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, ".."))); } } } diff --git a/tests/compile-fail/hashmap.rs b/tests/compile-fail/hashmap.rs index 9b15acf40727..9aeacac0e14a 100644 --- a/tests/compile-fail/hashmap.rs +++ b/tests/compile-fail/hashmap.rs @@ -15,11 +15,9 @@ fn insert_if_absent2(m: &mut HashMap, 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(m: &mut HashMap, 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() { } From 9945bd82a8273f9c5506e9c26d51b312188d620f Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 3 Jan 2016 17:19:49 +0100 Subject: [PATCH 4/4] Add better error messages for HashMapLint --- src/hashmap.rs | 37 ++++++++++++++++++++++++++--------- tests/compile-fail/hashmap.rs | 24 ++++++++++++++++++++--- 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/src/hashmap.rs b/src/hashmap.rs index 14f535b38207..095a00e3777b 100644 --- a/src/hashmap.rs +++ b/src/hashmap.rs @@ -14,7 +14,14 @@ use utils::HASHMAP_PATH; /// if !m.contains_key(k) { m.insert(k.clone(), v); } /// ``` /// -/// **Example:** `if !m.contains_key(&k) { m.insert(k, v) }` +/// **Example:** +/// ```rust +/// if !m.contains_key(&k) { m.insert(k, v) } +/// ``` +/// can be rewritten as: +/// ```rust +/// m.entry(k).or_insert(v); +/// ``` declare_lint! { pub HASHMAP_ENTRY, Warn, @@ -49,13 +56,15 @@ impl LateLintPass for HashMapLint { 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; + if let Some(ref then) = then.expr { - check_for_insert(cx, expr.span, map, key, then); + check_for_insert(cx, expr.span, map, key, then, sole_expr); } for stmt in &then.stmts { if let StmtSemi(ref stmt, _) = stmt.node { - check_for_insert(cx, expr.span, map, key, stmt); + check_for_insert(cx, expr.span, map, key, stmt, sole_expr); } } } @@ -64,7 +73,7 @@ impl LateLintPass for HashMapLint { } } -fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, key: &Expr, expr: &Expr) { +fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, key: &Expr, expr: &Expr, sole_expr: bool) { if_let_chain! { [ let ExprMethodCall(ref name, _, ref params) = expr.node, @@ -73,11 +82,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]) ], { - 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, ".."))); + 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, ".."))); + } + 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, ".."))); + } } } } diff --git a/tests/compile-fail/hashmap.rs b/tests/compile-fail/hashmap.rs index 9aeacac0e14a..a53566a794e8 100644 --- a/tests/compile-fail/hashmap.rs +++ b/tests/compile-fail/hashmap.rs @@ -7,12 +7,30 @@ use std::collections::HashMap; use std::hash::Hash; -fn insert_if_absent(m: &mut HashMap, k: K, v: V) { - if !m.contains_key(&k) { m.insert(k, v); } //~ERROR: usage of `contains_key` followed by `insert` on `HashMap` +fn foo() {} + +fn insert_if_absent0(m: &mut HashMap, k: K, v: V) { + if !m.contains_key(&k) { m.insert(k, v); } + //~^ERROR: usage of `contains_key` followed by `insert` on `HashMap` + //~^^HELP: Consider using `m.entry(k).or_insert(v)` +} + +fn insert_if_absent1(m: &mut HashMap, k: K, v: V) { + if !m.contains_key(&k) { foo(); m.insert(k, v); } + //~^ERROR: usage of `contains_key` followed by `insert` on `HashMap` + //~^^HELP: Consider using `m.entry(k)` } fn insert_if_absent2(m: &mut HashMap, 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` + if !m.contains_key(&k) { m.insert(k, v) } else { None }; + //~^ERROR: usage of `contains_key` followed by `insert` on `HashMap` + //~^^HELP: Consider using `m.entry(k).or_insert(v)` +} + +fn insert_if_absent3(m: &mut HashMap, 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 `HashMap` + //~^^HELP: Consider using `m.entry(k)` } fn insert_other_if_absent(m: &mut HashMap, k: K, o: K, v: V) {