Don't trigger on blocks containing return, break, or continue

JarredAllen · JarredAllen · commit 038ab65caf88 · 2020-04-14T13:30:06.000-07:00
diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs
@@ -70,38 +70,68 @@ declare_clippy_lint! {
 
 declare_lint_pass!(OptionIfLetElse => [OPTION_IF_LET_ELSE]);
 
-/// If this expression is the option if let/else construct we're
-/// detecting, then this function returns Some(Option<_> compared,
-/// expression if Option is Some, expression if Option is None).
-/// Otherwise, it returns None.
-fn detect_option_if_let_else<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr<'_>) -> Option<(String, String, String)> {
+/// If this expression is the option if let/else construct we're detecting, then
+/// this function returns Some(Option<_> tested against, the method to call on
+/// the object (map_or or map_or_else), expression if Option is Some,
+/// expression if Option is None). Otherwise, it returns None.
+fn detect_option_if_let_else<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr<'_>) -> Option<(String, String, String, String)> {
     if_chain! {
         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);
         if let PatKind::TupleStruct(path, &[inner_pat], _) = &arms[0].pat.kind;
         if let PatKind::Binding(_, _, id, _) = &inner_pat.kind;
         then {
-            let (mut some_body, mut none_body) = if match_qpath(path, &paths::OPTION_SOME) {
+            let (some_body, none_body) = if match_qpath(path, &paths::OPTION_SOME) {
                 (arms[0].body, arms[1].body)
             } else {
                 (arms[1].body, arms[0].body)
             };
-            some_body = if let ExprKind::Block(Block { stmts: &[], expr: Some(expr), .. }, _) = &some_body.kind {
-                expr
+            let some_body = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _) = &some_body.kind {
+                if let &[] = &statements {
+                    expr
+                } else {
+                    // Don't trigger if there is a return, break, or continue inside
+                    if statements.iter().any(|statement| {
+                        match &statement {
+                            Stmt { kind: StmtKind::Semi(Expr { kind: ExprKind::Ret(..), ..}), .. } => true,
+                            Stmt { kind: StmtKind::Semi(Expr { kind: ExprKind::Break(..), ..}), .. } => true,
+                            Stmt { kind: StmtKind::Semi(Expr { kind: ExprKind::Continue(..), ..}), .. } => true,
+                            _ => false,
+                        }
+                    }) {
+                        return None;
+                    }
+                    some_body
+                }
             } else {
-                some_body
+                return None;
             };
-            none_body = if let ExprKind::Block(Block { stmts: &[], expr: Some(expr), .. }, _) = &none_body.kind {
-                expr
+            let (none_body, method_sugg) = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _) = &none_body.kind {
+                if let &[] = &statements {
+                    (expr, "map_or")
+                } else {
+                    if statements.iter().any(|statement| {
+                        match &statement {
+                            Stmt { kind: StmtKind::Semi(Expr { kind: ExprKind::Ret(..), ..}), .. } => true,
+                            Stmt { kind: StmtKind::Semi(Expr { kind: ExprKind::Break(..), ..}), .. } => true,
+                            Stmt { kind: StmtKind::Semi(Expr { kind: ExprKind::Continue(..), ..}), .. } => true,
+                            _ => false,
+                        }
+                    }) {
+                        return None;
+                    }
+                    (&none_body, "map_or_else")
+                }
             } else {
-                none_body
+                return None;
             };
             let capture_name = id.name.to_ident_string();
             Some((
                 format!("{}", Sugg::hir(cx, let_body, "..")),
+                format!("{}", method_sugg),
                 format!("|{}| {}", capture_name, Sugg::hir(cx, some_body, "..")),
-                format!("{}", Sugg::hir(cx, none_body, ".."))
+                format!("{}{}", if method_sugg == "map_or" { "" } else { "||" }, Sugg::hir(cx, none_body, ".."))
             ))
         } else {
             None
@@ -111,15 +141,15 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr<'_>) -
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
-        if let Some((option, map, else_func)) = detect_option_if_let_else(cx, expr) {
+        if let Some((option, method, map, else_func)) = detect_option_if_let_else(cx, expr) {
             span_lint_and_sugg(
                 cx,
                 OPTION_IF_LET_ELSE,
                 expr.span,
-                "use Option::map_or instead of an if let/else",
+                format!("use Option::{} instead of an if let/else", method).as_str(),
                 "try",
-                format!("{}.map_or({}, {})", option, else_func, map),
-                Applicability::MaybeIncorrect,
+                format!("{}.{}({}, {})", option, method, else_func, map),
+                Applicability::MachineApplicable,
             );
         }
     }
diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed
@@ -0,0 +1,49 @@
+#![warn(clippy::option_if_let_else)]
+// run-rustfix
+
+fn bad1(string: Option<&str>) -> (bool, &str) {
+    string.map_or((false, "hello"), |x| (true, x))
+}
+
+fn longer_body(arg: Option<u32>) -> u32 {
+    arg.map_or(13, |x| {
+        let y = x * x;
+        y * y
+    })
+}
+
+fn test_map_or_else(arg: Option<u32>) {
+    let _ = arg.map_or_else(||{
+        let mut y = 1;
+        for _ in 0..10 {
+            y = (y + 2/y) / 2;
+        }
+        y
+    }, |x| x * x * x * x);
+}
+
+fn negative_tests(arg: Option<u32>) -> u32 {
+    let _ = if let Some(13) = arg { "unlucky" } else { "lucky" };
+    for _ in 0..10 {
+        let _ = if let Some(x) = arg {
+            x
+        } else {
+            continue;
+        };
+    }
+    let _ = if let Some(x) = arg {
+        return x;
+    } else {
+        5
+    };
+    7
+}
+
+fn main() {
+    let optional = Some(5);
+    let _ = optional.map_or(5, |x| x + 2);
+    let _ = bad1(None);
+    let _ = longer_body(None);
+    test_map_or_else(None);
+    let _ = negative_tests(None);
+}
diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs
@@ -1,4 +1,5 @@
 #![warn(clippy::option_if_let_else)]
+// run-rustfix
 
 fn bad1(string: Option<&str>) -> (bool, &str) {
     if let Some(x) = string {
@@ -17,13 +18,40 @@ fn longer_body(arg: Option<u32>) -> u32 {
     }
 }
 
-fn negative_tests(arg: Option<u32>) {
+fn test_map_or_else(arg: Option<u32>) {
+    let _ = if let Some(x) = arg {
+        x * x * x * x
+    } else {
+        let mut y = 1;
+        for _ in 0..10 {
+            y = (y + 2/y) / 2;
+        }
+        y
+    };
+}
+
+fn negative_tests(arg: Option<u32>) -> u32 {
     let _ = if let Some(13) = arg { "unlucky" } else { "lucky" };
+    for _ in 0..10 {
+        let _ = if let Some(x) = arg {
+            x
+        } else {
+            continue;
+        };
+    }
+    let _ = if let Some(x) = arg {
+        return x;
+    } else {
+        5
+    };
+    7
 }
 
 fn main() {
     let optional = Some(5);
     let _ = if let Some(x) = optional { x + 2 } else { 5 };
     let _ = bad1(None);
     let _ = longer_body(None);
+    test_map_or_else(None);
+    let _ = negative_tests(None);
 }
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:4:5
+  --> $DIR/option_if_let_else.rs:5:5
    |
 LL | /     if let Some(x) = string {
 LL | |         (true, x)
@@ -11,7 +11,7 @@ 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:12:5
+  --> $DIR/option_if_let_else.rs:13:5
    |
 LL | /     if let Some(x) = arg {
 LL | |         let y = x * x;
@@ -29,11 +29,34 @@ LL |         y * y
 LL |     })
    |
 
+error: use Option::map_or_else instead of an if let/else
+  --> $DIR/option_if_let_else.rs:22:13
+   |
+LL |       let _ = if let Some(x) = arg {
+   |  _____________^
+LL | |         x * x * x * x
+LL | |     } else {
+LL | |         let mut y = 1;
+...  |
+LL | |         y
+LL | |     };
+   | |_____^
+   |
+help: try
+   |
+LL |     let _ = arg.map_or_else(||{
+LL |         let mut y = 1;
+LL |         for _ in 0..10 {
+LL |             y = (y + 2/y) / 2;
+LL |         }
+LL |         y
+ ...
+
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:26:13
+  --> $DIR/option_if_let_else.rs:52: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 3 previous errors
+error: aborting due to 4 previous errors
 
