fix(map_entry): provide explicit type when suggesting None

Author: ada4a

clippy_lints/src/entry.rs

@@ -12,6 +12,7 @@ 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::ops::ControlFlow;
@@ -24,17 +25,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;
@@ -652,15 +642,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_str = snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0;
+                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 _ = if is_expr_final_block_expr(cx.tcx, insertion.call) {
                     write!(
                         res,
-                        "e.insert({value_str});\n{indent}None",
+                        "e.insert({value_str});\n{indent}{none_str}",
                         indent = snippet_indent(cx, insertion.call.span).as_deref().unwrap_or(""),
                     )
                 } else {
-                    write!(res, "{{ e.insert({value_str}); None }}")
+                    write!(res, "{{ e.insert({value_str}); {none_str} }}")
                 };
             }),
             "Vacant(e)",

tests/ui/entry.fixed

@@ -248,4 +248,11 @@ mod issue14449 {
     }
 }
 
+fn issue15307<K: Eq + Hash, V>(mut m: HashMap<K, V>, k: K, v: V) {
+    if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) {
+        //~^ map_entry
+        assert!({ e.insert(v); None::<V> }.is_none());
+    }
+}
+
 fn main() {}

tests/ui/entry.rs

@@ -254,4 +254,11 @@ mod issue14449 {
     }
 }
 
+fn issue15307<K: Eq + Hash, V>(mut m: HashMap<K, V>, k: K, v: V) {
+    if !m.contains_key(&k) {
+        //~^ map_entry
+        assert!(m.insert(k, v).is_none());
+    }
+}
+
 fn main() {}

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::<V> }.is_none());
+LL +     }
+   |
+
+error: aborting due to 12 previous errors
 

tests/ui/entry_with_else.fixed

@@ -62,7 +62,7 @@ fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, k: K,
         }
         std::collections::hash_map::Entry::Vacant(e) => {
             e.insert(v);
-            None
+            None::<V>
         }
     };
 
@@ -73,6 +73,15 @@ fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, 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::<V>
+    } else {
+        None
+    };
 }
 
 fn main() {}

tests/ui/entry_with_else.rs

@@ -60,6 +60,14 @@ fn insert_if_absent0<K: Eq + Hash + Copy, V: Copy>(m: &mut HashMap<K, V>, 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() {}

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::<V>
 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::<V>
+LL +     } else {
+LL +         None
+LL ~     };
+   |
+
+error: aborting due to 8 previous errors