Skip to content

Commit 407c352

Browse files
borsbors
committed
Auto merge of #10337 - EliasHolzmann:fix/10335, r=dswij
Fix: Some suggestions generated by the option_if_let_else lint did not compile This addresses a bug in Clippy where the fix suggestend by the `option_if_let_else` lint would not compile for `Result`s which have an impure expression in the `else` branch. --- changelog: [`option_if_let_else`]: Fixed incorrect suggestion for `Result`s [#10337](#10337) <!-- changelog_checked --> Fixes #10335.
2 parents 7e18451 + d80ca09 commit 407c352

File tree

4 files changed

+56
-15
lines changed

4 files changed

+56
-15
lines changed

‎clippy_lints/src/option_if_let_else.rs

+7-5
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ fn try_get_option_occurrence<'tcx>(
122122
ExprKind::Unary(UnOp::Deref, inner_expr) | ExprKind::AddrOf(_, _, inner_expr) => inner_expr,
123123
_ => expr,
124124
};
125-
let inner_pat = try_get_inner_pat(cx, pat)?;
125+
let (inner_pat, is_result) = try_get_inner_pat_and_is_result(cx, pat)?;
126126
if_chain! {
127127
if let PatKind::Binding(bind_annotation, _, id, None) = inner_pat.kind;
128128
if let Some(some_captures) = can_move_expr_to_closure(cx, if_then);
@@ -176,7 +176,7 @@ fn try_get_option_occurrence<'tcx>(
176176
),
177177
none_expr: format!(
178178
"{}{}",
179-
if method_sugg == "map_or" { "" } else { "|| " },
179+
if method_sugg == "map_or" { "" } else if is_result { "|_| " } else { "|| "},
180180
Sugg::hir_with_context(cx, none_body, ctxt, "..", &mut app),
181181
),
182182
});
@@ -186,11 +186,13 @@ fn try_get_option_occurrence<'tcx>(
186186
None
187187
}
188188

189-
fn try_get_inner_pat<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>) -> Option<&'tcx Pat<'tcx>> {
189+
fn try_get_inner_pat_and_is_result<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>) -> Option<(&'tcx Pat<'tcx>, bool)> {
190190
if let PatKind::TupleStruct(ref qpath, [inner_pat], ..) = pat.kind {
191191
let res = cx.qpath_res(qpath, pat.hir_id);
192-
if is_res_lang_ctor(cx, res, OptionSome) || is_res_lang_ctor(cx, res, ResultOk) {
193-
return Some(inner_pat);
192+
if is_res_lang_ctor(cx, res, OptionSome) {
193+
return Some((inner_pat, false));
194+
} else if is_res_lang_ctor(cx, res, ResultOk) {
195+
return Some((inner_pat, true));
194196
}
195197
}
196198
None

‎tests/ui/option_if_let_else.fixed

+10
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,15 @@ fn pattern_to_vec(pattern: &str) -> Vec<String> {
9292
.collect::<Vec<_>>()
9393
}
9494

95+
// #10335
96+
fn test_result_impure_else(variable: Result<u32, &str>) {
97+
variable.map_or_else(|_| {
98+
println!("Err");
99+
}, |binding| {
100+
println!("Ok {binding}");
101+
})
102+
}
103+
95104
enum DummyEnum {
96105
One(u8),
97106
Two,
@@ -113,6 +122,7 @@ fn main() {
113122
unop_bad(&None, None);
114123
let _ = longer_body(None);
115124
test_map_or_else(None);
125+
test_result_impure_else(Ok(42));
116126
let _ = negative_tests(None);
117127
let _ = impure_else(None);
118128

‎tests/ui/option_if_let_else.rs

+10
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,15 @@ fn pattern_to_vec(pattern: &str) -> Vec<String> {
115115
.collect::<Vec<_>>()
116116
}
117117

118+
// #10335
119+
fn test_result_impure_else(variable: Result<u32, &str>) {
120+
if let Ok(binding) = variable {
121+
println!("Ok {binding}");
122+
} else {
123+
println!("Err");
124+
}
125+
}
126+
118127
enum DummyEnum {
119128
One(u8),
120129
Two,
@@ -136,6 +145,7 @@ fn main() {
136145
unop_bad(&None, None);
137146
let _ = longer_body(None);
138147
test_map_or_else(None);
148+
test_result_impure_else(Ok(42));
139149
let _ = negative_tests(None);
140150
let _ = impure_else(None);
141151

‎tests/ui/option_if_let_else.stderr

+29-10
Original file line numberDiff line numberDiff line change
@@ -152,14 +152,33 @@ LL | | vec![s.to_string()]
152152
LL | | }
153153
| |_____________^ help: try: `s.find('.').map_or_else(|| vec![s.to_string()], |idx| vec![s[..idx].to_string(), s[idx..].to_string()])`
154154

155+
error: use Option::map_or_else instead of an if let/else
156+
--> $DIR/option_if_let_else.rs:120:5
157+
|
158+
LL | / if let Ok(binding) = variable {
159+
LL | | println!("Ok {binding}");
160+
LL | | } else {
161+
LL | | println!("Err");
162+
LL | | }
163+
| |_____^
164+
|
165+
help: try
166+
|
167+
LL ~ variable.map_or_else(|_| {
168+
LL + println!("Err");
169+
LL + }, |binding| {
170+
LL + println!("Ok {binding}");
171+
LL + })
172+
|
173+
155174
error: use Option::map_or instead of an if let/else
156-
--> $DIR/option_if_let_else.rs:133:13
175+
--> $DIR/option_if_let_else.rs:142:13
157176
|
158177
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
159178
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
160179

161180
error: use Option::map_or instead of an if let/else
162-
--> $DIR/option_if_let_else.rs:142:13
181+
--> $DIR/option_if_let_else.rs:152:13
163182
|
164183
LL | let _ = if let Some(x) = Some(0) {
165184
| _____________^
@@ -181,13 +200,13 @@ LL ~ });
181200
|
182201

183202
error: use Option::map_or instead of an if let/else
184-
--> $DIR/option_if_let_else.rs:170:13
203+
--> $DIR/option_if_let_else.rs:180:13
185204
|
186205
LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };
187206
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or(s.len(), |x| s.len() + x)`
188207

189208
error: use Option::map_or instead of an if let/else
190-
--> $DIR/option_if_let_else.rs:174:13
209+
--> $DIR/option_if_let_else.rs:184:13
191210
|
192211
LL | let _ = if let Some(x) = Some(0) {
193212
| _____________^
@@ -207,7 +226,7 @@ LL ~ });
207226
|
208227

209228
error: use Option::map_or instead of an if let/else
210-
--> $DIR/option_if_let_else.rs:213:13
229+
--> $DIR/option_if_let_else.rs:223:13
211230
|
212231
LL | let _ = match s {
213232
| _____________^
@@ -217,7 +236,7 @@ LL | | };
217236
| |_____^ help: try: `s.map_or(1, |string| string.len())`
218237

219238
error: use Option::map_or instead of an if let/else
220-
--> $DIR/option_if_let_else.rs:217:13
239+
--> $DIR/option_if_let_else.rs:227:13
221240
|
222241
LL | let _ = match Some(10) {
223242
| _____________^
@@ -227,7 +246,7 @@ LL | | };
227246
| |_____^ help: try: `Some(10).map_or(5, |a| a + 1)`
228247

229248
error: use Option::map_or instead of an if let/else
230-
--> $DIR/option_if_let_else.rs:223:13
249+
--> $DIR/option_if_let_else.rs:233:13
231250
|
232251
LL | let _ = match res {
233252
| _____________^
@@ -237,7 +256,7 @@ LL | | };
237256
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
238257

239258
error: use Option::map_or instead of an if let/else
240-
--> $DIR/option_if_let_else.rs:227:13
259+
--> $DIR/option_if_let_else.rs:237:13
241260
|
242261
LL | let _ = match res {
243262
| _____________^
@@ -247,10 +266,10 @@ LL | | };
247266
| |_____^ help: try: `res.map_or(1, |a| a + 1)`
248267

249268
error: use Option::map_or instead of an if let/else
250-
--> $DIR/option_if_let_else.rs:231:13
269+
--> $DIR/option_if_let_else.rs:241:13
251270
|
252271
LL | let _ = if let Ok(a) = res { a + 1 } else { 5 };
253272
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `res.map_or(5, |a| a + 1)`
254273

255-
error: aborting due to 20 previous errors
274+
error: aborting due to 21 previous errors
256275

0 commit comments

Comments
 (0)