From ce5e927713ddd32096f4e73f7a2c339cc8163d82 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 25 Mar 2021 09:25:04 -0400 Subject: [PATCH 1/4] Improve `map_entry` lint Fix false positives where the map is used before inserting into the map. Fix false positives where two insertions happen. Suggest using `if let Entry::Vacant(e) = _.entry(_)` when `or_insert` might be a semantic change --- clippy_lints/src/entry.rs | 489 ++++++++++++++++++++++++-------- clippy_utils/src/lib.rs | 125 ++++++-- clippy_utils/src/paths.rs | 4 + clippy_utils/src/source.rs | 9 + tests/ui/entry.fixed | 101 +++++++ tests/ui/entry.rs | 103 +++++++ tests/ui/entry.stderr | 171 +++++++++++ tests/ui/entry_fixable.fixed | 15 - tests/ui/entry_fixable.rs | 17 -- tests/ui/entry_fixable.stderr | 12 - tests/ui/entry_unfixable.rs | 59 ++-- tests/ui/entry_unfixable.stderr | 57 ---- 12 files changed, 882 insertions(+), 280 deletions(-) create mode 100644 tests/ui/entry.fixed create mode 100644 tests/ui/entry.rs create mode 100644 tests/ui/entry.stderr delete mode 100644 tests/ui/entry_fixable.fixed delete mode 100644 tests/ui/entry_fixable.rs delete mode 100644 tests/ui/entry_fixable.stderr delete mode 100644 tests/ui/entry_unfixable.stderr diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index a815df1691a1..ca01d0a7f875 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -1,17 +1,18 @@ -use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::source::{snippet, snippet_opt, snippet_with_applicability}; -use clippy_utils::ty::{is_type_diagnostic_item, match_type}; -use clippy_utils::SpanlessEq; -use clippy_utils::{get_item_name, paths}; -use if_chain::if_chain; +use clippy_utils::{ + diagnostics::span_lint_and_sugg, + is_expr_final_block_expr, is_expr_used_or_unified, match_def_path, paths, peel_hir_expr_while, + source::{snippet_indent, snippet_with_applicability, snippet_with_context}, + SpanlessEq, +}; use rustc_errors::Applicability; -use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; -use rustc_hir::{BorrowKind, Expr, ExprKind, UnOp}; +use rustc_hir::{ + intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor}, + Expr, ExprKind, Guard, Local, Stmt, StmtKind, UnOp, +}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::hir::map::Map; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::source_map::Span; -use rustc_span::sym; +use rustc_span::{Span, SyntaxContext, DUMMY_SP}; +use std::fmt::Write; declare_clippy_lint! { /// **What it does:** Checks for uses of `contains_key` + `insert` on `HashMap` @@ -19,15 +20,14 @@ declare_clippy_lint! { /// /// **Why is this bad?** Using `entry` is more efficient. /// - /// **Known problems:** Some false negatives, eg.: + /// **Known problems:** The suggestion may have type inference errors in some cases. e.g. /// ```rust - /// # use std::collections::HashMap; - /// # let mut map = HashMap::new(); - /// # let v = 1; - /// # let k = 1; - /// if !map.contains_key(&k) { - /// map.insert(k.clone(), v); - /// } + /// let mut map = std::collections::HashMap::new(); + /// let _ = if !map.contains_key(&0) { + /// map.insert(0, 0) + /// } else { + /// None + /// }; /// ``` /// /// **Example:** @@ -56,132 +56,379 @@ declare_clippy_lint! { declare_lint_pass!(HashMapPass => [MAP_ENTRY]); impl<'tcx> LateLintPass<'tcx> for HashMapPass { + #[allow(clippy::too_many_lines)] fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let ExprKind::If(check, then_block, ref else_block) = expr.kind { - if let ExprKind::Unary(UnOp::Not, check) = check.kind { - if let Some((ty, map, key)) = check_cond(cx, check) { - // in case of `if !m.contains_key(&k) { m.insert(k, v); }` - // we can give a better error message - let sole_expr = { - else_block.is_none() - && if let ExprKind::Block(then_block, _) = then_block.kind { - (then_block.expr.is_some() as usize) + then_block.stmts.len() == 1 - } else { - true - } - // XXXManishearth we can also check for if/else blocks containing `None`. - }; + let (cond_expr, then_expr, else_expr) = match expr.kind { + ExprKind::If(c, t, e) => (c, t, e), + _ => return, + }; + let (map_ty, contains_expr) = match try_parse_contains(cx, cond_expr) { + Some(x) => x, + None => return, + }; - let mut visitor = InsertVisitor { - cx, - span: expr.span, - ty, - map, - key, - sole_expr, - }; + let then_search = match find_insert_calls(cx, &contains_expr, then_expr) { + Some(x) => x, + None => return, + }; - walk_expr(&mut visitor, then_block); - } - } else if let Some(else_block) = *else_block { - if let Some((ty, map, key)) = check_cond(cx, check) { - let mut visitor = InsertVisitor { - cx, - span: expr.span, - ty, - map, - key, - sole_expr: false, + let mut app = Applicability::MachineApplicable; + let map_str = snippet_with_context(cx, contains_expr.map.span, contains_expr.call_ctxt, "..", &mut app).0; + let key_str = snippet_with_context(cx, contains_expr.key.span, contains_expr.call_ctxt, "..", &mut app).0; + let sugg = if !contains_expr.negated || else_expr.is_some() || then_search.insertions.is_empty() { + return; + } else { + // if .. { insert } + match then_search.as_single_insertion() { + Some(insertion) if !insertion.value.can_have_side_effects() => { + format!( + "{}.entry({}).or_insert({});", + map_str, + key_str, + snippet_with_context(cx, insertion.value.span, insertion.call.span.ctxt(), "..", &mut app).0, + ) + }, + _ => { + let (body_str, entry_kind) = if contains_expr.negated { + (then_search.snippet_vacant(cx, then_expr.span, &mut app), "Vacant(e)") + } else { + ( + then_search.snippet_occupied(cx, then_expr.span, &mut app), + "Occupied(mut e)", + ) }; - - walk_expr(&mut visitor, else_block); - } + format!( + "if let {}::{} = {}.entry({}) {}", + map_ty.entry_path(), + entry_kind, + map_str, + key_str, + body_str, + ) + }, } + }; + + span_lint_and_sugg( + cx, + MAP_ENTRY, + expr.span, + &format!("usage of `contains_key` followed by `insert` on a `{}`", map_ty.name()), + "try this", + sugg, + app, + ); + } +} + +#[derive(Clone, Copy)] +enum MapType { + Hash, + BTree, +} +impl MapType { + fn name(self) -> &'static str { + match self { + Self::Hash => "HashMap", + Self::BTree => "BTreeMap", + } + } + fn entry_path(self) -> &'staic str { + match self { + Self::Hash => "std::collections::hash_map::Entry", + Self::BTree => "std::collections::btree_map::Entry", } } } -fn check_cond<'a>(cx: &LateContext<'_>, check: &'a Expr<'a>) -> Option<(&'static str, &'a Expr<'a>, &'a Expr<'a>)> { - if_chain! { - if let ExprKind::MethodCall(path, _, params, _) = check.kind; - if params.len() >= 2; - if path.ident.name == sym!(contains_key); - if let ExprKind::AddrOf(BorrowKind::Ref, _, key) = params[1].kind; - then { - let map = ¶ms[0]; - let obj_ty = cx.typeck_results().expr_ty(map).peel_refs(); - - return if match_type(cx, obj_ty, &paths::BTREEMAP) { - Some(("BTreeMap", map, key)) - } - else if is_type_diagnostic_item(cx, obj_ty, sym::hashmap_type) { - Some(("HashMap", map, key)) - } - else { - None +struct ContainsExpr<'tcx> { + negated: bool, + map: &'tcx Expr<'tcx>, + key: &'tcx Expr<'tcx>, + call_ctxt: SyntaxContext, +} +fn try_parse_contains(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<(MapType, ContainsExpr<'tcx>)> { + let mut negated = false; + let expr = peel_hir_expr_while(expr, |e| match e.kind { + ExprKind::Unary(UnOp::Not, e) => { + negated = !negated; + Some(e) + }, + _ => None, + }); + match expr.kind { + ExprKind::MethodCall( + _, + _, + [map, Expr { + kind: ExprKind::AddrOf(_, _, key), + span: key_span, + .. + }], + _, + ) if key_span.ctxt() == expr.span.ctxt() => { + let id = cx.typeck_results().type_dependent_def_id(expr.hir_id)?; + let expr = ContainsExpr { + negated, + map, + key, + call_ctxt: expr.span.ctxt(), }; + if match_def_path(cx, id, &paths::BTREEMAP_CONTAINS_KEY) { + Some((MapType::BTree, expr)) + } else if match_def_path(cx, id, &paths::HASHMAP_CONTAINS_KEY) { + Some((MapType::Hash, expr)) + } else { + None + } + }, + _ => None, + } +} + +struct InsertExpr<'tcx> { + map: &'tcx Expr<'tcx>, + key: &'tcx Expr<'tcx>, + value: &'tcx Expr<'tcx>, +} +fn try_parse_insert(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option> { + if let ExprKind::MethodCall(_, _, [map, key, value], _) = expr.kind { + let id = cx.typeck_results().type_dependent_def_id(expr.hir_id)?; + if match_def_path(cx, id, &paths::BTREEMAP_INSERT) || match_def_path(cx, id, &paths::HASHMAP_INSERT) { + Some(InsertExpr { map, key, value }) + } else { + None } + } else { + None } +} - None +#[derive(Clone, Copy)] +struct Insertion<'tcx> { + call: &'tcx Expr<'tcx>, + value: &'tcx Expr<'tcx>, } -struct InsertVisitor<'a, 'tcx, 'b> { - cx: &'a LateContext<'tcx>, - span: Span, - ty: &'static str, - map: &'b Expr<'b>, - key: &'b Expr<'b>, - sole_expr: bool, +// This visitor needs to do a multiple things: +// * Find all usages of the map. Only insertions into the map which share the same key are +// permitted. All others will prevent the lint. +// * Determine if the final statement executed is an insertion. This is needed to use `insert_with`. +// * Determine if there's any sub-expression that can't be placed in a closure. +// * Determine if there's only a single insert statement. This is needed to give better suggestions. + +#[allow(clippy::struct_excessive_bools)] +struct InsertSearcher<'cx, 'i, 'tcx> { + cx: &'cx LateContext<'tcx>, + /// The map expression used in the contains call. + map: &'tcx Expr<'tcx>, + /// The key expression used in the contains call. + key: &'tcx Expr<'tcx>, + /// The context of the top level block. All insert calls must be in the same context. + ctxt: SyntaxContext, + /// Whether this expression can use the entry api. + can_use_entry: bool, + // A single insert expression has a slightly different suggestion. + is_single_insert: bool, + is_map_used: bool, + insertions: &'i mut Vec>, +} +impl<'tcx> InsertSearcher<'_, '_, 'tcx> { + /// Visit the expression as a branch in control flow. Multiple insert calls can be used, but + /// only if they are on separate code paths. This will return whether the map was used in the + /// given expression. + fn visit_cond_arm(&mut self, e: &'tcx Expr<'_>) -> bool { + let is_map_used = self.is_map_used; + self.visit_expr(e); + let res = self.is_map_used; + self.is_map_used = is_map_used; + res + } } +impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> { + type Map = ErasedMap<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } -impl<'a, 'tcx, 'b> Visitor<'tcx> for InsertVisitor<'a, 'tcx, 'b> { - type Map = Map<'tcx>; + fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) { + match stmt.kind { + StmtKind::Semi(e) | StmtKind::Expr(e) => self.visit_expr(e), + StmtKind::Local(Local { init: Some(e), .. }) => { + self.is_single_insert = false; + self.visit_expr(e); + }, + _ => { + self.is_single_insert = false; + }, + } + } fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { - if_chain! { - if let ExprKind::MethodCall(path, _, params, _) = expr.kind; - if params.len() == 3; - if path.ident.name == sym!(insert); - if get_item_name(self.cx, self.map) == get_item_name(self.cx, ¶ms[0]); - if SpanlessEq::new(self.cx).eq_expr(self.key, ¶ms[1]); - if snippet_opt(self.cx, self.map.span) == snippet_opt(self.cx, params[0].span); - then { - span_lint_and_then(self.cx, MAP_ENTRY, self.span, - &format!("usage of `contains_key` followed by `insert` on a `{}`", self.ty), |diag| { - if self.sole_expr { - let mut app = Applicability::MachineApplicable; - let help = format!("{}.entry({}).or_insert({});", - snippet_with_applicability(self.cx, self.map.span, "map", &mut app), - snippet_with_applicability(self.cx, params[1].span, "..", &mut app), - snippet_with_applicability(self.cx, params[2].span, "..", &mut app)); - - diag.span_suggestion( - self.span, - "consider using", - help, - Applicability::MachineApplicable, // snippet - ); + if !self.can_use_entry { + return; + } + + match try_parse_insert(self.cx, expr) { + Some(insert_expr) if SpanlessEq::new(self.cx).eq_expr(self.map, insert_expr.map) => { + // Multiple inserts, inserts with a different key, and inserts from a macro can't use the entry api. + if self.is_map_used + || !SpanlessEq::new(self.cx).eq_expr(self.key, insert_expr.key) + || expr.span.ctxt() != self.ctxt + { + self.can_use_entry = false; + return; + } + + self.insertions.push(Insertion { + call: expr, + value: insert_expr.value, + }); + self.is_map_used = true; + + // The value doesn't affect whether there is only a single insert expression. + let is_single_insert = self.is_single_insert; + self.visit_expr(insert_expr.value); + self.is_single_insert = is_single_insert; + }, + _ if SpanlessEq::new(self.cx).eq_expr(self.map, expr) => { + self.is_map_used = true; + }, + _ => match expr.kind { + ExprKind::If(cond_expr, then_expr, Some(else_expr)) => { + self.is_single_insert = false; + self.visit_expr(cond_expr); + // Each branch may contain it's own insert expression. + let mut is_map_used = self.visit_cond_arm(then_expr); + is_map_used |= self.visit_cond_arm(else_expr); + self.is_map_used = is_map_used; + }, + ExprKind::Match(scrutinee_expr, arms, _) => { + self.is_single_insert = false; + self.visit_expr(scrutinee_expr); + // Each branch may contain it's own insert expression. + let mut is_map_used = self.is_map_used; + for arm in arms { + if let Some(Guard::If(guard) | Guard::IfLet(_, guard)) = arm.guard { + self.visit_expr(guard) + } + is_map_used |= self.visit_cond_arm(arm.body); } - else { - let help = format!("consider using `{}.entry({})`", - snippet(self.cx, self.map.span, "map"), - snippet(self.cx, params[1].span, "..")); - - diag.span_label( - self.span, - &help, - ); + self.is_map_used = is_map_used; + }, + ExprKind::Loop(block, ..) => { + // Don't allow insertions inside of a loop. + let insertions_len = self.insertions.len(); + self.visit_block(block); + if self.insertions.len() != insertions_len { + self.can_use_entry = false; } - }); - } + }, + ExprKind::Block(block, _) => self.visit_block(block), + ExprKind::InlineAsm(_) | ExprKind::LlvmInlineAsm(_) => { + self.can_use_entry = false; + }, + _ => { + self.is_single_insert = false; + walk_expr(self, expr); + }, + }, } + } +} - if !self.sole_expr { - walk_expr(self, expr); +struct InsertSearchResults<'tcx> { + insertions: Vec>, + is_single_insert: bool, +} +impl InsertSearchResults<'tcx> { + fn as_single_insertion(&self) -> Option> { + self.is_single_insert.then(|| self.insertions[0]) + } + + fn snippet_occupied(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String { + let ctxt = span.ctxt(); + let mut res = String::new(); + for insertion in self.insertions.iter() { + res.push_str(&snippet_with_applicability( + cx, + span.until(insertion.call.span), + "..", + app, + )); + if is_expr_used_or_unified(cx.tcx, insertion.call) { + res.push_str("Some(e.insert("); + res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0); + res.push_str("))"); + } else { + res.push_str("e.insert("); + res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0); + res.push(')'); + } + span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP); } + res.push_str(&snippet_with_applicability(cx, span, "..", app)); + res } - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::None + + fn snippet_vacant(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String { + let ctxt = span.ctxt(); + let mut res = String::new(); + for insertion in self.insertions.iter() { + res.push_str(&snippet_with_applicability( + cx, + span.until(insertion.call.span), + "..", + app, + )); + if is_expr_used_or_unified(cx.tcx, insertion.call) { + if is_expr_final_block_expr(cx.tcx, insertion.call) { + let _ = write!( + res, + "e.insert({});\n{}None", + snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0, + snippet_indent(cx, insertion.call.span).as_deref().unwrap_or(""), + ); + } else { + let _ = write!( + res, + "{{ e.insert({}); None }}", + snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0, + ); + } + } else { + let _ = write!( + res, + "e.insert({})", + snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0, + ); + } + span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP); + } + res.push_str(&snippet_with_applicability(cx, span, "..", app)); + res } } +fn find_insert_calls( + cx: &LateContext<'tcx>, + contains_expr: &ContainsExpr<'tcx>, + expr: &'tcx Expr<'_>, +) -> Option> { + let mut insertions = Vec::new(); + let mut s = InsertSearcher { + cx, + map: contains_expr.map, + key: contains_expr.key, + ctxt: expr.span.ctxt(), + insertions: &mut insertions, + is_map_used: false, + can_use_entry: true, + is_single_insert: true, + }; + s.visit_expr(expr); + let is_single_insert = s.is_single_insert; + s.can_use_entry.then(|| InsertSearchResults { + insertions, + is_single_insert, + }) +} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 8e1a2105b961..0abee49f40ce 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -63,9 +63,9 @@ use rustc_hir::def_id::{DefId, LOCAL_CRATE}; use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc_hir::LangItem::{ResultErr, ResultOk}; use rustc_hir::{ - def, Arm, BindingAnnotation, Block, Body, Constness, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl, ImplItem, - ImplItemKind, Item, ItemKind, LangItem, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath, - TraitItem, TraitItemKind, TraitRef, TyKind, + def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl, + ImplItem, ImplItemKind, Item, ItemKind, LangItem, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath, + Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind, }; use rustc_lint::{LateContext, Level, Lint, LintContext}; use rustc_middle::hir::exports::Export; @@ -1245,6 +1245,82 @@ 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> { + 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 get_expr_use_or_unification_node(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option> { + 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::Match(_, [arm], _) if arm.hir_id == child_id => child_id = expr.hir_id, + ExprKind::Block(..) | ExprKind::DropTemps(_) => child_id = expr.hir_id, + ExprKind::If(_, then_expr, None) if then_expr.hir_id == child_id => break None, + _ => break Some(Node::Expr(expr)), + }, + Some((_, node)) => break Some(node), + } + } +} + +pub fn is_expr_used_or_unified(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool { + !matches!( + get_expr_use_or_unification_node(tcx, expr), + None | Some(Node::Stmt(Stmt { + kind: StmtKind::Expr(_) | StmtKind::Semi(_), + .. + })) + ) +} + +pub fn is_expr_final_block_expr(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool { + matches!(get_parent_node(tcx, expr.hir_id), Some(Node::Block(..))) +} + 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 { @@ -1414,28 +1490,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] diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index ed8915f59e1d..8066f6c223e7 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.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"]; @@ -45,7 +47,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"]; diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs index 2d794d48dc5f..53180d1f9f54 100644 --- a/clippy_utils/src/source.rs +++ b/clippy_utils/src/source.rs @@ -66,6 +66,15 @@ pub fn indent_of(cx: &T, span: Span) -> Option { snippet_opt(cx, line_span(cx, span)).and_then(|snip| snip.find(|c: char| !c.is_whitespace())) } +/// Gets a snippet of the indentation of the line of a span +pub fn snippet_indent(cx: &T, span: Span) -> Option { + snippet_opt(cx, line_span(cx, span)).map(|mut s| { + let len = s.len() - s.trim_start().len(); + s.truncate(len); + s + }) +} + // If the snippet is empty, it's an attribute that was inserted during macro // expansion and we want to ignore those, because they could come from external // sources that the user has no control over. diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed new file mode 100644 index 000000000000..60371c9833c2 --- /dev/null +++ b/tests/ui/entry.fixed @@ -0,0 +1,101 @@ +// run-rustfix + +#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![warn(clippy::map_entry)] + +use std::collections::{BTreeMap, HashMap}; +use std::hash::Hash; + +macro_rules! m { + ($e:expr) => {{ $e }}; +} + +fn foo() {} + +fn hash_map(m: &mut HashMap, k: K, v: V, v2: V) { + m.entry(k).or_insert(v); + + if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + if true { + e.insert(v); + } else { + e.insert(v2); + } + } + + if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + if true { + e.insert(v); + } else { + e.insert(v2); + return; + } + } + + if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + foo(); + e.insert(v); + } + + if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + match 0 { + 1 if true => { + e.insert(v); + }, + _ => { + e.insert(v2); + }, + }; + } + + if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + match 0 { + 0 => {}, + 1 => { + e.insert(v); + }, + _ => { + e.insert(v2); + }, + }; + } + + if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + foo(); + match 0 { + 0 if false => { + e.insert(v); + }, + 1 => { + foo(); + e.insert(v); + }, + 2 | 3 => { + for _ in 0..2 { + foo(); + } + if true { + e.insert(v); + } else { + e.insert(v2); + }; + }, + _ => { + e.insert(v2); + }, + } + } + + if let std::collections::hash_map::Entry::Vacant(e) = m.entry(m!(k)) { + e.insert(m!(v)); + } +} + +fn btree_map(m: &mut BTreeMap, k: K, v: V, v2: V) { + if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { + e.insert(v); + foo(); + } +} + +fn main() {} diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs new file mode 100644 index 000000000000..4d3e241de785 --- /dev/null +++ b/tests/ui/entry.rs @@ -0,0 +1,103 @@ +// run-rustfix + +#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![warn(clippy::map_entry)] + +use std::collections::{BTreeMap, HashMap}; +use std::hash::Hash; + +macro_rules! m { + ($e:expr) => {{ $e }}; +} + +fn foo() {} + +fn hash_map(m: &mut HashMap, k: K, v: V, v2: V) { + if !m.contains_key(&k) { + m.insert(k, v); + } + + if !m.contains_key(&k) { + if true { + m.insert(k, v); + } else { + m.insert(k, v2); + } + } + + if !m.contains_key(&k) { + if true { + m.insert(k, v); + } else { + m.insert(k, v2); + return; + } + } + + if !m.contains_key(&k) { + foo(); + m.insert(k, v); + } + + if !m.contains_key(&k) { + match 0 { + 1 if true => { + m.insert(k, v); + }, + _ => { + m.insert(k, v2); + }, + }; + } + + if !m.contains_key(&k) { + match 0 { + 0 => {}, + 1 => { + m.insert(k, v); + }, + _ => { + m.insert(k, v2); + }, + }; + } + + if !m.contains_key(&k) { + foo(); + match 0 { + 0 if false => { + m.insert(k, v); + }, + 1 => { + foo(); + m.insert(k, v); + }, + 2 | 3 => { + for _ in 0..2 { + foo(); + } + if true { + m.insert(k, v); + } else { + m.insert(k, v2); + }; + }, + _ => { + m.insert(k, v2); + }, + } + } + + if !m.contains_key(&m!(k)) { + m.insert(m!(k), m!(v)); + } +} + +fn btree_map(m: &mut BTreeMap, k: K, v: V, v2: V) { + if !m.contains_key(&k) { + m.insert(k, v); + foo(); + } +} + +fn main() {} diff --git a/tests/ui/entry.stderr b/tests/ui/entry.stderr new file mode 100644 index 000000000000..ab108ed6861e --- /dev/null +++ b/tests/ui/entry.stderr @@ -0,0 +1,171 @@ +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:16:5 + | +LL | / if !m.contains_key(&k) { +LL | | m.insert(k, v); +LL | | } + | |_____^ help: try this: `m.entry(k).or_insert(v);` + | + = note: `-D clippy::map-entry` implied by `-D warnings` + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:20:5 + | +LL | / if !m.contains_key(&k) { +LL | | if true { +LL | | m.insert(k, v); +LL | | } else { +LL | | m.insert(k, v2); +LL | | } +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | if true { +LL | e.insert(v); +LL | } else { +LL | e.insert(v2); +LL | } + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:28:5 + | +LL | / if !m.contains_key(&k) { +LL | | if true { +LL | | m.insert(k, v); +LL | | } else { +... | +LL | | } +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | if true { +LL | e.insert(v); +LL | } else { +LL | e.insert(v2); +LL | return; + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:37:5 + | +LL | / if !m.contains_key(&k) { +LL | | foo(); +LL | | m.insert(k, v); +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | foo(); +LL | e.insert(v); +LL | } + | + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:42:5 + | +LL | / if !m.contains_key(&k) { +LL | | match 0 { +LL | | 1 if true => { +LL | | m.insert(k, v); +... | +LL | | }; +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | match 0 { +LL | 1 if true => { +LL | e.insert(v); +LL | }, +LL | _ => { + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:53:5 + | +LL | / if !m.contains_key(&k) { +LL | | match 0 { +LL | | 0 => {}, +LL | | 1 => { +... | +LL | | }; +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | match 0 { +LL | 0 => {}, +LL | 1 => { +LL | e.insert(v); +LL | }, + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:65:5 + | +LL | / if !m.contains_key(&k) { +LL | | foo(); +LL | | match 0 { +LL | | 0 if false => { +... | +LL | | } +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | foo(); +LL | match 0 { +LL | 0 if false => { +LL | e.insert(v); +LL | }, + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:91:5 + | +LL | / if !m.contains_key(&m!(k)) { +LL | | m.insert(m!(k), m!(v)); +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(m!(k)) { +LL | e.insert(m!(v)); +LL | } + | + +error: usage of `contains_key` followed by `insert` on a `BTreeMap` + --> $DIR/entry.rs:97:5 + | +LL | / if !m.contains_key(&k) { +LL | | m.insert(k, v); +LL | | foo(); +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { +LL | e.insert(v); +LL | foo(); +LL | } + | + +error: aborting due to 9 previous errors + diff --git a/tests/ui/entry_fixable.fixed b/tests/ui/entry_fixable.fixed deleted file mode 100644 index dcdaae7e7243..000000000000 --- a/tests/ui/entry_fixable.fixed +++ /dev/null @@ -1,15 +0,0 @@ -// run-rustfix - -#![allow(unused, clippy::needless_pass_by_value)] -#![warn(clippy::map_entry)] - -use std::collections::{BTreeMap, HashMap}; -use std::hash::Hash; - -fn foo() {} - -fn insert_if_absent0(m: &mut HashMap, k: K, v: V) { - m.entry(k).or_insert(v); -} - -fn main() {} diff --git a/tests/ui/entry_fixable.rs b/tests/ui/entry_fixable.rs deleted file mode 100644 index 55d5b21568d0..000000000000 --- a/tests/ui/entry_fixable.rs +++ /dev/null @@ -1,17 +0,0 @@ -// run-rustfix - -#![allow(unused, clippy::needless_pass_by_value)] -#![warn(clippy::map_entry)] - -use std::collections::{BTreeMap, HashMap}; -use std::hash::Hash; - -fn foo() {} - -fn insert_if_absent0(m: &mut HashMap, k: K, v: V) { - if !m.contains_key(&k) { - m.insert(k, v); - } -} - -fn main() {} diff --git a/tests/ui/entry_fixable.stderr b/tests/ui/entry_fixable.stderr deleted file mode 100644 index 87403200ced5..000000000000 --- a/tests/ui/entry_fixable.stderr +++ /dev/null @@ -1,12 +0,0 @@ -error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry_fixable.rs:12:5 - | -LL | / if !m.contains_key(&k) { -LL | | m.insert(k, v); -LL | | } - | |_____^ help: consider using: `m.entry(k).or_insert(v);` - | - = note: `-D clippy::map-entry` implied by `-D warnings` - -error: aborting due to previous error - diff --git a/tests/ui/entry_unfixable.rs b/tests/ui/entry_unfixable.rs index f530fc023cfb..beb2d5c97b1d 100644 --- a/tests/ui/entry_unfixable.rs +++ b/tests/ui/entry_unfixable.rs @@ -4,50 +4,14 @@ use std::collections::{BTreeMap, HashMap}; use std::hash::Hash; -fn foo() {} - -fn insert_if_absent2(m: &mut HashMap, k: K, v: V) { - if !m.contains_key(&k) { - m.insert(k, v) - } else { - None - }; -} - -fn insert_if_present2(m: &mut HashMap, k: K, v: V) { - if m.contains_key(&k) { - None - } else { - m.insert(k, v) - }; -} - -fn insert_if_absent3(m: &mut HashMap, k: K, v: V) { - if !m.contains_key(&k) { - foo(); - m.insert(k, v) - } else { - None - }; -} - -fn insert_if_present3(m: &mut HashMap, k: K, v: V) { - if m.contains_key(&k) { - None - } else { - foo(); - m.insert(k, v) +macro_rules! m { + ($map:expr, $key:expr, $value:expr) => { + $map.insert($key, $value) }; + ($e:expr) => {{ $e }}; } -fn insert_in_btreemap(m: &mut BTreeMap, k: K, v: V) { - if !m.contains_key(&k) { - foo(); - m.insert(k, v) - } else { - None - }; -} +fn foo() {} // should not trigger fn insert_other_if_absent(m: &mut HashMap, k: K, o: K, v: V) { @@ -70,4 +34,17 @@ fn insert_from_different_map2(m: &mut HashMap, n: &mut Ha } } +fn insert_in_macro(m: &mut HashMap, k: K, v: V) { + if !m.contains_key(&k) { + m!(m, k, v); + } +} + +fn use_map_then_insert(m: &mut HashMap, k: K, v: V) { + if !m.contains_key(&k) { + let _ = m.len(); + m.insert(k, v); + } +} + fn main() {} diff --git a/tests/ui/entry_unfixable.stderr b/tests/ui/entry_unfixable.stderr deleted file mode 100644 index e58c8d22dc45..000000000000 --- a/tests/ui/entry_unfixable.stderr +++ /dev/null @@ -1,57 +0,0 @@ -error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry_unfixable.rs:10:5 - | -LL | / if !m.contains_key(&k) { -LL | | m.insert(k, v) -LL | | } else { -LL | | None -LL | | }; - | |_____^ consider using `m.entry(k)` - | - = note: `-D clippy::map-entry` implied by `-D warnings` - -error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry_unfixable.rs:18:5 - | -LL | / if m.contains_key(&k) { -LL | | None -LL | | } else { -LL | | m.insert(k, v) -LL | | }; - | |_____^ consider using `m.entry(k)` - -error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry_unfixable.rs:26:5 - | -LL | / if !m.contains_key(&k) { -LL | | foo(); -LL | | m.insert(k, v) -LL | | } else { -LL | | None -LL | | }; - | |_____^ consider using `m.entry(k)` - -error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry_unfixable.rs:35:5 - | -LL | / if m.contains_key(&k) { -LL | | None -LL | | } else { -LL | | foo(); -LL | | m.insert(k, v) -LL | | }; - | |_____^ consider using `m.entry(k)` - -error: usage of `contains_key` followed by `insert` on a `BTreeMap` - --> $DIR/entry_unfixable.rs:44:5 - | -LL | / if !m.contains_key(&k) { -LL | | foo(); -LL | | m.insert(k, v) -LL | | } else { -LL | | None -LL | | }; - | |_____^ consider using `m.entry(k)` - -error: aborting due to 5 previous errors - From b63a5b56d6a37029970445ab5cbb77aeb5e19e84 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 25 Mar 2021 09:36:38 -0400 Subject: [PATCH 2/4] `map_entry` improvements Lint `if _.[!]contains_key(&_) { .. } else { .. }` so long as one of the branches contains an insertion. --- clippy_lints/src/entry.rs | 81 +++++++++++++++++- tests/ui/entry_with_else.fixed | 73 ++++++++++++++++ tests/ui/entry_with_else.rs | 60 ++++++++++++++ tests/ui/entry_with_else.stderr | 142 ++++++++++++++++++++++++++++++++ 4 files changed, 354 insertions(+), 2 deletions(-) create mode 100644 tests/ui/entry_with_else.fixed create mode 100644 tests/ui/entry_with_else.rs create mode 100644 tests/ui/entry_with_else.stderr diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index ca01d0a7f875..3e1d70b2e407 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -1,7 +1,7 @@ use clippy_utils::{ diagnostics::span_lint_and_sugg, is_expr_final_block_expr, is_expr_used_or_unified, match_def_path, paths, peel_hir_expr_while, - source::{snippet_indent, snippet_with_applicability, snippet_with_context}, + source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context}, SpanlessEq, }; use rustc_errors::Applicability; @@ -75,7 +75,84 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { let mut app = Applicability::MachineApplicable; let map_str = snippet_with_context(cx, contains_expr.map.span, contains_expr.call_ctxt, "..", &mut app).0; let key_str = snippet_with_context(cx, contains_expr.key.span, contains_expr.call_ctxt, "..", &mut app).0; - let sugg = if !contains_expr.negated || else_expr.is_some() || then_search.insertions.is_empty() { + let sugg = if let Some(else_expr) = else_expr { + // if .. { .. } else { .. } + let else_search = match find_insert_calls(cx, &contains_expr, else_expr) { + Some(search) if !(then_search.insertions.is_empty() && search.insertions.is_empty()) => search, + _ => return, + }; + + if then_search.insertions.is_empty() || else_search.insertions.is_empty() { + // if .. { insert } else { .. } or if .. { .. } else { then } of + let (then_str, else_str, entry_kind) = if else_search.insertions.is_empty() { + if contains_expr.negated { + ( + then_search.snippet_vacant(cx, then_expr.span, &mut app), + snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app), + "Vacant(e)", + ) + } else { + ( + then_search.snippet_occupied(cx, then_expr.span, &mut app), + snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app), + "Occupied(mut e)", + ) + } + } else if contains_expr.negated { + ( + else_search.snippet_occupied(cx, else_expr.span, &mut app), + snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app), + "Occupied(mut e)", + ) + } else { + ( + else_search.snippet_vacant(cx, else_expr.span, &mut app), + snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app), + "Vacant(e)", + ) + }; + format!( + "if let {}::{} = {}.entry({}) {} else {}", + map_ty.entry_path(), + entry_kind, + map_str, + key_str, + then_str, + else_str, + ) + } else { + // if .. { insert } else { insert } + let (then_str, else_str, then_entry, else_entry) = if contains_expr.negated { + ( + then_search.snippet_vacant(cx, then_expr.span, &mut app), + else_search.snippet_occupied(cx, else_expr.span, &mut app), + "Vacant(e)", + "Occupied(mut e)", + ) + } else { + ( + then_search.snippet_occupied(cx, then_expr.span, &mut app), + else_search.snippet_vacant(cx, else_expr.span, &mut app), + "Occupied(mut e)", + "Vacant(e)", + ) + }; + let indent_str = snippet_indent(cx, expr.span); + let indent_str = indent_str.as_deref().unwrap_or(""); + format!( + "match {}.entry({}) {{\n{indent} {entry}::{} => {}\n\ + {indent} {entry}::{} => {}\n{indent}}}", + map_str, + key_str, + then_entry, + reindent_multiline(then_str.into(), true, Some(4 + indent_str.len())), + else_entry, + reindent_multiline(else_str.into(), true, Some(4 + indent_str.len())), + entry = map_ty.entry_path(), + indent = indent_str, + ) + } + } else if then_search.insertions.is_empty() || !contains_expr.negated { return; } else { // if .. { insert } diff --git a/tests/ui/entry_with_else.fixed b/tests/ui/entry_with_else.fixed new file mode 100644 index 000000000000..2332fa6313ff --- /dev/null +++ b/tests/ui/entry_with_else.fixed @@ -0,0 +1,73 @@ +// run-rustfix + +#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![warn(clippy::map_entry)] + +use std::collections::{BTreeMap, HashMap}; +use std::hash::Hash; + +macro_rules! m { + ($e:expr) => {{ $e }}; +} + +fn foo() {} + +fn insert_if_absent0(m: &mut HashMap, k: K, v: V, v2: V) { + match m.entry(k) { + std::collections::hash_map::Entry::Vacant(e) => { + e.insert(v); + } + std::collections::hash_map::Entry::Occupied(mut e) => { + e.insert(v2); + } + } + + match m.entry(k) { + std::collections::hash_map::Entry::Occupied(mut e) => { + e.insert(v); + } + std::collections::hash_map::Entry::Vacant(e) => { + e.insert(v2); + } + } + + if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + e.insert(v); + } else { + foo(); + } + + if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) { + e.insert(v); + } else { + foo(); + } + + match m.entry(k) { + std::collections::hash_map::Entry::Vacant(e) => { + e.insert(v); + } + std::collections::hash_map::Entry::Occupied(mut e) => { + e.insert(v2); + } + } + + match m.entry(k) { + std::collections::hash_map::Entry::Occupied(mut e) => { + if true { Some(e.insert(v)) } else { Some(e.insert(v2)) } + } + std::collections::hash_map::Entry::Vacant(e) => { + e.insert(v); + None + } + }; + + if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) { + foo(); + Some(e.insert(v)) + } else { + None + }; +} + +fn main() {} diff --git a/tests/ui/entry_with_else.rs b/tests/ui/entry_with_else.rs new file mode 100644 index 000000000000..2ff0c038efe2 --- /dev/null +++ b/tests/ui/entry_with_else.rs @@ -0,0 +1,60 @@ +// run-rustfix + +#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![warn(clippy::map_entry)] + +use std::collections::{BTreeMap, HashMap}; +use std::hash::Hash; + +macro_rules! m { + ($e:expr) => {{ $e }}; +} + +fn foo() {} + +fn insert_if_absent0(m: &mut HashMap, k: K, v: V, v2: V) { + if !m.contains_key(&k) { + m.insert(k, v); + } else { + m.insert(k, v2); + } + + if m.contains_key(&k) { + m.insert(k, v); + } else { + m.insert(k, v2); + } + + if !m.contains_key(&k) { + m.insert(k, v); + } else { + foo(); + } + + if !m.contains_key(&k) { + foo(); + } else { + m.insert(k, v); + } + + if !m.contains_key(&k) { + m.insert(k, v); + } else { + m.insert(k, v2); + } + + if m.contains_key(&k) { + if true { m.insert(k, v) } else { m.insert(k, v2) } + } else { + m.insert(k, v) + }; + + if m.contains_key(&k) { + foo(); + m.insert(k, v) + } else { + None + }; +} + +fn main() {} diff --git a/tests/ui/entry_with_else.stderr b/tests/ui/entry_with_else.stderr new file mode 100644 index 000000000000..6f62ff8d3745 --- /dev/null +++ b/tests/ui/entry_with_else.stderr @@ -0,0 +1,142 @@ +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry_with_else.rs:16:5 + | +LL | / if !m.contains_key(&k) { +LL | | m.insert(k, v); +LL | | } else { +LL | | m.insert(k, v2); +LL | | } + | |_____^ + | + = note: `-D clippy::map-entry` implied by `-D warnings` +help: try this + | +LL | match m.entry(k) { +LL | std::collections::hash_map::Entry::Vacant(e) => { +LL | e.insert(v); +LL | } +LL | std::collections::hash_map::Entry::Occupied(mut e) => { +LL | e.insert(v2); + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry_with_else.rs:22:5 + | +LL | / if m.contains_key(&k) { +LL | | m.insert(k, v); +LL | | } else { +LL | | m.insert(k, v2); +LL | | } + | |_____^ + | +help: try this + | +LL | match m.entry(k) { +LL | std::collections::hash_map::Entry::Occupied(mut e) => { +LL | e.insert(v); +LL | } +LL | std::collections::hash_map::Entry::Vacant(e) => { +LL | e.insert(v2); + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry_with_else.rs:28:5 + | +LL | / if !m.contains_key(&k) { +LL | | m.insert(k, v); +LL | | } else { +LL | | foo(); +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | e.insert(v); +LL | } else { +LL | foo(); +LL | } + | + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry_with_else.rs:34:5 + | +LL | / if !m.contains_key(&k) { +LL | | foo(); +LL | | } else { +LL | | m.insert(k, v); +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) { +LL | e.insert(v); +LL | } else { +LL | foo(); +LL | } + | + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry_with_else.rs:40:5 + | +LL | / if !m.contains_key(&k) { +LL | | m.insert(k, v); +LL | | } else { +LL | | m.insert(k, v2); +LL | | } + | |_____^ + | +help: try this + | +LL | match m.entry(k) { +LL | std::collections::hash_map::Entry::Vacant(e) => { +LL | e.insert(v); +LL | } +LL | std::collections::hash_map::Entry::Occupied(mut e) => { +LL | e.insert(v2); + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry_with_else.rs:46:5 + | +LL | / if m.contains_key(&k) { +LL | | if true { m.insert(k, v) } else { m.insert(k, v2) } +LL | | } else { +LL | | m.insert(k, v) +LL | | }; + | |_____^ + | +help: try this + | +LL | match m.entry(k) { +LL | std::collections::hash_map::Entry::Occupied(mut e) => { +LL | if true { Some(e.insert(v)) } else { Some(e.insert(v2)) } +LL | } +LL | std::collections::hash_map::Entry::Vacant(e) => { +LL | e.insert(v); + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry_with_else.rs:52:5 + | +LL | / if m.contains_key(&k) { +LL | | foo(); +LL | | m.insert(k, v) +LL | | } else { +LL | | None +LL | | }; + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Occupied(mut e) = m.entry(k) { +LL | foo(); +LL | Some(e.insert(v)) +LL | } else { +LL | None +LL | }; + | + +error: aborting due to 7 previous errors + From 3323ff71455c763a03353527d6203f90b53058fd Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 25 Mar 2021 09:47:56 -0400 Subject: [PATCH 3/4] `map_entry` improvements Suggest using `or_insert_with` when possible --- clippy_lints/src/entry.rs | 220 ++++++++++++++++++++++++++------- clippy_lints/src/manual_map.rs | 53 +------- clippy_utils/src/lib.rs | 71 ++++++++++- tests/ui/entry.fixed | 52 ++++---- tests/ui/entry.rs | 8 ++ tests/ui/entry.stderr | 65 ++++++---- 6 files changed, 322 insertions(+), 147 deletions(-) diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index 3e1d70b2e407..0decb22a4914 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -1,4 +1,5 @@ use clippy_utils::{ + can_move_expr_to_closure_no_visit, diagnostics::span_lint_and_sugg, is_expr_final_block_expr, is_expr_used_or_unified, match_def_path, paths, peel_hir_expr_while, source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context}, @@ -7,7 +8,7 @@ use clippy_utils::{ use rustc_errors::Applicability; use rustc_hir::{ intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor}, - Expr, ExprKind, Guard, Local, Stmt, StmtKind, UnOp, + Block, Expr, ExprKind, Guard, HirId, Local, Stmt, StmtKind, UnOp, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -78,13 +79,13 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { let sugg = if let Some(else_expr) = else_expr { // if .. { .. } else { .. } let else_search = match find_insert_calls(cx, &contains_expr, else_expr) { - Some(search) if !(then_search.insertions.is_empty() && search.insertions.is_empty()) => search, + Some(search) if !(then_search.edits.is_empty() && search.edits.is_empty()) => search, _ => return, }; - if then_search.insertions.is_empty() || else_search.insertions.is_empty() { + if then_search.edits.is_empty() || else_search.edits.is_empty() { // if .. { insert } else { .. } or if .. { .. } else { then } of - let (then_str, else_str, entry_kind) = if else_search.insertions.is_empty() { + let (then_str, else_str, entry_kind) = if else_search.edits.is_empty() { if contains_expr.negated { ( then_search.snippet_vacant(cx, then_expr.span, &mut app), @@ -152,37 +153,48 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { indent = indent_str, ) } - } else if then_search.insertions.is_empty() || !contains_expr.negated { + } else if then_search.edits.is_empty() { + // no insertions return; } else { // if .. { insert } - match then_search.as_single_insertion() { - Some(insertion) if !insertion.value.can_have_side_effects() => { - format!( - "{}.entry({}).or_insert({});", - map_str, - key_str, - snippet_with_context(cx, insertion.value.span, insertion.call.span.ctxt(), "..", &mut app).0, + if !then_search.allow_insert_closure { + let (body_str, entry_kind) = if contains_expr.negated { + (then_search.snippet_vacant(cx, then_expr.span, &mut app), "Vacant(e)") + } else { + ( + then_search.snippet_occupied(cx, then_expr.span, &mut app), + "Occupied(mut e)", ) - }, - _ => { - let (body_str, entry_kind) = if contains_expr.negated { - (then_search.snippet_vacant(cx, then_expr.span, &mut app), "Vacant(e)") + }; + format!( + "if let {}::{} = {}.entry({}) {}", + map_ty.entry_path(), + entry_kind, + map_str, + key_str, + body_str, + ) + } else if let Some(insertion) = then_search.as_single_insertion() { + let value_str = snippet_with_context(cx, insertion.value.span, then_expr.span.ctxt(), "..", &mut app).0; + if contains_expr.negated { + if insertion.value.can_have_side_effects() { + format!("{}.entry({}).or_insert_with(|| {});", map_str, key_str, value_str) } else { - ( - then_search.snippet_occupied(cx, then_expr.span, &mut app), - "Occupied(mut e)", - ) - }; - format!( - "if let {}::{} = {}.entry({}) {}", - map_ty.entry_path(), - entry_kind, - map_str, - key_str, - body_str, - ) - }, + format!("{}.entry({}).or_insert({});", map_str, key_str, value_str) + } + } else { + // Todo: if let Some(v) = map.get_mut(k) + return; + } + } else { + let block_str = then_search.snippet_closure(cx, then_expr.span, &mut app); + if contains_expr.negated { + format!("{}.entry({}).or_insert_with(|| {});", map_str, key_str, block_str) + } else { + // Todo: if let Some(v) = map.get_mut(k) + return; + } } }; @@ -281,6 +293,19 @@ fn try_parse_insert(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option { + /// A semicolon that needs to be removed. Used to create a closure for `insert_with`. + RemoveSemi(Span), + /// An insertion into the map. + Insertion(Insertion<'tcx>), +} +impl Edit<'tcx> { + fn as_insertion(self) -> Option> { + if let Self::Insertion(i) = self { Some(i) } else { None } + } +} #[derive(Clone, Copy)] struct Insertion<'tcx> { call: &'tcx Expr<'tcx>, @@ -303,12 +328,17 @@ struct InsertSearcher<'cx, 'i, 'tcx> { key: &'tcx Expr<'tcx>, /// The context of the top level block. All insert calls must be in the same context. ctxt: SyntaxContext, + /// Whether this expression can be safely moved into a closure. + allow_insert_closure: bool, /// Whether this expression can use the entry api. can_use_entry: bool, + /// Whether this expression is the final expression in this code path. This may be a statement. + in_tail_pos: bool, // A single insert expression has a slightly different suggestion. is_single_insert: bool, is_map_used: bool, - insertions: &'i mut Vec>, + edits: &'i mut Vec>, + loops: Vec, } impl<'tcx> InsertSearcher<'_, '_, 'tcx> { /// Visit the expression as a branch in control flow. Multiple insert calls can be used, but @@ -316,11 +346,22 @@ impl<'tcx> InsertSearcher<'_, '_, 'tcx> { /// given expression. fn visit_cond_arm(&mut self, e: &'tcx Expr<'_>) -> bool { let is_map_used = self.is_map_used; + let in_tail_pos = self.in_tail_pos; self.visit_expr(e); let res = self.is_map_used; self.is_map_used = is_map_used; + self.in_tail_pos = in_tail_pos; res } + + /// Visits an expression which is not itself in a tail position, but other sibling expressions + /// may be. e.g. if conditions + fn visit_non_tail_expr(&mut self, e: &'tcx Expr<'_>) { + let in_tail_pos = self.in_tail_pos; + self.in_tail_pos = false; + self.visit_expr(e); + self.in_tail_pos = in_tail_pos; + } } impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> { type Map = ErasedMap<'tcx>; @@ -330,17 +371,63 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> { fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) { match stmt.kind { - StmtKind::Semi(e) | StmtKind::Expr(e) => self.visit_expr(e), + StmtKind::Semi(e) => { + self.visit_expr(e); + + if self.in_tail_pos && self.allow_insert_closure { + // The spans are used to slice the top level expression into multiple parts. This requires that + // they all come from the same part of the source code. + if stmt.span.ctxt() == self.ctxt && e.span.ctxt() == self.ctxt { + self.edits + .push(Edit::RemoveSemi(stmt.span.trim_start(e.span).unwrap_or(DUMMY_SP))); + } else { + self.allow_insert_closure = false; + } + } + }, + StmtKind::Expr(e) => self.visit_expr(e), StmtKind::Local(Local { init: Some(e), .. }) => { + self.allow_insert_closure &= !self.in_tail_pos; + self.in_tail_pos = false; self.is_single_insert = false; self.visit_expr(e); }, _ => { + self.allow_insert_closure &= !self.in_tail_pos; self.is_single_insert = false; }, } } + fn visit_block(&mut self, block: &'tcx Block<'_>) { + // If the block is in a tail position, then the last expression (possibly a statement) is in the + // tail position. The rest, however, are not. + match (block.stmts, block.expr) { + ([], None) => { + self.allow_insert_closure &= !self.in_tail_pos; + }, + ([], Some(expr)) => self.visit_expr(expr), + (stmts, Some(expr)) => { + let in_tail_pos = self.in_tail_pos; + self.in_tail_pos = false; + for stmt in stmts { + self.visit_stmt(stmt); + } + self.in_tail_pos = in_tail_pos; + self.visit_expr(expr); + }, + ([stmts @ .., stmt], None) => { + let in_tail_pos = self.in_tail_pos; + self.in_tail_pos = false; + for stmt in stmts { + self.visit_stmt(stmt); + } + self.in_tail_pos = in_tail_pos; + self.visit_stmt(stmt); + }, + } + } + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { if !self.can_use_entry { return; @@ -357,15 +444,16 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> { return; } - self.insertions.push(Insertion { + self.edits.push(Edit::Insertion(Insertion { call: expr, value: insert_expr.value, - }); + })); self.is_map_used = true; + self.allow_insert_closure &= self.in_tail_pos; // The value doesn't affect whether there is only a single insert expression. let is_single_insert = self.is_single_insert; - self.visit_expr(insert_expr.value); + self.visit_non_tail_expr(insert_expr.value); self.is_single_insert = is_single_insert; }, _ if SpanlessEq::new(self.cx).eq_expr(self.map, expr) => { @@ -374,7 +462,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> { _ => match expr.kind { ExprKind::If(cond_expr, then_expr, Some(else_expr)) => { self.is_single_insert = false; - self.visit_expr(cond_expr); + self.visit_non_tail_expr(cond_expr); // Each branch may contain it's own insert expression. let mut is_map_used = self.visit_cond_arm(then_expr); is_map_used |= self.visit_cond_arm(else_expr); @@ -382,31 +470,38 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> { }, ExprKind::Match(scrutinee_expr, arms, _) => { self.is_single_insert = false; - self.visit_expr(scrutinee_expr); + self.visit_non_tail_expr(scrutinee_expr); // Each branch may contain it's own insert expression. let mut is_map_used = self.is_map_used; for arm in arms { if let Some(Guard::If(guard) | Guard::IfLet(_, guard)) = arm.guard { - self.visit_expr(guard) + self.visit_non_tail_expr(guard) } is_map_used |= self.visit_cond_arm(arm.body); } self.is_map_used = is_map_used; }, ExprKind::Loop(block, ..) => { + self.loops.push(expr.hir_id); + self.allow_insert_closure &= !self.in_tail_pos; // Don't allow insertions inside of a loop. - let insertions_len = self.insertions.len(); + let edit_len = self.edits.len(); self.visit_block(block); - if self.insertions.len() != insertions_len { + if self.edits.len() != edit_len { self.can_use_entry = false; } + self.loops.pop(); }, ExprKind::Block(block, _) => self.visit_block(block), ExprKind::InlineAsm(_) | ExprKind::LlvmInlineAsm(_) => { self.can_use_entry = false; }, _ => { + self.allow_insert_closure &= !self.in_tail_pos; + self.allow_insert_closure &= can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops); + // Sub expressions are no longer in the tail position. self.is_single_insert = false; + self.in_tail_pos = false; walk_expr(self, expr); }, }, @@ -415,18 +510,19 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> { } struct InsertSearchResults<'tcx> { - insertions: Vec>, + edits: Vec>, + allow_insert_closure: bool, is_single_insert: bool, } impl InsertSearchResults<'tcx> { fn as_single_insertion(&self) -> Option> { - self.is_single_insert.then(|| self.insertions[0]) + self.is_single_insert.then(|| self.edits[0].as_insertion().unwrap()) } fn snippet_occupied(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String { let ctxt = span.ctxt(); let mut res = String::new(); - for insertion in self.insertions.iter() { + for insertion in self.edits.iter().filter_map(|e| e.as_insertion()) { res.push_str(&snippet_with_applicability( cx, span.until(insertion.call.span), @@ -451,7 +547,7 @@ impl InsertSearchResults<'tcx> { fn snippet_vacant(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String { let ctxt = span.ctxt(); let mut res = String::new(); - for insertion in self.insertions.iter() { + for insertion in self.edits.iter().filter_map(|e| e.as_insertion()) { res.push_str(&snippet_with_applicability( cx, span.until(insertion.call.span), @@ -485,27 +581,57 @@ impl InsertSearchResults<'tcx> { res.push_str(&snippet_with_applicability(cx, span, "..", app)); res } + + fn snippet_closure(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String { + let ctxt = span.ctxt(); + let mut res = String::new(); + for edit in &self.edits { + match *edit { + Edit::Insertion(insertion) => { + res.push_str(&snippet_with_applicability( + cx, + span.until(insertion.call.span), + "..", + app, + )); + res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0); + span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP); + }, + Edit::RemoveSemi(semi_span) => { + res.push_str(&snippet_with_applicability(cx, span.until(semi_span), "..", app)); + span = span.trim_start(semi_span).unwrap_or(DUMMY_SP); + }, + } + } + res.push_str(&snippet_with_applicability(cx, span, "..", app)); + res + } } fn find_insert_calls( cx: &LateContext<'tcx>, contains_expr: &ContainsExpr<'tcx>, expr: &'tcx Expr<'_>, ) -> Option> { - let mut insertions = Vec::new(); + let mut edits = Vec::new(); let mut s = InsertSearcher { cx, map: contains_expr.map, key: contains_expr.key, ctxt: expr.span.ctxt(), - insertions: &mut insertions, + edits: &mut edits, is_map_used: false, + allow_insert_closure: true, can_use_entry: true, + in_tail_pos: true, is_single_insert: true, + loops: Vec::new(), }; s.visit_expr(expr); + let allow_insert_closure = s.allow_insert_closure; let is_single_insert = s.is_single_insert; s.can_use_entry.then(|| InsertSearchResults { - insertions, + edits, + allow_insert_closure, is_single_insert, }) } diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs index 29be07399774..3f8b976fcac0 100644 --- a/clippy_lints/src/manual_map.rs +++ b/clippy_lints/src/manual_map.rs @@ -1,15 +1,13 @@ 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::{in_constant, is_allowed, is_else_clause, is_lang_ctor, match_var, 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, in_constant, is_allowed, is_else_clause, is_lang_ctor, match_var, peel_hir_expr_refs}; use rustc_ast::util::parser::PREC_POSTFIX; use rustc_errors::Applicability; use rustc_hir::LangItem::{OptionNone, OptionSome}; use rustc_hir::{ - def::Res, - intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor}, - Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, Pat, PatKind, Path, QPath, + Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, Pat, PatKind, QPath, }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; @@ -193,51 +191,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 { - 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>> { diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 0abee49f40ce..5d6c1337be6a 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -60,7 +60,7 @@ use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; use rustc_hir::def::{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::LangItem::{ResultErr, ResultOk}; use rustc_hir::{ def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl, @@ -82,7 +82,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) -> Option { if let Ok(version) = RustcVersion::parse(msrv) { @@ -539,6 +539,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, + allow_closure: bool, + } + impl Visitor<'tcx> for V<'_, 'tcx> { + type Map = ErasedMap<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap { + 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>( diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed index 60371c9833c2..524f2132797e 100644 --- a/tests/ui/entry.fixed +++ b/tests/ui/entry.fixed @@ -15,13 +15,21 @@ fn foo() {} fn hash_map(m: &mut HashMap, k: K, v: V, v2: V) { m.entry(k).or_insert(v); - if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + m.entry(k).or_insert_with(|| { if true { - e.insert(v); + v } else { - e.insert(v2); + v2 } - } + }); + + m.entry(k).or_insert_with(|| { + if true { + v + } else { + v2 + } + }); if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { if true { @@ -32,21 +40,21 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: } } - if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + m.entry(k).or_insert_with(|| { foo(); - e.insert(v); - } + v + }); - if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + m.entry(k).or_insert_with(|| { match 0 { 1 if true => { - e.insert(v); + v }, _ => { - e.insert(v2); + v2 }, - }; - } + } + }); if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { match 0 { @@ -60,35 +68,33 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: }; } - if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + m.entry(k).or_insert_with(|| { foo(); match 0 { 0 if false => { - e.insert(v); + v }, 1 => { foo(); - e.insert(v); + v }, 2 | 3 => { for _ in 0..2 { foo(); } if true { - e.insert(v); + v } else { - e.insert(v2); - }; + v2 + } }, _ => { - e.insert(v2); + v2 }, } - } + }); - if let std::collections::hash_map::Entry::Vacant(e) = m.entry(m!(k)) { - e.insert(m!(v)); - } + m.entry(m!(k)).or_insert_with(|| m!(v)); } fn btree_map(m: &mut BTreeMap, k: K, v: V, v2: V) { diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs index 4d3e241de785..ff4890eeeb63 100644 --- a/tests/ui/entry.rs +++ b/tests/ui/entry.rs @@ -25,6 +25,14 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: } } + if !m.contains_key(&k) { + if true { + m.insert(k, v) + } else { + m.insert(k, v2) + }; + } + if !m.contains_key(&k) { if true { m.insert(k, v); diff --git a/tests/ui/entry.stderr b/tests/ui/entry.stderr index ab108ed6861e..63b5f5a0b2cf 100644 --- a/tests/ui/entry.stderr +++ b/tests/ui/entry.stderr @@ -22,11 +22,11 @@ LL | | } | help: try this | -LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | m.entry(k).or_insert_with(|| { LL | if true { -LL | e.insert(v); +LL | v LL | } else { -LL | e.insert(v2); +LL | v2 LL | } ... @@ -35,6 +35,28 @@ error: usage of `contains_key` followed by `insert` on a `HashMap` | LL | / if !m.contains_key(&k) { LL | | if true { +LL | | m.insert(k, v) +LL | | } else { +LL | | m.insert(k, v2) +LL | | }; +LL | | } + | |_____^ + | +help: try this + | +LL | m.entry(k).or_insert_with(|| { +LL | if true { +LL | v +LL | } else { +LL | v2 +LL | } + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:36:5 + | +LL | / if !m.contains_key(&k) { +LL | | if true { LL | | m.insert(k, v); LL | | } else { ... | @@ -53,7 +75,7 @@ LL | return; ... error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:37:5 + --> $DIR/entry.rs:45:5 | LL | / if !m.contains_key(&k) { LL | | foo(); @@ -63,14 +85,14 @@ LL | | } | help: try this | -LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | m.entry(k).or_insert_with(|| { LL | foo(); -LL | e.insert(v); -LL | } +LL | v +LL | }); | error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:42:5 + --> $DIR/entry.rs:50:5 | LL | / if !m.contains_key(&k) { LL | | match 0 { @@ -83,16 +105,16 @@ LL | | } | help: try this | -LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | m.entry(k).or_insert_with(|| { LL | match 0 { LL | 1 if true => { -LL | e.insert(v); +LL | v LL | }, LL | _ => { ... error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:53:5 + --> $DIR/entry.rs:61:5 | LL | / if !m.contains_key(&k) { LL | | match 0 { @@ -114,7 +136,7 @@ LL | }, ... error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:65:5 + --> $DIR/entry.rs:73:5 | LL | / if !m.contains_key(&k) { LL | | foo(); @@ -127,31 +149,24 @@ LL | | } | help: try this | -LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | m.entry(k).or_insert_with(|| { LL | foo(); LL | match 0 { LL | 0 if false => { -LL | e.insert(v); +LL | v LL | }, ... error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:91:5 + --> $DIR/entry.rs:99:5 | LL | / if !m.contains_key(&m!(k)) { LL | | m.insert(m!(k), m!(v)); LL | | } - | |_____^ - | -help: try this - | -LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(m!(k)) { -LL | e.insert(m!(v)); -LL | } - | + | |_____^ help: try this: `m.entry(m!(k)).or_insert_with(|| m!(v));` error: usage of `contains_key` followed by `insert` on a `BTreeMap` - --> $DIR/entry.rs:97:5 + --> $DIR/entry.rs:105:5 | LL | / if !m.contains_key(&k) { LL | | m.insert(k, v); @@ -167,5 +182,5 @@ LL | foo(); LL | } | -error: aborting due to 9 previous errors +error: aborting due to 10 previous errors From bcf348800751e8a03fdbaa49130e53a463e3a441 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 29 Mar 2021 20:17:03 -0400 Subject: [PATCH 4/4] Minor cleanup of `map_entry` and a few additional tests. --- clippy_lints/src/entry.rs | 187 ++++++++++++++-------------- clippy_lints/src/manual_map.rs | 8 +- clippy_utils/src/lib.rs | 59 +++------ tests/ui/entry.fixed | 58 ++++++++- tests/ui/entry.rs | 58 ++++++++- tests/ui/entry.stderr | 30 ++--- tests/ui/entry_unfixable.rs | 50 -------- tests/ui/string_lit_as_bytes.fixed | 2 +- tests/ui/string_lit_as_bytes.rs | 2 +- tests/ui/string_lit_as_bytes.stderr | 4 +- 10 files changed, 238 insertions(+), 220 deletions(-) delete mode 100644 tests/ui/entry_unfixable.rs diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index 0decb22a4914..8db5050a5ac3 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -77,40 +77,33 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { let map_str = snippet_with_context(cx, contains_expr.map.span, contains_expr.call_ctxt, "..", &mut app).0; let key_str = snippet_with_context(cx, contains_expr.key.span, contains_expr.call_ctxt, "..", &mut app).0; let sugg = if let Some(else_expr) = else_expr { - // if .. { .. } else { .. } let else_search = match find_insert_calls(cx, &contains_expr, else_expr) { - Some(search) if !(then_search.edits.is_empty() && search.edits.is_empty()) => search, - _ => return, + Some(search) => search, + None => return, }; - if then_search.edits.is_empty() || else_search.edits.is_empty() { - // if .. { insert } else { .. } or if .. { .. } else { then } of - let (then_str, else_str, entry_kind) = if else_search.edits.is_empty() { - if contains_expr.negated { - ( - then_search.snippet_vacant(cx, then_expr.span, &mut app), - snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app), - "Vacant(e)", - ) - } else { - ( - then_search.snippet_occupied(cx, then_expr.span, &mut app), - snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app), - "Occupied(mut e)", - ) - } - } else if contains_expr.negated { - ( + if then_search.edits.is_empty() && else_search.edits.is_empty() { + // No insertions + return; + } else if then_search.edits.is_empty() || else_search.edits.is_empty() { + // if .. { insert } else { .. } or if .. { .. } else { insert } + let ((then_str, entry_kind), else_str) = match (else_search.edits.is_empty(), contains_expr.negated) { + (true, true) => ( + then_search.snippet_vacant(cx, then_expr.span, &mut app), + snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app), + ), + (true, false) => ( + then_search.snippet_occupied(cx, then_expr.span, &mut app), + snippet_with_applicability(cx, else_expr.span, "{ .. }", &mut app), + ), + (false, true) => ( else_search.snippet_occupied(cx, else_expr.span, &mut app), snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app), - "Occupied(mut e)", - ) - } else { - ( + ), + (false, false) => ( else_search.snippet_vacant(cx, else_expr.span, &mut app), snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app), - "Vacant(e)", - ) + ), }; format!( "if let {}::{} = {}.entry({}) {} else {}", @@ -123,19 +116,15 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { ) } else { // if .. { insert } else { insert } - let (then_str, else_str, then_entry, else_entry) = if contains_expr.negated { + let ((then_str, then_entry), (else_str, else_entry)) = if contains_expr.negated { ( then_search.snippet_vacant(cx, then_expr.span, &mut app), else_search.snippet_occupied(cx, else_expr.span, &mut app), - "Vacant(e)", - "Occupied(mut e)", ) } else { ( then_search.snippet_occupied(cx, then_expr.span, &mut app), else_search.snippet_vacant(cx, else_expr.span, &mut app), - "Occupied(mut e)", - "Vacant(e)", ) }; let indent_str = snippet_indent(cx, expr.span); @@ -153,19 +142,18 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { indent = indent_str, ) } - } else if then_search.edits.is_empty() { - // no insertions - return; } else { + if then_search.edits.is_empty() { + // no insertions + return; + } + // if .. { insert } if !then_search.allow_insert_closure { let (body_str, entry_kind) = if contains_expr.negated { - (then_search.snippet_vacant(cx, then_expr.span, &mut app), "Vacant(e)") + then_search.snippet_vacant(cx, then_expr.span, &mut app) } else { - ( - then_search.snippet_occupied(cx, then_expr.span, &mut app), - "Occupied(mut e)", - ) + then_search.snippet_occupied(cx, then_expr.span, &mut app) }; format!( "if let {}::{} = {}.entry({}) {}", @@ -184,7 +172,8 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { format!("{}.entry({}).or_insert({});", map_str, key_str, value_str) } } else { - // Todo: if let Some(v) = map.get_mut(k) + // TODO: suggest using `if let Some(v) = map.get_mut(k) { .. }` here. + // This would need to be a different lint. return; } } else { @@ -192,7 +181,8 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { if contains_expr.negated { format!("{}.entry({}).or_insert_with(|| {});", map_str, key_str, block_str) } else { - // Todo: if let Some(v) = map.get_mut(k) + // TODO: suggest using `if let Some(v) = map.get_mut(k) { .. }` here. + // This would need to be a different lint. return; } } @@ -222,7 +212,7 @@ impl MapType { Self::BTree => "BTreeMap", } } - fn entry_path(self) -> &'staic str { + fn entry_path(self) -> &'static str { match self { Self::Hash => "std::collections::hash_map::Entry", Self::BTree => "std::collections::btree_map::Entry", @@ -312,15 +302,16 @@ struct Insertion<'tcx> { value: &'tcx Expr<'tcx>, } -// This visitor needs to do a multiple things: -// * Find all usages of the map. Only insertions into the map which share the same key are -// permitted. All others will prevent the lint. -// * Determine if the final statement executed is an insertion. This is needed to use `insert_with`. -// * Determine if there's any sub-expression that can't be placed in a closure. -// * Determine if there's only a single insert statement. This is needed to give better suggestions. - +/// This visitor needs to do a multiple things: +/// * Find all usages of the map. An insertion can only be made before any other usages of the map. +/// * Determine if there's an insertion using the same key. There's no need for the entry api +/// otherwise. +/// * Determine if the final statement executed is an insertion. This is needed to use +/// `or_insert_with`. +/// * Determine if there's any sub-expression that can't be placed in a closure. +/// * Determine if there's only a single insert statement. `or_insert` can be used in this case. #[allow(clippy::struct_excessive_bools)] -struct InsertSearcher<'cx, 'i, 'tcx> { +struct InsertSearcher<'cx, 'tcx> { cx: &'cx LateContext<'tcx>, /// The map expression used in the contains call. map: &'tcx Expr<'tcx>, @@ -334,13 +325,16 @@ struct InsertSearcher<'cx, 'i, 'tcx> { can_use_entry: bool, /// Whether this expression is the final expression in this code path. This may be a statement. in_tail_pos: bool, - // A single insert expression has a slightly different suggestion. + // Is this expression a single insert. A slightly better suggestion can be made in this case. is_single_insert: bool, + /// If the visitor has seen the map being used. is_map_used: bool, - edits: &'i mut Vec>, + /// The locations where changes need to be made for the suggestion. + edits: Vec>, + /// A stack of loops the visitor is currently in. loops: Vec, } -impl<'tcx> InsertSearcher<'_, '_, 'tcx> { +impl<'tcx> InsertSearcher<'_, 'tcx> { /// Visit the expression as a branch in control flow. Multiple insert calls can be used, but /// only if they are on separate code paths. This will return whether the map was used in the /// given expression. @@ -363,7 +357,7 @@ impl<'tcx> InsertSearcher<'_, '_, 'tcx> { self.in_tail_pos = in_tail_pos; } } -impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> { +impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { type Map = ErasedMap<'tcx>; fn nested_visit_map(&mut self) -> NestedVisitorMap { NestedVisitorMap::None @@ -483,6 +477,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, '_, 'tcx> { }, ExprKind::Loop(block, ..) => { self.loops.push(expr.hir_id); + self.is_single_insert = false; self.allow_insert_closure &= !self.in_tail_pos; // Don't allow insertions inside of a loop. let edit_len = self.edits.len(); @@ -519,7 +514,13 @@ impl InsertSearchResults<'tcx> { self.is_single_insert.then(|| self.edits[0].as_insertion().unwrap()) } - fn snippet_occupied(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String { + fn snippet( + &self, + cx: &LateContext<'_>, + mut span: Span, + app: &mut Applicability, + write_wrapped: impl Fn(&mut String, Insertion<'_>, SyntaxContext, &mut Applicability), + ) -> String { let ctxt = span.ctxt(); let mut res = String::new(); for insertion in self.edits.iter().filter_map(|e| e.as_insertion()) { @@ -530,13 +531,13 @@ impl InsertSearchResults<'tcx> { app, )); if is_expr_used_or_unified(cx.tcx, insertion.call) { - res.push_str("Some(e.insert("); - res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0); - res.push_str("))"); + write_wrapped(&mut res, insertion, ctxt, app); } else { - res.push_str("e.insert("); - res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0); - res.push(')'); + let _ = write!( + res, + "e.insert({})", + snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0 + ); } span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP); } @@ -544,42 +545,41 @@ impl InsertSearchResults<'tcx> { res } - fn snippet_vacant(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String { - let ctxt = span.ctxt(); - let mut res = String::new(); - for insertion in self.edits.iter().filter_map(|e| e.as_insertion()) { - res.push_str(&snippet_with_applicability( - cx, - span.until(insertion.call.span), - "..", - app, - )); - if is_expr_used_or_unified(cx.tcx, insertion.call) { - if is_expr_final_block_expr(cx.tcx, insertion.call) { - let _ = write!( + fn snippet_occupied(&self, cx: &LateContext<'_>, span: Span, app: &mut Applicability) -> (String, &'static str) { + ( + self.snippet(cx, span, app, |res, insertion, ctxt, app| { + // Insertion into a map would return `Some(&mut value)`, but the entry returns `&mut value` + let _ = write!( + res, + "Some(e.insert({}))", + snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0 + ); + }), + "Occupied(mut e)", + ) + } + + fn snippet_vacant(&self, cx: &LateContext<'_>, span: Span, app: &mut Applicability) -> (String, &'static str) { + ( + self.snippet(cx, span, app, |res, insertion, ctxt, app| { + // Insertion into a map would return `None`, but the entry returns a mutable reference. + let _ = if is_expr_final_block_expr(cx.tcx, insertion.call) { + write!( res, "e.insert({});\n{}None", snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0, snippet_indent(cx, insertion.call.span).as_deref().unwrap_or(""), - ); + ) } else { - let _ = write!( + write!( res, "{{ e.insert({}); None }}", snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0, - ); - } - } else { - let _ = write!( - res, - "e.insert({})", - snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0, - ); - } - span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP); - } - res.push_str(&snippet_with_applicability(cx, span, "..", app)); - res + ) + }; + }), + "Vacant(e)", + ) } fn snippet_closure(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String { @@ -588,6 +588,7 @@ impl InsertSearchResults<'tcx> { for edit in &self.edits { match *edit { Edit::Insertion(insertion) => { + // Cut out the value from `map.insert(key, value)` res.push_str(&snippet_with_applicability( cx, span.until(insertion.call.span), @@ -598,6 +599,7 @@ impl InsertSearchResults<'tcx> { span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP); }, Edit::RemoveSemi(semi_span) => { + // Cut out the semicolon. This allows the value to be returned from the closure. res.push_str(&snippet_with_applicability(cx, span.until(semi_span), "..", app)); span = span.trim_start(semi_span).unwrap_or(DUMMY_SP); }, @@ -607,18 +609,18 @@ impl InsertSearchResults<'tcx> { res } } + fn find_insert_calls( cx: &LateContext<'tcx>, contains_expr: &ContainsExpr<'tcx>, expr: &'tcx Expr<'_>, ) -> Option> { - let mut edits = Vec::new(); let mut s = InsertSearcher { cx, map: contains_expr.map, key: contains_expr.key, ctxt: expr.span.ctxt(), - edits: &mut edits, + edits: Vec::new(), is_map_used: false, allow_insert_closure: true, can_use_entry: true, @@ -629,6 +631,7 @@ fn find_insert_calls( s.visit_expr(expr); let allow_insert_closure = s.allow_insert_closure; let is_single_insert = s.is_single_insert; + let edits = s.edits; s.can_use_entry.then(|| InsertSearchResults { edits, allow_insert_closure, diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs index 3f8b976fcac0..d9b75149b9f1 100644 --- a/clippy_lints/src/manual_map.rs +++ b/clippy_lints/src/manual_map.rs @@ -2,13 +2,13 @@ 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::{is_type_diagnostic_item, peel_mid_ty_refs_is_mutable}; -use clippy_utils::{can_move_expr_to_closure, in_constant, is_allowed, is_else_clause, is_lang_ctor, match_var, peel_hir_expr_refs}; +use clippy_utils::{ + can_move_expr_to_closure, in_constant, is_allowed, is_else_clause, is_lang_ctor, match_var, peel_hir_expr_refs, +}; use rustc_ast::util::parser::PREC_POSTFIX; use rustc_errors::Applicability; use rustc_hir::LangItem::{OptionNone, OptionSome}; -use rustc_hir::{ - Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, Pat, PatKind, QPath, -}; +use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, Pat, PatKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_lint_pass, declare_tool_lint}; diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 5d6c1337be6a..90a6fd225ab0 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -64,8 +64,8 @@ use rustc_hir::intravisit::{self, walk_expr, ErasedMap, NestedVisitorMap, Visito use rustc_hir::LangItem::{ResultErr, ResultOk}; use rustc_hir::{ def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl, - ImplItem, ImplItemKind, Item, ItemKind, LangItem, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath, - Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind, + ImplItem, ImplItemKind, Item, ItemKind, LangItem, Local, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, + QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind, }; use rustc_lint::{LateContext, Level, Lint, LintContext}; use rustc_middle::hir::exports::Export; @@ -1312,48 +1312,7 @@ 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> { - 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(_), - .. - })) - ) -} - +/// Gets the node where an expression is either used, or it's type is unified with another branch. pub fn get_expr_use_or_unification_node(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option> { let map = tcx.hir(); let mut child_id = expr.hir_id; @@ -1374,16 +1333,26 @@ pub fn get_expr_use_or_unification_node(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> O } } +/// Checks if the result of an expression is used, or it's type is unified with another branch. pub fn is_expr_used_or_unified(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool { !matches!( get_expr_use_or_unification_node(tcx, expr), None | Some(Node::Stmt(Stmt { - kind: StmtKind::Expr(_) | StmtKind::Semi(_), + kind: StmtKind::Expr(_) + | StmtKind::Semi(_) + | StmtKind::Local(Local { + pat: Pat { + kind: PatKind::Wild, + .. + }, + .. + }), .. })) ) } +/// Checks if the expression is the final expression returned from a block. pub fn is_expr_final_block_expr(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool { matches!(get_parent_node(tcx, expr.hir_id), Some(Node::Block(..))) } diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed index 524f2132797e..cfad3090ba38 100644 --- a/tests/ui/entry.fixed +++ b/tests/ui/entry.fixed @@ -2,6 +2,7 @@ #![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] +#![feature(asm)] use std::collections::{BTreeMap, HashMap}; use std::hash::Hash; @@ -10,11 +11,19 @@ macro_rules! m { ($e:expr) => {{ $e }}; } +macro_rules! insert { + ($map:expr, $key:expr, $val:expr) => { + $map.insert($key, $val) + }; +} + fn foo() {} -fn hash_map(m: &mut HashMap, k: K, v: V, v2: V) { +fn hash_map(m: &mut HashMap, m2: &mut HashMap, k: K, k2: K, v: V, v2: V) { + // or_insert(v) m.entry(k).or_insert(v); + // semicolon on insert, use or_insert_with(..) m.entry(k).or_insert_with(|| { if true { v @@ -23,6 +32,7 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: } }); + // semicolon on if, use or_insert_with(..) m.entry(k).or_insert_with(|| { if true { v @@ -31,6 +41,7 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: } }); + // early return, use if let if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { if true { e.insert(v); @@ -40,11 +51,13 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: } } + // use or_insert_with(..) m.entry(k).or_insert_with(|| { foo(); v }); + // semicolon on insert and match, use or_insert_with(..) m.entry(k).or_insert_with(|| { match 0 { 1 if true => { @@ -56,18 +69,17 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: } }); + // one branch doesn't insert, use if let if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { match 0 { - 0 => {}, - 1 => { - e.insert(v); - }, + 0 => foo(), _ => { e.insert(v2); }, }; } + // use or_insert_with m.entry(k).or_insert_with(|| { foo(); match 0 { @@ -94,10 +106,46 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: } }); + // ok, insert in loop + if !m.contains_key(&k) { + for _ in 0..2 { + m.insert(k, v); + } + } + + // macro_expansion test, use or_insert(..) m.entry(m!(k)).or_insert_with(|| m!(v)); + + // ok, map used before insertion + if !m.contains_key(&k) { + let _ = m.len(); + m.insert(k, v); + } + + // ok, inline asm + if !m.contains_key(&k) { + unsafe { asm!("nop") } + m.insert(k, v); + } + + // ok, different keys. + if !m.contains_key(&k) { + m.insert(k2, v); + } + + // ok, different maps + if !m.contains_key(&k) { + m2.insert(k, v); + } + + // ok, insert in macro + if !m.contains_key(&k) { + insert!(m, k, v); + } } fn btree_map(m: &mut BTreeMap, k: K, v: V, v2: V) { + // insert then do something, use if let if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { e.insert(v); foo(); diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs index ff4890eeeb63..fa9280b58de1 100644 --- a/tests/ui/entry.rs +++ b/tests/ui/entry.rs @@ -2,6 +2,7 @@ #![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] +#![feature(asm)] use std::collections::{BTreeMap, HashMap}; use std::hash::Hash; @@ -10,13 +11,21 @@ macro_rules! m { ($e:expr) => {{ $e }}; } +macro_rules! insert { + ($map:expr, $key:expr, $val:expr) => { + $map.insert($key, $val) + }; +} + fn foo() {} -fn hash_map(m: &mut HashMap, k: K, v: V, v2: V) { +fn hash_map(m: &mut HashMap, m2: &mut HashMap, k: K, k2: K, v: V, v2: V) { + // or_insert(v) if !m.contains_key(&k) { m.insert(k, v); } + // semicolon on insert, use or_insert_with(..) if !m.contains_key(&k) { if true { m.insert(k, v); @@ -25,6 +34,7 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: } } + // semicolon on if, use or_insert_with(..) if !m.contains_key(&k) { if true { m.insert(k, v) @@ -33,6 +43,7 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: }; } + // early return, use if let if !m.contains_key(&k) { if true { m.insert(k, v); @@ -42,11 +53,13 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: } } + // use or_insert_with(..) if !m.contains_key(&k) { foo(); m.insert(k, v); } + // semicolon on insert and match, use or_insert_with(..) if !m.contains_key(&k) { match 0 { 1 if true => { @@ -58,18 +71,17 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: }; } + // one branch doesn't insert, use if let if !m.contains_key(&k) { match 0 { - 0 => {}, - 1 => { - m.insert(k, v); - }, + 0 => foo(), _ => { m.insert(k, v2); }, }; } + // use or_insert_with if !m.contains_key(&k) { foo(); match 0 { @@ -96,12 +108,48 @@ fn hash_map(m: &mut HashMap, k: K, v: V, v2: } } + // ok, insert in loop + if !m.contains_key(&k) { + for _ in 0..2 { + m.insert(k, v); + } + } + + // macro_expansion test, use or_insert(..) if !m.contains_key(&m!(k)) { m.insert(m!(k), m!(v)); } + + // ok, map used before insertion + if !m.contains_key(&k) { + let _ = m.len(); + m.insert(k, v); + } + + // ok, inline asm + if !m.contains_key(&k) { + unsafe { asm!("nop") } + m.insert(k, v); + } + + // ok, different keys. + if !m.contains_key(&k) { + m.insert(k2, v); + } + + // ok, different maps + if !m.contains_key(&k) { + m2.insert(k, v); + } + + // ok, insert in macro + if !m.contains_key(&k) { + insert!(m, k, v); + } } fn btree_map(m: &mut BTreeMap, k: K, v: V, v2: V) { + // insert then do something, use if let if !m.contains_key(&k) { m.insert(k, v); foo(); diff --git a/tests/ui/entry.stderr b/tests/ui/entry.stderr index 63b5f5a0b2cf..2f075a97010a 100644 --- a/tests/ui/entry.stderr +++ b/tests/ui/entry.stderr @@ -1,5 +1,5 @@ error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:16:5 + --> $DIR/entry.rs:24:5 | LL | / if !m.contains_key(&k) { LL | | m.insert(k, v); @@ -9,7 +9,7 @@ LL | | } = note: `-D clippy::map-entry` implied by `-D warnings` error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:20:5 + --> $DIR/entry.rs:29:5 | LL | / if !m.contains_key(&k) { LL | | if true { @@ -31,7 +31,7 @@ LL | } ... error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:28:5 + --> $DIR/entry.rs:38:5 | LL | / if !m.contains_key(&k) { LL | | if true { @@ -53,7 +53,7 @@ LL | } ... error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:36:5 + --> $DIR/entry.rs:47:5 | LL | / if !m.contains_key(&k) { LL | | if true { @@ -75,7 +75,7 @@ LL | return; ... error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:45:5 + --> $DIR/entry.rs:57:5 | LL | / if !m.contains_key(&k) { LL | | foo(); @@ -92,7 +92,7 @@ LL | }); | error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:50:5 + --> $DIR/entry.rs:63:5 | LL | / if !m.contains_key(&k) { LL | | match 0 { @@ -114,12 +114,12 @@ LL | _ => { ... error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:61:5 + --> $DIR/entry.rs:75:5 | LL | / if !m.contains_key(&k) { LL | | match 0 { -LL | | 0 => {}, -LL | | 1 => { +LL | | 0 => foo(), +LL | | _ => { ... | LL | | }; LL | | } @@ -129,14 +129,14 @@ help: try this | LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { LL | match 0 { -LL | 0 => {}, -LL | 1 => { -LL | e.insert(v); +LL | 0 => foo(), +LL | _ => { +LL | e.insert(v2); LL | }, ... error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:73:5 + --> $DIR/entry.rs:85:5 | LL | / if !m.contains_key(&k) { LL | | foo(); @@ -158,7 +158,7 @@ LL | }, ... error: usage of `contains_key` followed by `insert` on a `HashMap` - --> $DIR/entry.rs:99:5 + --> $DIR/entry.rs:119:5 | LL | / if !m.contains_key(&m!(k)) { LL | | m.insert(m!(k), m!(v)); @@ -166,7 +166,7 @@ LL | | } | |_____^ help: try this: `m.entry(m!(k)).or_insert_with(|| m!(v));` error: usage of `contains_key` followed by `insert` on a `BTreeMap` - --> $DIR/entry.rs:105:5 + --> $DIR/entry.rs:153:5 | LL | / if !m.contains_key(&k) { LL | | m.insert(k, v); diff --git a/tests/ui/entry_unfixable.rs b/tests/ui/entry_unfixable.rs deleted file mode 100644 index beb2d5c97b1d..000000000000 --- a/tests/ui/entry_unfixable.rs +++ /dev/null @@ -1,50 +0,0 @@ -#![allow(unused, clippy::needless_pass_by_value)] -#![warn(clippy::map_entry)] - -use std::collections::{BTreeMap, HashMap}; -use std::hash::Hash; - -macro_rules! m { - ($map:expr, $key:expr, $value:expr) => { - $map.insert($key, $value) - }; - ($e:expr) => {{ $e }}; -} - -fn foo() {} - -// should not trigger -fn insert_other_if_absent(m: &mut HashMap, k: K, o: K, v: V) { - if !m.contains_key(&k) { - m.insert(o, v); - } -} - -// should not trigger, because the one uses different HashMap from another one -fn insert_from_different_map(m: HashMap, n: &mut HashMap, k: K, v: V) { - if !m.contains_key(&k) { - n.insert(k, v); - } -} - -// should not trigger, because the one uses different HashMap from another one -fn insert_from_different_map2(m: &mut HashMap, n: &mut HashMap, k: K, v: V) { - if !m.contains_key(&k) { - n.insert(k, v); - } -} - -fn insert_in_macro(m: &mut HashMap, k: K, v: V) { - if !m.contains_key(&k) { - m!(m, k, v); - } -} - -fn use_map_then_insert(m: &mut HashMap, k: K, v: V) { - if !m.contains_key(&k) { - let _ = m.len(); - m.insert(k, v); - } -} - -fn main() {} diff --git a/tests/ui/string_lit_as_bytes.fixed b/tests/ui/string_lit_as_bytes.fixed index dd22bfa5c53e..df2256e4f97d 100644 --- a/tests/ui/string_lit_as_bytes.fixed +++ b/tests/ui/string_lit_as_bytes.fixed @@ -22,7 +22,7 @@ fn str_lit_as_bytes() { let current_version = env!("CARGO_PKG_VERSION").as_bytes(); - let includestr = include_bytes!("entry_unfixable.rs"); + let includestr = include_bytes!("string_lit_as_bytes.rs"); let _ = b"string with newline\t\n"; } diff --git a/tests/ui/string_lit_as_bytes.rs b/tests/ui/string_lit_as_bytes.rs index d2a710ed6b8c..c6bf8f732ed9 100644 --- a/tests/ui/string_lit_as_bytes.rs +++ b/tests/ui/string_lit_as_bytes.rs @@ -22,7 +22,7 @@ fn str_lit_as_bytes() { let current_version = env!("CARGO_PKG_VERSION").as_bytes(); - let includestr = include_str!("entry_unfixable.rs").as_bytes(); + let includestr = include_str!("string_lit_as_bytes.rs").as_bytes(); let _ = "string with newline\t\n".as_bytes(); } diff --git a/tests/ui/string_lit_as_bytes.stderr b/tests/ui/string_lit_as_bytes.stderr index e0ddb070b504..f47d6161c6cf 100644 --- a/tests/ui/string_lit_as_bytes.stderr +++ b/tests/ui/string_lit_as_bytes.stderr @@ -27,8 +27,8 @@ LL | let bs = "lit to owned".to_owned().into_bytes(); error: calling `as_bytes()` on `include_str!(..)` --> $DIR/string_lit_as_bytes.rs:25:22 | -LL | let includestr = include_str!("entry_unfixable.rs").as_bytes(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `include_bytes!(..)` instead: `include_bytes!("entry_unfixable.rs")` +LL | let includestr = include_str!("string_lit_as_bytes.rs").as_bytes(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `include_bytes!(..)` instead: `include_bytes!("string_lit_as_bytes.rs")` error: calling `as_bytes()` on a string literal --> $DIR/string_lit_as_bytes.rs:27:13