New lint: manual_ok_err (#13740)

changelog: [`manual_ok_err`]: new lint

Detect manual implementations of `.ok()` or `.err()`, as in

```rust
let a = match func() {
    Ok(v) => Some(v),
    Err(_) => None,
};
let b = if let Err(v) = func() {
    Some(v)
} else {
    None
};
```

which can be replaced by

```rust
let a = func().ok();
let b = func().err();
```

This pattern was detected in the wild in the Rust reimplementation of
coreutils:
uutils/coreutils#6886 (review)

CHANGELOG.md

@@ -5762,6 +5762,7 @@ Released 2018-09-13
 [`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
 [`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back
 [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
+[`manual_ok_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_err
 [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or
 [`manual_pattern_char_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_pattern_char_comparison
 [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains

clippy_lints/src/declared_lints.rs

@@ -335,6 +335,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::matches::INFALLIBLE_DESTRUCTURING_MATCH_INFO,
     crate::matches::MANUAL_FILTER_INFO,
     crate::matches::MANUAL_MAP_INFO,
+    crate::matches::MANUAL_OK_ERR_INFO,
     crate::matches::MANUAL_UNWRAP_OR_INFO,
     crate::matches::MATCH_AS_REF_INFO,
     crate::matches::MATCH_BOOL_INFO,

clippy_lints/src/matches/manual_ok_err.rs

@@ -0,0 +1,135 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::sugg::Sugg;
+use clippy_utils::ty::option_arg_ty;
+use clippy_utils::{is_res_lang_ctor, path_res, peel_blocks, span_contains_comment};
+use rustc_ast::BindingMode;
+use rustc_errors::Applicability;
+use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr};
+use rustc_hir::def::{DefKind, Res};
+use rustc_hir::{Arm, Expr, ExprKind, Pat, PatKind, Path, QPath};
+use rustc_lint::{LateContext, LintContext};
+use rustc_middle::ty::Ty;
+use rustc_span::symbol::Ident;
+
+use super::MANUAL_OK_ERR;
+
+pub(crate) fn check_if_let(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    let_pat: &Pat<'_>,
+    let_expr: &Expr<'_>,
+    if_then: &Expr<'_>,
+    else_expr: &Expr<'_>,
+) {
+    if let Some(inner_expr_ty) = option_arg_ty(cx, cx.typeck_results().expr_ty(expr))
+        && let Some((is_ok, ident)) = is_ok_or_err(cx, let_pat)
+        && is_some_ident(cx, if_then, ident, inner_expr_ty)
+        && is_none(cx, else_expr)
+    {
+        apply_lint(cx, expr, let_expr, is_ok);
+    }
+}
+
+pub(crate) fn check_match(cx: &LateContext<'_>, expr: &Expr<'_>, scrutinee: &Expr<'_>, arms: &[Arm<'_>]) {
+    if let Some(inner_expr_ty) = option_arg_ty(cx, cx.typeck_results().expr_ty(expr))
+        && arms.len() == 2
+        && arms.iter().all(|arm| arm.guard.is_none())
+        && let Some((idx, is_ok)) = arms.iter().enumerate().find_map(|(arm_idx, arm)| {
+            // Check if the arm is a `Ok(x) => x` or `Err(x) => x` alternative.
+            // In this case, return its index and whether it uses `Ok` or `Err`.
+             if let Some((is_ok, ident)) = is_ok_or_err(cx, arm.pat)
+                && is_some_ident(cx, arm.body, ident, inner_expr_ty)
+            {
+                Some((arm_idx, is_ok))
+            } else {
+                None
+            }
+        })
+        // Accept wildcard only as the second arm
+        && is_variant_or_wildcard(cx, arms[1-idx].pat, idx == 0, is_ok)
+        // Check that the body of the non `Ok`/`Err` arm is `None`
+        && is_none(cx, arms[1 - idx].body)
+    {
+        apply_lint(cx, expr, scrutinee, is_ok);
+    }
+}
+
+/// Check that `pat` applied to a `Result` only matches `Ok(_)`, `Err(_)`, not a subset or a
+/// superset of it. If `can_be_wild` is `true`, wildcards are also accepted. In the case of
+/// a non-wildcard, `must_match_err` indicates whether the `Err` or the `Ok` variant should be
+/// accepted.
+fn is_variant_or_wildcard(cx: &LateContext<'_>, pat: &Pat<'_>, can_be_wild: bool, must_match_err: bool) -> bool {
+    match pat.kind {
+        PatKind::Wild | PatKind::Path(..) | PatKind::Binding(_, _, _, None) if can_be_wild => true,
+        PatKind::TupleStruct(qpath, ..) => {
+            is_res_lang_ctor(cx, cx.qpath_res(&qpath, pat.hir_id), ResultErr) == must_match_err
+        },
+        PatKind::Binding(_, _, _, Some(pat)) | PatKind::Ref(pat, _) => {
+            is_variant_or_wildcard(cx, pat, can_be_wild, must_match_err)
+        },
+        _ => false,
+    }
+}
+
+/// Return `Some((true, IDENT))` if `pat` contains `Ok(IDENT)`, `Some((false, IDENT))` if it
+/// contains `Err(IDENT)`, `None` otherwise.
+fn is_ok_or_err<'hir>(cx: &LateContext<'_>, pat: &Pat<'hir>) -> Option<(bool, &'hir Ident)> {
+    if let PatKind::TupleStruct(qpath, [arg], _) = &pat.kind
+        && let PatKind::Binding(BindingMode::NONE, _, ident, _) = &arg.kind
+        && let res = cx.qpath_res(qpath, pat.hir_id)
+        && let Res::Def(DefKind::Ctor(..), id) = res
+        && let id @ Some(_) = cx.tcx.opt_parent(id)
+    {
+        let lang_items = cx.tcx.lang_items();
+        if id == lang_items.result_ok_variant() {
+            return Some((true, ident));
+        } else if id == lang_items.result_err_variant() {
+            return Some((false, ident));
+        }
+    }
+    None
+}
+
+/// Check if `expr` contains `Some(ident)`, possibly as a block
+fn is_some_ident<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, ident: &Ident, ty: Ty<'tcx>) -> bool {
+    if let ExprKind::Call(body_callee, [body_arg]) = peel_blocks(expr).kind
+        && is_res_lang_ctor(cx, path_res(cx, body_callee), OptionSome)
+        && cx.typeck_results().expr_ty(body_arg) == ty
+        && let ExprKind::Path(QPath::Resolved(
+            _,
+            Path {
+                segments: [segment], ..
+            },
+        )) = body_arg.kind
+    {
+        segment.ident.name == ident.name
+    } else {
+        false
+    }
+}
+
+/// Check if `expr` is `None`, possibly as a block
+fn is_none(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    is_res_lang_ctor(cx, path_res(cx, peel_blocks(expr)), OptionNone)
+}
+
+/// Suggest replacing `expr` by `scrutinee.METHOD()`, where `METHOD` is either `ok` or
+/// `err`, depending on `is_ok`.
+fn apply_lint(cx: &LateContext<'_>, expr: &Expr<'_>, scrutinee: &Expr<'_>, is_ok: bool) {
+    let method = if is_ok { "ok" } else { "err" };
+    let mut app = if span_contains_comment(cx.sess().source_map(), expr.span) {
+        Applicability::MaybeIncorrect
+    } else {
+        Applicability::MachineApplicable
+    };
+    let scrut = Sugg::hir_with_applicability(cx, scrutinee, "..", &mut app).maybe_par();
+    span_lint_and_sugg(
+        cx,
+        MANUAL_OK_ERR,
+        expr.span,
+        format!("manual implementation of `{method}`"),
+        "replace with",
+        format!("{scrut}.{method}()"),
+        app,
+    );
+}

clippy_lints/src/matches/mod.rs

@@ -2,6 +2,7 @@ mod collapsible_match;
 mod infallible_destructuring_match;
 mod manual_filter;
 mod manual_map;
+mod manual_ok_err;
 mod manual_unwrap_or;
 mod manual_utils;
 mod match_as_ref;
@@ -972,6 +973,40 @@ declare_clippy_lint! {
     "checks for unnecessary guards in match expressions"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for manual implementation of `.ok()` or `.err()`
+    /// on `Result` values.
+    ///
+    /// ### Why is this bad?
+    /// Using `.ok()` or `.err()` rather than a `match` or
+    /// `if let` is less complex and more readable.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// # fn func() -> Result<u32, &'static str> { Ok(0) }
+    /// let a = match func() {
+    ///     Ok(v) => Some(v),
+    ///     Err(_) => None,
+    /// };
+    /// let b = if let Err(v) = func() {
+    ///     Some(v)
+    /// } else {
+    ///     None
+    /// };
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// # fn func() -> Result<u32, &'static str> { Ok(0) }
+    /// let a = func().ok();
+    /// let b = func().err();
+    /// ```
+    #[clippy::version = "1.86.0"]
+    pub MANUAL_OK_ERR,
+    complexity,
+    "find manual implementations of `.ok()` or `.err()` on `Result`"
+}
+
 pub struct Matches {
     msrv: Msrv,
     infallible_destructuring_match_linted: bool,
@@ -1013,6 +1048,7 @@ impl_lint_pass!(Matches => [
     MANUAL_MAP,
     MANUAL_FILTER,
     REDUNDANT_GUARDS,
+    MANUAL_OK_ERR,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Matches {
@@ -1091,6 +1127,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
                         manual_unwrap_or::check_match(cx, expr, ex, arms);
                         manual_map::check_match(cx, expr, ex, arms);
                         manual_filter::check_match(cx, ex, arms, expr);
+                        manual_ok_err::check_match(cx, expr, ex, arms);
                     }
 
                     if self.infallible_destructuring_match_linted {
@@ -1134,6 +1171,14 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
                             if_let.if_then,
                             else_expr,
                         );
+                        manual_ok_err::check_if_let(
+                            cx,
+                            expr,
+                            if_let.let_pat,
+                            if_let.let_expr,
+                            if_let.if_then,
+                            else_expr,
+                        );
                     }
                 }
                 redundant_pattern_match::check_if_let(

clippy_utils/src/ty/mod.rs

@@ -1341,3 +1341,14 @@ pub fn get_field_by_name<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, name: Symbol) ->
         _ => None,
     }
 }
+
+/// Check if `ty` is an `Option` and return its argument type if it is.
+pub fn option_arg_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
+    match ty.kind() {
+        ty::Adt(adt, args) => cx
+            .tcx
+            .is_diagnostic_item(sym::Option, adt.did())
+            .then(|| args.type_at(0)),
+        _ => None,
+    }
+}

tests/ui/manual_ok_err.fixed

@@ -0,0 +1,91 @@
+#![warn(clippy::manual_ok_err)]
+
+fn funcall() -> Result<u32, &'static str> {
+    todo!()
+}
+
+fn main() {
+    let _ = funcall().ok();
+
+    let _ = funcall().ok();
+
+    let _ = funcall().err();
+
+    let _ = funcall().err();
+
+    let _ = funcall().ok();
+
+    let _ = funcall().err();
+
+    #[allow(clippy::redundant_pattern)]
+    let _ = funcall().ok();
+
+    struct S;
+
+    impl std::ops::Neg for S {
+        type Output = Result<u32, &'static str>;
+
+        fn neg(self) -> Self::Output {
+            funcall()
+        }
+    }
+
+    // Suggestion should be properly parenthesized
+    let _ = (-S).ok();
+
+    no_lint();
+}
+
+fn no_lint() {
+    let _ = match funcall() {
+        Ok(v) if v > 3 => Some(v),
+        _ => None,
+    };
+
+    let _ = match funcall() {
+        Err(_) => None,
+        Ok(3) => None,
+        Ok(v) => Some(v),
+    };
+
+    let _ = match funcall() {
+        _ => None,
+        Ok(v) => Some(v),
+    };
+
+    let _ = match funcall() {
+        Err(_) | Ok(3) => None,
+        Ok(v) => Some(v),
+    };
+
+    #[expect(clippy::redundant_pattern)]
+    let _ = match funcall() {
+        _v @ _ => None,
+        Ok(v) => Some(v),
+    };
+
+    // Content of `Option` and matching content of `Result` do
+    // not have the same type.
+    let _: Option<&dyn std::any::Any> = match Ok::<_, ()>(&1) {
+        Ok(v) => Some(v),
+        _ => None,
+    };
+
+    let _ = match Ok::<_, ()>(&1) {
+        _x => None,
+        Ok(v) => Some(v),
+    };
+
+    let _ = match Ok::<_, std::convert::Infallible>(1) {
+        Ok(3) => None,
+        Ok(v) => Some(v),
+    };
+}
+
+const fn cf(x: Result<u32, &'static str>) -> Option<u32> {
+    // Do not lint in const code
+    match x {
+        Ok(v) => Some(v),
+        Err(_) => None,
+    }
+}