Auto merge of #3555 - daxpedda:master, r=oli-obk

Fix `implicit_return` false positives.

Fixing some false positives on the `implicit_return` lint.

Basically it should only check for missing return statements in `block.stmts.last()` if it's a `break`, otherwise it should skip because it would either be an error, or a false positive in the case of a `loop` (which I'm trying to fix with this PR).

**Question:**
- I say "we" inside of comments ([`// make sure it's a break, otherwise we want to skip`](https://github.com/rust-lang/rust-clippy/pull/3555/files#diff-11d233fe8c8414214c2b8732b8c9877aR71)). Any alternatives or is that okay?
- I named a test [`test_loop_with_nests()`](https://github.com/rust-lang/rust-clippy/blob/6870638c3fb66c2abb20633bf40cc09ccc760047/tests/ui/implicit_return.rs#L54-L64), any better suggestions?

Author: bors

clippy_lints/src/implicit_return.rs

@@ -45,6 +45,19 @@ declare_clippy_lint! {
 pub struct Pass;
 
 impl Pass {
+    fn lint(cx: &LateContext<'_, '_>, outer_span: syntax_pos::Span, inner_span: syntax_pos::Span, msg: &str) {
+        span_lint_and_then(cx, IMPLICIT_RETURN, outer_span, "missing return statement", |db| {
+            if let Some(snippet) = snippet_opt(cx, inner_span) {
+                db.span_suggestion_with_applicability(
+                    outer_span,
+                    msg,
+                    format!("return {}", snippet),
+                    Applicability::MachineApplicable,
+                );
+            }
+        });
+    }
+
     fn expr_match(cx: &LateContext<'_, '_>, expr: &rustc::hir::Expr) {
         match &expr.node {
             // loops could be using `break` instead of `return`
@@ -55,23 +68,19 @@ impl Pass {
                 // only needed in the case of `break` with `;` at the end
                 else if let Some(stmt) = block.stmts.last() {
                     if let rustc::hir::StmtKind::Semi(expr, ..) = &stmt.node {
-                        Self::expr_match(cx, expr);
+                        // make sure it's a break, otherwise we want to skip
+                        if let ExprKind::Break(.., break_expr) = &expr.node {
+                            if let Some(break_expr) = break_expr {
+                                Self::lint(cx, expr.span, break_expr.span, "change `break` to `return` as shown");
+                            }
+                        }
                     }
                 }
             },
             // use `return` instead of `break`
             ExprKind::Break(.., break_expr) => {
                 if let Some(break_expr) = break_expr {
-                    span_lint_and_then(cx, IMPLICIT_RETURN, expr.span, "missing return statement", |db| {
-                        if let Some(snippet) = snippet_opt(cx, break_expr.span) {
-                            db.span_suggestion_with_applicability(
-                                expr.span,
-                                "change `break` to `return` as shown",
-                                format!("return {}", snippet),
-                                Applicability::MachineApplicable,
-                            );
-                        }
-                    });
+                    Self::lint(cx, expr.span, break_expr.span, "change `break` to `return` as shown");
                 }
             },
             ExprKind::If(.., if_expr, else_expr) => {
@@ -89,16 +98,7 @@ impl Pass {
             // skip if it already has a return statement
             ExprKind::Ret(..) => (),
             // everything else is missing `return`
-            _ => span_lint_and_then(cx, IMPLICIT_RETURN, expr.span, "missing return statement", |db| {
-                if let Some(snippet) = snippet_opt(cx, expr.span) {
-                    db.span_suggestion_with_applicability(
-                        expr.span,
-                        "add `return` as shown",
-                        format!("return {}", snippet),
-                        Applicability::MachineApplicable,
-                    );
-                }
-            }),
+            _ => Self::lint(cx, expr.span, expr.span, "add `return` as shown"),
         }
     }
 }

tests/ui/implicit_return.rs

@@ -42,6 +42,27 @@ fn test_loop() -> bool {
     }
 }
 
+#[allow(clippy::never_loop)]
+fn test_loop_with_block() -> bool {
+    loop {
+        {
+            break true;
+        }
+    }
+}
+
+#[allow(clippy::never_loop)]
+fn test_loop_with_nests() -> bool {
+    loop {
+        if true {
+            break true;
+        }
+        else {
+            let _ = true;
+        }
+    }
+}
+
 fn test_closure() {
     #[rustfmt::skip]
     let _ = || { true };
@@ -53,5 +74,7 @@ fn main() {
     let _ = test_if_block();
     let _ = test_match(true);
     let _ = test_loop();
+    let _ = test_loop_with_block();
+    let _ = test_loop_with_nests();
     test_closure();
 }

tests/ui/implicit_return.stderr

@@ -37,16 +37,28 @@ error: missing return statement
    |         ^^^^^^^^^^ help: change `break` to `return` as shown: `return true`
 
 error: missing return statement
-  --> $DIR/implicit_return.rs:47:18
+  --> $DIR/implicit_return.rs:49:13
    |
-47 |     let _ = || { true };
+49 |             break true;
+   |             ^^^^^^^^^^ help: change `break` to `return` as shown: `return true`
+
+error: missing return statement
+  --> $DIR/implicit_return.rs:58:13
+   |
+58 |             break true;
+   |             ^^^^^^^^^^ help: change `break` to `return` as shown: `return true`
+
+error: missing return statement
+  --> $DIR/implicit_return.rs:68:18
+   |
+68 |     let _ = || { true };
    |                  ^^^^ help: add `return` as shown: `return true`
 
 error: missing return statement
-  --> $DIR/implicit_return.rs:48:16
+  --> $DIR/implicit_return.rs:69:16
    |
-48 |     let _ = || true;
+69 |     let _ = || true;
    |                ^^^^ help: add `return` as shown: `return true`
 
-error: aborting due to 8 previous errors
+error: aborting due to 10 previous errors