Auto merge of #9342 - relrelb:or_default, r=dswij

Suggest `Entry::or_default` for `Entry::or_insert(Default::default())`

Unlike past similar work done in #6228, expand the existing `or_fun_call`
lint to detect `or_insert` calls with a `T::new()` or `T::default()`
argument, much like currently done for `unwrap_or` calls. In that case,
suggest the use of `or_default`, which is more idiomatic.

Note that even with this change, `or_insert_with(T::default)` calls
aren't detected as candidates for `or_default()`, in the same manner
that currently `unwrap_or_else(T::default)` calls aren't detected as
candidates for `unwrap_or_default()`.

Also, as a nearby cleanup, change `KNOW_TYPES` from `static` to `const`,
since as far as I understand it's preferred (should Clippy have a lint
for that?).

Addresses #3812.

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: [`or_fun_call`]: Suggest `Entry::or_default` for `Entry::or_insert(Default::default())`

Author: bors

clippy_lints/src/methods/mod.rs

@@ -825,8 +825,9 @@ declare_clippy_lint! {
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for calls to `.or(foo(..))`, `.unwrap_or(foo(..))`,
-    /// etc., and suggests to use `or_else`, `unwrap_or_else`, etc., or
-    /// `unwrap_or_default` instead.
+    /// `.or_insert(foo(..))` etc., and suggests to use `.or_else(|| foo(..))`,
+    /// `.unwrap_or_else(|| foo(..))`, `.unwrap_or_default()` or `.or_default()`
+    /// etc. instead.
     ///
     /// ### Why is this bad?
     /// The function will always be called and potentially

clippy_lints/src/methods/or_fun_call.rs

@@ -22,7 +22,8 @@ pub(super) fn check<'tcx>(
     name: &str,
     args: &'tcx [hir::Expr<'_>],
 ) {
-    /// Checks for `unwrap_or(T::new())` or `unwrap_or(T::default())`.
+    /// Checks for `unwrap_or(T::new())`, `unwrap_or(T::default())`,
+    /// `or_insert(T::new())` or `or_insert(T::default())`.
     #[allow(clippy::too_many_arguments)]
     fn check_unwrap_or_default(
         cx: &LateContext<'_>,
@@ -42,7 +43,11 @@ pub(super) fn check<'tcx>(
 
         if_chain! {
             if !or_has_args;
-            if name == "unwrap_or";
+            if let Some(sugg) = match name {
+                "unwrap_or" => Some("unwrap_or_default"),
+                "or_insert" => Some("or_default"),
+                _ => None,
+            };
             if let hir::ExprKind::Path(ref qpath) = fun.kind;
             if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default);
             let path = last_path_segment(qpath).ident.name;
@@ -58,7 +63,7 @@ pub(super) fn check<'tcx>(
                     method_span.with_hi(span.hi()),
                     &format!("use of `{}` followed by a call to `{}`", name, path),
                     "try this",
-                    "unwrap_or_default()".to_string(),
+                    format!("{}()", sugg),
                     Applicability::MachineApplicable,
                 );
 
@@ -82,7 +87,7 @@ pub(super) fn check<'tcx>(
         fun_span: Option<Span>,
     ) {
         // (path, fn_has_argument, methods, suffix)
-        static KNOW_TYPES: [(&[&str], bool, &[&str], &str); 4] = [
+        const KNOW_TYPES: [(&[&str], bool, &[&str], &str); 4] = [
             (&paths::BTREEMAP_ENTRY, false, &["or_insert"], "with"),
             (&paths::HASHMAP_ENTRY, false, &["or_insert"], "with"),
             (&paths::OPTION, false, &["map_or", "ok_or", "or", "unwrap_or"], "else"),

src/docs/or_fun_call.txt

@@ -1,7 +1,8 @@
 ### What it does
 Checks for calls to `.or(foo(..))`, `.unwrap_or(foo(..))`,
-etc., and suggests to use `or_else`, `unwrap_or_else`, etc., or
-`unwrap_or_default` instead.
+`.or_insert(foo(..))` etc., and suggests to use `.or_else(|| foo(..))`,
+`.unwrap_or_else(|| foo(..))`, `.unwrap_or_default()` or `.or_default()`
+etc. instead.
 
 ### Why is this bad?
 The function will always be called and potentially

tests/ui/or_fun_call.fixed

@@ -79,16 +79,16 @@ fn or_fun_call() {
     without_default.unwrap_or_else(Foo::new);
 
     let mut map = HashMap::<u64, String>::new();
-    map.entry(42).or_insert(String::new());
+    map.entry(42).or_default();
 
     let mut map_vec = HashMap::<u64, Vec<i32>>::new();
-    map_vec.entry(42).or_insert(vec![]);
+    map_vec.entry(42).or_default();
 
     let mut btree = BTreeMap::<u64, String>::new();
-    btree.entry(42).or_insert(String::new());
+    btree.entry(42).or_default();
 
     let mut btree_vec = BTreeMap::<u64, Vec<i32>>::new();
-    btree_vec.entry(42).or_insert(vec![]);
+    btree_vec.entry(42).or_default();
 
     let stringy = Some(String::new());
     let _ = stringy.unwrap_or_default();

tests/ui/or_fun_call.stderr

@@ -66,6 +66,30 @@ error: use of `unwrap_or` followed by a function call
 LL |     without_default.unwrap_or(Foo::new());
    |                     ^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(Foo::new)`
 
+error: use of `or_insert` followed by a call to `new`
+  --> $DIR/or_fun_call.rs:82:19
+   |
+LL |     map.entry(42).or_insert(String::new());
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_default()`
+
+error: use of `or_insert` followed by a call to `new`
+  --> $DIR/or_fun_call.rs:85:23
+   |
+LL |     map_vec.entry(42).or_insert(vec![]);
+   |                       ^^^^^^^^^^^^^^^^^ help: try this: `or_default()`
+
+error: use of `or_insert` followed by a call to `new`
+  --> $DIR/or_fun_call.rs:88:21
+   |
+LL |     btree.entry(42).or_insert(String::new());
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_default()`
+
+error: use of `or_insert` followed by a call to `new`
+  --> $DIR/or_fun_call.rs:91:25
+   |
+LL |     btree_vec.entry(42).or_insert(vec![]);
+   |                         ^^^^^^^^^^^^^^^^^ help: try this: `or_default()`
+
 error: use of `unwrap_or` followed by a call to `new`
   --> $DIR/or_fun_call.rs:94:21
    |
@@ -132,5 +156,5 @@ error: use of `unwrap_or` followed by a call to `new`
 LL |         .unwrap_or(String::new());
    |          ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_default()`
 
-error: aborting due to 22 previous errors
+error: aborting due to 26 previous errors