diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index a5ec6777b434..5d606cb95e4f 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -6,14 +6,16 @@ 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, }; -use core::fmt::{self, Write}; +use core::fmt::Write; use rustc_errors::Applicability; use rustc_hir::hir_id::HirIdSet; use rustc_hir::intravisit::{Visitor, walk_body, walk_expr}; use rustc_hir::{Block, Expr, ExprKind, HirId, Pat, Stmt, StmtKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::IsSuggestable; use rustc_session::declare_lint_pass; use rustc_span::{DUMMY_SP, Span, SyntaxContext, sym}; +use std::fmt; use std::ops::ControlFlow; declare_clippy_lint! { @@ -24,17 +26,6 @@ declare_clippy_lint! { /// ### Why is this bad? /// Using `entry` is more efficient. /// - /// ### Known problems - /// The suggestion may have type inference errors in some cases. e.g. - /// ```no_run - /// let mut map = std::collections::HashMap::new(); - /// let _ = if !map.contains_key(&0) { - /// map.insert(0, 0) - /// } else { - /// None - /// }; - /// ``` - /// /// ### Example /// ```no_run /// # use std::collections::HashMap; @@ -628,11 +619,8 @@ impl<'tcx> InsertSearchResults<'tcx> { if is_expr_used_or_unified(cx.tcx, insertion.call) { write_wrapped(&mut 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); } @@ -644,11 +632,8 @@ impl<'tcx> InsertSearchResults<'tcx> { ( 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 _: fmt::Result = write!( - res, - "Some(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, "Some(e.insert({value_str}))"); }), "Occupied(mut e)", ) @@ -658,19 +643,24 @@ impl<'tcx> InsertSearchResults<'tcx> { ( self.snippet(cx, span, app, |res, insertion, ctxt, app| { // Insertion into a map would return `None`, but the entry returns a mutable reference. + let value = insertion.value; + let value_str = snippet_with_context(cx, value.span, ctxt, "..", app).0; + + let value_ty = cx.typeck_results().expr_ty(value); + debug_assert!( + value_ty.is_suggestable(cx.tcx, true), + "an unsuggestable type used as the element type:{value_ty:#?}\nexpr={value:#?}" + ); + let none_str = format_args!("None::<{value_ty}>"); + 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(""), + "e.insert({value_str});\n{indent}{none_str}", + indent = 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, - ) + write!(res, "{{ e.insert({value_str}); {none_str} }}") }; }), "Vacant(e)", @@ -690,7 +680,8 @@ impl<'tcx> InsertSearchResults<'tcx> { "..", 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; + let _: fmt::Result = write!(res, "{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..b8df15a0495d 100644 --- a/tests/ui/entry.fixed +++ b/tests/ui/entry.fixed @@ -248,4 +248,11 @@ mod issue14449 { } } +fn issue15307(mut m: HashMap, k: K, v: V) { + if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + //~^ map_entry + assert!({ e.insert(v); None:: }.is_none()); + } +} + fn main() {} diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs index 166eea417ac2..bdfa344e28a4 100644 --- a/tests/ui/entry.rs +++ b/tests/ui/entry.rs @@ -254,4 +254,11 @@ mod issue14449 { } } +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.stderr b/tests/ui/entry.stderr index 009b78d29073..48911ad53e6e 100644 --- a/tests/ui/entry.stderr +++ b/tests/ui/entry.stderr @@ -246,5 +246,22 @@ LL + e.insert(42); LL + } | -error: aborting due to 11 previous errors +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> tests/ui/entry.rs:258:5 + | +LL | / if !m.contains_key(&k) { +LL | | +LL | | assert!(m.insert(k, v).is_none()); +LL | | } + | |_____^ + | +help: try + | +LL ~ if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL + +LL + assert!({ e.insert(v); None:: }.is_none()); +LL + } + | + +error: aborting due to 12 previous errors diff --git a/tests/ui/entry_with_else.fixed b/tests/ui/entry_with_else.fixed index 76e6998235c2..869f477eb1d6 100644 --- a/tests/ui/entry_with_else.fixed +++ b/tests/ui/entry_with_else.fixed @@ -62,7 +62,7 @@ fn insert_if_absent0(m: &mut HashMap, k: K, } std::collections::hash_map::Entry::Vacant(e) => { e.insert(v); - None + None:: } }; @@ -73,6 +73,15 @@ fn insert_if_absent0(m: &mut HashMap, k: K, } else { None }; + + // used to result in type ambiguity before #15759 + let _ = if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { + //~^ map_entry + e.insert(v); + None:: + } else { + None + }; } fn main() {} diff --git a/tests/ui/entry_with_else.rs b/tests/ui/entry_with_else.rs index 1669cdc0c7cf..07173b3b74a9 100644 --- a/tests/ui/entry_with_else.rs +++ b/tests/ui/entry_with_else.rs @@ -60,6 +60,14 @@ fn insert_if_absent0(m: &mut HashMap, k: K, } else { None }; + + // used to result in type ambiguity before #15759 + let _ = if !m.contains_key(&k) { + //~^ map_entry + m.insert(k, v) + } else { + None + }; } fn main() {} diff --git a/tests/ui/entry_with_else.stderr b/tests/ui/entry_with_else.stderr index d483ac95ad5b..d56764361f61 100644 --- a/tests/ui/entry_with_else.stderr +++ b/tests/ui/entry_with_else.stderr @@ -134,7 +134,7 @@ 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 + None:: LL + } LL ~ }; | @@ -162,5 +162,28 @@ LL + None LL ~ }; | -error: aborting due to 7 previous errors +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> tests/ui/entry_with_else.rs:65:13 + | +LL | let _ = if !m.contains_key(&k) { + | _____________^ +LL | | +LL | | m.insert(k, v) +LL | | } else { +LL | | None +LL | | }; + | |_____^ + | +help: try + | +LL ~ let _ = if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { +LL + +LL + e.insert(v); +LL + None:: +LL + } else { +LL + None +LL ~ }; + | + +error: aborting due to 8 previous errors