Stop linting on macros and correctly use braces for constructs

Author: JarredAllen

clippy_lints/src/option_if_let_else.rs

@@ -1,3 +1,4 @@
+use crate::utils;
 use crate::utils::sugg::Sugg;
 use crate::utils::{match_type, paths, span_lint_and_sugg};
 use if_chain::if_chain;
@@ -89,6 +90,7 @@ struct OptionIfLetElseOccurence {
     method_sugg: String,
     some_expr: String,
     none_expr: String,
+    wrap_braces: bool,
 }
 
 struct ReturnBreakContinueVisitor<'tcx> {
@@ -140,6 +142,7 @@ fn contains_return_break_continue<'tcx>(expression: &'tcx Expr<'tcx>) -> bool {
 fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -> Option<OptionIfLetElseOccurence> {
     //(String, String, String, String)> {
     if_chain! {
+        // if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly
         if let ExprKind::Match(let_body, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind;
         if arms.len() == 2;
         if match_type(cx, &cx.tables.expr_ty(let_body), &paths::OPTION);
@@ -170,11 +173,23 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -
                 return None;
             };
             let capture_name = id.name.to_ident_string();
+            let wrap_braces = utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| {
+                if_chain! {
+                    if let Some(Expr { kind: ExprKind::Match(condition, arms, MatchSource::IfDesugar{contains_else_clause: true}|MatchSource::IfLetDesugar{contains_else_clause: true}), .. } ) = parent.expr;
+                    if expr.hir_id == arms[1].body.hir_id;
+                    then {
+                        true
+                    } else {
+                        false
+                    }
+                }
+            });
             Some(OptionIfLetElseOccurence {
                 option: format!("{}", Sugg::hir(cx, let_body, "..")),
                 method_sugg: format!("{}", method_sugg),
                 some_expr: format!("|{}| {}", capture_name, Sugg::hir(cx, some_body, "..")),
-                none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, ".."))
+                none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, "..")),
+                wrap_braces,
             })
         } else {
             None
@@ -192,8 +207,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse {
                 format!("use Option::{} instead of an if let/else", detection.method_sugg).as_str(),
                 "try",
                 format!(
-                    "{}.{}({}, {})",
-                    detection.option, detection.method_sugg, detection.none_expr, detection.some_expr
+                    "{}{}.{}({}, {}){}",
+                    if detection.wrap_braces { "{ " } else { "" },
+                    detection.option, detection.method_sugg, detection.none_expr, detection.some_expr,
+                    if detection.wrap_braces { " }" } else { "" },
                 ),
                 Applicability::MachineApplicable,
             );

tests/ui/option_if_let_else.fixed

@@ -5,6 +5,12 @@ fn bad1(string: Option<&str>) -> (bool, &str) {
     string.map_or((false, "hello"), |x| (true, x))
 }
 
+fn bad2(string: Option<&str>) -> Option<(bool, &str)> {
+    if string.is_none() {
+        None
+    } else { string.map_or(Some((false, "")), |x| Some((true, x))) }
+}
+
 fn longer_body(arg: Option<u32>) -> u32 {
     arg.map_or(13, |x| {
         let y = x * x;
@@ -42,6 +48,7 @@ fn main() {
     let optional = Some(5);
     let _ = optional.map_or(5, |x| x + 2);
     let _ = bad1(None);
+    let _ = bad2(None);
     let _ = longer_body(None);
     test_map_or_else(None);
     let _ = negative_tests(None);

tests/ui/option_if_let_else.rs

@@ -9,6 +9,16 @@ fn bad1(string: Option<&str>) -> (bool, &str) {
     }
 }
 
+fn bad2(string: Option<&str>) -> Option<(bool, &str)> {
+    if string.is_none() {
+        None
+    } else if let Some(x) = string {
+        Some((true, x))
+    } else {
+        Some((false, ""))
+    }
+}
+
 fn longer_body(arg: Option<u32>) -> u32 {
     if let Some(x) = arg {
         let y = x * x;
@@ -50,6 +60,7 @@ fn main() {
     let optional = Some(5);
     let _ = if let Some(x) = optional { x + 2 } else { 5 };
     let _ = bad1(None);
+    let _ = bad2(None);
     let _ = longer_body(None);
     test_map_or_else(None);
     let _ = negative_tests(None);

tests/ui/option_if_let_else.stderr

@@ -11,7 +11,18 @@ 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:13:5
+  --> $DIR/option_if_let_else.rs:15:12
+   |
+LL |       } else if let Some(x) = string {
+   |  ____________^
+LL | |         Some((true, x))
+LL | |     } else {
+LL | |         Some((false, ""))
+LL | |     }
+   | |_____^ help: try: `{ string.map_or(Some((false, "")), |x| Some((true, x))) }`
+
+error: use Option::map_or instead of an if let/else
+  --> $DIR/option_if_let_else.rs:23:5
    |
 LL | /     if let Some(x) = arg {
 LL | |         let y = x * x;
@@ -30,7 +41,7 @@ LL |     })
    |
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:22:13
+  --> $DIR/option_if_let_else.rs:32:13
    |
 LL |       let _ = if let Some(x) = arg {
    |  _____________^
@@ -53,10 +64,10 @@ LL |     }, |x| x * x * x * x);
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:51:13
+  --> $DIR/option_if_let_else.rs:61:13
    |
 LL |     let _ = if let Some(x) = optional { x + 2 } else { 5 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
 
-error: aborting due to 4 previous errors
+error: aborting due to 5 previous errors