Add unwrap_or_else_default lint

lf- · lf- · commit 11ef04728c90 · 2021-08-10T14:40:26.000-07:00
This will catch `unwrap_or_else(Default::default)` on Result and Option
and suggest `unwrap_or_default()` instead.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2999,6 +2999,7 @@ Released 2018-09-13
 [`unused_unit`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_unit
 [`unusual_byte_groupings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unusual_byte_groupings
 [`unwrap_in_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_in_result
+[`unwrap_or_else_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_or_else_default
 [`unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_used
 [`upper_case_acronyms`]: https://rust-lang.github.io/rust-clippy/master/index.html#upper_case_acronyms
 [`use_debug`]: https://rust-lang.github.io/rust-clippy/master/index.html#use_debug
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -797,6 +797,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         methods::UNNECESSARY_FILTER_MAP,
         methods::UNNECESSARY_FOLD,
         methods::UNNECESSARY_LAZY_EVALUATIONS,
+        methods::UNWRAP_OR_ELSE_DEFAULT,
         methods::UNWRAP_USED,
         methods::USELESS_ASREF,
         methods::WRONG_SELF_CONVENTION,
@@ -1341,6 +1342,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(methods::UNNECESSARY_FILTER_MAP),
         LintId::of(methods::UNNECESSARY_FOLD),
         LintId::of(methods::UNNECESSARY_LAZY_EVALUATIONS),
+        LintId::of(methods::UNWRAP_OR_ELSE_DEFAULT),
         LintId::of(methods::USELESS_ASREF),
         LintId::of(methods::WRONG_SELF_CONVENTION),
         LintId::of(methods::ZST_OFFSET),
@@ -1535,6 +1537,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(methods::STRING_EXTEND_CHARS),
         LintId::of(methods::UNNECESSARY_FOLD),
         LintId::of(methods::UNNECESSARY_LAZY_EVALUATIONS),
+        LintId::of(methods::UNWRAP_OR_ELSE_DEFAULT),
         LintId::of(methods::WRONG_SELF_CONVENTION),
         LintId::of(misc::TOPLEVEL_REF_ARG),
         LintId::of(misc::ZERO_PTR),
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -56,6 +56,7 @@ mod uninit_assumed_init;
 mod unnecessary_filter_map;
 mod unnecessary_fold;
 mod unnecessary_lazy_eval;
+mod unwrap_or_else_default;
 mod unwrap_used;
 mod useless_asref;
 mod utils;
@@ -310,6 +311,31 @@ declare_clippy_lint! {
     "using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usages of `_.unwrap_or_else(Default::default)` on Option and
+    /// Result values.
+    ///
+    /// ### Why is this bad?
+    /// Readability, these can be written as `option.unwrap_or_default` or
+    /// `result.unwrap_or_default`.
+    ///
+    /// ### Examples
+    /// ```rust
+    /// # let x = Some(1);
+    ///
+    /// // Bad
+    /// x.unwrap_or_else(Default::default);
+    /// x.unwrap_or_else(u32::default);
+    ///
+    /// // Good
+    /// x.unwrap_or_default();
+    /// ```
+    pub UNWRAP_OR_ELSE_DEFAULT,
+    style,
+    "using `.unwrap_or_else(Default::default)`, which is more succinctly expressed as `.unwrap_or_default()`"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for usage of `option.map(_).unwrap_or(_)` or `option.map(_).unwrap_or_else(_)` or
@@ -1766,6 +1792,7 @@ impl_lint_pass!(Methods => [
     SHOULD_IMPLEMENT_TRAIT,
     WRONG_SELF_CONVENTION,
     OK_EXPECT,
+    UNWRAP_OR_ELSE_DEFAULT,
     MAP_UNWRAP_OR,
     RESULT_MAP_OR_INTO_OPTION,
     OPTION_MAP_OR_NONE,
@@ -2172,7 +2199,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
             },
             ("unwrap_or_else", [u_arg]) => match method_call!(recv) {
                 Some(("map", [recv, map_arg], _)) if map_unwrap_or::check(cx, expr, recv, map_arg, u_arg, msrv) => {},
-                _ => unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or"),
+                _ => {
+                    unwrap_or_else_default::check(cx, expr, recv, u_arg);
+                    unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or");
+                },
             },
             _ => {},
         }
diff --git a/clippy_lints/src/methods/or_fun_call.rs b/clippy_lints/src/methods/or_fun_call.rs
@@ -1,7 +1,8 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::eager_or_lazy::is_lazyness_candidate;
 use clippy_utils::source::{snippet, snippet_with_applicability, snippet_with_macro_callsite};
-use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, match_type};
+use clippy_utils::ty::{implements_trait, qpath_target_trait};
+use clippy_utils::ty::{is_type_diagnostic_item, match_type};
 use clippy_utils::{contains_return, last_path_segment, paths};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
@@ -34,15 +35,25 @@ pub(super) fn check<'tcx>(
         or_has_args: bool,
         span: Span,
     ) -> bool {
+        let is_default_default = |qpath, default_trait_id| {
+            qpath_target_trait(cx, qpath, fun.hir_id).map_or(false, |target_trait| target_trait == default_trait_id)
+        };
+
+        let implements_default = |arg, default_trait_id| {
+            let arg_ty = cx.typeck_results().expr_ty(arg);
+            implements_trait(cx, arg_ty, default_trait_id, &[])
+        };
+
         if_chain! {
             if !or_has_args;
             if name == "unwrap_or";
             if let hir::ExprKind::Path(ref qpath) = fun.kind;
-            let path = last_path_segment(qpath).ident.name;
-            if matches!(path, kw::Default | sym::new);
-            let arg_ty = cx.typeck_results().expr_ty(arg);
             if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default);
-            if implements_trait(cx, arg_ty, default_trait_id, &[]);
+            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(qpath, default_trait_id))
+                || (matches!(path, sym::new) && implements_default(arg, default_trait_id));
 
             then {
                 let mut applicability = Applicability::MachineApplicable;
diff --git a/clippy_lints/src/methods/unwrap_or_else_default.rs b/clippy_lints/src/methods/unwrap_or_else_default.rs
@@ -0,0 +1,50 @@
+//! Lint for `some_result_or_option.unwrap_or_else(Default::default)`
+
+use super::UNWRAP_OR_ELSE_DEFAULT;
+use clippy_utils::{
+    diagnostics::span_lint_and_sugg,
+    source::snippet_with_applicability,
+    ty::{is_type_diagnostic_item, qpath_target_trait},
+};
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_lint::LateContext;
+use rustc_span::sym;
+
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx hir::Expr<'_>,
+    recv: &'tcx hir::Expr<'_>,
+    u_arg: &'tcx hir::Expr<'_>,
+) {
+    // something.unwrap_or_else(Default::default)
+    // ^^^^^^^^^- recv          ^^^^^^^^^^^^^^^^- u_arg
+    // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- expr
+    let recv_ty = cx.typeck_results().expr_ty(recv);
+    let is_option = is_type_diagnostic_item(cx, recv_ty, sym::option_type);
+    let is_result = is_type_diagnostic_item(cx, recv_ty, sym::result_type);
+
+    if_chain! {
+        if is_option || is_result;
+        if let hir::ExprKind::Path(ref qpath) = u_arg.kind;
+        if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default);
+        if let Some(target_trait) = qpath_target_trait(cx, qpath, u_arg.hir_id);
+        if target_trait == default_trait_id;
+        then {
+            let mut applicability = Applicability::MachineApplicable;
+
+            span_lint_and_sugg(
+                cx,
+                UNWRAP_OR_ELSE_DEFAULT,
+                expr.span,
+                "use of `.unwrap_or_else(..)` to construct default value",
+                "try",
+                format!(
+                    "{}.unwrap_or_default()",
+                    snippet_with_applicability(cx, recv.span, "..", &mut applicability)
+                ),
+                applicability,
+            );
+        }
+    }
+}
diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs
@@ -168,7 +168,7 @@ pub fn snippet<'a, T: LintContext>(cx: &T, span: Span, default: &'a str) -> Cow<
     snippet_opt(cx, span).map_or_else(|| Cow::Borrowed(default), From::from)
 }
 
-/// Same as `snippet`, but it adapts the applicability level by following rules:
+/// Same as [`snippet`], but it adapts the applicability level by following rules:
 ///
 /// - Applicability level `Unspecified` will never be changed.
 /// - If the span is inside a macro, change the applicability level to `MaybeIncorrect`.
diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs
@@ -2,6 +2,7 @@
 
 #![allow(clippy::module_name_repetitions)]
 
+use hir::{HirId, QPath};
 use rustc_ast::ast::Mutability;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_hir as hir;
@@ -114,7 +115,7 @@ pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option<
 
 /// Checks whether a type implements a trait.
 /// The function returns false in case the type contains an inference variable.
-/// See also `get_trait_def_id`.
+/// See also [`get_trait_def_id`].
 pub fn implements_trait<'tcx>(
     cx: &LateContext<'tcx>,
     ty: Ty<'tcx>,
@@ -136,6 +137,21 @@ pub fn implements_trait<'tcx>(
     })
 }
 
+/// Gets the trait that a path targets. For example `<SomeTy as Trait>::a` would return the
+/// [`DefId`] for `Trait`.
+///
+/// `cx` must be in a body.
+pub fn qpath_target_trait<'tcx>(cx: &LateContext<'tcx>, qpath: &QPath<'_>, expr_id: HirId) -> Option<DefId> {
+    let method_res = cx.typeck_results().qpath_res(qpath, expr_id);
+    let method_id = match method_res {
+        hir::def::Res::Def(_kind, id) => Some(id),
+        _ => None,
+    };
+    let method_id = method_id?;
+
+    cx.tcx.trait_of_item(method_id)
+}
+
 /// Checks whether this type implements `Drop`.
 pub fn has_drop<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
     match ty.ty_adt_def() {
diff --git a/tests/ui/or_fun_call.fixed b/tests/ui/or_fun_call.fixed
@@ -18,6 +18,19 @@ fn or_fun_call() {
         }
     }
 
+    struct FakeDefault;
+    impl FakeDefault {
+        fn default() -> Self {
+            FakeDefault
+        }
+    }
+
+    impl Default for FakeDefault {
+        fn default() -> Self {
+            FakeDefault
+        }
+    }
+
     enum Enum {
         A(i32),
     }
@@ -53,6 +66,12 @@ fn or_fun_call() {
     let with_default_type = Some(1);
     with_default_type.unwrap_or_default();
 
+    let self_default = None::<FakeDefault>;
+    self_default.unwrap_or_else(<FakeDefault>::default);
+
+    let real_default = None::<FakeDefault>;
+    real_default.unwrap_or_default();
+
     let with_vec = Some(vec![1]);
     with_vec.unwrap_or_default();
 
diff --git a/tests/ui/or_fun_call.rs b/tests/ui/or_fun_call.rs
@@ -18,6 +18,19 @@ fn or_fun_call() {
         }
     }
 
+    struct FakeDefault;
+    impl FakeDefault {
+        fn default() -> Self {
+            FakeDefault
+        }
+    }
+
+    impl Default for FakeDefault {
+        fn default() -> Self {
+            FakeDefault
+        }
+    }
+
     enum Enum {
         A(i32),
     }
@@ -53,6 +66,12 @@ fn or_fun_call() {
     let with_default_type = Some(1);
     with_default_type.unwrap_or(u64::default());
 
+    let self_default = None::<FakeDefault>;
+    self_default.unwrap_or(<FakeDefault>::default());
+
+    let real_default = None::<FakeDefault>;
+    real_default.unwrap_or(<FakeDefault as Default>::default());
+
     let with_vec = Some(vec![1]);
     with_vec.unwrap_or(vec![]);
 
diff --git a/tests/ui/or_fun_call.stderr b/tests/ui/or_fun_call.stderr
diff --git a/tests/ui/unwrap_or_else_default.fixed b/tests/ui/unwrap_or_else_default.fixed
diff --git a/tests/ui/unwrap_or_else_default.rs b/tests/ui/unwrap_or_else_default.rs
diff --git a/tests/ui/unwrap_or_else_default.stderr b/tests/ui/unwrap_or_else_default.stderr

Original file line number	Diff line number	Diff line change
`@@ -168,7 +168,7 @@ pub fn snippet<'a, T: LintContext>(cx: &T, span: Span, default: &'a str) -> Cow<`
`168`	`168`	`snippet_opt(cx, span).map_or_else(\|\| Cow::Borrowed(default), From::from)`
`169`	`169`	`}`
`170`	`170`
`171`		-/// Same as `snippet`, but it adapts the applicability level by following rules:
	`171`	+/// Same as [`snippet`], but it adapts the applicability level by following rules:
`172`	`172`	`///`
`173`	`173`	/// - Applicability level `Unspecified` will never be changed.
`174`	`174`	/// - If the span is inside a macro, change the applicability level to `MaybeIncorrect`.