More suggestions from flip1995

Author: JarredAllen

clippy_lints/src/option_if_let_else.rs

@@ -7,9 +7,10 @@ use rustc_hir::intravisit::{NestedVisitorMap, Visitor};
 use rustc_hir::*;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::Map;
-use rustc_middle::ty::TyCtxt;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
+use std::marker::PhantomData;
+
 declare_clippy_lint! {
     /// **What it does:**
     /// Lints usage of  `if let Some(v) = ... { y } else { x }` which is more
@@ -28,6 +29,11 @@ declare_clippy_lint! {
     /// an expensive function, then it would be more efficient to use
     /// `Option::map_or_else`, but this lint would suggest `Option::map_or`.
     ///
+    /// Also, this lint uses a deliberately conservative metric for checking
+    /// if the inside of either body contains breaks or continues which will
+    /// cause it to not suggest a fix if either block contains a loop with
+    /// continues or breaks contained within the loop.
+    ///
     /// **Example:**
     ///
     /// ```rust
@@ -86,25 +92,21 @@ struct OptionIfLetElseOccurence {
 }
 
 struct ReturnBreakContinueVisitor<'tcx> {
-    tcx: TyCtxt<'tcx>,
     seen_return_break_continue: bool,
+    phantom_data: PhantomData<&'tcx bool>,
 }
 impl<'tcx> ReturnBreakContinueVisitor<'tcx> {
-    fn new(tcx: TyCtxt<'tcx>) -> ReturnBreakContinueVisitor<'tcx> {
+    fn new() -> ReturnBreakContinueVisitor<'tcx> {
         ReturnBreakContinueVisitor {
             seen_return_break_continue: false,
-            tcx,
+            phantom_data: PhantomData,
         }
     }
-
-    fn seen_return_break_continue(&self) -> bool {
-        self.seen_return_break_continue
-    }
 }
 impl<'tcx> Visitor<'tcx> for ReturnBreakContinueVisitor<'tcx> {
     type Map = Map<'tcx>;
     fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
-        NestedVisitorMap::OnlyBodies(self.tcx.hir())
+        NestedVisitorMap::None
     }
 
     fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) {
@@ -116,22 +118,20 @@ impl<'tcx> Visitor<'tcx> for ReturnBreakContinueVisitor<'tcx> {
             ExprKind::Ret(..) | ExprKind::Break(..) | ExprKind::Continue(..) => {
                 self.seen_return_break_continue = true;
             },
-            // Do nothing if encountering a loop, because internal breaks and
-            // continues shouldn't be flagged
-            ExprKind::Loop(..) => {},
+            // Something special could be done here to handle while or for loop
+            // desugaring, as this will detect a break if there's a while loop
+            // or a for loop inside the expression.
             _ => {
                 rustc_hir::intravisit::walk_expr(self, ex);
             },
         }
     }
 }
 
-fn contains_return_break_continue<'tcx>(cx: &LateContext<'_, 'tcx>, statements: &'tcx [Stmt<'tcx>]) -> bool {
-    let mut recursive_visitor: ReturnBreakContinueVisitor<'tcx> = ReturnBreakContinueVisitor::new(cx.tcx);
-    for statement in statements {
-        recursive_visitor.visit_stmt(statement);
-    }
-    recursive_visitor.seen_return_break_continue()
+fn contains_return_break_continue<'tcx>(expression: &'tcx Expr<'tcx>) -> bool {
+    let mut recursive_visitor: ReturnBreakContinueVisitor<'tcx> = ReturnBreakContinueVisitor::new();
+    recursive_visitor.visit_expr(expression);
+    recursive_visitor.seen_return_break_continue
 }
 
 /// If this expression is the option if let/else construct we're detecting, then
@@ -146,16 +146,14 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -
         if !is_result_ok(cx, let_body); // Don't lint on Result::ok because a different lint does it already
         if let PatKind::TupleStruct(_, &[inner_pat], _) = &arms[0].pat.kind;
         if let PatKind::Binding(_, _, id, _) = &inner_pat.kind;
+        if !contains_return_break_continue(arms[0].body);
+        if !contains_return_break_continue(arms[1].body);
         then {
             let some_body = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _)
                 = &arms[0].body.kind {
                 if let &[] = &statements {
                     expr
                 } else {
-                    // Don't trigger if there is a return, break, or continue inside
-                    if contains_return_break_continue(cx, statements) {
-                        return None;
-                    }
                     &arms[0].body
                 }
             } else {
@@ -166,9 +164,6 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -
                 if let &[] = &statements {
                     (expr, "map_or")
                 } else {
-                    if contains_return_break_continue(cx, statements) {
-                        return None;
-                    }
                     (&arms[1].body, "map_or_else")
                 }
             } else {
@@ -179,7 +174,7 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -
                 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, ".."))
             })
         } else {
             None

tests/ui/option_if_let_else.fixed

@@ -13,11 +13,10 @@ fn longer_body(arg: Option<u32>) -> u32 {
 }
 
 fn test_map_or_else(arg: Option<u32>) {
-    let _ = arg.map_or_else(||{
+    let _ = arg.map_or_else(|| {
         let mut y = 1;
-        for _ in 0..10 {
-            y = (y + 2 / y) / 2;
-        }
+        y = (y + 2 / y) / 2;
+        y = (y + 2 / y) / 2;
         y
     }, |x| x * x * x * x);
 }

tests/ui/option_if_let_else.rs

@@ -23,9 +23,8 @@ fn test_map_or_else(arg: Option<u32>) {
         x * x * x * x
     } else {
         let mut y = 1;
-        for _ in 0..10 {
-            y = (y + 2 / y) / 2;
-        }
+        y = (y + 2 / y) / 2;
+        y = (y + 2 / y) / 2;
         y
     };
 }

tests/ui/option_if_let_else.stderr

@@ -44,16 +44,16 @@ LL | |     };
    |
 help: try
    |
-LL |     let _ = arg.map_or_else(||{
+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 = (y + 2 / y) / 2;
+LL |         y = (y + 2 / y) / 2;
 LL |         y
- ...
+LL |     }, |x| x * x * x * x);
+   |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:52:13
+  --> $DIR/option_if_let_else.rs:51:13
    |
 LL |     let _ = if let Some(x) = optional { x + 2 } else { 5 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`