Add new `unnecessary_result_map_or_else` lint

4 files changed, 125 insertions, 1 deletions

diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
index 20230106d53..cab4647e08a 100644
--- a/clippy_lints/src/declared_lints.rs
+++ b/clippy_lints/src/declared_lints.rs
@@ -451,6 +451,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::UNNECESSARY_JOIN_INFO,
     crate::methods::UNNECESSARY_LAZY_EVALUATIONS_INFO,
     crate::methods::UNNECESSARY_LITERAL_UNWRAP_INFO,
+    crate::methods::UNNECESSARY_RESULT_MAP_OR_ELSE_INFO,
     crate::methods::UNNECESSARY_SORT_BY_INFO,
     crate::methods::UNNECESSARY_TO_OWNED_INFO,
     crate::methods::UNWRAP_OR_DEFAULT_INFO,
diff --git a/clippy_lints/src/methods/map_unwrap_or.rs b/clippy_lints/src/methods/map_unwrap_or.rs
index 52ea584a2c8..3226fa9cd3f 100644
--- a/clippy_lints/src/methods/map_unwrap_or.rs
+++ b/clippy_lints/src/methods/map_unwrap_or.rs
@@ -21,7 +21,7 @@ pub(super) fn check<'tcx>(
     unwrap_arg: &'tcx hir::Expr<'_>,
     msrv: &Msrv,
 ) -> bool {
-    // lint if the caller of `map()` is an `Option`
+    // lint if the caller of `map()` is an `Option` or a `Result`.
     let is_option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Option);
     let is_result = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Result);
 
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index 89ea3597dc0..f769696edc4 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -113,6 +113,7 @@ mod unnecessary_iter_cloned;
 mod unnecessary_join;
 mod unnecessary_lazy_eval;
 mod unnecessary_literal_unwrap;
+mod unnecessary_result_map_or_else;
 mod unnecessary_sort_by;
 mod unnecessary_to_owned;
 mod unwrap_expect_used;
@@ -3913,6 +3914,31 @@ declare_clippy_lint! {
     "cloning an `Option` via `as_ref().cloned()`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of `.map_or_else()` "map closure" for `Result` type.
+    ///
+    /// ### Why is this bad?
+    /// This can be written more concisely by using `unwrap_or_else()`.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// # fn handle_error(_: ()) -> u32 { 0 }
+    /// let x: Result<u32, ()> = Ok(0);
+    /// let y = x.map_or_else(|err| handle_error(err), |n| n);
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// # fn handle_error(_: ()) -> u32 { 0 }
+    /// let x: Result<u32, ()> = Ok(0);
+    /// let y = x.unwrap_or_else(|err| handle_error(err));
+    /// ```
+    #[clippy::version = "1.77.0"]
+    pub UNNECESSARY_RESULT_MAP_OR_ELSE,
+    suspicious,
+    "making no use of the \"map closure\" when calling `.map_or_else(|err| handle_error(err), |n| n)`"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -4070,6 +4096,7 @@ impl_lint_pass!(Methods => [
     MANUAL_IS_VARIANT_AND,
     STR_SPLIT_AT_NEWLINE,
     OPTION_AS_REF_CLONED,
+    UNNECESSARY_RESULT_MAP_OR_ELSE,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -4553,6 +4580,7 @@ impl Methods {
                 },
                 ("map_or_else", [def, map]) => {
                     result_map_or_else_none::check(cx, expr, recv, def, map);
+                    unnecessary_result_map_or_else::check(cx, expr, recv, def, map);
                 },
                 ("next", []) => {
                     if let Some((name2, recv2, args2, _, _)) = method_call(recv) {
diff --git a/clippy_lints/src/methods/unnecessary_result_map_or_else.rs b/clippy_lints/src/methods/unnecessary_result_map_or_else.rs
new file mode 100644
index 00000000000..7b0cf48ac43
--- /dev/null
+++ b/clippy_lints/src/methods/unnecessary_result_map_or_else.rs
@@ -0,0 +1,95 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::peel_blocks;
+use clippy_utils::source::snippet;
+use clippy_utils::ty::is_type_diagnostic_item;
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_hir::{Closure, Expr, ExprKind, HirId, QPath, Stmt, StmtKind};
+use rustc_lint::LateContext;
+use rustc_span::symbol::sym;
+
+use super::UNNECESSARY_RESULT_MAP_OR_ELSE;
+
+fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>) {
+    let msg = "unused \"map closure\" when calling `Result::map_or_else` value";
+    let self_snippet = snippet(cx, recv.span, "..");
+    let err_snippet = snippet(cx, def_arg.span, "..");
+    span_lint_and_sugg(
+        cx,
+        UNNECESSARY_RESULT_MAP_OR_ELSE,
+        expr.span,
+        msg,
+        "consider using `unwrap_or_else`",
+        format!("{self_snippet}.unwrap_or_else({err_snippet})"),
+        Applicability::MachineApplicable,
+    );
+}
+
+fn get_last_chain_binding_hir_id(mut hir_id: HirId, statements: &[Stmt<'_>]) -> Option<HirId> {
+    for stmt in statements {
+        if let StmtKind::Local(local) = stmt.kind
+            && let Some(init) = local.init
+            && let ExprKind::Path(QPath::Resolved(_, path)) = init.kind
+            && let hir::def::Res::Local(local_hir_id) = path.res
+            && local_hir_id == hir_id
+        {
+            hir_id = local.pat.hir_id;
+        } else {
+            return None;
+        }
+    }
+    Some(hir_id)
+}
+
+fn handle_qpath(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    recv: &Expr<'_>,
+    def_arg: &Expr<'_>,
+    expected_hir_id: HirId,
+    qpath: QPath<'_>,
+) {
+    if let QPath::Resolved(_, path) = qpath
+        && let hir::def::Res::Local(hir_id) = path.res
+        && expected_hir_id == hir_id
+    {
+        emit_lint(cx, expr, recv, def_arg);
+    }
+}
+
+/// lint use of `_.map_or_else(|err| err, |n| n)` for `Result`s.
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'_>,
+    recv: &'tcx Expr<'_>,
+    def_arg: &'tcx Expr<'_>,
+    map_arg: &'tcx Expr<'_>,
+) {
+    // lint if the caller of `map_or_else()` is a `Result`
+    if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Result)
+        && let ExprKind::Closure(&Closure { body, .. }) = map_arg.kind
+        && let body = cx.tcx.hir().body(body)
+        && let Some(first_param) = body.params.first()
+    {
+        let body_expr = peel_blocks(body.value);
+
+        match body_expr.kind {
+            ExprKind::Path(qpath) => {
+                handle_qpath(cx, expr, recv, def_arg, first_param.pat.hir_id, qpath);
+            },
+            // If this is a block (that wasn't peeled off), then it means there are statements.
+            ExprKind::Block(block, _) => {
+                if let Some(block_expr) = block.expr
+                    // First we ensure that this is a "binding chain" (each statement is a binding
+                    // of the previous one) and that it is a binding of the closure argument.
+                    && let Some(last_chain_binding_id) =
+                        get_last_chain_binding_hir_id(first_param.pat.hir_id, block.stmts)
+                    && let ExprKind::Path(qpath) = block_expr.kind
+                {
+                    handle_qpath(cx, expr, recv, def_arg, last_chain_binding_id, qpath);
+                }
+            },
+            _ => {},
+        }
+    }
+}