Skip to content

Commit 438f934

Browse files
J-ZhengLiJ-ZhengLi
committed
suggest passing function instead of calling it in [option_if_let_else]
1 parent 3da21b0 commit 438f934

File tree

4 files changed

+93
-29
lines changed

4 files changed

+93
-29
lines changed

‎clippy_lints/src/option_if_let_else.rs‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,13 @@ fn try_get_option_occurrence<'tcx>(
165165
}
166166

167167
let mut app = Applicability::Unspecified;
168+
169+
let (none_body, is_argless_call) = if let Some(call_expr) = try_get_argless_call_expr(none_body) {
170+
(call_expr, true)
171+
} else {
172+
(none_body, false)
173+
};
174+
168175
return Some(OptionOccurrence {
169176
option: format_option_in_sugg(
170177
Sugg::hir_with_context(cx, cond_expr, ctxt, "..", &mut app),
@@ -178,7 +185,7 @@ fn try_get_option_occurrence<'tcx>(
178185
),
179186
none_expr: format!(
180187
"{}{}",
181-
if method_sugg == "map_or" { "" } else if is_result { "|_| " } else { "|| "},
188+
if method_sugg == "map_or" || is_argless_call { "" } else if is_result { "|_| " } else { "|| "},
182189
Sugg::hir_with_context(cx, none_body, ctxt, "..", &mut app),
183190
),
184191
});
@@ -188,6 +195,16 @@ fn try_get_option_occurrence<'tcx>(
188195
None
189196
}
190197

198+
/// Gets the call expr iff it does not have any args and was not from macro expansion.
199+
fn try_get_argless_call_expr<'tcx>(expr: &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> {
200+
if !expr.span.from_expansion() &&
201+
let ExprKind::Call(call_expr, []) = expr.kind
202+
{
203+
return Some(call_expr);
204+
}
205+
None
206+
}
207+
191208
fn try_get_inner_pat_and_is_result<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>) -> Option<(&'tcx Pat<'tcx>, bool)> {
192209
if let PatKind::TupleStruct(ref qpath, [inner_pat], ..) = pat.kind {
193210
let res = cx.qpath_res(qpath, pat.hir_id);

‎tests/ui/option_if_let_else.fixed‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
#![warn(clippy::option_if_let_else)]
22
#![allow(
33
unused_tuple_struct_fields,
4-
clippy::redundant_closure,
54
clippy::ref_option_ref,
65
clippy::equatable_if_let,
76
clippy::let_unit_value,
@@ -52,7 +51,7 @@ fn impure_else(arg: Option<i32>) {
5251
println!("return 1");
5352
1
5453
};
55-
let _ = arg.map_or_else(|| side_effect(), |x| x);
54+
let _ = arg.map_or_else(side_effect, |x| x);
5655
}
5756

5857
fn test_map_or_else(arg: Option<u32>) {
@@ -224,3 +223,17 @@ mod issue10729 {
224223
fn do_something(_value: &str) {}
225224
fn do_something2(_value: &mut str) {}
226225
}
226+
227+
fn issue11429() {
228+
use std::collections::HashMap;
229+
230+
macro_rules! new_map {
231+
() => {{ HashMap::new() }};
232+
}
233+
234+
let opt: Option<HashMap<u8, u8>> = None;
235+
236+
let mut _hashmap = opt.as_ref().map_or_else(HashMap::new, |hm| hm.clone());
237+
238+
let mut _hm = opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone());
239+
}

‎tests/ui/option_if_let_else.rs‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
#![warn(clippy::option_if_let_else)]
22
#![allow(
33
unused_tuple_struct_fields,
4-
clippy::redundant_closure,
54
clippy::ref_option_ref,
65
clippy::equatable_if_let,
76
clippy::let_unit_value,
@@ -271,3 +270,21 @@ mod issue10729 {
271270
fn do_something(_value: &str) {}
272271
fn do_something2(_value: &mut str) {}
273272
}
273+
274+
fn issue11429() {
275+
use std::collections::HashMap;
276+
277+
macro_rules! new_map {
278+
() => {{ HashMap::new() }};
279+
}
280+
281+
let opt: Option<HashMap<u8, u8>> = None;
282+
283+
let mut _hashmap = if let Some(hm) = &opt {
284+
hm.clone()
285+
} else {
286+
HashMap::new()
287+
};
288+
289+
let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() };
290+
}

‎tests/ui/option_if_let_else.stderr‎

Lines changed: 42 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-
--> $DIR/option_if_let_else.rs:12:5
2+
--> $DIR/option_if_let_else.rs:11:5
33
|
44
LL | / if let Some(x) = string {
55
LL | | (true, x)
@@ -11,19 +11,19 @@ LL | | }
1111
= note: `-D clippy::option-if-let-else` implied by `-D warnings`
1212

1313
error: use Option::map_or instead of an if let/else
14-
--> $DIR/option_if_let_else.rs:30:13
14+
--> $DIR/option_if_let_else.rs:29:13
1515
|
1616
LL | let _ = if let Some(s) = *string { s.len() } else { 0 };
1717
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())`
1818

1919
error: use Option::map_or instead of an if let/else
20-
--> $DIR/option_if_let_else.rs:31:13
20+
--> $DIR/option_if_let_else.rs:30:13
2121
|
2222
LL | let _ = if let Some(s) = &num { s } else { &0 };
2323
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
2424

2525
error: use Option::map_or instead of an if let/else
26-
--> $DIR/option_if_let_else.rs:32:13
26+
--> $DIR/option_if_let_else.rs:31:13
2727
|
2828
LL | let _ = if let Some(s) = &mut num {
2929
| _____________^
@@ -43,13 +43,13 @@ LL ~ });
4343
|
4444

4545
error: use Option::map_or instead of an if let/else
46-
--> $DIR/option_if_let_else.rs:38:13
46+
--> $DIR/option_if_let_else.rs:37:13
4747
|
4848
LL | let _ = if let Some(ref s) = num { s } else { &0 };
4949
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
5050

5151
error: use Option::map_or instead of an if let/else
52-
--> $DIR/option_if_let_else.rs:39:13
52+
--> $DIR/option_if_let_else.rs:38:13
5353
|
5454
LL | let _ = if let Some(mut s) = num {
5555
| _____________^
@@ -69,7 +69,7 @@ LL ~ });
6969
|
7070

7171
error: use Option::map_or instead of an if let/else
72-
--> $DIR/option_if_let_else.rs:45:13
72+
--> $DIR/option_if_let_else.rs:44:13
7373
|
7474
LL | let _ = if let Some(ref mut s) = num {
7575
| _____________^
@@ -89,7 +89,7 @@ LL ~ });
8989
|
9090

9191
error: use Option::map_or instead of an if let/else
92-
--> $DIR/option_if_let_else.rs:54:5
92+
--> $DIR/option_if_let_else.rs:53:5
9393
|
9494
LL | / if let Some(x) = arg {
9595
LL | | let y = x * x;
@@ -108,7 +108,7 @@ LL + })
108108
|
109109

110110
error: use Option::map_or_else instead of an if let/else
111-
--> $DIR/option_if_let_else.rs:67:13
111+
--> $DIR/option_if_let_else.rs:66:13
112112
|
113113
LL | let _ = if let Some(x) = arg {
114114
| _____________^
@@ -117,10 +117,10 @@ LL | | } else {
117117
LL | | // map_or_else must be suggested
118118
LL | | side_effect()
119119
LL | | };
120-
| |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)`
120+
| |_____^ help: try: `arg.map_or_else(side_effect, |x| x)`
121121

122122
error: use Option::map_or_else instead of an if let/else
123-
--> $DIR/option_if_let_else.rs:76:13
123+
--> $DIR/option_if_let_else.rs:75:13
124124
|
125125
LL | let _ = if let Some(x) = arg {
126126
| _____________^
@@ -143,7 +143,7 @@ LL ~ }, |x| x * x * x * x);
143143
|
144144

145145
error: use Option::map_or_else instead of an if let/else
146-
--> $DIR/option_if_let_else.rs:109:13
146+
--> $DIR/option_if_let_else.rs:108:13
147147
|
148148
LL | / if let Some(idx) = s.find('.') {
149149
LL | | vec![s[..idx].to_string(), s[idx..].to_string()]
@@ -153,7 +153,7 @@ LL | | }
153153
| |_____________^ help: try: `s.find('.').map_or_else(|| vec![s.to_string()], |idx| vec![s[..idx].to_string(), s[idx..].to_string()])`
154154

155155
error: use Option::map_or_else instead of an if let/else
156-
--> $DIR/option_if_let_else.rs:120:5
156+
--> $DIR/option_if_let_else.rs:119:5
157157
|
158158
LL | / if let Ok(binding) = variable {
159159
LL | | println!("Ok {binding}");
@@ -172,13 +172,13 @@ LL + })
172172
|
173173

174174
error: use Option::map_or instead of an if let/else
175-
--> $DIR/option_if_let_else.rs:142:13
175+
--> $DIR/option_if_let_else.rs:141:13
176176
|
177177
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
178178
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
179179

180180
error: use Option::map_or instead of an if let/else
181-
--> $DIR/option_if_let_else.rs:152:13
181+
--> $DIR/option_if_let_else.rs:151:13
182182
|
183183
LL | let _ = if let Some(x) = Some(0) {
184184
| _____________^
@@ -200,13 +200,13 @@ LL ~ });
200200
|
201201

202202
error: use Option::map_or instead of an if let/else
203-
--> $DIR/option_if_let_else.rs:180:13
203+
--> $DIR/option_if_let_else.rs:179:13
204204
|
205205
LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };
206206
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or(s.len(), |x| s.len() + x)`
207207

208208
error: use Option::map_or instead of an if let/else
209-
--> $DIR/option_if_let_else.rs:184:13
209+
--> $DIR/option_if_let_else.rs:183:13
210210
|
211211
LL | let _ = if let Some(x) = Some(0) {
212212
| _____________^
@@ -226,7 +226,7 @@ LL ~ });
226226
|
227227

228228
error: use Option::map_or instead of an if let/else
229-
--> $DIR/option_if_let_else.rs:223:13
229+
--> $DIR/option_if_let_else.rs:222:13
230230
|
231231
LL | let _ = match s {
232232
| _____________^
@@ -236,7 +236,7 @@ LL | | };
236236
| |_____^ help: try: `s.map_or(1, |string| string.len())`
237237

238238
error: use Option::map_or instead of an if let/else
239-
--> $DIR/option_if_let_else.rs:227:13
239+
--> $DIR/option_if_let_else.rs:226:13
240240
|
241241
LL | let _ = match Some(10) {
242242
| _____________^
@@ -246,7 +246,7 @@ LL | | };
246246
| |_____^ help: try: `Some(10).map_or(5, |a| a + 1)`
247247

248248
error: use Option::map_or instead of an if let/else
249-
--> $DIR/option_if_let_else.rs:233:13
249+
--> $DIR/option_if_let_else.rs:232:13
250250
|
251251
LL | let _ = match res {
252252
| _____________^
@@ -256,7 +256,7 @@ LL | | };
256256
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
257257

258258
error: use Option::map_or instead of an if let/else
259-
--> $DIR/option_if_let_else.rs:237:13
259+
--> $DIR/option_if_let_else.rs:236:13
260260
|
261261
LL | let _ = match res {
262262
| _____________^
@@ -266,13 +266,13 @@ LL | | };
266266
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
267267

268268
error: use Option::map_or instead of an if let/else
269-
--> $DIR/option_if_let_else.rs:241:13
269+
--> $DIR/option_if_let_else.rs:240:13
270270
|
271271
LL | let _ = if let Ok(a) = res { a + 1 } else { 5 };
272272
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `res.map_or(5, |a| a + 1)`
273273

274274
error: use Option::map_or instead of an if let/else
275-
--> $DIR/option_if_let_else.rs:258:9
275+
--> $DIR/option_if_let_else.rs:257:9
276276
|
277277
LL | / match initial {
278278
LL | | Some(value) => do_something(value),
@@ -281,13 +281,30 @@ LL | | }
281281
| |_________^ help: try: `initial.as_ref().map_or({}, |value| do_something(value))`
282282

283283
error: use Option::map_or instead of an if let/else
284-
--> $DIR/option_if_let_else.rs:265:9
284+
--> $DIR/option_if_let_else.rs:264:9
285285
|
286286
LL | / match initial {
287287
LL | | Some(value) => do_something2(value),
288288
LL | | None => {},
289289
LL | | }
290290
| |_________^ help: try: `initial.as_mut().map_or({}, |value| do_something2(value))`
291291

292-
error: aborting due to 23 previous errors
292+
error: use Option::map_or_else instead of an if let/else
293+
--> $DIR/option_if_let_else.rs:283:24
294+
|
295+
LL | let mut _hashmap = if let Some(hm) = &opt {
296+
| ________________________^
297+
LL | | hm.clone()
298+
LL | | } else {
299+
LL | | HashMap::new()
300+
LL | | };
301+
| |_____^ help: try: `opt.as_ref().map_or_else(HashMap::new, |hm| hm.clone())`
302+
303+
error: use Option::map_or_else instead of an if let/else
304+
--> $DIR/option_if_let_else.rs:289:19
305+
|
306+
LL | let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() };
307+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone())`
308+
309+
error: aborting due to 25 previous errors
293310

0 commit comments

Comments
 (0)