Use a Visitor to check for nested break, return, or continue statements

Author: JarredAllen

clippy_lints/src/option_if_let_else.rs

@@ -3,13 +3,16 @@ use crate::utils::{match_type, paths, span_lint_and_sugg};
 use if_chain::if_chain;
 
 use rustc_errors::Applicability;
+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};
 
 declare_clippy_lint! {
     /// **What it does:**
-    /// Lints usage of  `if let Some(v) = ... { y } else { x }` which is more 
+    /// Lints usage of  `if let Some(v) = ... { y } else { x }` which is more
     /// idiomatically done with `Option::map_or` (if the else bit is a simple
     /// expression) or `Option::map_or_else` (if the else bit is a longer
     /// block).
@@ -82,13 +85,60 @@ struct OptionIfLetElseOccurence {
     none_expr: String,
 }
 
+struct ReturnBreakContinueVisitor<'tcx> {
+    tcx: TyCtxt<'tcx>,
+    seen_return_break_continue: bool,
+}
+impl<'tcx> ReturnBreakContinueVisitor<'tcx> {
+    fn new(tcx: TyCtxt<'tcx>) -> ReturnBreakContinueVisitor<'tcx> {
+        ReturnBreakContinueVisitor {
+            seen_return_break_continue: false,
+            tcx,
+        }
+    }
+
+    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())
+    }
+
+    fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) {
+        if self.seen_return_break_continue {
+            // No need to look farther if we've already seen one of them
+            return;
+        }
+        match &ex.kind {
+            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(..) => {},
+            _ => {
+                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()
+}
+
 /// If this expression is the option if let/else construct we're detecting, then
 /// this function returns an OptionIfLetElseOccurence struct with details if
 /// this construct is found, or None if this construct is not found.
-fn detect_option_if_let_else<'a>(
-    cx: &LateContext<'_, '_>,
-    expr: &'a Expr<'_>,
-) -> Option<OptionIfLetElseOccurence> { //(String, String, String, String)> {
+fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -> Option<OptionIfLetElseOccurence> {
+    //(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;
@@ -103,14 +153,7 @@ fn detect_option_if_let_else<'a>(
                     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,
-                        }
-                    }) {
+                    if contains_return_break_continue(cx, statements) {
                         return None;
                     }
                     &arms[0].body
@@ -123,14 +166,7 @@ fn detect_option_if_let_else<'a>(
                 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,
-                        }
-                    }) {
+                    if contains_return_break_continue(cx, statements) {
                         return None;
                     }
                     (&arms[1].body, "map_or_else")
@@ -160,7 +196,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse {
                 expr.span,
                 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),
+                format!(
+                    "{}.{}({}, {})",
+                    detection.option, detection.method_sugg, detection.none_expr, detection.some_expr
+                ),
                 Applicability::MachineApplicable,
             );
         }

tests/ui/option_if_let_else.stderr

@@ -47,7 +47,7 @@ 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 |             y = (y + 2 / y) / 2;
 LL |         }
 LL |         y
  ...