Auto merge of #12169 - GuillaumeGomez:unnecessary_result_map_or_else, r=llogiq

Add new `unnecessary_result_map_or_else` lint

Fixes https://github.com/rust-lang/rust-clippy/issues/7328.

r? `@llogiq`

changelog: Add new `unnecessary_result_map_or_else` lint

CHANGELOG.md

@@ -5679,6 +5679,7 @@ Released 2018-09-13
 [`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed
 [`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
 [`unnecessary_owned_empty_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_owned_empty_strings
+[`unnecessary_result_map_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_result_map_or_else
 [`unnecessary_safety_comment`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment
 [`unnecessary_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_doc
 [`unnecessary_self_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_self_imports

clippy_lints/src/declared_lints.rs

@@ -453,6 +453,7 @@
     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,

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);
 

clippy_lints/src/methods/mod.rs

@@ -113,6 +113,7 @@
 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;
@@ -3951,6 +3952,31 @@
     "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,
@@ -4109,6 +4135,7 @@ pub fn new(
     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.
@@ -4592,6 +4619,7 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
                 },
                 ("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) {

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);
+                }
+            },
+            _ => {},
+        }
+    }
+}

tests/ui/unnecessary_result_map_or_else.fixed

@@ -0,0 +1,61 @@
+#![warn(clippy::unnecessary_result_map_or_else)]
+#![allow(clippy::unnecessary_literal_unwrap, clippy::let_and_return, clippy::let_unit_value)]
+
+fn main() {
+    let x: Result<(), ()> = Ok(());
+    x.unwrap_or_else(|err| err); //~ ERROR: unused "map closure" when calling
+
+    // Type ascribtion.
+    let x: Result<(), ()> = Ok(());
+    x.unwrap_or_else(|err: ()| err); //~ ERROR: unused "map closure" when calling
+
+    // Auto-deref.
+    let y = String::new();
+    let x: Result<&String, &String> = Ok(&y);
+    let y: &str = x.unwrap_or_else(|err| err); //~ ERROR: unused "map closure" when calling
+
+    // Temporary variable.
+    let x: Result<(), ()> = Ok(());
+    x.unwrap_or_else(|err| err);
+
+    // Should not warn.
+    let x: Result<usize, usize> = Ok(0);
+    x.map_or_else(|err| err, |n| n + 1);
+
+    // Should not warn.
+    let y = ();
+    let x: Result<(), ()> = Ok(());
+    x.map_or_else(|err| err, |_| y);
+
+    // Should not warn.
+    let y = ();
+    let x: Result<(), ()> = Ok(());
+    x.map_or_else(
+        |err| err,
+        |_| {
+            let tmp = y;
+            tmp
+        },
+    );
+
+    // Should not warn.
+    let x: Result<usize, usize> = Ok(1);
+    x.map_or_else(
+        |err| err,
+        |n| {
+            let tmp = n + 1;
+            tmp
+        },
+    );
+
+    // Should not warn.
+    let y = 0;
+    let x: Result<usize, usize> = Ok(1);
+    x.map_or_else(
+        |err| err,
+        |n| {
+            let tmp = n;
+            y
+        },
+    );
+}

tests/ui/unnecessary_result_map_or_else.rs

@@ -0,0 +1,69 @@
+#![warn(clippy::unnecessary_result_map_or_else)]
+#![allow(clippy::unnecessary_literal_unwrap, clippy::let_and_return, clippy::let_unit_value)]
+
+fn main() {
+    let x: Result<(), ()> = Ok(());
+    x.map_or_else(|err| err, |n| n); //~ ERROR: unused "map closure" when calling
+
+    // Type ascribtion.
+    let x: Result<(), ()> = Ok(());
+    x.map_or_else(|err: ()| err, |n: ()| n); //~ ERROR: unused "map closure" when calling
+
+    // Auto-deref.
+    let y = String::new();
+    let x: Result<&String, &String> = Ok(&y);
+    let y: &str = x.map_or_else(|err| err, |n| n); //~ ERROR: unused "map closure" when calling
+
+    // Temporary variable.
+    let x: Result<(), ()> = Ok(());
+    x.map_or_else(
+        //~^ ERROR: unused "map closure" when calling
+        |err| err,
+        |n| {
+            let tmp = n;
+            let tmp2 = tmp;
+            tmp2
+        },
+    );
+
+    // Should not warn.
+    let x: Result<usize, usize> = Ok(0);
+    x.map_or_else(|err| err, |n| n + 1);
+
+    // Should not warn.
+    let y = ();
+    let x: Result<(), ()> = Ok(());
+    x.map_or_else(|err| err, |_| y);
+
+    // Should not warn.
+    let y = ();
+    let x: Result<(), ()> = Ok(());
+    x.map_or_else(
+        |err| err,
+        |_| {
+            let tmp = y;
+            tmp
+        },
+    );
+
+    // Should not warn.
+    let x: Result<usize, usize> = Ok(1);
+    x.map_or_else(
+        |err| err,
+        |n| {
+            let tmp = n + 1;
+            tmp
+        },
+    );
+
+    // Should not warn.
+    let y = 0;
+    let x: Result<usize, usize> = Ok(1);
+    x.map_or_else(
+        |err| err,
+        |n| {
+            let tmp = n;
+            y
+        },
+    );
+}

tests/ui/unnecessary_result_map_or_else.stderr

@@ -0,0 +1,35 @@
+error: unused "map closure" when calling `Result::map_or_else` value
+  --> $DIR/unnecessary_result_map_or_else.rs:6:5
+   |
+LL |     x.map_or_else(|err| err, |n| n);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `x.unwrap_or_else(|err| err)`
+   |
+   = note: `-D clippy::unnecessary-result-map-or-else` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::unnecessary_result_map_or_else)]`
+
+error: unused "map closure" when calling `Result::map_or_else` value
+  --> $DIR/unnecessary_result_map_or_else.rs:10:5
+   |
+LL |     x.map_or_else(|err: ()| err, |n: ()| n);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `x.unwrap_or_else(|err: ()| err)`
+
+error: unused "map closure" when calling `Result::map_or_else` value
+  --> $DIR/unnecessary_result_map_or_else.rs:15:19
+   |
+LL |     let y: &str = x.map_or_else(|err| err, |n| n);
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `x.unwrap_or_else(|err| err)`
+
+error: unused "map closure" when calling `Result::map_or_else` value
+  --> $DIR/unnecessary_result_map_or_else.rs:19:5
+   |
+LL | /     x.map_or_else(
+LL | |
+LL | |         |err| err,
+LL | |         |n| {
+...  |
+LL | |         },
+LL | |     );
+   | |_____^ help: consider using `unwrap_or_else`: `x.unwrap_or_else(|err| err)`
+
+error: aborting due to 4 previous errors
+