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`]
rust-lang · Oct 23, 2023 · 9f5de66 · 9f5de66
2 parents f942470 + fb4f603
commit 9f5de66
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 29 deletions.
diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs
@@ -165,6 +165,12 @@ fn try_get_option_occurrence<'tcx>(
             }
 
             let mut app = Applicability::Unspecified;
+
+            let (none_body, is_argless_call) = match none_body.kind {
+                ExprKind::Call(call_expr, []) if !none_body.span.from_expansion() => (call_expr, true),
+                _ => (none_body, false),
+            };
+
             return Some(OptionOccurrence {
                 option: format_option_in_sugg(
                     Sugg::hir_with_context(cx, cond_expr, ctxt, "..", &mut app),
@@ -178,7 +184,7 @@ fn try_get_option_occurrence<'tcx>(
                 ),
                 none_expr: format!(
                     "{}{}",
-                    if method_sugg == "map_or" { "" } else if is_result { "|_| " } else { "|| "},
+                    if method_sugg == "map_or" || is_argless_call { "" } else if is_result { "|_| " } else { "|| "},
                     Sugg::hir_with_context(cx, none_body, ctxt, "..", &mut app),
                 ),
             });

diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed
@@ -1,7 +1,6 @@
 #![warn(clippy::option_if_let_else)]
 #![allow(
     unused_tuple_struct_fields,
-    clippy::redundant_closure,
     clippy::ref_option_ref,
     clippy::equatable_if_let,
     clippy::let_unit_value,
@@ -52,7 +51,7 @@ fn impure_else(arg: Option<i32>) {
         println!("return 1");
         1
     };
-    let _ = arg.map_or_else(|| side_effect(), |x| x);
+    let _ = arg.map_or_else(side_effect, |x| x);
 }
 
 fn test_map_or_else(arg: Option<u32>) {
@@ -224,3 +223,17 @@ mod issue10729 {
     fn do_something(_value: &str) {}
     fn do_something2(_value: &mut str) {}
 }
+
+fn issue11429() {
+    use std::collections::HashMap;
+
+    macro_rules! new_map {
+        () => {{ HashMap::new() }};
+    }
+
+    let opt: Option<HashMap<u8, u8>> = None;
+
+    let mut _hashmap = opt.as_ref().map_or_else(HashMap::new, |hm| hm.clone());
+
+    let mut _hm = opt.as_ref().map_or_else(|| new_map!(), |hm| hm.clone());
+}
diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs
@@ -1,7 +1,6 @@
 #![warn(clippy::option_if_let_else)]
 #![allow(
     unused_tuple_struct_fields,
-    clippy::redundant_closure,
     clippy::ref_option_ref,
     clippy::equatable_if_let,
     clippy::let_unit_value,
@@ -271,3 +270,21 @@ mod issue10729 {
     fn do_something(_value: &str) {}
     fn do_something2(_value: &mut str) {}
 }
+
+fn issue11429() {
+    use std::collections::HashMap;
+
+    macro_rules! new_map {
+        () => {{ HashMap::new() }};
+    }
+
+    let opt: Option<HashMap<u8, u8>> = None;
+
+    let mut _hashmap = if let Some(hm) = &opt {
+        hm.clone()
+    } else {
+        HashMap::new()
+    };
+
+    let mut _hm = if let Some(hm) = &opt { hm.clone() } else { new_map!() };
+}
diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr
@@ -1,5 +1,5 @@
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:12:5
+  --> $DIR/option_if_let_else.rs:11:5
    |
 LL | /     if let Some(x) = string {
 LL | |         (true, x)
@@ -12,19 +12,19 @@ LL | |     }
    = help: to override `-D warnings` add `#[allow(clippy::option_if_let_else)]`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:30:13
+  --> $DIR/option_if_let_else.rs:29:13
    |
 LL |     let _ = if let Some(s) = *string { s.len() } else { 0 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:31:13
+  --> $DIR/option_if_let_else.rs:30:13
    |
 LL |     let _ = if let Some(s) = &num { s } else { &0 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:32:13
+  --> $DIR/option_if_let_else.rs:31:13
    |
 LL |       let _ = if let Some(s) = &mut num {
    |  _____________^
@@ -44,13 +44,13 @@ LL ~     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:38:13
+  --> $DIR/option_if_let_else.rs:37:13
    |
 LL |     let _ = if let Some(ref s) = num { s } else { &0 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:39:13
+  --> $DIR/option_if_let_else.rs:38:13
    |
 LL |       let _ = if let Some(mut s) = num {
    |  _____________^
@@ -70,7 +70,7 @@ LL ~     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:45:13
+  --> $DIR/option_if_let_else.rs:44:13
    |
 LL |       let _ = if let Some(ref mut s) = num {
    |  _____________^
@@ -90,7 +90,7 @@ LL ~     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:54:5
+  --> $DIR/option_if_let_else.rs:53:5
    |
 LL | /     if let Some(x) = arg {
 LL | |         let y = x * x;
@@ -109,7 +109,7 @@ LL +     })
    |
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:67:13
+  --> $DIR/option_if_let_else.rs:66:13
    |
 LL |       let _ = if let Some(x) = arg {
    |  _____________^
@@ -118,10 +118,10 @@ LL | |     } else {
 LL | |         // map_or_else must be suggested
 LL | |         side_effect()
 LL | |     };
-   | |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)`
+   | |_____^ help: try: `arg.map_or_else(side_effect, |x| x)`
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:76:13
+  --> $DIR/option_if_let_else.rs:75:13
    |
 LL |       let _ = if let Some(x) = arg {
    |  _____________^
@@ -144,7 +144,7 @@ LL ~     }, |x| x * x * x * x);
    |
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:109:13
+  --> $DIR/option_if_let_else.rs:108:13
    |
 LL | /             if let Some(idx) = s.find('.') {
 LL | |                 vec![s[..idx].to_string(), s[idx..].to_string()]
@@ -154,7 +154,7 @@ 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
+  --> $DIR/option_if_let_else.rs:119:5
    |
 LL | /     if let Ok(binding) = variable {
 LL | |         println!("Ok {binding}");
@@ -173,13 +173,13 @@ LL +     })
    |
 
 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:141: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:152:13
+  --> $DIR/option_if_let_else.rs:151:13
    |
 LL |       let _ = if let Some(x) = Some(0) {
    |  _____________^
@@ -201,13 +201,13 @@ LL ~         });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:180:13
+  --> $DIR/option_if_let_else.rs:179: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:184:13
+  --> $DIR/option_if_let_else.rs:183:13
    |
 LL |       let _ = if let Some(x) = Some(0) {
    |  _____________^
@@ -227,7 +227,7 @@ LL ~     });
    |
 
 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:222:13
    |
 LL |       let _ = match s {
    |  _____________^
@@ -237,7 +237,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:227:13
+  --> $DIR/option_if_let_else.rs:226:13
    |
 LL |       let _ = match Some(10) {
    |  _____________^
@@ -247,7 +247,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:233:13
+  --> $DIR/option_if_let_else.rs:232:13
    |
 LL |       let _ = match res {
    |  _____________^
@@ -257,7 +257,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:237:13
+  --> $DIR/option_if_let_else.rs:236:13
    |
 LL |       let _ = match res {
    |  _____________^
@@ -267,13 +267,13 @@ 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:241:13
+  --> $DIR/option_if_let_else.rs:240: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:258:9
+  --> $DIR/option_if_let_else.rs:257:9
    |
 LL | /         match initial {
 LL | |             Some(value) => do_something(value),
@@ -282,13 +282,30 @@ LL | |         }
    | |_________^ help: try: `initial.as_ref().map_or({}, |value| do_something(value))`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:265:9
+  --> $DIR/option_if_let_else.rs:264:9
    |
 LL | /         match initial {
 LL | |             Some(value) => do_something2(value),
 LL | |             None => {},
 LL | |         }
    | |_________^ help: try: `initial.as_mut().map_or({}, |value| do_something2(value))`
 
-error: aborting due to 23 previous errors
+error: use Option::map_or_else instead of an if let/else
+  --> $DIR/option_if_let_else.rs:283:24
+   |
+LL |       let mut _hashmap = if let Some(hm) = &opt {
+   |  ________________________^
+LL | |         hm.clone()
+LL | |     } else {
+LL | |         HashMap::new()
+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
+   |
+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())`
+
+error: aborting due to 25 previous errors