Auto merge of #4522 - mikerite:fix-4514, r=phansch

Fix `or_fun_call` bad suggestion Closes #4514 changelog: Fix `or_fun_call` bad suggestion
rust-lang · Sep 9, 2019 · c733376 · c733376
2 parents 8af4e09 + f88c224
commit c733376
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 25 deletions.
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -1376,6 +1376,7 @@ fn lint_or_fun_call<'a, 'tcx>(
 
             let mut finder = FunCallFinder { cx: &cx, found: false };
             if { finder.visit_expr(&arg); finder.found };
+            if !contains_return(&arg);
 
             let self_ty = cx.tables.expr_ty(self_expr);
 
@@ -2189,28 +2190,6 @@ fn lint_option_and_then_some(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &
     const LINT_MSG: &str = "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`";
     const NO_OP_MSG: &str = "using `Option.and_then(Some)`, which is a no-op";
 
-    // Searches an return expressions in `y` in `_.and_then(|x| Some(y))`, which we don't lint
-    struct RetCallFinder {
-        found: bool,
-    }
-
-    impl<'tcx> intravisit::Visitor<'tcx> for RetCallFinder {
-        fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
-            if self.found {
-                return;
-            }
-            if let hir::ExprKind::Ret(..) = &expr.node {
-                self.found = true;
-            } else {
-                intravisit::walk_expr(self, expr);
-            }
-        }
-
-        fn nested_visit_map<'this>(&'this mut self) -> intravisit::NestedVisitorMap<'this, 'tcx> {
-            intravisit::NestedVisitorMap::None
-        }
-    }
-
     let ty = cx.tables.expr_ty(&args[0]);
     if !match_type(cx, ty, &paths::OPTION) {
         return;
@@ -2228,9 +2207,7 @@ fn lint_option_and_then_some(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &
                 then {
                     let inner_expr = &some_args[0];
 
-                    let mut finder = RetCallFinder { found: false };
-                    finder.visit_expr(inner_expr);
-                    if finder.found {
+                    if contains_return(inner_expr) {
                         return;
                     }
 
@@ -2987,3 +2964,31 @@ fn is_bool(ty: &hir::Ty) -> bool {
         false
     }
 }
+
+// Returns `true` if `expr` contains a return expression
+fn contains_return(expr: &hir::Expr) -> bool {
+    struct RetCallFinder {
+        found: bool,
+    }
+
+    impl<'tcx> intravisit::Visitor<'tcx> for RetCallFinder {
+        fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
+            if self.found {
+                return;
+            }
+            if let hir::ExprKind::Ret(..) = &expr.node {
+                self.found = true;
+            } else {
+                intravisit::walk_expr(self, expr);
+            }
+        }
+
+        fn nested_visit_map<'this>(&'this mut self) -> intravisit::NestedVisitorMap<'this, 'tcx> {
+            intravisit::NestedVisitorMap::None
+        }
+    }
+
+    let mut visitor = RetCallFinder{ found: false };
+    visitor.visit_expr(expr);
+    visitor.found
+}
diff --git a/tests/ui/or_fun_call.fixed b/tests/ui/or_fun_call.fixed
@@ -99,4 +99,16 @@ fn test_or_with_ctors() {
         .or(Some(Bar(b, Duration::from_secs(2))));
 }
 
+
+// Issue 4514 - early return
+fn f() -> Option<()> {
+    let a = Some(1);
+    let b = 1i32;
+
+    let _ = a.unwrap_or(b.checked_mul(3)?.min(240));
+
+    Some(())
+}
+
+
 fn main() {}
diff --git a/tests/ui/or_fun_call.rs b/tests/ui/or_fun_call.rs
@@ -99,4 +99,16 @@ fn test_or_with_ctors() {
         .or(Some(Bar(b, Duration::from_secs(2))));
 }
 
+
+// Issue 4514 - early return
+fn f() -> Option<()> {
+    let a = Some(1);
+    let b = 1i32;
+
+    let _ = a.unwrap_or(b.checked_mul(3)?.min(240));
+
+    Some(())
+}
+
+
 fn main() {}