Improve map_entry suggestion. Will now suggest or_insert, insert_with or match _.entry(_) as appopriate.

Fix `map_entry` false positives where the entry api can't be used. e.g. when the map is used for multiple things.

Author: Jarcho

clippy_lints/src/entry.rs

@@ -1,15 +1,14 @@
 use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF};
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
-use clippy_utils::ty::{can_partially_move_ty, is_type_diagnostic_item, peel_mid_ty_refs_is_mutable};
-use clippy_utils::{is_allowed, is_else_clause_of_if_let_else, match_def_path, match_var, paths, peel_hir_expr_refs};
+use clippy_utils::ty::{is_type_diagnostic_item, peel_mid_ty_refs_is_mutable};
+use clippy_utils::{
+    can_move_expr_to_closure, is_allowed, is_else_clause_of_if_let_else, match_def_path, match_var, paths,
+    peel_hir_expr_refs,
+};
 use rustc_ast::util::parser::PREC_POSTFIX;
 use rustc_errors::Applicability;
-use rustc_hir::{
-    def::Res,
-    intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
-    Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, Pat, PatKind, Path, QPath,
-};
+use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, Pat, PatKind, QPath};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -193,51 +192,6 @@ impl LateLintPass<'_> for ManualMap {
     }
 }
 
-// Checks if the expression can be moved into a closure as is.
-fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
-    struct V<'cx, 'tcx> {
-        cx: &'cx LateContext<'tcx>,
-        make_closure: bool,
-    }
-    impl Visitor<'tcx> for V<'_, 'tcx> {
-        type Map = ErasedMap<'tcx>;
-        fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
-            NestedVisitorMap::None
-        }
-
-        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
-            match e.kind {
-                ExprKind::Break(..)
-                | ExprKind::Continue(_)
-                | ExprKind::Ret(_)
-                | ExprKind::Yield(..)
-                | ExprKind::InlineAsm(_)
-                | ExprKind::LlvmInlineAsm(_) => {
-                    self.make_closure = false;
-                },
-                // Accessing a field of a local value can only be done if the type isn't
-                // partially moved.
-                ExprKind::Field(base_expr, _)
-                    if matches!(
-                        base_expr.kind,
-                        ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. }))
-                    ) && can_partially_move_ty(self.cx, self.cx.typeck_results().expr_ty(base_expr)) =>
-                {
-                    // TODO: check if the local has been partially moved. Assume it has for now.
-                    self.make_closure = false;
-                    return;
-                }
-                _ => (),
-            };
-            walk_expr(self, e);
-        }
-    }
-
-    let mut v = V { cx, make_closure: true };
-    v.visit_expr(expr);
-    v.make_closure
-}
-
 // Checks whether the expression could be passed as a function, or whether a closure is needed.
 // Returns the function to be passed to `map` if it exists.
 fn can_pass_as_func(cx: &LateContext<'tcx>, binding: Ident, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {

clippy_lints/src/manual_map.rs

@@ -59,12 +59,12 @@ use rustc_data_structures::fx::FxHashMap;
 use rustc_hir as hir;
 use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
-use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
+use rustc_hir::intravisit::{self, walk_expr, ErasedMap, NestedVisitorMap, Visitor};
 use rustc_hir::{
-    def, Arm, BindingAnnotation, Block, Body, Constness, CrateItem, Expr, ExprKind, FnDecl, ForeignItem, GenericArgs,
-    GenericParam, HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Lifetime, Local, MacroDef,
-    MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath, Stmt, StructField, TraitItem, TraitItemKind,
-    TraitRef, TyKind, Variant, Visibility,
+    def, Arm, BindingAnnotation, Block, Body, Constness, CrateItem, Destination, Expr, ExprKind, FnDecl, ForeignItem,
+    GenericArgs, GenericParam, HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Lifetime, Local,
+    MacroDef, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath, Stmt, StmtKind, StructField, TraitItem,
+    TraitItemKind, TraitRef, TyKind, Variant, Visibility,
 };
 use rustc_lint::{LateContext, Level, Lint, LintContext};
 use rustc_middle::hir::exports::Export;
@@ -81,7 +81,7 @@ use rustc_span::{Span, DUMMY_SP};
 use rustc_target::abi::Integer;
 
 use crate::consts::{constant, Constant};
-use crate::ty::is_recursively_primitive_type;
+use crate::ty::{can_partially_move_ty, is_recursively_primitive_type};
 
 pub fn parse_msrv(msrv: &str, sess: Option<&Session>, span: Option<Span>) -> Option<RustcVersion> {
     if let Ok(version) = RustcVersion::parse(msrv) {
@@ -515,6 +515,73 @@ pub fn trait_ref_of_method<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio
     None
 }
 
+/// Checks if the top level expression can be moved into a closure as is.
+pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, jump_targets: &[HirId]) -> bool {
+    match expr.kind {
+        ExprKind::Break(Destination { target_id: Ok(id), .. }, _)
+        | ExprKind::Continue(Destination { target_id: Ok(id), .. })
+            if jump_targets.contains(&id) =>
+        {
+            true
+        },
+        ExprKind::Break(..)
+        | ExprKind::Continue(_)
+        | ExprKind::Ret(_)
+        | ExprKind::Yield(..)
+        | ExprKind::InlineAsm(_)
+        | ExprKind::LlvmInlineAsm(_) => false,
+        // Accessing a field of a local value can only be done if the type isn't
+        // partially moved.
+        ExprKind::Field(base_expr, _)
+            if matches!(
+                base_expr.kind,
+                ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. }))
+            ) && can_partially_move_ty(cx, cx.typeck_results().expr_ty(base_expr)) =>
+        {
+            // TODO: check if the local has been partially moved. Assume it has for now.
+            false
+        }
+        _ => true,
+    }
+}
+
+/// Checks if the expression can be moved into a closure as is.
+pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
+    struct V<'cx, 'tcx> {
+        cx: &'cx LateContext<'tcx>,
+        loops: Vec<HirId>,
+        allow_closure: bool,
+    }
+    impl Visitor<'tcx> for V<'_, 'tcx> {
+        type Map = ErasedMap<'tcx>;
+        fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+            NestedVisitorMap::None
+        }
+
+        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+            if !self.allow_closure {
+                return;
+            }
+            if let ExprKind::Loop(b, ..) = e.kind {
+                self.loops.push(e.hir_id);
+                self.visit_block(b);
+                self.loops.pop();
+            } else {
+                self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops);
+                walk_expr(self, e);
+            }
+        }
+    }
+
+    let mut v = V {
+        cx,
+        allow_closure: true,
+        loops: Vec::new(),
+    };
+    v.visit_expr(expr);
+    v.allow_closure
+}
+
 /// Returns the method names and argument list of nested method call expressions that make up
 /// `expr`. method/span lists are sorted with the most recent call first.
 pub fn method_calls<'tcx>(
@@ -1254,6 +1321,48 @@ pub fn is_must_use_func_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     did.map_or(false, |did| must_use_attr(&cx.tcx.get_attrs(did)).is_some())
 }
 
+pub fn get_expr_use_node(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<Node<'tcx>> {
+    let map = tcx.hir();
+    let mut child_id = expr.hir_id;
+    let mut iter = map.parent_iter(child_id);
+    loop {
+        match iter.next() {
+            None => break None,
+            Some((id, Node::Block(_))) => child_id = id,
+            Some((id, Node::Arm(arm))) if arm.body.hir_id == child_id => child_id = id,
+            Some((_, Node::Expr(expr))) => match expr.kind {
+                ExprKind::Break(
+                    Destination {
+                        target_id: Ok(dest), ..
+                    },
+                    _,
+                ) => {
+                    iter = map.parent_iter(dest);
+                    child_id = dest;
+                },
+                ExprKind::DropTemps(_) | ExprKind::Block(..) => child_id = expr.hir_id,
+                ExprKind::If(control_expr, ..) | ExprKind::Match(control_expr, ..)
+                    if control_expr.hir_id != child_id =>
+                {
+                    child_id = expr.hir_id
+                },
+                _ => break Some(Node::Expr(expr)),
+            },
+            Some((_, node)) => break Some(node),
+        }
+    }
+}
+
+pub fn is_expr_used(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
+    !matches!(
+        get_expr_use_node(tcx, expr),
+        Some(Node::Stmt(Stmt {
+            kind: StmtKind::Expr(_) | StmtKind::Semi(_),
+            ..
+        }))
+    )
+}
+
 pub fn is_no_std_crate(cx: &LateContext<'_>) -> bool {
     cx.tcx.hir().attrs(hir::CRATE_HIR_ID).iter().any(|attr| {
         if let ast::AttrKind::Normal(ref attr, _) = attr.kind {
@@ -1413,28 +1522,43 @@ pub fn peel_hir_pat_refs(pat: &'a Pat<'a>) -> (&'a Pat<'a>, usize) {
     peel(pat, 0)
 }
 
+/// Peels of expressions while the given closure returns `Some`.
+pub fn peel_hir_expr_while<'tcx>(
+    mut expr: &'tcx Expr<'tcx>,
+    mut f: impl FnMut(&'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>>,
+) -> &'tcx Expr<'tcx> {
+    while let Some(e) = f(expr) {
+        expr = e;
+    }
+    expr
+}
+
 /// Peels off up to the given number of references on the expression. Returns the underlying
 /// expression and the number of references removed.
 pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) {
-    fn f(expr: &'a Expr<'a>, count: usize, target: usize) -> (&'a Expr<'a>, usize) {
-        match expr.kind {
-            ExprKind::AddrOf(_, _, expr) if count != target => f(expr, count + 1, target),
-            _ => (expr, count),
-        }
-    }
-    f(expr, 0, count)
+    let mut remaining = count;
+    let e = peel_hir_expr_while(expr, |e| match e.kind {
+        ExprKind::AddrOf(BorrowKind::Ref, _, e) if remaining != 0 => {
+            remaining -= 1;
+            Some(e)
+        },
+        _ => None,
+    });
+    (e, count - remaining)
 }
 
 /// Peels off all references on the expression. Returns the underlying expression and the number of
 /// references removed.
 pub fn peel_hir_expr_refs(expr: &'a Expr<'a>) -> (&'a Expr<'a>, usize) {
-    fn f(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) {
-        match expr.kind {
-            ExprKind::AddrOf(BorrowKind::Ref, _, expr) => f(expr, count + 1),
-            _ => (expr, count),
-        }
-    }
-    f(expr, 0)
+    let mut count = 0;
+    let e = peel_hir_expr_while(expr, |e| match e.kind {
+        ExprKind::AddrOf(BorrowKind::Ref, _, e) => {
+            count += 1;
+            Some(e)
+        },
+        _ => None,
+    });
+    (e, count)
 }
 
 #[macro_export]

clippy_utils/src/lib.rs

@@ -13,7 +13,9 @@ pub(super) const BEGIN_PANIC_FMT: [&str; 3] = ["std", "panicking", "begin_panic_
 pub const BINARY_HEAP: [&str; 4] = ["alloc", "collections", "binary_heap", "BinaryHeap"];
 pub const BORROW_TRAIT: [&str; 3] = ["core", "borrow", "Borrow"];
 pub const BTREEMAP: [&str; 5] = ["alloc", "collections", "btree", "map", "BTreeMap"];
+pub const BTREEMAP_CONTAINS_KEY: [&str; 6] = ["alloc", "collections", "btree", "map", "BTreeMap", "contains_key"];
 pub const BTREEMAP_ENTRY: [&str; 6] = ["alloc", "collections", "btree", "map", "entry", "Entry"];
+pub const BTREEMAP_INSERT: [&str; 6] = ["alloc", "collections", "btree", "map", "BTreeMap", "insert"];
 pub const BTREESET: [&str; 5] = ["alloc", "collections", "btree", "set", "BTreeSet"];
 pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"];
 pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"];
@@ -46,7 +48,9 @@ pub const FROM_ITERATOR: [&str; 5] = ["core", "iter", "traits", "collect", "From
 pub const FUTURE_FROM_GENERATOR: [&str; 3] = ["core", "future", "from_generator"];
 pub const HASH: [&str; 3] = ["core", "hash", "Hash"];
 pub const HASHMAP: [&str; 5] = ["std", "collections", "hash", "map", "HashMap"];
+pub const HASHMAP_CONTAINS_KEY: [&str; 6] = ["std", "collections", "hash", "map", "HashMap", "contains_key"];
 pub const HASHMAP_ENTRY: [&str; 5] = ["std", "collections", "hash", "map", "Entry"];
+pub const HASHMAP_INSERT: [&str; 6] = ["std", "collections", "hash", "map", "HashMap", "insert"];
 pub const HASHSET: [&str; 5] = ["std", "collections", "hash", "set", "HashSet"];
 #[cfg(feature = "internal-lints")]
 pub const IDENT: [&str; 3] = ["rustc_span", "symbol", "Ident"];