Skip to content

Commit e410d6f

Browse files
pnuts93pnuts93
committed
Add test cases for closure binding and function identity
1 parent 4c128d9 commit e410d6f

File tree

4 files changed

+126
-20
lines changed

4 files changed

+126
-20
lines changed

‎clippy_lints/src/methods/unnecessary_option_map_or_else.rs‎

Lines changed: 53 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
2-
use clippy_utils::peel_blocks;
3-
use clippy_utils::source::snippet;
2+
use clippy_utils::source::snippet_with_applicability;
43
use clippy_utils::ty::is_type_diagnostic_item;
4+
use clippy_utils::{expr_or_init, find_binding_init, peel_blocks};
55
use rustc_errors::Applicability;
6-
use rustc_hir::{Closure, Expr, ExprKind, HirId, QPath};
6+
use rustc_hir::def::{DefKind, Res};
7+
use rustc_hir::{Body, BodyId, Closure, Expr, ExprKind, HirId, QPath};
78
use rustc_lint::LateContext;
89
use rustc_span::symbol::sym;
910

@@ -12,8 +13,9 @@ use super::utils::get_last_chain_binding_hir_id;
1213

1314
fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>) {
1415
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, "..");
16+
let mut applicability = Applicability::MachineApplicable;
17+
let self_snippet = snippet_with_applicability(cx, recv.span, "_", &mut applicability);
18+
let err_snippet = snippet_with_applicability(cx, def_arg.span, "..", &mut applicability);
1719
span_lint_and_sugg(
1820
cx,
1921
UNNECESSARY_OPTION_MAP_OR_ELSE,
@@ -34,23 +36,21 @@ fn handle_qpath(
3436
qpath: QPath<'_>,
3537
) {
3638
if let QPath::Resolved(_, path) = qpath
37-
&& let rustc_hir::def::Res::Local(hir_id) = path.res
39+
&& let Res::Local(hir_id) = path.res
3840
&& expected_hir_id == hir_id
3941
{
4042
emit_lint(cx, expr, recv, def_arg);
4143
}
4244
}
4345

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);
46+
fn handle_closure(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>, body_id: BodyId) {
47+
let body = cx.tcx.hir_body(body_id);
48+
handle_fn_body(cx, expr, recv, def_arg, body);
49+
}
5350

51+
fn handle_fn_body(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>, body: &Body<'_>) {
52+
if let Some(first_param) = body.params.first() {
53+
let body_expr = peel_blocks(body.value);
5454
match body_expr.kind {
5555
ExprKind::Path(qpath) => {
5656
handle_qpath(cx, expr, recv, def_arg, first_param.pat.hir_id, qpath);
@@ -71,3 +71,41 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_
7171
}
7272
}
7373
}
74+
75+
/// lint use of `_.map_or_else(|err| err, |n| n)` for `Option`s.
76+
pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>, map_arg: &Expr<'_>) {
77+
// lint if the caller of `map_or_else()` is an `Option`
78+
if !is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Option) {
79+
return;
80+
}
81+
match map_arg.kind {
82+
// If the second argument is a closure, we can check its body.
83+
ExprKind::Closure(&Closure { body, .. }) => {
84+
handle_closure(cx, expr, recv, def_arg, body);
85+
},
86+
ExprKind::Path(qpath) => {
87+
let res = cx.qpath_res(&qpath, map_arg.hir_id);
88+
match res {
89+
// Case 1: Local variable (could be a closure)
90+
Res::Local(hir_id) => {
91+
if let Some(init_expr) = find_binding_init(cx, hir_id) {
92+
let origin = expr_or_init(cx, init_expr);
93+
if let ExprKind::Closure(&Closure { body, .. }) = origin.kind {
94+
handle_closure(cx, expr, recv, def_arg, body);
95+
}
96+
}
97+
},
98+
// Case 2: Function definition
99+
Res::Def(DefKind::Fn, def_id) => {
100+
if let Some(local_def_id) = def_id.as_local()
101+
&& let Some(body) = cx.tcx.hir_maybe_body_owned_by(local_def_id)
102+
{
103+
handle_fn_body(cx, expr, recv, def_arg, body);
104+
}
105+
},
106+
_ => (),
107+
}
108+
},
109+
_ => (),
110+
}
111+
}

‎tests/ui/unnecessary_option_map_or_else.fixed‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,14 @@
66
clippy::unnecessary_literal_unwrap
77
)]
88

9+
const fn identity<T>(x: T) -> T {
10+
x
11+
}
12+
13+
const fn double_it(x: i32) -> i32 {
14+
x * 2
15+
}
16+
917
fn main() {
1018
// Expected errors
1119
// Basic scenario
@@ -25,6 +33,17 @@ fn main() {
2533
let option = Some(());
2634
option.unwrap_or_else(|| ());
2735

36+
// Identity
37+
let string = String::new();
38+
let option = Some(&string);
39+
let _: &str = option.unwrap_or_else(|| &string); //~ ERROR: unused "map closure" when calling
40+
41+
// Closure bound to a variable
42+
let do_nothing = |x: String| x;
43+
let string = String::new();
44+
let option = Some(string.clone());
45+
let _: String = option.unwrap_or_else(|| string); //~ ERROR: unused "map closure" when calling
46+
2847
// Correct usages
2948
let option = Some(());
3049
option.map_or_else(|| (), |_| ());
@@ -44,4 +63,13 @@ fn main() {
4463
tmp
4564
},
4665
);
66+
67+
let num = 5;
68+
let option = Some(num);
69+
let _: i32 = option.map_or_else(|| 0, double_it);
70+
71+
let increase = |x: i32| x + 1;
72+
let num = 5;
73+
let option = Some(num);
74+
let _: i32 = option.map_or_else(|| 0, increase);
4775
}

‎tests/ui/unnecessary_option_map_or_else.rs‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,14 @@
66
clippy::unnecessary_literal_unwrap
77
)]
88

9+
const fn identity<T>(x: T) -> T {
10+
x
11+
}
12+
13+
const fn double_it(x: i32) -> i32 {
14+
x * 2
15+
}
16+
917
fn main() {
1018
// Expected errors
1119
// Basic scenario
@@ -32,6 +40,17 @@ fn main() {
3240
},
3341
);
3442

43+
// Identity
44+
let string = String::new();
45+
let option = Some(&string);
46+
let _: &str = option.map_or_else(|| &string, identity); //~ ERROR: unused "map closure" when calling
47+
48+
// Closure bound to a variable
49+
let do_nothing = |x: String| x;
50+
let string = String::new();
51+
let option = Some(string.clone());
52+
let _: String = option.map_or_else(|| string, do_nothing); //~ ERROR: unused "map closure" when calling
53+
3554
// Correct usages
3655
let option = Some(());
3756
option.map_or_else(|| (), |_| ());
@@ -51,4 +70,13 @@ fn main() {
5170
tmp
5271
},
5372
);
73+
74+
let num = 5;
75+
let option = Some(num);
76+
let _: i32 = option.map_or_else(|| 0, double_it);
77+
78+
let increase = |x: i32| x + 1;
79+
let num = 5;
80+
let option = Some(num);
81+
let _: i32 = option.map_or_else(|| 0, increase);
5482
}

‎tests/ui/unnecessary_option_map_or_else.stderr‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: unused "map closure" when calling `Option::map_or_else` value
2-
--> tests/ui/unnecessary_option_map_or_else.rs:13:5
2+
--> tests/ui/unnecessary_option_map_or_else.rs:21:5
33
|
44
LL | option.map_or_else(|| (), |x| x);
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| ())`
@@ -8,19 +8,19 @@ LL | option.map_or_else(|| (), |x| x);
88
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_option_map_or_else)]`
99

1010
error: unused "map closure" when calling `Option::map_or_else` value
11-
--> tests/ui/unnecessary_option_map_or_else.rs:17:5
11+
--> tests/ui/unnecessary_option_map_or_else.rs:25:5
1212
|
1313
LL | option.map_or_else(|| (), |x: ()| x);
1414
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| ())`
1515

1616
error: unused "map closure" when calling `Option::map_or_else` value
17-
--> tests/ui/unnecessary_option_map_or_else.rs:22:19
17+
--> tests/ui/unnecessary_option_map_or_else.rs:30:19
1818
|
1919
LL | let _: &str = option.map_or_else(|| &string, |x| x);
2020
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| &string)`
2121

2222
error: unused "map closure" when calling `Option::map_or_else` value
23-
--> tests/ui/unnecessary_option_map_or_else.rs:26:5
23+
--> tests/ui/unnecessary_option_map_or_else.rs:34:5
2424
|
2525
LL | / option.map_or_else(
2626
LL | |
@@ -31,5 +31,17 @@ LL | | },
3131
LL | | );
3232
| |_____^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| ())`
3333

34-
error: aborting due to 4 previous errors
34+
error: unused "map closure" when calling `Option::map_or_else` value
35+
--> tests/ui/unnecessary_option_map_or_else.rs:46:19
36+
|
37+
LL | let _: &str = option.map_or_else(|| &string, identity);
38+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| &string)`
39+
40+
error: unused "map closure" when calling `Option::map_or_else` value
41+
--> tests/ui/unnecessary_option_map_or_else.rs:52:21
42+
|
43+
LL | let _: String = option.map_or_else(|| string, do_nothing);
44+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| string)`
45+
46+
error: aborting due to 6 previous errors
3547

0 commit comments

Comments
 (0)