diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index a5ec6777b434..baf6a8339b65 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -3,8 +3,7 @@ use clippy_utils::source::{reindent_multiline, snippet_indent, snippet_with_appl use clippy_utils::ty::is_copy; use clippy_utils::visitors::for_each_expr; use clippy_utils::{ - SpanlessEq, can_move_expr_to_closure_no_visit, higher, is_expr_final_block_expr, is_expr_used_or_unified, - peel_hir_expr_while, + SpanlessEq, can_move_expr_to_closure_no_visit, higher, is_expr_used_or_unified, peel_hir_expr_while, }; use core::fmt::{self, Write}; use rustc_errors::Applicability; @@ -87,107 +86,134 @@ impl<'tcx> LateLintPass<'tcx> for HashMapPass { let lint_msg = format!("usage of `contains_key` followed by `insert` on a `{}`", map_ty.name()); 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 Some(else_search) = find_insert_calls(cx, &contains_expr, else_expr) else { - return; - }; - - if then_search.edits.is_empty() && else_search.edits.is_empty() { - // No insertions - return; - } else if then_search.is_key_used_and_no_copy || else_search.is_key_used_and_no_copy { - // If there are other uses of the key, and the key is not copy, - // we cannot perform a fix automatically, but continue to emit a lint. - None - } 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), - ), - }; - Some(format!( - "if let {}::{entry_kind} = {map_str}.entry({key_str}) {then_str} else {else_str}", - map_ty.entry_path(), - )) - } 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 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 = 'sugg: { + if let Some(else_expr) = else_expr { + let Some(else_search) = find_insert_calls(cx, &contains_expr, else_expr) else { + return; }; - let indent_str = snippet_indent(cx, expr.span); - let indent_str = indent_str.as_deref().unwrap_or(""); - Some(format!( - "match {map_str}.entry({key_str}) {{\n{indent_str} {entry}::{then_entry} => {}\n\ - {indent_str} {entry}::{else_entry} => {}\n{indent_str}}}", - reindent_multiline(&then_str, true, Some(4 + indent_str.len())), - reindent_multiline(&else_str, true, Some(4 + indent_str.len())), - entry = map_ty.entry_path(), - )) - } - } 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) + if then_search.edits.is_empty() && else_search.edits.is_empty() { + // No insertions + return; + } else if then_search.is_key_used_and_no_copy || else_search.is_key_used_and_no_copy { + // If there are other uses of the key, and the key is not copy, + // we cannot perform a fix automatically, but continue to emit a lint. + None + } 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) => ( + if let Some(snippet_vacant) = then_search.snippet_vacant(cx, then_expr.span, &mut app) { + snippet_vacant + } else { + break 'sugg None; + }, + 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) => ( + if let Some(snippet_vacant) = else_search.snippet_vacant(cx, else_expr.span, &mut app) { + snippet_vacant + } else { + break 'sugg None; + }, + snippet_with_applicability(cx, then_expr.span, "{ .. }", &mut app), + ), + }; + Some(format!( + "if let {}::{entry_kind} = {map_str}.entry({key_str}) {then_str} else {else_str}", + map_ty.entry_path(), + )) } else { - then_search.snippet_occupied(cx, then_expr.span, &mut app) - }; - Some(format!( - "if let {}::{entry_kind} = {map_str}.entry({key_str}) {body_str}", - map_ty.entry_path(), - )) - } 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() { - Some(format!("{map_str}.entry({key_str}).or_insert_with(|| {value_str});")) + // if .. { insert } else { insert } + let ((then_str, then_entry), (else_str, else_entry)) = if contains_expr.negated { + ( + if let Some(snippet_vacant) = then_search.snippet_vacant(cx, then_expr.span, &mut app) { + snippet_vacant + } else { + break 'sugg None; + }, + else_search.snippet_occupied(cx, else_expr.span, &mut app), + ) } else { - Some(format!("{map_str}.entry({key_str}).or_insert({value_str});")) - } - } else { - // TODO: suggest using `if let Some(v) = map.get_mut(k) { .. }` here. - // This would need to be a different lint. - return; + ( + then_search.snippet_occupied(cx, then_expr.span, &mut app), + if let Some(snippet_vacant) = else_search.snippet_vacant(cx, else_expr.span, &mut app) { + snippet_vacant + } else { + break 'sugg None; + }, + ) + }; + let indent_str = snippet_indent(cx, expr.span); + let indent_str = indent_str.as_deref().unwrap_or(""); + Some(format!( + "\ + match {map_str}.entry({key_str}) {{ +{indent_str} {entry}::{then_entry} => {} +{indent_str} {entry}::{else_entry} => {} +{indent_str}}}", + reindent_multiline(&then_str, true, Some(4 + indent_str.len())), + reindent_multiline(&else_str, true, Some(4 + indent_str.len())), + entry = map_ty.entry_path(), + )) } } else { - let block_str = then_search.snippet_closure(cx, then_expr.span, &mut app); - if contains_expr.negated { - Some(format!("{map_str}.entry({key_str}).or_insert_with(|| {block_str});")) - } else { - // TODO: suggest using `if let Some(v) = map.get_mut(k) { .. }` here. - // This would need to be a different lint. + 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 { + if let Some(snippet_vacant) = then_search.snippet_vacant(cx, then_expr.span, &mut app) { + snippet_vacant + } else { + break 'sugg None; + } + } else { + then_search.snippet_occupied(cx, then_expr.span, &mut app) + }; + Some(format!( + "if let {}::{entry_kind} = {map_str}.entry({key_str}) {body_str}", + map_ty.entry_path(), + )) + } 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() { + Some(format!("{map_str}.entry({key_str}).or_insert_with(|| {value_str});")) + } else { + Some(format!("{map_str}.entry({key_str}).or_insert({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 { + let block_str = then_search.snippet_closure(cx, then_expr.span, &mut app); + if contains_expr.negated { + Some(format!("{map_str}.entry({key_str}).or_insert_with(|| {block_str});")) + } else { + // TODO: suggest using `if let Some(v) = map.get_mut(k) { .. }` here. + // This would need to be a different lint. + return; + } + } } }; @@ -312,15 +338,12 @@ struct InsertExpr<'tcx> { /// /// If the given expression is not an `insert` call into a `BTreeMap` or a `HashMap`, return `None`. fn try_parse_insert<'tcx>(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 let Some(insert) = cx.tcx.get_diagnostic_name(id) - && matches!(insert, sym::btreemap_insert | sym::hashmap_insert) - { - Some(InsertExpr { map, key, value }) - } else { - None - } + if let ExprKind::MethodCall(_, map, [key, value], _) = expr.kind + && let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + && let Some(insert) = cx.tcx.get_diagnostic_name(id) + && matches!(insert, sym::btreemap_insert | sym::hashmap_insert) + { + Some(InsertExpr { map, key, value }) } else { None } @@ -524,7 +547,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { 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. + // Each branch may contain its 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; @@ -532,7 +555,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { 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. + // Each branch may contain its own insert expression. let mut is_map_used = self.is_map_used; for arm in arms { self.visit_pat(arm.pat); @@ -609,16 +632,39 @@ impl<'tcx> InsertSearchResults<'tcx> { self.is_single_insert.then(|| self.edits[0].as_insertion().unwrap()) } + /// Create a snippet with all the `insert`s on the map replaced with `insert`s on the entry. + /// + /// `write_wrapped` will be called whenever the `insert` is an expression that is used, or has + /// its type unified with another branch -- for example, in cases like: + /// ```ignore + /// let _ = if !m.contains(&k) { + /// m.insert(k, v) + /// } else { + /// None + /// }; + /// + /// let _ = if m.contains(&k) { + /// if true { m.insert(k, v) } else { m.insert(k, v2) } + /// } else { + /// m.insert(k, v3) + /// } + /// ``` + /// The idea is that it will either: + /// - wrap the `insert` call so that it still type-checks with the code surrounding it + /// - or bail out if the result would be too ugly + /// + /// In the latter case, the whole function bails out as well fn snippet( &self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability, - write_wrapped: impl Fn(&mut String, Insertion<'_>, SyntaxContext, &mut Applicability), - ) -> String { + write_wrapped: impl Fn(String, Insertion<'tcx>, SyntaxContext, &mut Applicability) -> Option, + ) -> Option { let ctxt = span.ctxt(); let mut res = String::new(); for insertion in self.edits.iter().filter_map(|e| e.as_insertion()) { + // Take everything until the call verbatim res.push_str(&snippet_with_applicability( cx, span.until(insertion.call.span), @@ -626,55 +672,41 @@ impl<'tcx> InsertSearchResults<'tcx> { app, )); if is_expr_used_or_unified(cx.tcx, insertion.call) { - write_wrapped(&mut res, insertion, ctxt, app); + res = write_wrapped(res, insertion, ctxt, app)?; } else { - let _: fmt::Result = write!( - res, - "e.insert({})", - snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0 - ); + let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "_", app).0; + let _: fmt::Result = write!(res, "e.insert({value_str})"); } span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP); } res.push_str(&snippet_with_applicability(cx, span, "..", app)); - res + Some(res) } fn snippet_occupied(&self, cx: &LateContext<'_>, span: Span, app: &mut Applicability) -> (String, &'static str) { - ( - self.snippet(cx, span, app, |res, insertion, ctxt, app| { + let snippet = self + .snippet(cx, span, app, |mut res, insertion, ctxt, app| { // Insertion into a map would return `Some(&mut value)`, but the entry returns `&mut value` - let _: fmt::Result = write!( - res, - "Some(e.insert({}))", - snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0 - ); - }), - "Occupied(mut e)", - ) + let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "_", app).0; + let _: fmt::Result = write!(res, "Some(e.insert({value_str}))"); + Some(res) + }) + .expect("we pass `write_wrapped` that always returns `Some`, so this will always return `Some`"); + (snippet, "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 _: fmt::Result = 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_vacant( + &self, + cx: &LateContext<'_>, + span: Span, + app: &mut Applicability, + ) -> Option<(String, &'static str)> { + let snippet = self.snippet(cx, span, app, |_, _, _, _| { + // Insertion into a map would return `None`, but the entry returns a mutable reference; + // we used to suggest `{ e.insert(value); None }` here, but that was deemed too ugly so we stopped + None + })?; + Some((snippet, "Vacant(e)")) } fn snippet_closure(&self, cx: &LateContext<'_>, mut span: Span, app: &mut Applicability) -> String { @@ -683,14 +715,15 @@ impl<'tcx> InsertSearchResults<'tcx> { for edit in &self.edits { match *edit { Edit::Insertion(insertion) => { - // Cut out the value from `map.insert(key, value)` + // Take everything until the call verbatim 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); + let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "_", app).0; + res.push_str(&value_str); span = span.trim_start(insertion.call.span).unwrap_or(DUMMY_SP); }, Edit::RemoveSemi(semi_span) => { diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed index f2df9f0204ea..087e70696210 100644 --- a/tests/ui/entry.fixed +++ b/tests/ui/entry.fixed @@ -1,6 +1,6 @@ //@needs-asm-support -#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![allow(clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] use std::arch::asm; diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs index 166eea417ac2..1c3f640fbcb8 100644 --- a/tests/ui/entry.rs +++ b/tests/ui/entry.rs @@ -1,6 +1,6 @@ //@needs-asm-support -#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![allow(clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] use std::arch::asm; diff --git a/tests/ui/entry_btree.fixed b/tests/ui/entry_btree.fixed index 1218202f02b9..8e8563ea3051 100644 --- a/tests/ui/entry_btree.fixed +++ b/tests/ui/entry_btree.fixed @@ -1,5 +1,4 @@ #![warn(clippy::map_entry)] -#![allow(dead_code)] use std::collections::BTreeMap; diff --git a/tests/ui/entry_btree.rs b/tests/ui/entry_btree.rs index b795e32158ad..96a5c1bf2643 100644 --- a/tests/ui/entry_btree.rs +++ b/tests/ui/entry_btree.rs @@ -1,5 +1,4 @@ #![warn(clippy::map_entry)] -#![allow(dead_code)] use std::collections::BTreeMap; diff --git a/tests/ui/entry_btree.stderr b/tests/ui/entry_btree.stderr index 671340b89532..c2196fb4782e 100644 --- a/tests/ui/entry_btree.stderr +++ b/tests/ui/entry_btree.stderr @@ -1,5 +1,5 @@ error: usage of `contains_key` followed by `insert` on a `BTreeMap` - --> tests/ui/entry_btree.rs:10:5 + --> tests/ui/entry_btree.rs:9:5 | LL | / if !m.contains_key(&k) { LL | | diff --git a/tests/ui/entry_unfixable.rs b/tests/ui/entry_unfixable.rs index c4c055572086..3e7d538f77c6 100644 --- a/tests/ui/entry_unfixable.rs +++ b/tests/ui/entry_unfixable.rs @@ -14,40 +14,6 @@ macro_rules! insert { }; } -mod issue13306 { - use std::collections::HashMap; - - struct Env { - enclosing: Option>, - values: HashMap, - } - - impl Env { - fn assign(&mut self, name: String, value: usize) -> bool { - if !self.values.contains_key(&name) { - //~^ map_entry - self.values.insert(name, value); - true - } else if let Some(enclosing) = &mut self.enclosing { - enclosing.assign(name, value) - } else { - false - } - } - } -} - -fn issue9925(mut hm: HashMap) { - let key = "hello".to_string(); - if hm.contains_key(&key) { - //~^ map_entry - let bval = hm.get_mut(&key).unwrap(); - *bval = false; - } else { - hm.insert(key, true); - } -} - mod issue9470 { use std::collections::HashMap; use std::sync::Mutex; @@ -90,4 +56,45 @@ mod issue9470 { } } +fn issue9925(mut hm: HashMap) { + let key = "hello".to_string(); + if hm.contains_key(&key) { + //~^ map_entry + let bval = hm.get_mut(&key).unwrap(); + *bval = false; + } else { + hm.insert(key, true); + } +} + +mod issue13306 { + use std::collections::HashMap; + + struct Env { + enclosing: Option>, + values: HashMap, + } + + impl Env { + fn assign(&mut self, name: String, value: usize) -> bool { + if !self.values.contains_key(&name) { + //~^ map_entry + self.values.insert(name, value); + true + } else if let Some(enclosing) = &mut self.enclosing { + enclosing.assign(name, value) + } else { + false + } + } + } +} + +fn issue15307(mut m: HashMap, k: K, v: V) { + if !m.contains_key(&k) { + //~^ map_entry + assert!(m.insert(k, v).is_none()); + } +} + fn main() {} diff --git a/tests/ui/entry_unfixable.stderr b/tests/ui/entry_unfixable.stderr index f92f472512f6..8fb232f6e771 100644 --- a/tests/ui/entry_unfixable.stderr +++ b/tests/ui/entry_unfixable.stderr @@ -1,12 +1,12 @@ error: usage of `contains_key` followed by `insert` on a `HashMap` - --> tests/ui/entry_unfixable.rs:27:13 + --> tests/ui/entry_unfixable.rs:46:13 | -LL | / if !self.values.contains_key(&name) { +LL | / if self.globals.contains_key(&name) { LL | | -LL | | self.values.insert(name, value); -LL | | true -... | -LL | | false +LL | | self.globals.insert(name, value); +LL | | } else { +LL | | let interner = INTERNER.lock().unwrap(); +LL | | return Err(interner.resolve(name).unwrap().to_owned()); LL | | } | |_____________^ | @@ -15,7 +15,7 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::map_entry)]` error: usage of `contains_key` followed by `insert` on a `HashMap` - --> tests/ui/entry_unfixable.rs:42:5 + --> tests/ui/entry_unfixable.rs:61:5 | LL | / if hm.contains_key(&key) { LL | | @@ -31,16 +31,27 @@ LL | | } error: usage of `contains_key` followed by `insert` on a `HashMap` --> tests/ui/entry_unfixable.rs:80:13 | -LL | / if self.globals.contains_key(&name) { +LL | / if !self.values.contains_key(&name) { LL | | -LL | | self.globals.insert(name, value); -LL | | } else { -LL | | let interner = INTERNER.lock().unwrap(); -LL | | return Err(interner.resolve(name).unwrap().to_owned()); +LL | | self.values.insert(name, value); +LL | | true +... | +LL | | false LL | | } | |_____________^ | = help: consider using the `Entry` API: https://doc.rust-lang.org/std/collections/struct.HashMap.html#entry-api -error: aborting due to 3 previous errors +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> tests/ui/entry_unfixable.rs:94:5 + | +LL | / if !m.contains_key(&k) { +LL | | +LL | | assert!(m.insert(k, v).is_none()); +LL | | } + | |_____^ + | + = help: consider using the `Entry` API: https://doc.rust-lang.org/std/collections/struct.HashMap.html#entry-api + +error: aborting due to 4 previous errors diff --git a/tests/ui/entry_with_else.fixed b/tests/ui/entry_with_else.fixed index 76e6998235c2..66309f86c016 100644 --- a/tests/ui/entry_with_else.fixed +++ b/tests/ui/entry_with_else.fixed @@ -1,4 +1,4 @@ -#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![allow(clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] use std::collections::{BTreeMap, HashMap}; @@ -55,17 +55,6 @@ fn insert_if_absent0(m: &mut HashMap, k: K, } } - match m.entry(k) { - std::collections::hash_map::Entry::Occupied(mut e) => { - //~^ map_entry - 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) { //~^ map_entry foo(); diff --git a/tests/ui/entry_with_else.rs b/tests/ui/entry_with_else.rs index 1669cdc0c7cf..6b65f788ed49 100644 --- a/tests/ui/entry_with_else.rs +++ b/tests/ui/entry_with_else.rs @@ -1,4 +1,4 @@ -#![allow(unused, clippy::needless_pass_by_value, clippy::collapsible_if)] +#![allow(clippy::needless_pass_by_value, clippy::collapsible_if)] #![warn(clippy::map_entry)] use std::collections::{BTreeMap, HashMap}; @@ -46,13 +46,6 @@ fn insert_if_absent0(m: &mut HashMap, k: K, m.insert(k, v2); } - if m.contains_key(&k) { - //~^ map_entry - if true { m.insert(k, v) } else { m.insert(k, v2) } - } else { - m.insert(k, v) - }; - if m.contains_key(&k) { //~^ map_entry foo(); diff --git a/tests/ui/entry_with_else.stderr b/tests/ui/entry_with_else.stderr index d483ac95ad5b..e0be8552a367 100644 --- a/tests/ui/entry_with_else.stderr +++ b/tests/ui/entry_with_else.stderr @@ -119,31 +119,6 @@ error: usage of `contains_key` followed by `insert` on a `HashMap` | LL | / if m.contains_key(&k) { LL | | -LL | | if true { m.insert(k, v) } else { m.insert(k, v2) } -LL | | } else { -LL | | m.insert(k, v) -LL | | }; - | |_____^ - | -help: try - | -LL ~ match m.entry(k) { -LL + std::collections::hash_map::Entry::Occupied(mut e) => { -LL + -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); -LL + None -LL + } -LL ~ }; - | - -error: usage of `contains_key` followed by `insert` on a `HashMap` - --> tests/ui/entry_with_else.rs:56:5 - | -LL | / if m.contains_key(&k) { -LL | | LL | | foo(); LL | | m.insert(k, v) LL | | } else { @@ -162,5 +137,5 @@ LL + None LL ~ }; | -error: aborting due to 7 previous errors +error: aborting due to 6 previous errors diff --git a/tests/ui/entry_with_else_unfixable.rs b/tests/ui/entry_with_else_unfixable.rs new file mode 100644 index 000000000000..984bc057f9ca --- /dev/null +++ b/tests/ui/entry_with_else_unfixable.rs @@ -0,0 +1,16 @@ +//@no-rustfix +#![warn(clippy::map_entry)] + +use std::collections::HashMap; +use std::hash::Hash; + +fn foo() {} + +fn insert_if_absent0(m: &mut HashMap, k: K, v: V, v2: V) { + if m.contains_key(&k) { + //~^ map_entry + if true { m.insert(k, v) } else { m.insert(k, v2) } + } else { + m.insert(k, v) + }; +} diff --git a/tests/ui/entry_with_else_unfixable.stderr b/tests/ui/entry_with_else_unfixable.stderr new file mode 100644 index 000000000000..8d9793cec5cb --- /dev/null +++ b/tests/ui/entry_with_else_unfixable.stderr @@ -0,0 +1,17 @@ +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> tests/ui/entry_with_else_unfixable.rs:10:5 + | +LL | / if m.contains_key(&k) { +LL | | +LL | | if true { m.insert(k, v) } else { m.insert(k, v2) } +LL | | } else { +LL | | m.insert(k, v) +LL | | }; + | |_____^ + | + = help: consider using the `Entry` API: https://doc.rust-lang.org/std/collections/struct.HashMap.html#entry-api + = note: `-D clippy::map-entry` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::map_entry)]` + +error: aborting due to 1 previous error +