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

[`needless_return`] Recursively remove unneeded semicolons

fix #8336,
fix #8156,
fix #7358,
fix #9192,
fix #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.

Author: bors

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,62 +160,67 @@ 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);
+                    }
+                },
+                _ => (),
+            }
         }
     }
 }
 
 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() {
                 let borrows = inner.map_or(false, |inner| last_statement_borrows(cx, inner));
                 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 {

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() {}

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() {}