diff options
| -rw-r--r-- | clippy_lints/src/entry.rs | 697 | ||||
| -rw-r--r-- | clippy_lints/src/manual_map.rs | 57 | ||||
| -rw-r--r-- | clippy_utils/src/lib.rs | 165 | ||||
| -rw-r--r-- | clippy_utils/src/paths.rs | 4 | ||||
| -rw-r--r-- | clippy_utils/src/source.rs | 9 | ||||
| -rw-r--r-- | tests/ui/entry.fixed | 155 | ||||
| -rw-r--r-- | tests/ui/entry.rs | 159 | ||||
| -rw-r--r-- | tests/ui/entry.stderr | 186 | ||||
| -rw-r--r-- | tests/ui/entry_fixable.fixed | 15 | ||||
| -rw-r--r-- | tests/ui/entry_fixable.rs | 17 | ||||
| -rw-r--r-- | tests/ui/entry_fixable.stderr | 12 | ||||
| -rw-r--r-- | tests/ui/entry_unfixable.rs | 73 | ||||
| -rw-r--r-- | tests/ui/entry_unfixable.stderr | 57 | ||||
| -rw-r--r-- | tests/ui/entry_with_else.fixed | 73 | ||||
| -rw-r--r-- | tests/ui/entry_with_else.rs | 60 | ||||
| -rw-r--r-- | tests/ui/entry_with_else.stderr | 142 | ||||
| -rw-r--r-- | tests/ui/string_lit_as_bytes.fixed | 2 | ||||
| -rw-r--r-- | tests/ui/string_lit_as_bytes.rs | 2 | ||||
| -rw-r--r-- | tests/ui/string_lit_as_bytes.stderr | 4 |
19 files changed, 1518 insertions, 371 deletions
diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index a815df1691a..8db5050a5ac 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -1,17 +1,19 @@ -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::{ + 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}, + 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}, + Block, Expr, ExprKind, Guard, HirId, 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 +21,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 +57,584 @@ 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 mut visitor = InsertVisitor { - cx, - span: expr.span, - ty, - map, - key, - sole_expr, - }; - - walk_expr(&mut visitor, then_block); + 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 then_search = match find_insert_calls(cx, &contains_expr, then_expr) { + Some(x) => x, + None => return, + }; + + 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 let Some(else_expr) = else_expr { + let else_search = match find_insert_calls(cx, &contains_expr, else_expr) { + Some(search) => search, + None => return, + }; + + 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), + ), + (false, false) => ( + else_search.snippet_vacant(cx, else_expr.span, &mut app), + snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app), + ), + }; + 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, 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), + ) + } else { + ( + then_search.snippet_occupied(cx, then_expr.span, &mut app), + else_search.snippet_vacant(cx, else_expr.span, &mut app), + ) + }; + 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.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) + } else { + then_search.snippet_occupied(cx, then_expr.span, &mut app) + }; + 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 { + format!("{}.entry({}).or_insert({});", map_str, key_str, value_str) + } + } else { + // TODO: suggest using `if let Some(v) = map.get_mut(k) { .. }` here. + // This would need to be a different lint. + return; } - } 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, - }; - - walk_expr(&mut visitor, else_block); + } 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: suggest using `if let Some(v) = map.get_mut(k) { .. }` here. + // This would need to be a different lint. + return; } } - } + }; + + 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, + ); } } -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(); +#[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) -> &'static str { + match self { + Self::Hash => "std::collections::hash_map::Entry", + Self::BTree => "std::collections::btree_map::Entry", + } + } +} - 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<InsertExpr<'tcx>> { + 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 +/// An edit that will need to be made to move the expression to use the entry api +#[derive(Clone, Copy)] +enum Edit<'tcx> { + /// 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<Insertion<'tcx>> { + if let Self::Insertion(i) = self { Some(i) } else { 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. 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, '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 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, + // 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, + /// The locations where changes need to be made for the suggestion. + edits: Vec<Edit<'tcx>>, + /// A stack of loops the visitor is currently in. + loops: Vec<HirId>, } +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; + 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 + } -impl<'a, 'tcx, 'b> Visitor<'tcx> for InsertVisitor<'a, 'tcx, 'b> { - type Map = Map<'tcx>; + /// 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>; + fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { + NestedVisitorMap::None + } + + fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) { + match stmt.kind { + 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_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.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_non_tail_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_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); + self.is_map_used = is_map_used; + }, + ExprKind::Match(scrutinee_expr, arms, _) => { + self.is_single_insert = false; + 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_non_tail_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, ..) => { + 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(); + self.visit_block(block); + 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); + }, + }, } + } +} + +struct InsertSearchResults<'tcx> { + edits: Vec<Edit<'tcx>>, + allow_insert_closure: bool, + is_single_insert: bool, +} +impl InsertSearchResults<'tcx> { + fn as_single_insertion(&self) -> Option<Insertion<'tcx>> { + self.is_single_insert.then(|| self.edits[0].as_insertion().unwrap()) + } - if !self.sole_expr { - walk_expr(self, expr); + 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()) { + res.push_str(&snippet_with_applicability( + cx, + span.until(insertion.call.span), + "..", + app, + )); + if is_expr_used_or_unified(cx.tcx, insertion.call) { + write_wrapped(&mut res, insertion, ctxt, app); + } 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 nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { - NestedVisitorMap::None + + 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 { + write!( + res, + "{{ e.insert({}); None }}", + snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0, + ) + }; + }), + "Vacant(e)", + ) + } + + 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) => { + // Cut out the value from `map.insert(key, value)` + 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) => { + // 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); + }, + } + } + 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<InsertSearchResults<'tcx>> { + let mut s = InsertSearcher { + cx, + map: contains_expr.map, + key: contains_expr.key, + ctxt: expr.span.ctxt(), + edits: Vec::new(), + 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; + let edits = s.edits; + s.can_use_entry.then(|| InsertSearchResults { + edits, + allow_insert_closure, + is_single_insert, + }) } diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs index f16ed4104fe..0b873534f2c 100644 --- a/clippy_lints/src/manual_map.rs +++ b/clippy_lints/src/manual_map.rs @@ -1,16 +1,14 @@ use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF}; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; -use clippy_utils::ty::{can_partially_move_ty, is_type_diagnostic_item, peel_mid_ty_refs_is_mutable}; -use clippy_utils::{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, -}; +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}; @@ -199,51 +197,6 @@ impl LateLintPass<'_> for ManualMap { } } -// Checks if the expression can be moved into a closure as is. -fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { - struct V<'cx, 'tcx> { - cx: &'cx LateContext<'tcx>, - make_closure: bool, - } - impl Visitor<'tcx> for V<'_, 'tcx> { - type Map = ErasedMap<'tcx>; - fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { - NestedVisitorMap::None - } - - fn visit_expr(&mut self, e: &'tcx Expr<'_>) { - match e.kind { - ExprKind::Break(..) - | ExprKind::Continue(_) - | ExprKind::Ret(_) - | ExprKind::Yield(..) - | ExprKind::InlineAsm(_) - | ExprKind::LlvmInlineAsm(_) => { - self.make_closure = false; - }, - // Accessing a field of a local value can only be done if the type isn't - // partially moved. - ExprKind::Field(base_expr, _) - if matches!( - base_expr.kind, - ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. })) - ) && can_partially_move_ty(self.cx, self.cx.typeck_results().expr_ty(base_expr)) => - { - // TODO: check if the local has been partially moved. Assume it has for now. - self.make_closure = false; - return; - } - _ => (), - }; - walk_expr(self, e); - } - } - - let mut v = V { cx, make_closure: true }; - v.visit_expr(expr); - v.make_closure -} - // Checks whether the expression could be passed as a function, or whether a closure is needed. // Returns the function to be passed to `map` if it exists. fn can_pass_as_func(cx: &LateContext<'tcx>, binding: Ident, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 4a523520a43..6c8a3302aa4 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -60,12 +60,12 @@ 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, 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, 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; @@ -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<Span>) -> Option<RustcVersion> { if let Ok(version) = RustcVersion::parse(msrv) { @@ -548,6 +548,73 @@ pub fn trait_ref_of_method<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio None } +/// Checks if the top level expression can be moved into a closure as is. +pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, jump_targets: &[HirId]) -> bool { + match expr.kind { + ExprKind::Break(Destination { target_id: Ok(id), .. }, _) + | ExprKind::Continue(Destination { target_id: Ok(id), .. }) + if jump_targets.contains(&id) => + { + true + }, + ExprKind::Break(..) + | ExprKind::Continue(_) + | ExprKind::Ret(_) + | ExprKind::Yield(..) + | ExprKind::InlineAsm(_) + | ExprKind::LlvmInlineAsm(_) => false, + // Accessing a field of a local value can only be done if the type isn't + // partially moved. + ExprKind::Field(base_expr, _) + if matches!( + base_expr.kind, + ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. })) + ) && can_partially_move_ty(cx, cx.typeck_results().expr_ty(base_expr)) => + { + // TODO: check if the local has been partially moved. Assume it has for now. + false + } + _ => true, + } +} + +/// Checks if the expression can be moved into a closure as is. +pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { + struct V<'cx, 'tcx> { + cx: &'cx LateContext<'tcx>, + loops: Vec<HirId>, + allow_closure: bool, + } + impl Visitor<'tcx> for V<'_, 'tcx> { + type Map = ErasedMap<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { + NestedVisitorMap::None + } + + fn visit_expr(&mut self, e: &'tcx Expr<'_>) { + if !self.allow_closure { + return; + } + if let ExprKind::Loop(b, ..) = e.kind { + self.loops.push(e.hir_id); + self.visit_block(b); + self.loops.pop(); + } else { + self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops); + walk_expr(self, e); + } + } + } + + let mut v = V { + cx, + allow_closure: true, + loops: Vec::new(), + }; + v.visit_expr(expr); + v.allow_closure +} + /// Returns the method names and argument list of nested method call expressions that make up /// `expr`. method/span lists are sorted with the most recent call first. pub fn method_calls<'tcx>( @@ -1272,6 +1339,51 @@ 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()) } +/// 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<Node<'tcx>> { + let map = tcx.hir(); + let mut child_id = expr.hir_id; + let mut iter = map.parent_iter(child_id); + loop { + match iter.next() { + None => break None, + Some((id, Node::Block(_))) => child_id = id, + Some((id, Node::Arm(arm))) if arm.body.hir_id == child_id => child_id = id, + Some((_, Node::Expr(expr))) => match expr.kind { + ExprKind::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), + } + } +} + +/// 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(_) + | 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(..))) +} + 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 { @@ -1441,28 +1553,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 21011dbded6..5e6733a300f 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"]; @@ -47,7 +49,9 @@ pub const FROM_STR_METHOD: [&str; 5] = ["core", "str", "traits", "FromStr", "fro 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 2d794d48dc5..53180d1f9f5 100644 --- a/clippy_utils/src/source.rs +++ b/clippy_utils/src/source.rs @@ -66,6 +66,15 @@ pub fn indent_of<T: LintContext>(cx: &T, span: Span) -> Option<usize> { 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<T: LintContext>(cx: &T, span: Span) -> Option<String> { + 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 00000000000..cfad3090ba3 --- /dev/null +++ b/tests/ui/entry.fixed @@ -0,0 +1,155 @@ +// run-rustfix + +#![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; + +macro_rules! m { + ($e:expr) => {{ $e }}; +} + +macro_rules! insert { + ($map:expr, $key:expr, $val:expr) => { + $map.insert($key, $val) + }; +} + +fn foo() {} + +fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, m2: &mut HashMap<K, V>, 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 + } else { + v2 + } + }); + + // semicolon on if, use or_insert_with(..) + m.entry(k).or_insert_with(|| { + if true { + v + } else { + v2 + } + }); + + // early return, use if let + if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + if true { + e.insert(v); + } else { + e.insert(v2); + return; + } + } + + // 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 => { + 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 => foo(), + _ => { + e.insert(v2); + }, + }; + } + + // use or_insert_with + m.entry(k).or_insert_with(|| { + foo(); + match 0 { + 0 if false => { + v + }, + 1 => { + foo(); + v + }, + 2 | 3 => { + for _ in 0..2 { + foo(); + } + if true { + v + } else { + v2 + } + }, + _ => { + 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<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, 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(); + } +} + +fn main() {} diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs new file mode 100644 index 00000000000..fa9280b58de --- /dev/null +++ b/tests/ui/entry.rs @@ -0,0 +1,159 @@ +// run-rustfix + +#![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; + +macro_rules! m { + ($e:expr) => {{ $e }}; +} + +macro_rules! insert { + ($map:expr, $key:expr, $val:expr) => { + $map.insert($key, $val) + }; +} + +fn foo() {} + +fn hash_map<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, m2: &mut HashMap<K, V>, 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); + } else { + m.insert(k, v2); + } + } + + // semicolon on if, use or_insert_with(..) + if !m.contains_key(&k) { + if true { + m.insert(k, v) + } else { + m.insert(k, v2) + }; + } + + // early return, use if let + if !m.contains_key(&k) { + if true { + m.insert(k, v); + } else { + m.insert(k, v2); + return; + } + } + + // 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 => { + m.insert(k, v); + }, + _ => { + m.insert(k, v2); + }, + }; + } + + // one branch doesn't insert, use if let + if !m.contains_key(&k) { + match 0 { + 0 => foo(), + _ => { + m.insert(k, v2); + }, + }; + } + + // use or_insert_with + 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); + }, + } + } + + // 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<K: Eq + Ord + Copy, V: Copy>(m: &mut BTreeMap<K, V>, k: K, v: V, v2: V) { + // insert then do something, use if let + 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 00000000000..2f075a97010 --- /dev/null +++ b/tests/ui/entry.stderr @@ -0,0 +1,186 @@ +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:24: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:29: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 | 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:38: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 | 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:47: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:57:5 + | +LL | / if !m.contains_key(&k) { +LL | | foo(); +LL | | m.insert(k, v); +LL | | } + | |_____^ + | +help: try this + | +LL | m.entry(k).or_insert_with(|| { +LL | foo(); +LL | v +LL | }); + | + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:63: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 | m.entry(k).or_insert_with(|| { +LL | match 0 { +LL | 1 if true => { +LL | v +LL | }, +LL | _ => { + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:75:5 + | +LL | / if !m.contains_key(&k) { +LL | | match 0 { +LL | | 0 => foo(), +LL | | _ => { +... | +LL | | }; +LL | | } + | |_____^ + | +help: try this + | +LL | if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL | match 0 { +LL | 0 => foo(), +LL | _ => { +LL | e.insert(v2); +LL | }, + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:85:5 + | +LL | / if !m.contains_key(&k) { +LL | | foo(); +LL | | match 0 { +LL | | 0 if false => { +... | +LL | | } +LL | | } + | |_____^ + | +help: try this + | +LL | m.entry(k).or_insert_with(|| { +LL | foo(); +LL | match 0 { +LL | 0 if false => { +LL | v +LL | }, + ... + +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:119:5 + | +LL | / if !m.contains_key(&m!(k)) { +LL | | m.insert(m!(k), 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:153: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 10 previous errors + diff --git a/tests/ui/entry_fixable.fixed b/tests/ui/entry_fixable.fixed deleted file mode 100644 index dcdaae7e724..00000000000 --- 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<K: Eq + Hash, V>(m: &mut HashMap<K, V>, 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 55d5b21568d..00000000000 --- 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<K: Eq + Hash, V>(m: &mut HashMap<K, V>, 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 87403200ced..00000000000 --- 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 deleted file mode 100644 index f530fc023cf..00000000000 --- a/tests/ui/entry_unfixable.rs +++ /dev/null @@ -1,73 +0,0 @@ -#![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_absent2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) { - if !m.contains_key(&k) { - m.insert(k, v) - } else { - None - }; -} - -fn insert_if_present2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) { - if m.contains_key(&k) { - None - } else { - m.insert(k, v) - }; -} - -fn insert_if_absent3<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) { - if !m.contains_key(&k) { - foo(); - m.insert(k, v) - } else { - None - }; -} - -fn insert_if_present3<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) { - if m.contains_key(&k) { - None - } else { - foo(); - m.insert(k, v) - }; -} - -fn insert_in_btreemap<K: Ord, V>(m: &mut BTreeMap<K, V>, k: K, v: V) { - if !m.contains_key(&k) { - foo(); - m.insert(k, v) - } else { - None - }; -} - -// should not trigger -fn insert_other_if_absent<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, o: K, v: V) { - if !m.contains_key(&k) { - m.insert(o, v); - } -} - -// should not trigger, because the one uses different HashMap from another one -fn insert_from_different_map<K: Eq + Hash, V>(m: HashMap<K, V>, n: &mut HashMap<K, V>, 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<K: Eq + Hash, V>(m: &mut HashMap<K, V>, n: &mut HashMap<K, V>, k: K, v: V) { - if !m.contains_key(&k) { - n.insert(k, v); - } -} - -fn main() {} diff --git a/tests/ui/entry_unfixable.stderr b/tests/ui/entry_unfixable.stderr deleted file mode 100644 index e58c8d22dc4..00000000000 --- 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 - diff --git a/tests/ui/entry_with_else.fixed b/tests/ui/entry_with_else.fixed new file mode 100644 index 00000000000..2332fa6313f --- /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<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, 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 00000000000..2ff0c038efe --- /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<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, 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 00000000000..6f62ff8d374 --- /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 + diff --git a/tests/ui/string_lit_as_bytes.fixed b/tests/ui/string_lit_as_bytes.fixed index dd22bfa5c53..df2256e4f97 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 d2a710ed6b8..c6bf8f732ed 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 e0ddb070b50..f47d6161c6c 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 |