Skip to content

Commit 225ce5f

Browse files
committed
Auto merge of #6233 - montrivo:manual_ok_or, r=flip1995
add manual_ok_or lint Implements partially #5923 changelog: add lint manual_ok_or
2 parents 2fe87a8 + d780f61 commit 225ce5f

File tree

7 files changed

+236
-0
lines changed

7 files changed

+236
-0
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -1797,6 +1797,7 @@ Released 2018-09-13
17971797
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
17981798
[`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
17991799
[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
1800+
[`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or
18001801
[`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
18011802
[`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
18021803
[`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip

clippy_lints/src/lib.rs

+4
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,7 @@ mod macro_use;
234234
mod main_recursion;
235235
mod manual_async_fn;
236236
mod manual_non_exhaustive;
237+
mod manual_ok_or;
237238
mod manual_strip;
238239
mod manual_unwrap_or;
239240
mod map_clone;
@@ -651,6 +652,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
651652
&main_recursion::MAIN_RECURSION,
652653
&manual_async_fn::MANUAL_ASYNC_FN,
653654
&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
655+
&manual_ok_or::MANUAL_OK_OR,
654656
&manual_strip::MANUAL_STRIP,
655657
&manual_unwrap_or::MANUAL_UNWRAP_OR,
656658
&map_clone::MAP_CLONE,
@@ -1152,6 +1154,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
11521154
store.register_late_pass(|| box unwrap_in_result::UnwrapInResult);
11531155
store.register_late_pass(|| box self_assignment::SelfAssignment);
11541156
store.register_late_pass(|| box manual_unwrap_or::ManualUnwrapOr);
1157+
store.register_late_pass(|| box manual_ok_or::ManualOkOr);
11551158
store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs);
11561159
store.register_late_pass(|| box async_yields_async::AsyncYieldsAsync);
11571160
store.register_late_pass(|| box manual_strip::ManualStrip);
@@ -1240,6 +1243,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
12401243
LintId::of(&loops::EXPLICIT_INTO_ITER_LOOP),
12411244
LintId::of(&loops::EXPLICIT_ITER_LOOP),
12421245
LintId::of(&macro_use::MACRO_USE_IMPORTS),
1246+
LintId::of(&manual_ok_or::MANUAL_OK_OR),
12431247
LintId::of(&map_err_ignore::MAP_ERR_IGNORE),
12441248
LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS),
12451249
LintId::of(&matches::MATCH_BOOL),

clippy_lints/src/manual_ok_or.rs

+99
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
use crate::utils::{
2+
indent_of, is_type_diagnostic_item, match_qpath, paths, reindent_multiline, snippet_opt, span_lint_and_sugg,
3+
};
4+
use if_chain::if_chain;
5+
use rustc_errors::Applicability;
6+
use rustc_hir::{def, Expr, ExprKind, PatKind, QPath};
7+
use rustc_lint::LintContext;
8+
use rustc_lint::{LateContext, LateLintPass};
9+
use rustc_middle::lint::in_external_macro;
10+
use rustc_session::{declare_lint_pass, declare_tool_lint};
11+
12+
declare_clippy_lint! {
13+
/// **What it does:**
14+
/// Finds patterns that reimplement `Option::ok_or`.
15+
///
16+
/// **Why is this bad?**
17+
/// Concise code helps focusing on behavior instead of boilerplate.
18+
///
19+
/// **Known problems:** None.
20+
///
21+
/// **Examples:**
22+
/// ```rust
23+
/// let foo: Option<i32> = None;
24+
/// foo.map_or(Err("error"), |v| Ok(v));
25+
///
26+
/// let foo: Option<i32> = None;
27+
/// foo.map_or(Err("error"), |v| Ok(v));
28+
/// ```
29+
///
30+
/// Use instead:
31+
/// ```rust
32+
/// let foo: Option<i32> = None;
33+
/// foo.ok_or("error");
34+
/// ```
35+
pub MANUAL_OK_OR,
36+
pedantic,
37+
"finds patterns that can be encoded more concisely with `Option::ok_or`"
38+
}
39+
40+
declare_lint_pass!(ManualOkOr => [MANUAL_OK_OR]);
41+
42+
impl LateLintPass<'_> for ManualOkOr {
43+
fn check_expr(&mut self, cx: &LateContext<'tcx>, scrutinee: &'tcx Expr<'tcx>) {
44+
if in_external_macro(cx.sess(), scrutinee.span) {
45+
return;
46+
}
47+
48+
if_chain! {
49+
if let ExprKind::MethodCall(method_segment, _, args, _) = scrutinee.kind;
50+
if method_segment.ident.name == sym!(map_or);
51+
if args.len() == 3;
52+
let method_receiver = &args[0];
53+
let ty = cx.typeck_results().expr_ty(method_receiver);
54+
if is_type_diagnostic_item(cx, ty, sym!(option_type));
55+
let or_expr = &args[1];
56+
if is_ok_wrapping(cx, &args[2]);
57+
if let ExprKind::Call(Expr { kind: ExprKind::Path(err_path), .. }, &[ref err_arg]) = or_expr.kind;
58+
if match_qpath(err_path, &paths::RESULT_ERR);
59+
if let Some(method_receiver_snippet) = snippet_opt(cx, method_receiver.span);
60+
if let Some(err_arg_snippet) = snippet_opt(cx, err_arg.span);
61+
if let Some(indent) = indent_of(cx, scrutinee.span);
62+
then {
63+
let reindented_err_arg_snippet =
64+
reindent_multiline(err_arg_snippet.into(), true, Some(indent + 4));
65+
span_lint_and_sugg(
66+
cx,
67+
MANUAL_OK_OR,
68+
scrutinee.span,
69+
"this pattern reimplements `Option::ok_or`",
70+
"replace with",
71+
format!(
72+
"{}.ok_or({})",
73+
method_receiver_snippet,
74+
reindented_err_arg_snippet
75+
),
76+
Applicability::MachineApplicable,
77+
);
78+
}
79+
}
80+
}
81+
}
82+
83+
fn is_ok_wrapping(cx: &LateContext<'_>, map_expr: &Expr<'_>) -> bool {
84+
if let ExprKind::Path(ref qpath) = map_expr.kind {
85+
if match_qpath(qpath, &paths::RESULT_OK) {
86+
return true;
87+
}
88+
}
89+
if_chain! {
90+
if let ExprKind::Closure(_, _, body_id, ..) = map_expr.kind;
91+
let body = cx.tcx.hir().body(body_id);
92+
if let PatKind::Binding(_, param_id, ..) = body.params[0].pat.kind;
93+
if let ExprKind::Call(Expr { kind: ExprKind::Path(ok_path), .. }, &[ref ok_arg]) = body.value.kind;
94+
if match_qpath(ok_path, &paths::RESULT_OK);
95+
if let ExprKind::Path(QPath::Resolved(_, ok_arg_path)) = ok_arg.kind;
96+
if let def::Res::Local(ok_arg_path_id) = ok_arg_path.res;
97+
then { param_id == ok_arg_path_id } else { false }
98+
}
99+
}

src/lintlist/mod.rs

+7
Original file line numberDiff line numberDiff line change
@@ -1187,6 +1187,13 @@ vec![
11871187
deprecation: None,
11881188
module: "manual_non_exhaustive",
11891189
},
1190+
Lint {
1191+
name: "manual_ok_or",
1192+
group: "pedantic",
1193+
desc: "finds patterns that can be encoded more concisely with `Option::ok_or`",
1194+
deprecation: None,
1195+
module: "manual_ok_or",
1196+
},
11901197
Lint {
11911198
name: "manual_range_contains",
11921199
group: "style",

tests/ui/manual_ok_or.fixed

+40
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// run-rustfix
2+
#![warn(clippy::manual_ok_or)]
3+
#![allow(clippy::blacklisted_name)]
4+
#![allow(clippy::redundant_closure)]
5+
#![allow(dead_code)]
6+
#![allow(unused_must_use)]
7+
8+
fn main() {
9+
// basic case
10+
let foo: Option<i32> = None;
11+
foo.ok_or("error");
12+
13+
// eta expansion case
14+
foo.ok_or("error");
15+
16+
// turbo fish syntax
17+
None::<i32>.ok_or("error");
18+
19+
// multiline case
20+
#[rustfmt::skip]
21+
foo.ok_or(&format!(
22+
"{}{}{}{}{}{}{}",
23+
"Alice", "Bob", "Sarah", "Marc", "Sandra", "Eric", "Jenifer"));
24+
25+
// not applicable, closure isn't direct `Ok` wrapping
26+
foo.map_or(Err("error"), |v| Ok(v + 1));
27+
28+
// not applicable, or side isn't `Result::Err`
29+
foo.map_or(Ok::<i32, &str>(1), |v| Ok(v));
30+
31+
// not applicatble, expr is not a `Result` value
32+
foo.map_or(42, |v| v);
33+
34+
// TODO patterns not covered yet
35+
match foo {
36+
Some(v) => Ok(v),
37+
None => Err("error"),
38+
};
39+
foo.map_or_else(|| Err("error"), |v| Ok(v));
40+
}

tests/ui/manual_ok_or.rs

+44
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
// run-rustfix
2+
#![warn(clippy::manual_ok_or)]
3+
#![allow(clippy::blacklisted_name)]
4+
#![allow(clippy::redundant_closure)]
5+
#![allow(dead_code)]
6+
#![allow(unused_must_use)]
7+
8+
fn main() {
9+
// basic case
10+
let foo: Option<i32> = None;
11+
foo.map_or(Err("error"), |v| Ok(v));
12+
13+
// eta expansion case
14+
foo.map_or(Err("error"), Ok);
15+
16+
// turbo fish syntax
17+
None::<i32>.map_or(Err("error"), |v| Ok(v));
18+
19+
// multiline case
20+
#[rustfmt::skip]
21+
foo.map_or(Err::<i32, &str>(
22+
&format!(
23+
"{}{}{}{}{}{}{}",
24+
"Alice", "Bob", "Sarah", "Marc", "Sandra", "Eric", "Jenifer")
25+
),
26+
|v| Ok(v),
27+
);
28+
29+
// not applicable, closure isn't direct `Ok` wrapping
30+
foo.map_or(Err("error"), |v| Ok(v + 1));
31+
32+
// not applicable, or side isn't `Result::Err`
33+
foo.map_or(Ok::<i32, &str>(1), |v| Ok(v));
34+
35+
// not applicatble, expr is not a `Result` value
36+
foo.map_or(42, |v| v);
37+
38+
// TODO patterns not covered yet
39+
match foo {
40+
Some(v) => Ok(v),
41+
None => Err("error"),
42+
};
43+
foo.map_or_else(|| Err("error"), |v| Ok(v));
44+
}

tests/ui/manual_ok_or.stderr

+41
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
error: this pattern reimplements `Option::ok_or`
2+
--> $DIR/manual_ok_or.rs:11:5
3+
|
4+
LL | foo.map_or(Err("error"), |v| Ok(v));
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `foo.ok_or("error")`
6+
|
7+
= note: `-D clippy::manual-ok-or` implied by `-D warnings`
8+
9+
error: this pattern reimplements `Option::ok_or`
10+
--> $DIR/manual_ok_or.rs:14:5
11+
|
12+
LL | foo.map_or(Err("error"), Ok);
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `foo.ok_or("error")`
14+
15+
error: this pattern reimplements `Option::ok_or`
16+
--> $DIR/manual_ok_or.rs:17:5
17+
|
18+
LL | None::<i32>.map_or(Err("error"), |v| Ok(v));
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `None::<i32>.ok_or("error")`
20+
21+
error: this pattern reimplements `Option::ok_or`
22+
--> $DIR/manual_ok_or.rs:21:5
23+
|
24+
LL | / foo.map_or(Err::<i32, &str>(
25+
LL | | &format!(
26+
LL | | "{}{}{}{}{}{}{}",
27+
LL | | "Alice", "Bob", "Sarah", "Marc", "Sandra", "Eric", "Jenifer")
28+
LL | | ),
29+
LL | | |v| Ok(v),
30+
LL | | );
31+
| |_____^
32+
|
33+
help: replace with
34+
|
35+
LL | foo.ok_or(&format!(
36+
LL | "{}{}{}{}{}{}{}",
37+
LL | "Alice", "Bob", "Sarah", "Marc", "Sandra", "Eric", "Jenifer"));
38+
|
39+
40+
error: aborting due to 4 previous errors
41+

0 commit comments

Comments
 (0)