Properly parenthesize to avoid operator precedence errors

Author: JarredAllen

clippy_lints/src/option_if_let_else.rs

@@ -175,7 +175,12 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -
             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 let Some(Expr { kind: ExprKind::Match(
+                                _,
+                                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
@@ -184,8 +189,19 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -
                     }
                 }
             });
+            let parens_around_option = match &let_body.kind {
+                ExprKind::Call(..)
+                        | ExprKind::MethodCall(..)
+                        | ExprKind::Loop(..)
+                        | ExprKind::Match(..)
+                        | ExprKind::Block(..)
+                        | ExprKind::Field(..)
+                        | ExprKind::Path(_)
+                    => false,
+                _ => true,
+            };
             Some(OptionIfLetElseOccurence {
-                option: format!("{}", Sugg::hir(cx, let_body, "..")),
+                option: format!("{}{}{}", if parens_around_option { "(" } else { "" }, Sugg::hir(cx, let_body, ".."), if parens_around_option { ")" } else { "" }),
                 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, "..")),
@@ -209,7 +225,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse {
                 format!(
                     "{}{}.{}({}, {}){}",
                     if detection.wrap_braces { "{ " } else { "" },
-                    detection.option, detection.method_sugg, detection.none_expr, detection.some_expr,
+                    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,12 +5,16 @@ fn bad1(string: Option<&str>) -> (bool, &str) {
     string.map_or((false, "hello"), |x| (true, x))
 }
 
-fn bad2(string: Option<&str>) -> Option<(bool, &str)> {
+fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
     if string.is_none() {
         None
     } else { string.map_or(Some((false, "")), |x| Some((true, x))) }
 }
 
+fn unop_bad(string: &Option<&str>) -> usize {
+    (*string).map_or(0, |s| s.len())
+}
+
 fn longer_body(arg: Option<u32>) -> u32 {
     arg.map_or(13, |x| {
         let y = x * x;
@@ -48,7 +52,8 @@ fn main() {
     let optional = Some(5);
     let _ = optional.map_or(5, |x| x + 2);
     let _ = bad1(None);
-    let _ = bad2(None);
+    let _ = else_if_option(None);
+    let _ = unop_bad(&None);
     let _ = longer_body(None);
     test_map_or_else(None);
     let _ = negative_tests(None);

tests/ui/option_if_let_else.rs

@@ -9,7 +9,7 @@ fn bad1(string: Option<&str>) -> (bool, &str) {
     }
 }
 
-fn bad2(string: Option<&str>) -> Option<(bool, &str)> {
+fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
     if string.is_none() {
         None
     } else if let Some(x) = string {
@@ -19,6 +19,14 @@ fn bad2(string: Option<&str>) -> Option<(bool, &str)> {
     }
 }
 
+fn unop_bad(string: &Option<&str>) -> usize {
+    if let Some(s) = *string {
+        s.len()
+    } else {
+        0
+    }
+}
+
 fn longer_body(arg: Option<u32>) -> u32 {
     if let Some(x) = arg {
         let y = x * x;
@@ -60,7 +68,8 @@ fn main() {
     let optional = Some(5);
     let _ = if let Some(x) = optional { x + 2 } else { 5 };
     let _ = bad1(None);
-    let _ = bad2(None);
+    let _ = else_if_option(None);
+    let _ = unop_bad(&None);
     let _ = longer_body(None);
     test_map_or_else(None);
     let _ = negative_tests(None);

tests/ui/option_if_let_else.stderr

@@ -24,6 +24,16 @@ LL | |     }
 error: use Option::map_or instead of an if let/else
   --> $DIR/option_if_let_else.rs:23:5
    |
+LL | /     if let Some(s) = *string {
+LL | |         s.len()
+LL | |     } else {
+LL | |         0
+LL | |     }
+   | |_____^ 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:5
+   |
 LL | /     if let Some(x) = arg {
 LL | |         let y = x * x;
 LL | |         y * y
@@ -41,7 +51,7 @@ LL |     })
    |
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:32:13
+  --> $DIR/option_if_let_else.rs:40:13
    |
 LL |       let _ = if let Some(x) = arg {
    |  _____________^
@@ -64,10 +74,10 @@ LL |     }, |x| x * x * x * x);
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:61:13
+  --> $DIR/option_if_let_else.rs:69: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 5 previous errors
+error: aborting due to 6 previous errors