Auto merge of #9497 - kraktus:needless_return2, r=llogiq

[`needless_return`] Recursively remove unneeded semicolons

fix #8336,
fix #8156,
fix https://github.com/rust-lang/rust-clippy/issues/7358,
fix #9192,
fix https://github.com/rust-lang/rust-clippy/issues/9503

changelog: [`needless_return`] Recursively remove unneeded semicolons

For now the suggestion about removing the semicolons are hidden because they would be very noisy and should be obvious if the user wants to apply the lint manually instead of using `--fix`. This could be an issue for beginner, but haven't found better way to display it.

4 files changed, 316 insertions, 136 deletions

diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs
index 91553240e3c..f758f4cff8b 100644
--- a/clippy_lints/src/returns.rs
+++ b/clippy_lints/src/returns.rs
@@ -1,4 +1,4 @@
-use clippy_utils::diagnostics::span_lint_hir_and_then;
+use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then};
 use clippy_utils::source::{snippet_opt, snippet_with_context};
 use clippy_utils::{fn_def_id, path_to_local_id};
 use if_chain::if_chain;
@@ -72,6 +72,27 @@ enum RetReplacement {
     Unit,
 }
 
+impl RetReplacement {
+    fn sugg_help(self) -> &'static str {
+        match self {
+            Self::Empty => "remove `return`",
+            Self::Block => "replace `return` with an empty block",
+            Self::Unit => "replace `return` with a unit value",
+        }
+    }
+}
+
+impl ToString for RetReplacement {
+    fn to_string(&self) -> String {
+        match *self {
+            Self::Empty => "",
+            Self::Block => "{}",
+            Self::Unit => "()",
+        }
+        .to_string()
+    }
+}
+
 declare_lint_pass!(Return => [LET_AND_RETURN, NEEDLESS_RETURN]);
 
 impl<'tcx> LateLintPass<'tcx> for Return {
@@ -139,26 +160,35 @@ impl<'tcx> LateLintPass<'tcx> for Return {
                 } else {
                     RetReplacement::Empty
                 };
-                check_final_expr(cx, body.value, Some(body.value.span), replacement);
+                check_final_expr(cx, body.value, vec![], replacement);
             },
             FnKind::ItemFn(..) | FnKind::Method(..) => {
-                if let ExprKind::Block(block, _) = body.value.kind {
-                    check_block_return(cx, block);
-                }
+                check_block_return(cx, &body.value.kind, vec![]);
             },
         }
     }
 }
 
-fn check_block_return<'tcx>(cx: &LateContext<'tcx>, block: &Block<'tcx>) {
-    if let Some(expr) = block.expr {
-        check_final_expr(cx, expr, Some(expr.span), RetReplacement::Empty);
-    } else if let Some(stmt) = block.stmts.iter().last() {
-        match stmt.kind {
-            StmtKind::Expr(expr) | StmtKind::Semi(expr) => {
-                check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty);
-            },
-            _ => (),
+// if `expr` is a block, check if there are needless returns in it
+fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>, semi_spans: Vec<Span>) {
+    if let ExprKind::Block(block, _) = expr_kind {
+        if let Some(block_expr) = block.expr {
+            check_final_expr(cx, block_expr, semi_spans, RetReplacement::Empty);
+        } else if let Some(stmt) = block.stmts.iter().last() {
+            match stmt.kind {
+                StmtKind::Expr(expr) => {
+                    check_final_expr(cx, expr, semi_spans, RetReplacement::Empty);
+                },
+                StmtKind::Semi(semi_expr) => {
+                    let mut semi_spans_and_this_one = semi_spans;
+                    // we only want the span containing the semicolon so we can remove it later. From `entry.rs:382`
+                    if let Some(semicolon_span) = stmt.span.trim_start(semi_expr.span) {
+                        semi_spans_and_this_one.push(semicolon_span);
+                        check_final_expr(cx, semi_expr, semi_spans_and_this_one, RetReplacement::Empty);
+                    }
+                },
+                _ => (),
+            }
         }
     }
 }
@@ -166,10 +196,12 @@ fn check_block_return<'tcx>(cx: &LateContext<'tcx>, block: &Block<'tcx>) {
 fn check_final_expr<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx Expr<'tcx>,
-    span: Option<Span>,
+    semi_spans: Vec<Span>, /* containing all the places where we would need to remove semicolons if finding an
+                            * needless return */
     replacement: RetReplacement,
 ) {
-    match expr.kind {
+    let peeled_drop_expr = expr.peel_drop_temps();
+    match &peeled_drop_expr.kind {
         // simple return is always "bad"
         ExprKind::Ret(ref inner) => {
             if cx.tcx.hir().attrs(expr.hir_id).is_empty() {
@@ -177,24 +209,18 @@ fn check_final_expr<'tcx>(
                 if !borrows {
                     emit_return_lint(
                         cx,
-                        inner.map_or(expr.hir_id, |inner| inner.hir_id),
-                        span.expect("`else return` is not possible"),
+                        peeled_drop_expr.span,
+                        semi_spans,
                         inner.as_ref().map(|i| i.span),
                         replacement,
                     );
                 }
             }
         },
-        // a whole block? check it!
-        ExprKind::Block(block, _) => {
-            check_block_return(cx, block);
-        },
         ExprKind::If(_, then, else_clause_opt) => {
-            if let ExprKind::Block(ifblock, _) = then.kind {
-                check_block_return(cx, ifblock);
-            }
+            check_block_return(cx, &then.kind, semi_spans.clone());
             if let Some(else_clause) = else_clause_opt {
-                check_final_expr(cx, else_clause, None, RetReplacement::Empty);
+                check_block_return(cx, &else_clause.kind, semi_spans);
             }
         },
         // a match expr, check all arms
@@ -203,93 +229,44 @@ fn check_final_expr<'tcx>(
         // (except for unit type functions) so we don't match it
         ExprKind::Match(_, arms, MatchSource::Normal) => {
             for arm in arms.iter() {
-                check_final_expr(cx, arm.body, Some(arm.body.span), RetReplacement::Unit);
+                check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Unit);
             }
         },
-        ExprKind::DropTemps(expr) => check_final_expr(cx, expr, None, RetReplacement::Empty),
-        _ => (),
+        // if it's a whole block, check it
+        other_expr_kind => check_block_return(cx, other_expr_kind, semi_spans),
     }
 }
 
 fn emit_return_lint(
     cx: &LateContext<'_>,
-    emission_place: HirId,
     ret_span: Span,
+    semi_spans: Vec<Span>,
     inner_span: Option<Span>,
     replacement: RetReplacement,
 ) {
     if ret_span.from_expansion() {
         return;
     }
-    match inner_span {
-        Some(inner_span) => {
-            let mut applicability = Applicability::MachineApplicable;
-            span_lint_hir_and_then(
-                cx,
-                NEEDLESS_RETURN,
-                emission_place,
-                ret_span,
-                "unneeded `return` statement",
-                |diag| {
-                    let (snippet, _) = snippet_with_context(cx, inner_span, ret_span.ctxt(), "..", &mut applicability);
-                    diag.span_suggestion(ret_span, "remove `return`", snippet, applicability);
-                },
-            );
-        },
-        None => match replacement {
-            RetReplacement::Empty => {
-                span_lint_hir_and_then(
-                    cx,
-                    NEEDLESS_RETURN,
-                    emission_place,
-                    ret_span,
-                    "unneeded `return` statement",
-                    |diag| {
-                        diag.span_suggestion(
-                            ret_span,
-                            "remove `return`",
-                            String::new(),
-                            Applicability::MachineApplicable,
-                        );
-                    },
-                );
-            },
-            RetReplacement::Block => {
-                span_lint_hir_and_then(
-                    cx,
-                    NEEDLESS_RETURN,
-                    emission_place,
-                    ret_span,
-                    "unneeded `return` statement",
-                    |diag| {
-                        diag.span_suggestion(
-                            ret_span,
-                            "replace `return` with an empty block",
-                            "{}".to_string(),
-                            Applicability::MachineApplicable,
-                        );
-                    },
-                );
-            },
-            RetReplacement::Unit => {
-                span_lint_hir_and_then(
-                    cx,
-                    NEEDLESS_RETURN,
-                    emission_place,
-                    ret_span,
-                    "unneeded `return` statement",
-                    |diag| {
-                        diag.span_suggestion(
-                            ret_span,
-                            "replace `return` with a unit value",
-                            "()".to_string(),
-                            Applicability::MachineApplicable,
-                        );
-                    },
-                );
-            },
+    let mut applicability = Applicability::MachineApplicable;
+    let return_replacement = inner_span.map_or_else(
+        || replacement.to_string(),
+        |inner_span| {
+            let (snippet, _) = snippet_with_context(cx, inner_span, ret_span.ctxt(), "..", &mut applicability);
+            snippet.to_string()
         },
-    }
+    );
+    let sugg_help = if inner_span.is_some() {
+        "remove `return`"
+    } else {
+        replacement.sugg_help()
+    };
+    span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded `return` statement", |diag| {
+        diag.span_suggestion_hidden(ret_span, sugg_help, return_replacement, applicability);
+        // for each parent statement, we need to remove the semicolon
+        for semi_stmt_span in semi_spans {
+            diag.tool_only_span_suggestion(semi_stmt_span, "remove this semicolon", "", applicability);
+        }
+    });
 }
 
 fn last_statement_borrows<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed
index 87c8fc03b3c..112a2f57b41 100644
--- a/tests/ui/needless_return.fixed
+++ b/tests/ui/needless_return.fixed
@@ -233,4 +233,41 @@ fn issue_9361() -> i32 {
     return 1 + 2;
 }
 
+fn issue8336(x: i32) -> bool {
+    if x > 0 {
+        println!("something");
+        true
+    } else {
+        false
+    }
+}
+
+fn issue8156(x: u8) -> u64 {
+    match x {
+        80 => {
+            10
+        },
+        _ => {
+            100
+        },
+    }
+}
+
+// Ideally the compiler should throw `unused_braces` in this case
+fn issue9192() -> i32 {
+    {
+        0
+    }
+}
+
+fn issue9503(x: usize) -> isize {
+    unsafe {
+        if x > 12 {
+            *(x as *const isize)
+        } else {
+            !*(x as *const isize)
+        }
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs
index 5a86e656255..22aa2e11fad 100644
--- a/tests/ui/needless_return.rs
+++ b/tests/ui/needless_return.rs
@@ -233,4 +233,41 @@ fn issue_9361() -> i32 {
     return 1 + 2;
 }
 
+fn issue8336(x: i32) -> bool {
+    if x > 0 {
+        println!("something");
+        return true;
+    } else {
+        return false;
+    };
+}
+
+fn issue8156(x: u8) -> u64 {
+    match x {
+        80 => {
+            return 10;
+        },
+        _ => {
+            return 100;
+        },
+    };
+}
+
+// Ideally the compiler should throw `unused_braces` in this case
+fn issue9192() -> i32 {
+    {
+        return 0;
+    };
+}
+
+fn issue9503(x: usize) -> isize {
+    unsafe {
+        if x > 12 {
+            return *(x as *const isize);
+        } else {
+            return !*(x as *const isize);
+        };
+    };
+}
+
 fn main() {}
diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr
index 83ff0763869..45090dbe206 100644
--- a/tests/ui/needless_return.stderr
+++ b/tests/ui/needless_return.stderr
@@ -2,225 +2,354 @@ error: unneeded `return` statement
   --> $DIR/needless_return.rs:27:5
    |
 LL |     return true;
-   |     ^^^^^^^^^^^^ help: remove `return`: `true`
+   |     ^^^^^^^^^^^
    |
    = note: `-D clippy::needless-return` implied by `-D warnings`
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:31:5
    |
 LL |     return true;
-   |     ^^^^^^^^^^^^ help: remove `return`: `true`
+   |     ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:36:9
    |
 LL |         return true;
-   |         ^^^^^^^^^^^^ help: remove `return`: `true`
+   |         ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:38:9
    |
 LL |         return false;
-   |         ^^^^^^^^^^^^^ help: remove `return`: `false`
+   |         ^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:44:17
    |
 LL |         true => return false,
-   |                 ^^^^^^^^^^^^ help: remove `return`: `false`
+   |                 ^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:46:13
    |
 LL |             return true;
-   |             ^^^^^^^^^^^^ help: remove `return`: `true`
+   |             ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:53:9
    |
 LL |         return true;
-   |         ^^^^^^^^^^^^ help: remove `return`: `true`
+   |         ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:55:16
    |
 LL |     let _ = || return true;
-   |                ^^^^^^^^^^^ help: remove `return`: `true`
+   |                ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:59:5
    |
 LL |     return the_answer!();
-   |     ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `the_answer!()`
+   |     ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:63:5
    |
 LL |     return;
-   |     ^^^^^^^ help: remove `return`
+   |     ^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:68:9
    |
 LL |         return;
-   |         ^^^^^^^ help: remove `return`
+   |         ^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:70:9
    |
 LL |         return;
-   |         ^^^^^^^ help: remove `return`
+   |         ^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:77:14
    |
 LL |         _ => return,
-   |              ^^^^^^ help: replace `return` with a unit value: `()`
+   |              ^^^^^^
+   |
+   = help: replace `return` with a unit value
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:86:13
    |
 LL |             return;
-   |             ^^^^^^^ help: remove `return`
+   |             ^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:88:14
    |
 LL |         _ => return,
-   |              ^^^^^^ help: replace `return` with a unit value: `()`
+   |              ^^^^^^
+   |
+   = help: replace `return` with a unit value
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:101:9
    |
 LL |         return String::from("test");
-   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::from("test")`
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:103:9
    |
 LL |         return String::new();
-   |         ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::new()`
+   |         ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:125:32
    |
 LL |         bar.unwrap_or_else(|_| return)
-   |                                ^^^^^^ help: replace `return` with an empty block: `{}`
+   |                                ^^^^^^
+   |
+   = help: replace `return` with an empty block
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:130:13
    |
 LL |             return;
-   |             ^^^^^^^ help: remove `return`
+   |             ^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:132:20
    |
 LL |         let _ = || return;
-   |                    ^^^^^^ help: replace `return` with an empty block: `{}`
+   |                    ^^^^^^
+   |
+   = help: replace `return` with an empty block
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:138:32
    |
 LL |         res.unwrap_or_else(|_| return Foo)
-   |                                ^^^^^^^^^^ help: remove `return`: `Foo`
+   |                                ^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:147:5
    |
 LL |     return true;
-   |     ^^^^^^^^^^^^ help: remove `return`: `true`
+   |     ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:151:5
    |
 LL |     return true;
-   |     ^^^^^^^^^^^^ help: remove `return`: `true`
+   |     ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:156:9
    |
 LL |         return true;
-   |         ^^^^^^^^^^^^ help: remove `return`: `true`
+   |         ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:158:9
    |
 LL |         return false;
-   |         ^^^^^^^^^^^^^ help: remove `return`: `false`
+   |         ^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:164:17
    |
 LL |         true => return false,
-   |                 ^^^^^^^^^^^^ help: remove `return`: `false`
+   |                 ^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:166:13
    |
 LL |             return true;
-   |             ^^^^^^^^^^^^ help: remove `return`: `true`
+   |             ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:173:9
    |
 LL |         return true;
-   |         ^^^^^^^^^^^^ help: remove `return`: `true`
+   |         ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:175:16
    |
 LL |     let _ = || return true;
-   |                ^^^^^^^^^^^ help: remove `return`: `true`
+   |                ^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:179:5
    |
 LL |     return the_answer!();
-   |     ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `the_answer!()`
+   |     ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:183:5
    |
 LL |     return;
-   |     ^^^^^^^ help: remove `return`
+   |     ^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:188:9
    |
 LL |         return;
-   |         ^^^^^^^ help: remove `return`
+   |         ^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:190:9
    |
 LL |         return;
-   |         ^^^^^^^ help: remove `return`
+   |         ^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:197:14
    |
 LL |         _ => return,
-   |              ^^^^^^ help: replace `return` with a unit value: `()`
+   |              ^^^^^^
+   |
+   = help: replace `return` with a unit value
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:210:9
    |
 LL |         return String::from("test");
-   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::from("test")`
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:212:9
    |
 LL |         return String::new();
-   |         ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::new()`
+   |         ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
 error: unneeded `return` statement
   --> $DIR/needless_return.rs:228:5
    |
 LL |     return format!("Hello {}", "world!");
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `format!("Hello {}", "world!")`
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: remove `return`
+
+error: unneeded `return` statement
+  --> $DIR/needless_return.rs:239:9
+   |
+LL |         return true;
+   |         ^^^^^^^^^^^
+   |
+   = help: remove `return`
+
+error: unneeded `return` statement
+  --> $DIR/needless_return.rs:241:9
+   |
+LL |         return false;
+   |         ^^^^^^^^^^^^
+   |
+   = help: remove `return`
+
+error: unneeded `return` statement
+  --> $DIR/needless_return.rs:248:13
+   |
+LL |             return 10;
+   |             ^^^^^^^^^
+   |
+   = help: remove `return`
+
+error: unneeded `return` statement
+  --> $DIR/needless_return.rs:251:13
+   |
+LL |             return 100;
+   |             ^^^^^^^^^^
+   |
+   = help: remove `return`
+
+error: unneeded `return` statement
+  --> $DIR/needless_return.rs:259:9
+   |
+LL |         return 0;
+   |         ^^^^^^^^
+   |
+   = help: remove `return`
+
+error: unneeded `return` statement
+  --> $DIR/needless_return.rs:266:13
+   |
+LL |             return *(x as *const isize);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: remove `return`
+
+error: unneeded `return` statement
+  --> $DIR/needless_return.rs:268:13
+   |
+LL |             return !*(x as *const isize);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: remove `return`
 
-error: aborting due to 37 previous errors
+error: aborting due to 44 previous errors