Merge unwrap_or_else_default.rs into or_fun_call.rs

Author: smoelius

clippy_lints/src/methods/mod.rs

@@ -103,7 +103,6 @@ mod unnecessary_lazy_eval;
 mod unnecessary_literal_unwrap;
 mod unnecessary_sort_by;
 mod unnecessary_to_owned;
-mod unwrap_or_else_default;
 mod unwrap_used;
 mod useless_asref;
 mod utils;
@@ -476,29 +475,40 @@ declare_clippy_lint! {
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for usage of `_.unwrap_or_else(Default::default)` on `Option` and
-    /// `Result` values.
+    /// Checks for usages of the following functions with an argument that constructs a default value
+    /// (e.g., `Default::default` or `String::new`):
+    /// - `unwrap_or`
+    /// - `unwrap_or_else`
+    /// - `or_insert`
+    /// - `or_insert_with`
     ///
     /// ### Why is this bad?
-    /// Readability, these can be written as `_.unwrap_or_default`, which is
-    /// simpler and more concise.
+    /// Readability. Using `unwrap_or_default` in place of `unwrap_or`/`unwrap_or_else`, or `or_default`
+    /// in place of `or_insert`/`or_insert_with`, is simpler and more concise.
+    ///
+    /// ### Known problems
+    /// In some cases, the argument of `unwrap_or`, etc. is needed for type inference. The lint uses a
+    /// heuristic to try to identify such cases. However, the heuristic can produce false negatives.
     ///
     /// ### Examples
     /// ```rust
     /// # let x = Some(1);
-    /// x.unwrap_or_else(Default::default);
-    /// x.unwrap_or_else(u32::default);
+    /// # let mut map = std::collections::HashMap::<u64, String>::new();
+    /// x.unwrap_or(Default::default());
+    /// map.entry(42).or_insert_with(String::new);
     /// ```
     ///
     /// Use instead:
     /// ```rust
     /// # let x = Some(1);
+    /// # let mut map = std::collections::HashMap::<u64, String>::new();
     /// x.unwrap_or_default();
+    /// map.entry(42).or_default();
     /// ```
     #[clippy::version = "1.56.0"]
     pub UNWRAP_OR_ELSE_DEFAULT,
     style,
-    "using `.unwrap_or_else(Default::default)`, which is more succinctly expressed as `.unwrap_or_default()`"
+    "using `.unwrap_or`, etc. with an argument that constructs a default value"
 }
 
 declare_clippy_lint! {
@@ -3756,8 +3766,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             then {
                 let first_arg_span = first_arg_ty.span;
                 let first_arg_ty = hir_ty_to_ty(cx.tcx, first_arg_ty);
-                let self_ty = TraitRef::identity(cx.tcx, item.owner_id.to_def_id())
-                    .self_ty();
+                let self_ty = TraitRef::identity(cx.tcx, item.owner_id.to_def_id()).self_ty();
                 wrong_self_convention::check(
                     cx,
                     item.ident.name.as_str(),
@@ -3774,8 +3783,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             if item.ident.name == sym::new;
             if let TraitItemKind::Fn(_, _) = item.kind;
             let ret_ty = return_ty(cx, item.owner_id);
-            let self_ty = TraitRef::identity(cx.tcx, item.owner_id.to_def_id())
-                .self_ty();
+            let self_ty = TraitRef::identity(cx.tcx, item.owner_id.to_def_id()).self_ty();
             if !ret_ty.contains(self_ty);
 
             then {
@@ -4134,7 +4142,6 @@ impl Methods {
                         Some(("map", recv, [map_arg], _, _))
                             if map_unwrap_or::check(cx, expr, recv, map_arg, u_arg, &self.msrv) => {},
                         _ => {
-                            unwrap_or_else_default::check(cx, expr, recv, u_arg);
                             unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or");
                         },
                     }

clippy_lints/src/methods/or_fun_call.rs

@@ -1,16 +1,17 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::eager_or_lazy::switch_to_lazy_eval;
 use clippy_utils::source::snippet_with_context;
-use clippy_utils::ty::{implements_trait, is_type_diagnostic_item};
-use clippy_utils::{contains_return, is_trait_item, last_path_segment};
+use clippy_utils::ty::{expr_type_is_certain, implements_trait, is_type_diagnostic_item};
+use clippy_utils::{contains_return, is_default_equivalent, is_default_equivalent_call, last_path_segment};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir as hir;
 use rustc_lint::LateContext;
+use rustc_middle::ty;
 use rustc_span::source_map::Span;
-use rustc_span::symbol::{kw, sym, Symbol};
+use rustc_span::symbol::{self, sym, Symbol};
+use {rustc_ast as ast, rustc_hir as hir};
 
-use super::OR_FUN_CALL;
+use super::{OR_FUN_CALL, UNWRAP_OR_ELSE_DEFAULT};
 
 /// Checks for the `OR_FUN_CALL` lint.
 #[allow(clippy::too_many_lines)]
@@ -24,44 +25,64 @@ pub(super) fn check<'tcx>(
 ) {
     /// Checks for `unwrap_or(T::new())`, `unwrap_or(T::default())`,
     /// `or_insert(T::new())` or `or_insert(T::default())`.
+    /// Similarly checks for `unwrap_or_else(T::new)`, `unwrap_or_else(T::default)`,
+    /// `or_insert_with(T::new)` or `or_insert_with(T::default)`.
     #[allow(clippy::too_many_arguments)]
     fn check_unwrap_or_default(
         cx: &LateContext<'_>,
         name: &str,
+        receiver: &hir::Expr<'_>,
         fun: &hir::Expr<'_>,
-        arg: &hir::Expr<'_>,
-        or_has_args: bool,
+        call_expr: Option<&hir::Expr<'_>>,
         span: Span,
         method_span: Span,
     ) -> bool {
-        let is_default_default = || is_trait_item(cx, fun, sym::Default);
+        if !expr_type_is_certain(cx, receiver) {
+            return false;
+        }
+
+        let is_new = |fun: &hir::Expr<'_>| {
+            if let hir::ExprKind::Path(ref qpath) = fun.kind {
+                let path = last_path_segment(qpath).ident.name;
+                matches!(path, sym::new)
+            } else {
+                false
+            }
+        };
 
-        let implements_default = |arg, default_trait_id| {
-            let arg_ty = cx.typeck_results().expr_ty(arg);
-            implements_trait(cx, arg_ty, default_trait_id, &[])
+        let output_type_implements_default = |fun| {
+            let fun_ty = cx.typeck_results().expr_ty(fun);
+            if let ty::FnDef(def_id, substs) = fun_ty.kind() {
+                let output_ty = cx.tcx.fn_sig(def_id).subst(cx.tcx, substs).skip_binder().output();
+                cx.tcx
+                    .get_diagnostic_item(sym::Default)
+                    .map_or(false, |default_trait_id| {
+                        implements_trait(cx, output_ty, default_trait_id, substs)
+                    })
+            } else {
+                false
+            }
         };
 
         if_chain! {
-            if !or_has_args;
-            if let Some(sugg) = match name {
-                "unwrap_or" => Some("unwrap_or_default"),
-                "or_insert" => Some("or_default"),
+            if let Some(sugg) = match (name, call_expr.is_some()) {
+                ("unwrap_or", true) | ("unwrap_or_else", false) => Some("unwrap_or_default"),
+                ("or_insert", true) | ("or_insert_with", false) => 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;
             // needs to target Default::default in particular or be *::new and have a Default impl
             // available
-            if (matches!(path, kw::Default) && is_default_default())
-                || (matches!(path, sym::new) && implements_default(arg, default_trait_id));
-
+            if (is_new(fun) && output_type_implements_default(fun))
+                || match call_expr {
+                    Some(call_expr) => is_default_equivalent(cx, call_expr),
+                    None => is_default_equivalent_call(cx, fun) || closure_body_returns_empty_to_string(cx, fun),
+                };
             then {
                 span_lint_and_sugg(
                     cx,
-                    OR_FUN_CALL,
+                    UNWRAP_OR_ELSE_DEFAULT,
                     method_span.with_hi(span.hi()),
-                    &format!("use of `{name}` followed by a call to `{path}`"),
+                    &format!("use of `{name}` to construct default value"),
                     "try",
                     format!("{sugg}()"),
                     Applicability::MachineApplicable,
@@ -168,11 +189,16 @@ pub(super) fn check<'tcx>(
         match inner_arg.kind {
             hir::ExprKind::Call(fun, or_args) => {
                 let or_has_args = !or_args.is_empty();
-                if !check_unwrap_or_default(cx, name, fun, arg, or_has_args, expr.span, method_span) {
+                if or_has_args
+                    || !check_unwrap_or_default(cx, name, receiver, fun, Some(inner_arg), expr.span, method_span)
+                {
                     let fun_span = if or_has_args { None } else { Some(fun.span) };
                     check_general_case(cx, name, method_span, receiver, arg, None, expr.span, fun_span);
                 }
             },
+            hir::ExprKind::Path(..) | hir::ExprKind::Closure(..) => {
+                check_unwrap_or_default(cx, name, receiver, inner_arg, None, expr.span, method_span);
+            },
             hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => {
                 check_general_case(cx, name, method_span, receiver, arg, None, expr.span, None);
             },
@@ -189,3 +215,22 @@ pub(super) fn check<'tcx>(
         }
     }
 }
+
+fn closure_body_returns_empty_to_string(cx: &LateContext<'_>, e: &hir::Expr<'_>) -> bool {
+    if let hir::ExprKind::Closure(&hir::Closure { body, .. }) = e.kind {
+        let body = cx.tcx.hir().body(body);
+
+        if body.params.is_empty()
+            && let hir::Expr{ kind, .. } = &body.value
+            && let hir::ExprKind::MethodCall(hir::PathSegment {ident, ..}, self_arg, _, _) = kind
+            && ident == &symbol::Ident::from_str("to_string")
+            && let hir::Expr{ kind, .. } = self_arg
+            && let hir::ExprKind::Lit(lit) = kind
+            && let ast::LitKind::Str(symbol::kw::Empty, _) = lit.node
+        {
+            return true;
+        }
+    }
+
+    false
+}

clippy_lints/src/methods/unwrap_or_else_default.rs

@@ -190,7 +190,7 @@ mod issue8239 {
                 acc.push_str(&f);
                 acc
             })
-            .unwrap_or_default();
+            .unwrap_or(String::new());
     }
 
     fn more_to_max_suggestion_highest_lines_1() {
@@ -203,7 +203,7 @@ mod issue8239 {
                 acc.push_str(&f);
                 acc
             })
-            .unwrap_or_default();
+            .unwrap_or(String::new());
     }
 
     fn equal_to_max_suggestion_highest_lines() {
@@ -215,7 +215,7 @@ mod issue8239 {
                 acc.push_str(&f);
                 acc
             })
-            .unwrap_or_default();
+            .unwrap_or(String::new());
     }
 
     fn less_than_max_suggestion_highest_lines() {
@@ -226,7 +226,7 @@ mod issue8239 {
             acc.push_str(&f);
             acc
         })
-        .unwrap_or_default();
+        .unwrap_or(String::new());
     }
 }
 
@@ -257,4 +257,59 @@ mod issue8993 {
     }
 }
 
+mod lazy {
+    use super::*;
+
+    fn foo() {
+        struct Foo;
+
+        impl Foo {
+            fn new() -> Foo {
+                Foo
+            }
+        }
+
+        struct FakeDefault;
+        impl FakeDefault {
+            fn default() -> Self {
+                FakeDefault
+            }
+        }
+
+        impl Default for FakeDefault {
+            fn default() -> Self {
+                FakeDefault
+            }
+        }
+
+        let with_new = Some(vec![1]);
+        with_new.unwrap_or_default();
+
+        let with_default_trait = Some(1);
+        with_default_trait.unwrap_or_default();
+
+        let with_default_type = Some(1);
+        with_default_type.unwrap_or_default();
+
+        let real_default = None::<FakeDefault>;
+        real_default.unwrap_or_default();
+
+        let mut map = HashMap::<u64, String>::new();
+        map.entry(42).or_default();
+
+        let mut btree = BTreeMap::<u64, String>::new();
+        btree.entry(42).or_default();
+
+        let stringy = Some(String::new());
+        let _ = stringy.unwrap_or_default();
+
+        // negative tests
+        let self_default = None::<FakeDefault>;
+        self_default.unwrap_or_else(<FakeDefault>::default);
+
+        let without_default = Some(Foo);
+        without_default.unwrap_or_else(Foo::new);
+    }
+}
+
 fn main() {}