Skip to content

Commit 0c250c9

Browse files
committed
Add lint unnecessary_option_map_or_else
1 parent 48430c2 commit 0c250c9

13 files changed

+308
-65
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6292,6 +6292,7 @@ Released 2018-09-13
62926292
[`unnecessary_min_or_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_min_or_max
62936293
[`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed
62946294
[`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
6295+
[`unnecessary_option_map_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_option_map_or_else
62956296
[`unnecessary_owned_empty_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_owned_empty_strings
62966297
[`unnecessary_result_map_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_result_map_or_else
62976298
[`unnecessary_safety_comment`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment

clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
477477
crate::methods::UNNECESSARY_LITERAL_UNWRAP_INFO,
478478
crate::methods::UNNECESSARY_MAP_OR_INFO,
479479
crate::methods::UNNECESSARY_MIN_OR_MAX_INFO,
480+
crate::methods::UNNECESSARY_OPTION_MAP_OR_ELSE_INFO,
480481
crate::methods::UNNECESSARY_RESULT_MAP_OR_ELSE_INFO,
481482
crate::methods::UNNECESSARY_SORT_BY_INFO,
482483
crate::methods::UNNECESSARY_TO_OWNED_INFO,

clippy_lints/src/methods/mod.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ mod unnecessary_lazy_eval;
130130
mod unnecessary_literal_unwrap;
131131
mod unnecessary_map_or;
132132
mod unnecessary_min_or_max;
133+
mod unnecessary_option_map_or_else;
133134
mod unnecessary_result_map_or_else;
134135
mod unnecessary_sort_by;
135136
mod unnecessary_to_owned;
@@ -4529,6 +4530,31 @@ declare_clippy_lint! {
45294530
"detect swap with a temporary value"
45304531
}
45314532

4533+
declare_clippy_lint! {
4534+
/// ### What it does
4535+
/// Checks for usage of `.map_or_else()` "map closure" for `Option` type.
4536+
///
4537+
/// ### Why is this bad?
4538+
/// This can be written more concisely by using `unwrap_or_else()`.
4539+
///
4540+
/// ### Example
4541+
/// ```no_run
4542+
/// let k = 10;
4543+
/// let x: Option<u32> = Some(4);
4544+
/// let y = x.map_or_else(|| 2 * k, |n| n);
4545+
/// ```
4546+
/// Use instead:
4547+
/// ```no_run
4548+
/// let k = 10;
4549+
/// let x: Option<u32> = Some(4);
4550+
/// let y = x.unwrap_or_else(|| 2 * k);
4551+
/// ```
4552+
#[clippy::version = "1.88.0"]
4553+
pub UNNECESSARY_OPTION_MAP_OR_ELSE,
4554+
suspicious,
4555+
"making no use of the \"map closure\" when calling `.map_or_else(|| 2 * k, |n| n)`"
4556+
}
4557+
45324558
#[expect(clippy::struct_excessive_bools)]
45334559
pub struct Methods {
45344560
avoid_breaking_exported_api: bool,
@@ -4707,6 +4733,7 @@ impl_lint_pass!(Methods => [
47074733
MANUAL_CONTAINS,
47084734
IO_OTHER_ERROR,
47094735
SWAP_WITH_TEMPORARY,
4736+
UNNECESSARY_OPTION_MAP_OR_ELSE,
47104737
]);
47114738

47124739
/// Extracts a method call name, args, and `Span` of the method name.
@@ -5262,6 +5289,7 @@ impl Methods {
52625289
},
52635290
("map_or_else", [def, map]) => {
52645291
result_map_or_else_none::check(cx, expr, recv, def, map);
5292+
unnecessary_option_map_or_else::check(cx, expr, recv, def, map);
52655293
unnecessary_result_map_or_else::check(cx, expr, recv, def, map);
52665294
},
52675295
("next", []) => {
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::peel_blocks;
3+
use clippy_utils::source::snippet;
4+
use clippy_utils::ty::is_type_diagnostic_item;
5+
use rustc_errors::Applicability;
6+
use rustc_hir::{Closure, Expr, ExprKind, HirId, QPath};
7+
use rustc_lint::LateContext;
8+
use rustc_span::symbol::sym;
9+
10+
use super::UNNECESSARY_OPTION_MAP_OR_ELSE;
11+
use super::utils::get_last_chain_binding_hir_id;
12+
13+
fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>) {
14+
let msg = "unused \"map closure\" when calling `Option::map_or_else` value";
15+
let self_snippet = snippet(cx, recv.span, "..");
16+
let err_snippet = snippet(cx, def_arg.span, "..");
17+
span_lint_and_sugg(
18+
cx,
19+
UNNECESSARY_OPTION_MAP_OR_ELSE,
20+
expr.span,
21+
msg,
22+
"consider using `unwrap_or_else`",
23+
format!("{self_snippet}.unwrap_or_else({err_snippet})"),
24+
Applicability::MachineApplicable,
25+
);
26+
}
27+
28+
fn handle_qpath(
29+
cx: &LateContext<'_>,
30+
expr: &Expr<'_>,
31+
recv: &Expr<'_>,
32+
def_arg: &Expr<'_>,
33+
expected_hir_id: HirId,
34+
qpath: QPath<'_>,
35+
) {
36+
if let QPath::Resolved(_, path) = qpath
37+
&& let rustc_hir::def::Res::Local(hir_id) = path.res
38+
&& expected_hir_id == hir_id
39+
{
40+
emit_lint(cx, expr, recv, def_arg);
41+
}
42+
}
43+
44+
/// lint use of `_.map_or_else(|err| err, |n| n)` for `Option`s.
45+
pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>, map_arg: &Expr<'_>) {
46+
// lint if the caller of `map_or_else()` is an `Option`
47+
if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Option)
48+
&& let ExprKind::Closure(&Closure { body, .. }) = map_arg.kind
49+
&& let body = cx.tcx.hir_body(body)
50+
&& let Some(first_param) = body.params.first()
51+
{
52+
let body_expr = peel_blocks(body.value);
53+
54+
match body_expr.kind {
55+
ExprKind::Path(qpath) => {
56+
handle_qpath(cx, expr, recv, def_arg, first_param.pat.hir_id, qpath);
57+
},
58+
// If this is a block (that wasn't peeled off), then it means there are statements.
59+
ExprKind::Block(block, _) => {
60+
if let Some(block_expr) = block.expr
61+
// First we ensure that this is a "binding chain" (each statement is a binding
62+
// of the previous one) and that it is a binding of the closure argument.
63+
&& let Some(last_chain_binding_id) =
64+
get_last_chain_binding_hir_id(first_param.pat.hir_id, block.stmts)
65+
&& let ExprKind::Path(qpath) = block_expr.kind
66+
{
67+
handle_qpath(cx, expr, recv, def_arg, last_chain_binding_id, qpath);
68+
}
69+
},
70+
_ => {},
71+
}
72+
}
73+
}

tests/ui/option_if_let_else.fixed

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
clippy::redundant_locals,
77
clippy::manual_midpoint,
88
clippy::manual_unwrap_or_default,
9-
clippy::manual_unwrap_or
9+
clippy::manual_unwrap_or,
10+
clippy::unnecessary_option_map_or_else
1011
)]
1112

1213
fn bad1(string: Option<&str>) -> (bool, &str) {

tests/ui/option_if_let_else.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
clippy::redundant_locals,
77
clippy::manual_midpoint,
88
clippy::manual_unwrap_or_default,
9-
clippy::manual_unwrap_or
9+
clippy::manual_unwrap_or,
10+
clippy::unnecessary_option_map_or_else
1011
)]
1112

1213
fn bad1(string: Option<&str>) -> (bool, &str) {

tests/ui/option_if_let_else.stderr

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: use Option::map_or instead of an if let/else
2-
--> tests/ui/option_if_let_else.rs:13:5
2+
--> tests/ui/option_if_let_else.rs:14:5
33
|
44
LL | / if let Some(x) = string {
55
LL | |
@@ -13,19 +13,19 @@ LL | | }
1313
= help: to override `-D warnings` add `#[allow(clippy::option_if_let_else)]`
1414

1515
error: use Option::map_or instead of an if let/else
16-
--> tests/ui/option_if_let_else.rs:32:13
16+
--> tests/ui/option_if_let_else.rs:33:13
1717
|
1818
LL | let _ = if let Some(s) = *string { s.len() } else { 0 };
1919
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())`
2020

2121
error: use Option::map_or instead of an if let/else
22-
--> tests/ui/option_if_let_else.rs:34:13
22+
--> tests/ui/option_if_let_else.rs:35:13
2323
|
2424
LL | let _ = if let Some(s) = &num { s } else { &0 };
2525
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
2626

2727
error: use Option::map_or instead of an if let/else
28-
--> tests/ui/option_if_let_else.rs:36:13
28+
--> tests/ui/option_if_let_else.rs:37:13
2929
|
3030
LL | let _ = if let Some(s) = &mut num {
3131
| _____________^
@@ -47,13 +47,13 @@ LL ~ });
4747
|
4848

4949
error: use Option::map_or instead of an if let/else
50-
--> tests/ui/option_if_let_else.rs:43:13
50+
--> tests/ui/option_if_let_else.rs:44:13
5151
|
5252
LL | let _ = if let Some(ref s) = num { s } else { &0 };
5353
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
5454

5555
error: use Option::map_or instead of an if let/else
56-
--> tests/ui/option_if_let_else.rs:45:13
56+
--> tests/ui/option_if_let_else.rs:46:13
5757
|
5858
LL | let _ = if let Some(mut s) = num {
5959
| _____________^
@@ -75,7 +75,7 @@ LL ~ });
7575
|
7676

7777
error: use Option::map_or instead of an if let/else
78-
--> tests/ui/option_if_let_else.rs:52:13
78+
--> tests/ui/option_if_let_else.rs:53:13
7979
|
8080
LL | let _ = if let Some(ref mut s) = num {
8181
| _____________^
@@ -97,7 +97,7 @@ LL ~ });
9797
|
9898

9999
error: use Option::map_or instead of an if let/else
100-
--> tests/ui/option_if_let_else.rs:62:5
100+
--> tests/ui/option_if_let_else.rs:63:5
101101
|
102102
LL | / if let Some(x) = arg {
103103
LL | |
@@ -118,7 +118,7 @@ LL + })
118118
|
119119

120120
error: use Option::map_or_else instead of an if let/else
121-
--> tests/ui/option_if_let_else.rs:76:13
121+
--> tests/ui/option_if_let_else.rs:77:13
122122
|
123123
LL | let _ = if let Some(x) = arg {
124124
| _____________^
@@ -131,7 +131,7 @@ LL | | };
131131
| |_____^ help: try: `arg.map_or_else(side_effect, |x| x)`
132132

133133
error: use Option::map_or_else instead of an if let/else
134-
--> tests/ui/option_if_let_else.rs:86:13
134+
--> tests/ui/option_if_let_else.rs:87:13
135135
|
136136
LL | let _ = if let Some(x) = arg {
137137
| _____________^
@@ -154,7 +154,7 @@ LL ~ }, |x| x * x * x * x);
154154
|
155155

156156
error: use Option::map_or_else instead of an if let/else
157-
--> tests/ui/option_if_let_else.rs:120:13
157+
--> tests/ui/option_if_let_else.rs:121:13
158158
|
159159
LL | / if let Some(idx) = s.find('.') {
160160
LL | |
@@ -165,7 +165,7 @@ LL | | }
165165
| |_____________^ help: try: `s.find('.').map_or_else(|| vec![s.to_string()], |idx| vec![s[..idx].to_string(), s[idx..].to_string()])`
166166

167167
error: use Option::map_or_else instead of an if let/else
168-
--> tests/ui/option_if_let_else.rs:132:5
168+
--> tests/ui/option_if_let_else.rs:133:5
169169
|
170170
LL | / if let Ok(binding) = variable {
171171
LL | |
@@ -189,13 +189,13 @@ LL + })
189189
|
190190

191191
error: use Option::map_or instead of an if let/else
192-
--> tests/ui/option_if_let_else.rs:157:13
192+
--> tests/ui/option_if_let_else.rs:158:13
193193
|
194194
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
195195
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
196196

197197
error: use Option::map_or instead of an if let/else
198-
--> tests/ui/option_if_let_else.rs:168:13
198+
--> tests/ui/option_if_let_else.rs:169:13
199199
|
200200
LL | let _ = if let Some(x) = Some(0) {
201201
| _____________^
@@ -217,13 +217,13 @@ LL ~ });
217217
|
218218

219219
error: use Option::map_or instead of an if let/else
220-
--> tests/ui/option_if_let_else.rs:197:13
220+
--> tests/ui/option_if_let_else.rs:198:13
221221
|
222222
LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };
223223
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or(s.len(), |x| s.len() + x)`
224224

225225
error: use Option::map_or instead of an if let/else
226-
--> tests/ui/option_if_let_else.rs:202:13
226+
--> tests/ui/option_if_let_else.rs:203:13
227227
|
228228
LL | let _ = if let Some(x) = Some(0) {
229229
| _____________^
@@ -245,7 +245,7 @@ LL ~ });
245245
|
246246

247247
error: use Option::map_or instead of an if let/else
248-
--> tests/ui/option_if_let_else.rs:242:13
248+
--> tests/ui/option_if_let_else.rs:243:13
249249
|
250250
LL | let _ = match s {
251251
| _____________^
@@ -256,7 +256,7 @@ LL | | };
256256
| |_____^ help: try: `s.map_or(1, |string| string.len())`
257257

258258
error: use Option::map_or instead of an if let/else
259-
--> tests/ui/option_if_let_else.rs:247:13
259+
--> tests/ui/option_if_let_else.rs:248:13
260260
|
261261
LL | let _ = match Some(10) {
262262
| _____________^
@@ -267,7 +267,7 @@ LL | | };
267267
| |_____^ help: try: `Some(10).map_or(5, |a| a + 1)`
268268

269269
error: use Option::map_or instead of an if let/else
270-
--> tests/ui/option_if_let_else.rs:254:13
270+
--> tests/ui/option_if_let_else.rs:255:13
271271
|
272272
LL | let _ = match res {
273273
| _____________^
@@ -278,7 +278,7 @@ LL | | };
278278
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
279279

280280
error: use Option::map_or instead of an if let/else
281-
--> tests/ui/option_if_let_else.rs:259:13
281+
--> tests/ui/option_if_let_else.rs:260:13
282282
|
283283
LL | let _ = match res {
284284
| _____________^
@@ -289,13 +289,13 @@ LL | | };
289289
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
290290

291291
error: use Option::map_or instead of an if let/else
292-
--> tests/ui/option_if_let_else.rs:264:13
292+
--> tests/ui/option_if_let_else.rs:265:13
293293
|
294294
LL | let _ = if let Ok(a) = res { a + 1 } else { 5 };
295295
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `res.map_or(5, |a| a + 1)`
296296

297297
error: use Option::map_or instead of an if let/else
298-
--> tests/ui/option_if_let_else.rs:282:17
298+
--> tests/ui/option_if_let_else.rs:283:17
299299
|
300300
LL | let _ = match initial {
301301
| _________________^
@@ -306,7 +306,7 @@ LL | | };
306306
| |_________^ help: try: `initial.as_ref().map_or(42, |value| do_something(value))`
307307

308308
error: use Option::map_or instead of an if let/else
309-
--> tests/ui/option_if_let_else.rs:290:17
309+
--> tests/ui/option_if_let_else.rs:291:17
310310
|
311311
LL | let _ = match initial {
312312
| _________________^
@@ -317,7 +317,7 @@ LL | | };
317317
| |_________^ help: try: `initial.as_mut().map_or(42, |value| do_something2(value))`
318318

319319
error: use Option::map_or_else instead of an if let/else
320-
--> tests/ui/option_if_let_else.rs:314:24
320+
--> tests/ui/option_if_let_else.rs:315:24
321321
|
322322
LL | let mut _hashmap = if let Some(hm) = &opt {
323323
| ________________________^
@@ -329,7 +329,7 @@ LL | | };
329329
| |_____^ help: try: `opt.as_ref().map_or_else(HashMap::new, |hm| hm.clone())`
330330

331331
error: use Option::map_or_else instead of an if let/else
332-
--> tests/ui/option_if_let_else.rs:321:19
332+
--> tests/ui/option_if_let_else.rs:322:19
333333
|
334334
LL | let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() };
335335
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone())`

tests/ui/or_fun_call.fixed

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
clippy::uninlined_format_args,
66
clippy::unnecessary_wraps,
77
clippy::unnecessary_literal_unwrap,
8+
clippy::unnecessary_option_map_or_else,
89
clippy::useless_vec
910
)]
1011

tests/ui/or_fun_call.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
clippy::uninlined_format_args,
66
clippy::unnecessary_wraps,
77
clippy::unnecessary_literal_unwrap,
8+
clippy::unnecessary_option_map_or_else,
89
clippy::useless_vec
910
)]
1011

0 commit comments

Comments
 (0)