Suggest is_ok/is_err for boolean Result mappings (#17537)

*[View all
comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust-clippy/pull/17537)*

changelog: [`unnecessary_map_or`]: suggest `Result::is_ok` and
`Result::is_err` for boolean `map_or` and `map_or_else` branches

Fixes #5718

## Summary

- recognize `Result::map_or` and `Result::map_or_else` calls whose
branches return opposite boolean literals without using their arguments
- suggest `is_ok()` when the `Ok` branch is `true`, and `is_err()` when
the `Err` branch is `true`
- keep rustfix machine-applicable when drop order is insignificant,
while downgrading the suggestion and explaining the difference when the
result or its temporaries need ordered drop
- extend the lint documentation and cover single-line, multiline,
rustfix, negative, and significant-drop cases

## Validation

- `TESTNAME=unnecessary_map_or cargo uitest`
- `cargo dev fmt --check`
- `cargo test`

Author: Manishearth

clippy_lints/src/methods/mod.rs

@@ -4453,37 +4453,41 @@ declare_clippy_lint! {
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Converts some constructs mapping an Enum value for equality comparison.
+    /// Converts some constructs mapping an enum value for equality or variant checks.
     ///
     /// ### Why is this bad?
     /// Calls such as `opt.map_or(false, |val| val == 5)` are needlessly long and cumbersome,
     /// and can be reduced to, for example, `opt == Some(5)` assuming `opt` implements `PartialEq`.
     /// Also, calls such as `opt.map_or(true, |val| val == 5)` can be reduced to
     /// `opt.is_none_or(|val| val == 5)`.
+    /// Calls that map the two variants of a `Result` to opposite boolean constants can be
+    /// reduced to `is_ok()` or `is_err()`.
     /// This lint offers readability and conciseness improvements.
     ///
     /// ### Example
     /// ```no_run
-    /// pub fn a(x: Option<i32>) -> (bool, bool) {
+    /// pub fn a(x: Option<i32>, result: Result<i32, i32>) -> (bool, bool, bool) {
     ///     (
     ///         x.map_or(false, |n| n == 5),
     ///         x.map_or(true, |n| n > 5),
+    ///         result.map_or_else(|_| false, |_| true),
     ///     )
     /// }
     /// ```
     /// Use instead:
     /// ```no_run
-    /// pub fn a(x: Option<i32>) -> (bool, bool) {
+    /// pub fn a(x: Option<i32>, result: Result<i32, i32>) -> (bool, bool, bool) {
     ///     (
     ///         x == Some(5),
     ///         x.is_none_or(|n| n > 5),
+    ///         result.is_ok(),
     ///     )
     /// }
     /// ```
     #[clippy::version = "1.84.0"]
     pub UNNECESSARY_MAP_OR,
     style,
-    "reduce unnecessary calls to `.map_or(bool, …)`"
+    "reduce unnecessary calls to `.map_or(bool, …)` and `.map_or_else(…, …)`"
 }
 
 declare_clippy_lint! {
@@ -5664,6 +5668,7 @@ impl Methods {
                 (sym::map_or_else, [def, map]) => {
                     result_map_or_else_none::check(cx, expr, recv, def, map);
                     unnecessary_map_or_else::check(cx, expr, recv, def, map, call_span);
+                    unnecessary_map_or::check_map_or_else(cx, expr, recv, def, map);
                 },
                 (sym::next, []) => {
                     if let Some((name2, recv2, args2, _, _)) = method_call(recv) {

clippy_lints/src/methods/unnecessary_map_or.rs

@@ -1,12 +1,13 @@
 use std::borrow::Cow;
 
+use clippy_utils::consts::{ConstEvalCtxt, Constant};
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::eager_or_lazy::switch_to_eager_eval;
 use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::res::{MaybeDef as _, MaybeResPath as _};
 use clippy_utils::sugg::{Sugg, make_binop};
-use clippy_utils::ty::{implements_trait, is_copy};
-use clippy_utils::visitors::is_local_used;
+use clippy_utils::ty::{implements_trait, is_copy, needs_ordered_drop};
+use clippy_utils::visitors::{any_temporaries_need_ordered_drop, is_local_used};
 use clippy_utils::{get_parent_expr, is_from_proc_macro};
 use rustc_ast::LitKind;
 use rustc_errors::Applicability;
@@ -36,15 +37,103 @@ impl Variant {
     }
 }
 
-pub(super) fn check<'a>(
-    cx: &LateContext<'a>,
-    expr: &Expr<'a>,
-    recv: &Expr<'_>,
-    def: &Expr<'_>,
-    map: &Expr<'_>,
+/// Evaluates `expr` and returns its value if it is a constant boolean.
+fn bool_constant(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<bool> {
+    let Some(Constant::Bool(value)) = ConstEvalCtxt::new(cx).eval(expr) else {
+        return None;
+    };
+    Some(value)
+}
+
+/// Returns the constant boolean produced by a one-parameter closure.
+fn closure_bool_constant(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<bool> {
+    let ExprKind::Closure(closure) = expr.kind else {
+        return None;
+    };
+    let body = cx.tcx.hir_body(closure.body);
+    let [_] = body.params else {
+        return None;
+    };
+    bool_constant(cx, body.value)
+}
+
+/// Checks whether a `Result::{map_or, map_or_else}` call is a variant query.
+///
+/// `expr` is the complete method call, `recv` is its `Result` receiver, `def` is the default
+/// argument, and `map` is the mapping closure. `check_if_bool` accounts for the eager default in
+/// `map_or` and the closure default in `map_or_else`.
+fn check_result_variant_query<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    recv: &'tcx Expr<'tcx>,
+    def: &'tcx Expr<'tcx>,
+    map: &'tcx Expr<'tcx>,
+    check_if_bool: impl FnOnce(&LateContext<'tcx>, &'tcx Expr<'tcx>) -> Option<bool>,
+) -> bool {
+    let ExprKind::MethodCall(path, _, _, call_span) = expr.kind else {
+        return false;
+    };
+    let recv_ty = cx.typeck_results().expr_ty_adjusted(recv);
+    if recv_ty.opt_diag_name(cx) != Some(sym::Result) {
+        return false;
+    }
+
+    let def_bool = check_if_bool(cx, def);
+    let Some((def_bool, map_bool)) = def_bool.zip(closure_bool_constant(cx, map)) else {
+        return false;
+    };
+    if def_bool == map_bool || is_from_proc_macro(cx, expr) {
+        return false;
+    }
+
+    let suggested_name = if map_bool { "is_ok" } else { "is_err" };
+    let changes_drop_order = needs_ordered_drop(cx, recv_ty) || any_temporaries_need_ordered_drop(cx, recv);
+    let applicability = if changes_drop_order {
+        Applicability::MaybeIncorrect
+    } else {
+        Applicability::MachineApplicable
+    };
+
+    span_lint_and_then(
+        cx,
+        UNNECESSARY_MAP_OR,
+        path.ident.span,
+        format!("this `{}` can be simplified", path.ident.name),
+        |diag| {
+            diag.span_suggestion(
+                call_span,
+                format!("use `{suggested_name}` instead"),
+                format!("{suggested_name}()"),
+                applicability,
+            );
+            if changes_drop_order {
+                diag.note("this will change drop order of the result, as well as all temporaries");
+                diag.note("add `#[allow(clippy::unnecessary_map_or)]` if this is important");
+            }
+        },
+    );
+    true
+}
+
+/// Checks a `map_or` call for both `Result` variant queries and the existing `Option`/`Result`
+/// simplifications.
+///
+/// `expr` is the complete method call, `recv` is its receiver, `def` is the eager default, and
+/// `map` is the mapping closure. `method_span` identifies `map_or` in diagnostics, while `msrv`
+/// controls which replacement methods can be suggested.
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    recv: &'tcx Expr<'tcx>,
+    def: &'tcx Expr<'tcx>,
+    map: &'tcx Expr<'tcx>,
     method_span: Span,
     msrv: Msrv,
 ) {
+    if check_result_variant_query(cx, expr, recv, def, map, bool_constant) {
+        return;
+    }
+
     let ExprKind::Lit(def_kind) = def.kind else {
         return;
     };
@@ -158,3 +247,17 @@ pub(super) fn check<'a>(
         },
     );
 }
+
+/// Checks a `map_or_else` call for a `Result` variant query.
+///
+/// `expr` is the complete method call, `recv` is its `Result` receiver, `def` is the lazy default
+/// closure, and `map` is the mapping closure.
+pub(super) fn check_map_or_else<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    recv: &'tcx Expr<'tcx>,
+    def: &'tcx Expr<'tcx>,
+    map: &'tcx Expr<'tcx>,
+) {
+    check_result_variant_query(cx, expr, recv, def, map, closure_bool_constant);
+}

tests/ui/unnecessary_map_or_result_bool.fixed

@@ -0,0 +1,51 @@
+//@aux-build:proc_macros.rs
+#![warn(clippy::unnecessary_map_or)]
+
+#[macro_use]
+extern crate proc_macros;
+
+const TRUE: bool = true;
+const FALSE: bool = false;
+
+fn main() {
+    let result = Ok::<i32, i32>(1);
+
+    let _ = result.is_ok();
+    //~^ unnecessary_map_or
+    let _ = result.is_err();
+    //~^ unnecessary_map_or
+    let _ = result.is_ok();
+    //~^ unnecessary_map_or
+    let _ = result.is_ok();
+    //~^ unnecessary_map_or
+
+    let _ = result.is_ok();
+    //~^ unnecessary_map_or
+    let _ = result.is_err();
+    //~^ unnecessary_map_or
+    let _ = result.is_ok();
+    //~^ unnecessary_map_or
+    let _ = result.is_ok();
+    //~^ unnecessary_map_or
+
+    // Calls in a closure body may have side effects. The lint does not inspect the callee body.
+    let _ = result.map_or_else(
+        |_| {
+            std::hint::black_box(());
+            false
+        },
+        |_| true,
+    );
+    let _ = result.map_or_else(|error| error > 0, |_| true);
+    let _ = result.map_or_else(|_| false, |value| value > 0);
+
+    external! {
+        let _ = Ok::<i32, i32>(1).map_or(false, |_| true);
+        let _ = Ok::<i32, i32>(1).map_or_else(|_| false, |_| true);
+    }
+
+    with_span! {
+        let _ = Ok::<i32, i32>(1).map_or(false, |_| true);
+        let _ = Ok::<i32, i32>(1).map_or_else(|_| false, |_| true);
+    }
+}

tests/ui/unnecessary_map_or_result_bool.rs

@@ -0,0 +1,51 @@
+//@aux-build:proc_macros.rs
+#![warn(clippy::unnecessary_map_or)]
+
+#[macro_use]
+extern crate proc_macros;
+
+const TRUE: bool = true;
+const FALSE: bool = false;
+
+fn main() {
+    let result = Ok::<i32, i32>(1);
+
+    let _ = result.map_or(false, |_| true);
+    //~^ unnecessary_map_or
+    let _ = result.map_or(true, |_| false);
+    //~^ unnecessary_map_or
+    let _ = result.map_or(false, |_: i32| true);
+    //~^ unnecessary_map_or
+    let _ = result.map_or(!true, |_| TRUE);
+    //~^ unnecessary_map_or
+
+    let _ = result.map_or_else(|_| false, |_| true);
+    //~^ unnecessary_map_or
+    let _ = result.map_or_else(|_| true, |_| false);
+    //~^ unnecessary_map_or
+    let _ = result.map_or_else(|_: i32| false, |_: i32| true);
+    //~^ unnecessary_map_or
+    let _ = result.map_or_else(|_| FALSE, |_| !false);
+    //~^ unnecessary_map_or
+
+    // Calls in a closure body may have side effects. The lint does not inspect the callee body.
+    let _ = result.map_or_else(
+        |_| {
+            std::hint::black_box(());
+            false
+        },
+        |_| true,
+    );
+    let _ = result.map_or_else(|error| error > 0, |_| true);
+    let _ = result.map_or_else(|_| false, |value| value > 0);
+
+    external! {
+        let _ = Ok::<i32, i32>(1).map_or(false, |_| true);
+        let _ = Ok::<i32, i32>(1).map_or_else(|_| false, |_| true);
+    }
+
+    with_span! {
+        let _ = Ok::<i32, i32>(1).map_or(false, |_| true);
+        let _ = Ok::<i32, i32>(1).map_or_else(|_| false, |_| true);
+    }
+}

tests/ui/unnecessary_map_or_result_bool.stderr

@@ -0,0 +1,69 @@
+error: this `map_or` can be simplified
+  --> tests/ui/unnecessary_map_or_result_bool.rs:13:20
+   |
+LL |     let _ = result.map_or(false, |_| true);
+   |                    ^^^^^^-----------------
+   |                    |
+   |                    help: use `is_ok` instead: `is_ok()`
+   |
+   = note: `-D clippy::unnecessary-map-or` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::unnecessary_map_or)]`
+
+error: this `map_or` can be simplified
+  --> tests/ui/unnecessary_map_or_result_bool.rs:15:20
+   |
+LL |     let _ = result.map_or(true, |_| false);
+   |                    ^^^^^^-----------------
+   |                    |
+   |                    help: use `is_err` instead: `is_err()`
+
+error: this `map_or` can be simplified
+  --> tests/ui/unnecessary_map_or_result_bool.rs:17:20
+   |
+LL |     let _ = result.map_or(false, |_: i32| true);
+   |                    ^^^^^^----------------------
+   |                    |
+   |                    help: use `is_ok` instead: `is_ok()`
+
+error: this `map_or` can be simplified
+  --> tests/ui/unnecessary_map_or_result_bool.rs:19:20
+   |
+LL |     let _ = result.map_or(!true, |_| TRUE);
+   |                    ^^^^^^-----------------
+   |                    |
+   |                    help: use `is_ok` instead: `is_ok()`
+
+error: this `map_or_else` can be simplified
+  --> tests/ui/unnecessary_map_or_result_bool.rs:22:20
+   |
+LL |     let _ = result.map_or_else(|_| false, |_| true);
+   |                    ^^^^^^^^^^^---------------------
+   |                    |
+   |                    help: use `is_ok` instead: `is_ok()`
+
+error: this `map_or_else` can be simplified
+  --> tests/ui/unnecessary_map_or_result_bool.rs:24:20
+   |
+LL |     let _ = result.map_or_else(|_| true, |_| false);
+   |                    ^^^^^^^^^^^---------------------
+   |                    |
+   |                    help: use `is_err` instead: `is_err()`
+
+error: this `map_or_else` can be simplified
+  --> tests/ui/unnecessary_map_or_result_bool.rs:26:20
+   |
+LL |     let _ = result.map_or_else(|_: i32| false, |_: i32| true);
+   |                    ^^^^^^^^^^^-------------------------------
+   |                    |
+   |                    help: use `is_ok` instead: `is_ok()`
+
+error: this `map_or_else` can be simplified
+  --> tests/ui/unnecessary_map_or_result_bool.rs:28:20
+   |
+LL |     let _ = result.map_or_else(|_| FALSE, |_| !false);
+   |                    ^^^^^^^^^^^-----------------------
+   |                    |
+   |                    help: use `is_ok` instead: `is_ok()`
+
+error: aborting due to 8 previous errors
+

tests/ui/unnecessary_map_or_result_bool_unfixable.rs

@@ -0,0 +1,14 @@
+//@no-rustfix: `is_ok` and `is_err` can change significant drop order
+#![warn(clippy::unnecessary_map_or)]
+
+fn main() {
+    let mutex = std::sync::Mutex::new(());
+
+    let result = Ok::<_, ()>(mutex.lock().unwrap());
+    let _ = result.map_or(false, |_| true);
+    //~^ unnecessary_map_or
+
+    let result = Err::<(), _>(mutex.lock().unwrap());
+    let _ = result.map_or_else(|_| true, |_| false);
+    //~^ unnecessary_map_or
+}