Skip to content

Commit e9ea195

Browse files
samueltardieusamueltardieu
committed
New lint: manual_ok_err
1 parent b4163f0 commit e9ea195

File tree

7 files changed

+318
-0
lines changed

7 files changed

+318
-0
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5672,6 +5672,7 @@ Released 2018-09-13
56725672
[`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
56735673
[`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back
56745674
[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
5675+
[`manual_ok_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_err
56755676
[`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or
56765677
[`manual_pattern_char_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_pattern_char_comparison
56775678
[`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains

‎clippy_lints/src/declared_lints.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
331331
crate::matches::INFALLIBLE_DESTRUCTURING_MATCH_INFO,
332332
crate::matches::MANUAL_FILTER_INFO,
333333
crate::matches::MANUAL_MAP_INFO,
334+
crate::matches::MANUAL_OK_ERR_INFO,
334335
crate::matches::MANUAL_UNWRAP_OR_INFO,
335336
crate::matches::MATCH_AS_REF_INFO,
336337
crate::matches::MATCH_BOOL_INFO,

‎clippy_lints/src/matches/manual_ok_err.rs‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::source::snippet_with_applicability;
3+
use clippy_utils::{is_res_lang_ctor, path_res, peel_blocks};
4+
use rustc_ast::BindingMode;
5+
use rustc_errors::Applicability;
6+
use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk};
7+
use rustc_hir::def::{DefKind, Res};
8+
use rustc_hir::{Arm, Expr, ExprKind, Pat, PatKind, Path, QPath};
9+
use rustc_lint::{LateContext, LintContext};
10+
use rustc_span::symbol::Ident;
11+
12+
use super::MANUAL_OK_ERR;
13+
14+
pub(crate) fn check_if_let(
15+
cx: &LateContext<'_>,
16+
expr: &Expr<'_>,
17+
let_pat: &Pat<'_>,
18+
let_expr: &Expr<'_>,
19+
if_then: &Expr<'_>,
20+
else_expr: &Expr<'_>,
21+
) {
22+
if let Some((is_ok, ident)) = is_ok_or_err(cx, let_pat)
23+
&& is_some_ident(cx, if_then, ident)
24+
&& is_none(cx, else_expr)
25+
{
26+
apply_lint(cx, expr, let_expr, is_ok);
27+
}
28+
}
29+
30+
pub(crate) fn check_match(cx: &LateContext<'_>, expr: &Expr<'_>, scrutinee: &Expr<'_>, arms: &[Arm<'_>]) {
31+
if arms.len() == 2
32+
&& arms.iter().all(|arm| arm.guard.is_none())
33+
&& let Some((idx, is_ok)) = arms.iter().enumerate().find_map(|(arm_idx, arm)| {
34+
if let Some((is_ok, ident)) = is_ok_or_err(cx, arm.pat)
35+
&& is_some_ident(cx, arm.body, ident)
36+
{
37+
Some((arm_idx, is_ok))
38+
} else {
39+
None
40+
}
41+
})
42+
&& is_none(cx, arms[1 - idx].body)
43+
{
44+
apply_lint(cx, expr, scrutinee, is_ok);
45+
}
46+
}
47+
48+
/// Return `Some((true, IDENT))` if `pat` contains `Ok(IDENT)`, `Some((false, IDENT))` if it
49+
/// contains `Err(IDENT)`, `None` otherwise.
50+
fn is_ok_or_err<'hir>(cx: &LateContext<'_>, pat: &Pat<'hir>) -> Option<(bool, &'hir Ident)> {
51+
if let PatKind::TupleStruct(qpath, [arg], _) = &pat.kind
52+
&& let PatKind::Binding(BindingMode::NONE, _, ident, _) = &arg.kind
53+
&& pat.default_binding_modes
54+
&& let res = cx.qpath_res(qpath, pat.hir_id)
55+
&& let Res::Def(DefKind::Ctor(..), id) = res
56+
&& let id @ Some(_) = cx.tcx.opt_parent(id)
57+
{
58+
if id == cx.tcx.lang_items().get(ResultOk) {
59+
return Some((true, ident));
60+
} else if id == cx.tcx.lang_items().get(ResultErr) {
61+
return Some((false, ident));
62+
}
63+
}
64+
None
65+
}
66+
67+
/// Check if `expr` contains `Some(ident)`, possibly as a block
68+
fn is_some_ident(cx: &LateContext<'_>, expr: &Expr<'_>, ident: &Ident) -> bool {
69+
if let ExprKind::Call(body_callee, [body_arg]) = peel_blocks(expr).kind
70+
&& is_res_lang_ctor(cx, path_res(cx, body_callee), OptionSome)
71+
&& let ExprKind::Path(QPath::Resolved(
72+
_,
73+
Path {
74+
segments: [segment], ..
75+
},
76+
)) = body_arg.kind
77+
{
78+
segment.ident.name == ident.name
79+
} else {
80+
false
81+
}
82+
}
83+
84+
/// Check if `expr` is `None`, possibly as a block
85+
fn is_none(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
86+
is_res_lang_ctor(cx, path_res(cx, peel_blocks(expr)), OptionNone)
87+
}
88+
89+
/// Suggest replacing `expr` by `scrutinee.METHOD()`, where `METHOD` is either `ok` or
90+
/// `err`, depending on `is_ok`.
91+
fn apply_lint(cx: &LateContext<'_>, expr: &Expr<'_>, scrutinee: &Expr<'_>, is_ok: bool) {
92+
let method = if is_ok { "ok" } else { "err" };
93+
let mut app = Applicability::MachineApplicable;
94+
let snippet = snippet_with_applicability(cx.sess(), scrutinee.span, "..", &mut app);
95+
span_lint_and_sugg(
96+
cx,
97+
MANUAL_OK_ERR,
98+
expr.span,
99+
format!("manual implementation of `{method}`"),
100+
"replace with",
101+
format!("{snippet}.{method}()"),
102+
app,
103+
);
104+
}

‎clippy_lints/src/matches/mod.rs‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ mod collapsible_match;
22
mod infallible_destructuring_match;
33
mod manual_filter;
44
mod manual_map;
5+
mod manual_ok_err;
56
mod manual_unwrap_or;
67
mod manual_utils;
78
mod match_as_ref;
@@ -975,6 +976,40 @@ declare_clippy_lint! {
975976
"checks for unnecessary guards in match expressions"
976977
}
977978

979+
declare_clippy_lint! {
980+
/// ### What it does
981+
/// Checks for manual implementation of `.ok()` or `.err()`
982+
/// on `Result` values.
983+
///
984+
/// ### Why is this bad?
985+
/// Using `.ok()` or `.err()` rather than a `match` or
986+
/// `if let` is less complex and more readable.
987+
///
988+
/// ### Example
989+
/// ```no_run
990+
/// # fn func() -> Result<u32, &'static str> { Ok(0) }
991+
/// let a = match func() {
992+
/// Ok(v) => Some(v),
993+
/// Err(_) => None,
994+
/// };
995+
/// let b = if let Err(v) = func() {
996+
/// Some(v)
997+
/// } else {
998+
/// None
999+
/// };
1000+
/// ```
1001+
/// Use instead:
1002+
/// ```no_run
1003+
/// # fn func() -> Result<u32, &'static str> { Ok(0) }
1004+
/// let a = func().ok();
1005+
/// let b = func().err();
1006+
/// ```
1007+
#[clippy::version = "1.84.0"]
1008+
pub MANUAL_OK_ERR,
1009+
complexity,
1010+
"find manual implementations of `.ok()` or `.err()` on `Result`"
1011+
}
1012+
9781013
pub struct Matches {
9791014
msrv: Msrv,
9801015
infallible_destructuring_match_linted: bool,
@@ -1016,6 +1051,7 @@ impl_lint_pass!(Matches => [
10161051
MANUAL_MAP,
10171052
MANUAL_FILTER,
10181053
REDUNDANT_GUARDS,
1054+
MANUAL_OK_ERR,
10191055
]);
10201056

10211057
impl<'tcx> LateLintPass<'tcx> for Matches {
@@ -1073,6 +1109,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
10731109
manual_unwrap_or::check_match(cx, expr, ex, arms);
10741110
manual_map::check_match(cx, expr, ex, arms);
10751111
manual_filter::check_match(cx, ex, arms, expr);
1112+
manual_ok_err::check_match(cx, expr, ex, arms);
10761113
}
10771114

10781115
if self.infallible_destructuring_match_linted {
@@ -1116,6 +1153,14 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
11161153
if_let.if_then,
11171154
else_expr,
11181155
);
1156+
manual_ok_err::check_if_let(
1157+
cx,
1158+
expr,
1159+
if_let.let_pat,
1160+
if_let.let_expr,
1161+
if_let.if_then,
1162+
else_expr,
1163+
);
11191164
}
11201165
}
11211166
redundant_pattern_match::check_if_let(

‎tests/ui/manual_ok_err.fixed‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
#![warn(clippy::manual_ok_err)]
2+
3+
fn funcall() -> Result<u32, &'static str> {
4+
todo!()
5+
}
6+
7+
fn main() {
8+
let _ = funcall().ok();
9+
10+
let _ = funcall().ok();
11+
12+
let _ = funcall().err();
13+
14+
let _ = funcall().err();
15+
16+
let _ = funcall().ok();
17+
18+
let _ = funcall().err();
19+
20+
no_lint();
21+
}
22+
23+
fn no_lint() {
24+
let _ = match funcall() {
25+
Ok(v) if v > 3 => Some(v),
26+
_ => None,
27+
};
28+
29+
let _ = match funcall() {
30+
Err(_) => None,
31+
Ok(3) => None,
32+
Ok(v) => Some(v),
33+
};
34+
}

‎tests/ui/manual_ok_err.rs‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
#![warn(clippy::manual_ok_err)]
2+
3+
fn funcall() -> Result<u32, &'static str> {
4+
todo!()
5+
}
6+
7+
fn main() {
8+
let _ = match funcall() {
9+
//~^ manual_ok_err
10+
Ok(v) => Some(v),
11+
Err(_) => None,
12+
};
13+
14+
let _ = match funcall() {
15+
//~^ manual_ok_err
16+
Ok(v) => Some(v),
17+
_v => None,
18+
};
19+
20+
let _ = match funcall() {
21+
//~^ manual_ok_err
22+
Err(v) => Some(v),
23+
Ok(_) => None,
24+
};
25+
26+
let _ = match funcall() {
27+
//~^ manual_ok_err
28+
Err(v) => Some(v),
29+
_v => None,
30+
};
31+
32+
let _ = if let Ok(v) = funcall() {
33+
//~^ manual_ok_err
34+
Some(v)
35+
} else {
36+
None
37+
};
38+
39+
let _ = if let Err(v) = funcall() {
40+
//~^ manual_ok_err
41+
Some(v)
42+
} else {
43+
None
44+
};
45+
46+
no_lint();
47+
}
48+
49+
fn no_lint() {
50+
let _ = match funcall() {
51+
Ok(v) if v > 3 => Some(v),
52+
_ => None,
53+
};
54+
55+
let _ = match funcall() {
56+
Err(_) => None,
57+
Ok(3) => None,
58+
Ok(v) => Some(v),
59+
};
60+
}

‎tests/ui/manual_ok_err.stderr‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
error: manual implementation of `ok`
2+
--> tests/ui/manual_ok_err.rs:8:13
3+
|
4+
LL | let _ = match funcall() {
5+
| _____________^
6+
LL | |
7+
LL | | Ok(v) => Some(v),
8+
LL | | Err(_) => None,
9+
LL | | };
10+
| |_____^ help: replace with: `funcall().ok()`
11+
|
12+
= note: `-D clippy::manual-ok-err` implied by `-D warnings`
13+
= help: to override `-D warnings` add `#[allow(clippy::manual_ok_err)]`
14+
15+
error: manual implementation of `ok`
16+
--> tests/ui/manual_ok_err.rs:14:13
17+
|
18+
LL | let _ = match funcall() {
19+
| _____________^
20+
LL | |
21+
LL | | Ok(v) => Some(v),
22+
LL | | _v => None,
23+
LL | | };
24+
| |_____^ help: replace with: `funcall().ok()`
25+
26+
error: manual implementation of `err`
27+
--> tests/ui/manual_ok_err.rs:20:13
28+
|
29+
LL | let _ = match funcall() {
30+
| _____________^
31+
LL | |
32+
LL | | Err(v) => Some(v),
33+
LL | | Ok(_) => None,
34+
LL | | };
35+
| |_____^ help: replace with: `funcall().err()`
36+
37+
error: manual implementation of `err`
38+
--> tests/ui/manual_ok_err.rs:26:13
39+
|
40+
LL | let _ = match funcall() {
41+
| _____________^
42+
LL | |
43+
LL | | Err(v) => Some(v),
44+
LL | | _v => None,
45+
LL | | };
46+
| |_____^ help: replace with: `funcall().err()`
47+
48+
error: manual implementation of `ok`
49+
--> tests/ui/manual_ok_err.rs:32:13
50+
|
51+
LL | let _ = if let Ok(v) = funcall() {
52+
| _____________^
53+
LL | |
54+
LL | | Some(v)
55+
LL | | } else {
56+
LL | | None
57+
LL | | };
58+
| |_____^ help: replace with: `funcall().ok()`
59+
60+
error: manual implementation of `err`
61+
--> tests/ui/manual_ok_err.rs:39:13
62+
|
63+
LL | let _ = if let Err(v) = funcall() {
64+
| _____________^
65+
LL | |
66+
LL | | Some(v)
67+
LL | | } else {
68+
LL | | None
69+
LL | | };
70+
| |_____^ help: replace with: `funcall().err()`
71+
72+
error: aborting due to 6 previous errors
73+

0 commit comments

Comments
 (0)