Skip to content

Commit 9f5de66

Browse files
borsbors
committed
Auto merge of #11460 - J-ZhengLi:issue11429, r=Centri3
suggest passing function instead of calling it in closure for [`option_if_let_else`] fixes: #11429 changelog: suggest passing function instead of calling it in closure for [`option_if_let_else`]
2 parents f942470 + fb4f603 commit 9f5de66

File tree

4 files changed

+82
-29
lines changed

4 files changed

+82
-29
lines changed

‎clippy_lints/src/option_if_let_else.rs‎

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

167167
let mut app = Applicability::Unspecified;
168+
169+
let (none_body, is_argless_call) = match none_body.kind {
170+
ExprKind::Call(call_expr, []) if !none_body.span.from_expansion() => (call_expr, true),
171+
_ => (none_body, false),
172+
};
173+
168174
return Some(OptionOccurrence {
169175
option: format_option_in_sugg(
170176
Sugg::hir_with_context(cx, cond_expr, ctxt, "..", &mut app),
@@ -178,7 +184,7 @@ fn try_get_option_occurrence<'tcx>(
178184
),
179185
none_expr: format!(
180186
"{}{}",
181-
if method_sugg == "map_or" { "" } else if is_result { "|_| " } else { "|| "},
187+
if method_sugg == "map_or" || is_argless_call { "" } else if is_result { "|_| " } else { "|| "},
182188
Sugg::hir_with_context(cx, none_body, ctxt, "..", &mut app),
183189
),
184190
});

‎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)
@@ -12,19 +12,19 @@ LL | | }
1212
= help: to override `-D warnings` add `#[allow(clippy::option_if_let_else)]`
1313

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

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

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

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

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

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

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

111111
error: use Option::map_or_else instead of an if let/else
112-
--> $DIR/option_if_let_else.rs:67:13
112+
--> $DIR/option_if_let_else.rs:66:13
113113
|
114114
LL | let _ = if let Some(x) = arg {
115115
| _____________^
@@ -118,10 +118,10 @@ LL | | } else {
118118
LL | | // map_or_else must be suggested
119119
LL | | side_effect()
120120
LL | | };
121-
| |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)`
121+
| |_____^ help: try: `arg.map_or_else(side_effect, |x| x)`
122122

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

0 commit comments

Comments
 (0)