Fix: Some suggestions generated by the option_if_let_else lint did no…

…t compile
rust-lang · May 5, 2023 · d80ca09 · d80ca09
1 parent 371120b
commit d80ca09
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 15 deletions.
diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs
@@ -122,7 +122,7 @@ fn try_get_option_occurrence<'tcx>(
         ExprKind::Unary(UnOp::Deref, inner_expr) | ExprKind::AddrOf(_, _, inner_expr) => inner_expr,
         _ => expr,
     };
-    let inner_pat = try_get_inner_pat(cx, pat)?;
+    let (inner_pat, is_result) = try_get_inner_pat_and_is_result(cx, pat)?;
     if_chain! {
         if let PatKind::Binding(bind_annotation, _, id, None) = inner_pat.kind;
         if let Some(some_captures) = can_move_expr_to_closure(cx, if_then);
@@ -176,7 +176,7 @@ fn try_get_option_occurrence<'tcx>(
                 ),
                 none_expr: format!(
                     "{}{}",
-                    if method_sugg == "map_or" { "" } else { "|| " },
+                    if method_sugg == "map_or" { "" } else if is_result { "|_| " } else { "|| "},
                     Sugg::hir_with_context(cx, none_body, ctxt, "..", &mut app),
                 ),
             });
@@ -186,11 +186,13 @@ fn try_get_option_occurrence<'tcx>(
     None
 }
 
-fn try_get_inner_pat<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>) -> Option<&'tcx Pat<'tcx>> {
+fn try_get_inner_pat_and_is_result<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>) -> Option<(&'tcx Pat<'tcx>, bool)> {
     if let PatKind::TupleStruct(ref qpath, [inner_pat], ..) = pat.kind {
         let res = cx.qpath_res(qpath, pat.hir_id);
-        if is_res_lang_ctor(cx, res, OptionSome) || is_res_lang_ctor(cx, res, ResultOk) {
-            return Some(inner_pat);
+        if is_res_lang_ctor(cx, res, OptionSome) {
+            return Some((inner_pat, false));
+        } else if is_res_lang_ctor(cx, res, ResultOk) {
+            return Some((inner_pat, true));
         }
     }
     None

diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed
@@ -92,6 +92,15 @@ fn pattern_to_vec(pattern: &str) -> Vec<String> {
         .collect::<Vec<_>>()
 }
 
+// #10335
+fn test_result_impure_else(variable: Result<u32, &str>) {
+    variable.map_or_else(|_| {
+        println!("Err");
+    }, |binding| {
+        println!("Ok {binding}");
+    })
+}
+
 enum DummyEnum {
     One(u8),
     Two,
@@ -113,6 +122,7 @@ fn main() {
     unop_bad(&None, None);
     let _ = longer_body(None);
     test_map_or_else(None);
+    test_result_impure_else(Ok(42));
     let _ = negative_tests(None);
     let _ = impure_else(None);
 

diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs
@@ -115,6 +115,15 @@ fn pattern_to_vec(pattern: &str) -> Vec<String> {
         .collect::<Vec<_>>()
 }
 
+// #10335
+fn test_result_impure_else(variable: Result<u32, &str>) {
+    if let Ok(binding) = variable {
+        println!("Ok {binding}");
+    } else {
+        println!("Err");
+    }
+}
+
 enum DummyEnum {
     One(u8),
     Two,
@@ -136,6 +145,7 @@ fn main() {
     unop_bad(&None, None);
     let _ = longer_body(None);
     test_map_or_else(None);
+    test_result_impure_else(Ok(42));
     let _ = negative_tests(None);
     let _ = impure_else(None);
 

diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr
@@ -152,14 +152,33 @@ LL | |                 vec![s.to_string()]
 LL | |             }
    | |_____________^ help: try: `s.find('.').map_or_else(|| vec![s.to_string()], |idx| vec![s[..idx].to_string(), s[idx..].to_string()])`
 
+error: use Option::map_or_else instead of an if let/else
+  --> $DIR/option_if_let_else.rs:120:5
+   |
+LL | /     if let Ok(binding) = variable {
+LL | |         println!("Ok {binding}");
+LL | |     } else {
+LL | |         println!("Err");
+LL | |     }
+   | |_____^
+   |
+help: try
+   |
+LL ~     variable.map_or_else(|_| {
+LL +         println!("Err");
+LL +     }, |binding| {
+LL +         println!("Ok {binding}");
+LL +     })
+   |
+
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:133:13
+  --> $DIR/option_if_let_else.rs:142:13
    |
 LL |     let _ = if let Some(x) = optional { x + 2 } else { 5 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:142:13
+  --> $DIR/option_if_let_else.rs:152:13
    |
 LL |       let _ = if let Some(x) = Some(0) {
    |  _____________^
@@ -181,13 +200,13 @@ LL ~         });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:170:13
+  --> $DIR/option_if_let_else.rs:180:13
    |
 LL |     let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or(s.len(), |x| s.len() + x)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:174:13
+  --> $DIR/option_if_let_else.rs:184:13
    |
 LL |       let _ = if let Some(x) = Some(0) {
    |  _____________^
@@ -207,7 +226,7 @@ LL ~     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:213:13
+  --> $DIR/option_if_let_else.rs:223:13
    |
 LL |       let _ = match s {
    |  _____________^
@@ -217,7 +236,7 @@ LL | |     };
    | |_____^ help: try: `s.map_or(1, |string| string.len())`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:217:13
+  --> $DIR/option_if_let_else.rs:227:13
    |
 LL |       let _ = match Some(10) {
    |  _____________^
@@ -227,7 +246,7 @@ LL | |     };
    | |_____^ help: try: `Some(10).map_or(5, |a| a + 1)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:223:13
+  --> $DIR/option_if_let_else.rs:233:13
    |
 LL |       let _ = match res {
    |  _____________^
@@ -237,7 +256,7 @@ LL | |     };
    | |_____^ help: try: `res.map_or(1, |a| a + 1)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:227:13
+  --> $DIR/option_if_let_else.rs:237:13
    |
 LL |       let _ = match res {
    |  _____________^
@@ -247,10 +266,10 @@ LL | |     };
    | |_____^ help: try: `res.map_or(1, |a| a + 1)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:231:13
+  --> $DIR/option_if_let_else.rs:241:13
    |
 LL |     let _ = if let Ok(a) = res { a + 1 } else { 5 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `res.map_or(5, |a| a + 1)`
 
-error: aborting due to 20 previous errors
+error: aborting due to 21 previous errors