Skip to content

Commit e3c73f1

Browse files
samueltardieusamueltardieu
committed
option_if_let_else: do not trigger on expressions returning ()
Fix rust-lang#11893 Trigerring on expressions returning `()` uses the arguments of the `map_or_else()` rewrite only for their side effects. This does lead to code which is harder to read than the original.
1 parent 8b0bf64 commit e3c73f1

File tree

4 files changed

+85
-44
lines changed

4 files changed

+85
-44
lines changed

‎clippy_lints/src/option_if_let_else.rs

+12-9
Original file line numberDiff line numberDiff line change
@@ -239,21 +239,24 @@ fn detect_option_if_let_else<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) ->
239239
if_then,
240240
if_else: Some(if_else),
241241
}) = higher::IfLet::hir(cx, expr)
242+
&& !cx.typeck_results().expr_ty(expr).is_unit()
243+
&& !is_else_clause(cx.tcx, expr)
242244
{
243-
if !is_else_clause(cx.tcx, expr) {
244-
return try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, let_expr, if_then, if_else);
245-
}
245+
try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, let_expr, if_then, if_else)
246+
} else {
247+
None
246248
}
247-
None
248249
}
249250

250251
fn detect_option_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option<OptionOccurrence> {
251-
if let ExprKind::Match(ex, arms, MatchSource::Normal) = expr.kind {
252-
if let Some((let_pat, if_then, if_else)) = try_convert_match(cx, arms) {
253-
return try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, ex, if_then, if_else);
254-
}
252+
if let ExprKind::Match(ex, arms, MatchSource::Normal) = expr.kind
253+
&& !cx.typeck_results().expr_ty(expr).is_unit()
254+
&& let Some((let_pat, if_then, if_else)) = try_convert_match(cx, arms)
255+
{
256+
try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, ex, if_then, if_else)
257+
} else {
258+
None
255259
}
256-
None
257260
}
258261

259262
fn try_convert_match<'tcx>(

‎tests/ui/option_if_let_else.fixed

+21-5
Original file line numberDiff line numberDiff line change
@@ -92,11 +92,13 @@ fn pattern_to_vec(pattern: &str) -> Vec<String> {
9292
}
9393

9494
// #10335
95-
fn test_result_impure_else(variable: Result<u32, &str>) {
95+
fn test_result_impure_else(variable: Result<u32, &str>) -> bool {
9696
variable.map_or_else(|_| {
9797
println!("Err");
98+
false
9899
}, |binding| {
99100
println!("Ok {binding}");
101+
true
100102
})
101103
}
102104

@@ -213,15 +215,19 @@ mod issue10729 {
213215

214216
pub fn reproduce(initial: &Option<String>) {
215217
// 👇 needs `.as_ref()` because initial is an `&Option<_>`
216-
initial.as_ref().map_or({}, |value| do_something(value))
218+
let _ = initial.as_ref().map_or(42, |value| do_something(value));
217219
}
218220

219221
pub fn reproduce2(initial: &mut Option<String>) {
220-
initial.as_mut().map_or({}, |value| do_something2(value))
222+
let _ = initial.as_mut().map_or(42, |value| do_something2(value));
221223
}
222224

223-
fn do_something(_value: &str) {}
224-
fn do_something2(_value: &mut str) {}
225+
fn do_something(_value: &str) -> u32 {
226+
todo!()
227+
}
228+
fn do_something2(_value: &mut str) -> u32 {
229+
todo!()
230+
}
225231
}
226232

227233
fn issue11429() {
@@ -237,3 +243,13 @@ fn issue11429() {
237243

238244
let mut _hm = opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone());
239245
}
246+
247+
fn issue11893() {
248+
use std::io::Write;
249+
let mut output = std::io::stdout().lock();
250+
if let Some(name) = Some("stuff") {
251+
writeln!(output, "{name:?}").unwrap();
252+
} else {
253+
panic!("Haven't thought about this condition.");
254+
}
255+
}

‎tests/ui/option_if_let_else.rs

+25-9
Original file line numberDiff line numberDiff line change
@@ -115,11 +115,13 @@ fn pattern_to_vec(pattern: &str) -> Vec<String> {
115115
}
116116

117117
// #10335
118-
fn test_result_impure_else(variable: Result<u32, &str>) {
118+
fn test_result_impure_else(variable: Result<u32, &str>) -> bool {
119119
if let Ok(binding) = variable {
120120
println!("Ok {binding}");
121+
true
121122
} else {
122123
println!("Err");
124+
false
123125
}
124126
}
125127

@@ -254,21 +256,25 @@ mod issue10729 {
254256

255257
pub fn reproduce(initial: &Option<String>) {
256258
// 👇 needs `.as_ref()` because initial is an `&Option<_>`
257-
match initial {
259+
let _ = match initial {
258260
Some(value) => do_something(value),
259-
None => {},
260-
}
261+
None => 42,
262+
};
261263
}
262264

263265
pub fn reproduce2(initial: &mut Option<String>) {
264-
match initial {
266+
let _ = match initial {
265267
Some(value) => do_something2(value),
266-
None => {},
267-
}
268+
None => 42,
269+
};
268270
}
269271

270-
fn do_something(_value: &str) {}
271-
fn do_something2(_value: &mut str) {}
272+
fn do_something(_value: &str) -> u32 {
273+
todo!()
274+
}
275+
fn do_something2(_value: &mut str) -> u32 {
276+
todo!()
277+
}
272278
}
273279

274280
fn issue11429() {
@@ -288,3 +294,13 @@ fn issue11429() {
288294

289295
let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() };
290296
}
297+
298+
fn issue11893() {
299+
use std::io::Write;
300+
let mut output = std::io::stdout().lock();
301+
if let Some(name) = Some("stuff") {
302+
writeln!(output, "{name:?}").unwrap();
303+
} else {
304+
panic!("Haven't thought about this condition.");
305+
}
306+
}

‎tests/ui/option_if_let_else.stderr

+27-21
Original file line numberDiff line numberDiff line change
@@ -158,28 +158,32 @@ error: use Option::map_or_else instead of an if let/else
158158
|
159159
LL | / if let Ok(binding) = variable {
160160
LL | | println!("Ok {binding}");
161+
LL | | true
161162
LL | | } else {
162163
LL | | println!("Err");
164+
LL | | false
163165
LL | | }
164166
| |_____^
165167
|
166168
help: try
167169
|
168170
LL ~ variable.map_or_else(|_| {
169171
LL + println!("Err");
172+
LL + false
170173
LL + }, |binding| {
171174
LL + println!("Ok {binding}");
175+
LL + true
172176
LL + })
173177
|
174178

175179
error: use Option::map_or instead of an if let/else
176-
--> $DIR/option_if_let_else.rs:141:13
180+
--> $DIR/option_if_let_else.rs:143:13
177181
|
178182
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
179183
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
180184

181185
error: use Option::map_or instead of an if let/else
182-
--> $DIR/option_if_let_else.rs:151:13
186+
--> $DIR/option_if_let_else.rs:153:13
183187
|
184188
LL | let _ = if let Some(x) = Some(0) {
185189
| _____________^
@@ -201,13 +205,13 @@ LL ~ });
201205
|
202206

203207
error: use Option::map_or instead of an if let/else
204-
--> $DIR/option_if_let_else.rs:179:13
208+
--> $DIR/option_if_let_else.rs:181:13
205209
|
206210
LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };
207211
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or(s.len(), |x| s.len() + x)`
208212

209213
error: use Option::map_or instead of an if let/else
210-
--> $DIR/option_if_let_else.rs:183:13
214+
--> $DIR/option_if_let_else.rs:185:13
211215
|
212216
LL | let _ = if let Some(x) = Some(0) {
213217
| _____________^
@@ -227,7 +231,7 @@ LL ~ });
227231
|
228232

229233
error: use Option::map_or instead of an if let/else
230-
--> $DIR/option_if_let_else.rs:222:13
234+
--> $DIR/option_if_let_else.rs:224:13
231235
|
232236
LL | let _ = match s {
233237
| _____________^
@@ -237,7 +241,7 @@ LL | | };
237241
| |_____^ help: try: `s.map_or(1, |string| string.len())`
238242

239243
error: use Option::map_or instead of an if let/else
240-
--> $DIR/option_if_let_else.rs:226:13
244+
--> $DIR/option_if_let_else.rs:228:13
241245
|
242246
LL | let _ = match Some(10) {
243247
| _____________^
@@ -247,7 +251,7 @@ LL | | };
247251
| |_____^ help: try: `Some(10).map_or(5, |a| a + 1)`
248252

249253
error: use Option::map_or instead of an if let/else
250-
--> $DIR/option_if_let_else.rs:232:13
254+
--> $DIR/option_if_let_else.rs:234:13
251255
|
252256
LL | let _ = match res {
253257
| _____________^
@@ -257,7 +261,7 @@ LL | | };
257261
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
258262

259263
error: use Option::map_or instead of an if let/else
260-
--> $DIR/option_if_let_else.rs:236:13
264+
--> $DIR/option_if_let_else.rs:238:13
261265
|
262266
LL | let _ = match res {
263267
| _____________^
@@ -267,31 +271,33 @@ LL | | };
267271
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
268272

269273
error: use Option::map_or instead of an if let/else
270-
--> $DIR/option_if_let_else.rs:240:13
274+
--> $DIR/option_if_let_else.rs:242:13
271275
|
272276
LL | let _ = if let Ok(a) = res { a + 1 } else { 5 };
273277
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `res.map_or(5, |a| a + 1)`
274278

275279
error: use Option::map_or instead of an if let/else
276-
--> $DIR/option_if_let_else.rs:257:9
280+
--> $DIR/option_if_let_else.rs:259:17
277281
|
278-
LL | / match initial {
282+
LL | let _ = match initial {
283+
| _________________^
279284
LL | | Some(value) => do_something(value),
280-
LL | | None => {},
281-
LL | | }
282-
| |_________^ help: try: `initial.as_ref().map_or({}, |value| do_something(value))`
285+
LL | | None => 42,
286+
LL | | };
287+
| |_________^ help: try: `initial.as_ref().map_or(42, |value| do_something(value))`
283288

284289
error: use Option::map_or instead of an if let/else
285-
--> $DIR/option_if_let_else.rs:264:9
290+
--> $DIR/option_if_let_else.rs:266:17
286291
|
287-
LL | / match initial {
292+
LL | let _ = match initial {
293+
| _________________^
288294
LL | | Some(value) => do_something2(value),
289-
LL | | None => {},
290-
LL | | }
291-
| |_________^ help: try: `initial.as_mut().map_or({}, |value| do_something2(value))`
295+
LL | | None => 42,
296+
LL | | };
297+
| |_________^ help: try: `initial.as_mut().map_or(42, |value| do_something2(value))`
292298

293299
error: use Option::map_or_else instead of an if let/else
294-
--> $DIR/option_if_let_else.rs:283:24
300+
--> $DIR/option_if_let_else.rs:289:24
295301
|
296302
LL | let mut _hashmap = if let Some(hm) = &opt {
297303
| ________________________^
@@ -302,7 +308,7 @@ LL | | };
302308
| |_____^ help: try: `opt.as_ref().map_or_else(HashMap::new, |hm| hm.clone())`
303309

304310
error: use Option::map_or_else instead of an if let/else
305-
--> $DIR/option_if_let_else.rs:289:19
311+
--> $DIR/option_if_let_else.rs:295:19
306312
|
307313
LL | let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() };
308314
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone())`

0 commit comments

Comments
 (0)