Auto merge of #11896 - samueltardieu:issue-11893, r=Alexendoo

`option_if_let_else`: do not trigger on expressions returning `()`

Fix #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.

changelog: [`option_if_let_else`]: do not trigger on unit expressions

4 files changed, 85 insertions, 44 deletions

diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs
index cca90d813e0..89e4e3c740d 100644
--- a/clippy_lints/src/option_if_let_else.rs
+++ b/clippy_lints/src/option_if_let_else.rs
@@ -239,21 +239,24 @@ fn detect_option_if_let_else<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) ->
         if_then,
         if_else: Some(if_else),
     }) = higher::IfLet::hir(cx, expr)
+        && !cx.typeck_results().expr_ty(expr).is_unit()
+        && !is_else_clause(cx.tcx, expr)
     {
-        if !is_else_clause(cx.tcx, expr) {
-            return try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, let_expr, if_then, if_else);
-        }
+        try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, let_expr, if_then, if_else)
+    } else {
+        None
     }
-    None
 }
 
 fn detect_option_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option<OptionOccurrence> {
-    if let ExprKind::Match(ex, arms, MatchSource::Normal) = expr.kind {
-        if let Some((let_pat, if_then, if_else)) = try_convert_match(cx, arms) {
-            return try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, ex, if_then, if_else);
-        }
+    if let ExprKind::Match(ex, arms, MatchSource::Normal) = expr.kind
+        && !cx.typeck_results().expr_ty(expr).is_unit()
+        && let Some((let_pat, if_then, if_else)) = try_convert_match(cx, arms)
+    {
+        try_get_option_occurrence(cx, expr.span.ctxt(), let_pat, ex, if_then, if_else)
+    } else {
+        None
     }
-    None
 }
 
 fn try_convert_match<'tcx>(
diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed
index f0113ca696e..363520112ef 100644
--- a/tests/ui/option_if_let_else.fixed
+++ b/tests/ui/option_if_let_else.fixed
@@ -92,11 +92,13 @@ fn pattern_to_vec(pattern: &str) -> Vec<String> {
 }
 
 // #10335
-fn test_result_impure_else(variable: Result<u32, &str>) {
+fn test_result_impure_else(variable: Result<u32, &str>) -> bool {
     variable.map_or_else(|_| {
         println!("Err");
+        false
     }, |binding| {
         println!("Ok {binding}");
+        true
     })
 }
 
@@ -213,15 +215,19 @@ mod issue10729 {
 
     pub fn reproduce(initial: &Option<String>) {
         // 👇 needs `.as_ref()` because initial is an `&Option<_>`
-        initial.as_ref().map_or({}, |value| do_something(value))
+        let _ = initial.as_ref().map_or(42, |value| do_something(value));
     }
 
     pub fn reproduce2(initial: &mut Option<String>) {
-        initial.as_mut().map_or({}, |value| do_something2(value))
+        let _ = initial.as_mut().map_or(42, |value| do_something2(value));
     }
 
-    fn do_something(_value: &str) {}
-    fn do_something2(_value: &mut str) {}
+    fn do_something(_value: &str) -> u32 {
+        todo!()
+    }
+    fn do_something2(_value: &mut str) -> u32 {
+        todo!()
+    }
 }
 
 fn issue11429() {
@@ -237,3 +243,13 @@ fn issue11429() {
 
     let mut _hm = opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone());
 }
+
+fn issue11893() {
+    use std::io::Write;
+    let mut output = std::io::stdout().lock();
+    if let Some(name) = Some("stuff") {
+        writeln!(output, "{name:?}").unwrap();
+    } else {
+        panic!("Haven't thought about this condition.");
+    }
+}
diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs
index 18b7af44392..aaa87a0db54 100644
--- a/tests/ui/option_if_let_else.rs
+++ b/tests/ui/option_if_let_else.rs
@@ -115,11 +115,13 @@ fn pattern_to_vec(pattern: &str) -> Vec<String> {
 }
 
 // #10335
-fn test_result_impure_else(variable: Result<u32, &str>) {
+fn test_result_impure_else(variable: Result<u32, &str>) -> bool {
     if let Ok(binding) = variable {
         println!("Ok {binding}");
+        true
     } else {
         println!("Err");
+        false
     }
 }
 
@@ -254,21 +256,25 @@ mod issue10729 {
 
     pub fn reproduce(initial: &Option<String>) {
         // 👇 needs `.as_ref()` because initial is an `&Option<_>`
-        match initial {
+        let _ = match initial {
             Some(value) => do_something(value),
-            None => {},
-        }
+            None => 42,
+        };
     }
 
     pub fn reproduce2(initial: &mut Option<String>) {
-        match initial {
+        let _ = match initial {
             Some(value) => do_something2(value),
-            None => {},
-        }
+            None => 42,
+        };
     }
 
-    fn do_something(_value: &str) {}
-    fn do_something2(_value: &mut str) {}
+    fn do_something(_value: &str) -> u32 {
+        todo!()
+    }
+    fn do_something2(_value: &mut str) -> u32 {
+        todo!()
+    }
 }
 
 fn issue11429() {
@@ -288,3 +294,13 @@ fn issue11429() {
 
     let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() };
 }
+
+fn issue11893() {
+    use std::io::Write;
+    let mut output = std::io::stdout().lock();
+    if let Some(name) = Some("stuff") {
+        writeln!(output, "{name:?}").unwrap();
+    } else {
+        panic!("Haven't thought about this condition.");
+    }
+}
diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr
index e36357bcb38..55a8360ffd0 100644
--- a/tests/ui/option_if_let_else.stderr
+++ b/tests/ui/option_if_let_else.stderr
@@ -158,8 +158,10 @@ error: use Option::map_or_else instead of an if let/else
    |
 LL | /     if let Ok(binding) = variable {
 LL | |         println!("Ok {binding}");
+LL | |         true
 LL | |     } else {
 LL | |         println!("Err");
+LL | |         false
 LL | |     }
    | |_____^
    |
@@ -167,19 +169,21 @@ help: try
    |
 LL ~     variable.map_or_else(|_| {
 LL +         println!("Err");
+LL +         false
 LL +     }, |binding| {
 LL +         println!("Ok {binding}");
+LL +         true
 LL +     })
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:141:13
+  --> $DIR/option_if_let_else.rs:143: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:151:13
+  --> $DIR/option_if_let_else.rs:153:13
    |
 LL |       let _ = if let Some(x) = Some(0) {
    |  _____________^
@@ -201,13 +205,13 @@ LL ~         });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:179:13
+  --> $DIR/option_if_let_else.rs:181: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:183:13
+  --> $DIR/option_if_let_else.rs:185:13
    |
 LL |       let _ = if let Some(x) = Some(0) {
    |  _____________^
@@ -227,7 +231,7 @@ LL ~     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:222:13
+  --> $DIR/option_if_let_else.rs:224:13
    |
 LL |       let _ = match s {
    |  _____________^
@@ -237,7 +241,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:226:13
+  --> $DIR/option_if_let_else.rs:228:13
    |
 LL |       let _ = match Some(10) {
    |  _____________^
@@ -247,7 +251,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:232:13
+  --> $DIR/option_if_let_else.rs:234:13
    |
 LL |       let _ = match res {
    |  _____________^
@@ -257,7 +261,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:236:13
+  --> $DIR/option_if_let_else.rs:238:13
    |
 LL |       let _ = match res {
    |  _____________^
@@ -267,31 +271,33 @@ 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:240:13
+  --> $DIR/option_if_let_else.rs:242:13
    |
 LL |     let _ = if let Ok(a) = res { a + 1 } else { 5 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `res.map_or(5, |a| a + 1)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:257:9
+  --> $DIR/option_if_let_else.rs:259:17
    |
-LL | /         match initial {
+LL |           let _ = match initial {
+   |  _________________^
 LL | |             Some(value) => do_something(value),
-LL | |             None => {},
-LL | |         }
-   | |_________^ help: try: `initial.as_ref().map_or({}, |value| do_something(value))`
+LL | |             None => 42,
+LL | |         };
+   | |_________^ help: try: `initial.as_ref().map_or(42, |value| do_something(value))`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:264:9
+  --> $DIR/option_if_let_else.rs:266:17
    |
-LL | /         match initial {
+LL |           let _ = match initial {
+   |  _________________^
 LL | |             Some(value) => do_something2(value),
-LL | |             None => {},
-LL | |         }
-   | |_________^ help: try: `initial.as_mut().map_or({}, |value| do_something2(value))`
+LL | |             None => 42,
+LL | |         };
+   | |_________^ help: try: `initial.as_mut().map_or(42, |value| do_something2(value))`
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:283:24
+  --> $DIR/option_if_let_else.rs:289:24
    |
 LL |       let mut _hashmap = if let Some(hm) = &opt {
    |  ________________________^
@@ -302,7 +308,7 @@ LL | |     };
    | |_____^ help: try: `opt.as_ref().map_or_else(HashMap::new, |hm| hm.clone())`
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:289:19
+  --> $DIR/option_if_let_else.rs:295:19
    |
 LL |     let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() };
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone())`