Skip to content

Commit

Permalink
New lint: manual_ok_err
Browse files Browse the repository at this point in the history
  • Loading branch information
samueltardieu committed Dec 15, 2024
1 parent 650e0c8 commit 3a88396
Show file tree
Hide file tree
Showing 7 changed files with 448 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5717,6 +5717,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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,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,
Expand Down
124 changes: 124 additions & 0 deletions clippy_lints/src/matches/manual_ok_err.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::sugg::Sugg;
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};
use rustc_hir::def::{DefKind, Res};
use rustc_hir::{Arm, Expr, ExprKind, Pat, PatKind, Path, QPath};
use rustc_lint::{LateContext, LintContext};
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((is_ok, ident)) = is_ok_or_err(cx, let_pat)
&& is_some_ident(cx, if_then, ident)
&& 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 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)
{
Some((arm_idx, is_ok))
} else {
None
}
})
// Accept wildcard only as the second arm
&& is_variant_or_wildcard(arms[1-idx].pat, idx == 0)
// 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.
fn is_variant_or_wildcard(pat: &Pat<'_>, can_be_wild: bool) -> bool {
match pat.kind {
PatKind::Wild | PatKind::Path(..) if can_be_wild => true,
PatKind::TupleStruct(..) | PatKind::Struct(..) | PatKind::Binding(_, _, _, None) => true,
PatKind::Binding(_, _, _, Some(pat)) | PatKind::Ref(pat, _) => is_variant_or_wildcard(pat, can_be_wild),
_ => 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(cx: &LateContext<'_>, expr: &Expr<'_>, ident: &Ident) -> bool {
if let ExprKind::Call(body_callee, [body_arg]) = peel_blocks(expr).kind
&& is_res_lang_ctor(cx, path_res(cx, body_callee), OptionSome)
&& 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,
);
}
45 changes: 45 additions & 0 deletions clippy_lints/src/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -975,6 +976,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.85.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,
Expand Down Expand Up @@ -1016,6 +1051,7 @@ impl_lint_pass!(Matches => [
MANUAL_MAP,
MANUAL_FILTER,
REDUNDANT_GUARDS,
MANUAL_OK_ERR,
]);

impl<'tcx> LateLintPass<'tcx> for Matches {
Expand Down Expand Up @@ -1073,6 +1109,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 {
Expand Down Expand Up @@ -1116,6 +1153,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(
Expand Down
74 changes: 74 additions & 0 deletions tests/ui/manual_ok_err.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
#![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),
};
}

const fn cf(x: Result<u32, &'static str>) -> Option<u32> {
// Do not lint in const code
match x {
Ok(v) => Some(v),
Err(_) => None,
}
}
Loading

0 comments on commit 3a88396

Please sign in to comment.