From 438f934f1c330cc43693a2b9ca404f3c704c2ca1 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Mon, 4 Sep 2023 16:15:45 +0800 Subject: [PATCH 1/3] suggest passing function instead of calling it in [`option_if_let_else`] --- clippy_lints/src/option_if_let_else.rs | 19 +++++++- tests/ui/option_if_let_else.fixed | 17 ++++++- tests/ui/option_if_let_else.rs | 19 +++++++- tests/ui/option_if_let_else.stderr | 67 ++++++++++++++++---------- 4 files changed, 93 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index a7a7f4fd8fa2..156b95d644ae 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -165,6 +165,13 @@ fn try_get_option_occurrence<'tcx>( } let mut app = Applicability::Unspecified; + + let (none_body, is_argless_call) = if let Some(call_expr) = try_get_argless_call_expr(none_body) { + (call_expr, true) + } else { + (none_body, false) + }; + return Some(OptionOccurrence { option: format_option_in_sugg( Sugg::hir_with_context(cx, cond_expr, ctxt, "..", &mut app), @@ -178,7 +185,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), ), }); @@ -188,6 +195,16 @@ fn try_get_option_occurrence<'tcx>( None } +/// Gets the call expr iff it does not have any args and was not from macro expansion. +fn try_get_argless_call_expr<'tcx>(expr: &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { + if !expr.span.from_expansion() && + let ExprKind::Call(call_expr, []) = expr.kind + { + return Some(call_expr); + } + None +} + 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); diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed index c3415a7df9e3..f0113ca696e1 100644 --- 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) { 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) { @@ -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> = 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 index 86537f62057b..18b7af443925 100644 --- 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> = 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 index aa2da2174003..fe436a349c57 100644 --- 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) @@ -11,19 +11,19 @@ LL | | } = note: `-D clippy::option-if-let-else` implied by `-D warnings` 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 { | _____________^ @@ -43,13 +43,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 { | _____________^ @@ -69,7 +69,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 { | _____________^ @@ -89,7 +89,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; @@ -108,7 +108,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 { | _____________^ @@ -117,10 +117,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 { | _____________^ @@ -143,7 +143,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()] @@ -153,7 +153,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}"); @@ -172,13 +172,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) { | _____________^ @@ -200,13 +200,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) { | _____________^ @@ -226,7 +226,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 { | _____________^ @@ -236,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:227:13 + --> $DIR/option_if_let_else.rs:226:13 | LL | let _ = match Some(10) { | _____________^ @@ -246,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:233:13 + --> $DIR/option_if_let_else.rs:232:13 | LL | let _ = match res { | _____________^ @@ -256,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:237:13 + --> $DIR/option_if_let_else.rs:236:13 | LL | let _ = match res { | _____________^ @@ -266,13 +266,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), @@ -281,7 +281,7 @@ 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), @@ -289,5 +289,22 @@ 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 From 22ba7925d6e7b08fe84ff315b95e2bffd058275f Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Tue, 12 Sep 2023 17:18:39 +0800 Subject: [PATCH 2/3] Update clippy_lints/src/option_if_let_else.rs Co-authored-by: Samuel Tardieu --- clippy_lints/src/option_if_let_else.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index 156b95d644ae..f94dbb40be7f 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -197,12 +197,10 @@ fn try_get_option_occurrence<'tcx>( /// Gets the call expr iff it does not have any args and was not from macro expansion. fn try_get_argless_call_expr<'tcx>(expr: &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { - if !expr.span.from_expansion() && - let ExprKind::Call(call_expr, []) = expr.kind - { - return Some(call_expr); + match expr.kind { + ExprKind::Call(call_expr, []) if !expr.span.from_expansion() => Some(call_expr), + _ => None, } - None } fn try_get_inner_pat_and_is_result<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>) -> Option<(&'tcx Pat<'tcx>, bool)> { From fb4f6035da41ada636bef3a7eed1ec4dae72c606 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Thu, 14 Sep 2023 10:17:27 +0800 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Samuel Tardieu Co-authored-by: Catherine Flores --- clippy_lints/src/option_if_let_else.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index f94dbb40be7f..cfe40a829ed2 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -166,10 +166,9 @@ fn try_get_option_occurrence<'tcx>( let mut app = Applicability::Unspecified; - let (none_body, is_argless_call) = if let Some(call_expr) = try_get_argless_call_expr(none_body) { - (call_expr, true) - } else { - (none_body, false) + 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 { @@ -195,14 +194,6 @@ fn try_get_option_occurrence<'tcx>( None } -/// Gets the call expr iff it does not have any args and was not from macro expansion. -fn try_get_argless_call_expr<'tcx>(expr: &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { - match expr.kind { - ExprKind::Call(call_expr, []) if !expr.span.from_expansion() => Some(call_expr), - _ => None, - } -} - 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);