1 files changed, 83 insertions, 59 deletions

diff --git a/src/tools/clippy/clippy_lints/src/returns.rs b/src/tools/clippy/clippy_lints/src/returns.rs
index bbbd9e498..f0d7dd23a 100644
--- a/src/tools/clippy/clippy_lints/src/returns.rs
+++ b/src/tools/clippy/clippy_lints/src/returns.rs
@@ -1,18 +1,20 @@
 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::visitors::{for_each_expr, Descend};
-use clippy_utils::{fn_def_id, path_to_local_id};
+use clippy_utils::{fn_def_id, path_to_local_id, span_find_starting_semi};
 use core::ops::ControlFlow;
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::FnKind;
-use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, HirId, LangItem, MatchSource, PatKind, QPath, StmtKind};
+use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, LangItem, MatchSource, PatKind, QPath, StmtKind};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_middle::ty::subst::GenericArgKind;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::def_id::LocalDefId;
 use rustc_span::source_map::Span;
 use rustc_span::{BytePos, Pos};
+use std::borrow::Cow;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -68,31 +70,41 @@ declare_clippy_lint! {
     "using a return statement like `return expr;` where an expression would suffice"
 }
 
-#[derive(PartialEq, Eq, Copy, Clone)]
-enum RetReplacement {
+#[derive(PartialEq, Eq, Clone)]
+enum RetReplacement<'tcx> {
     Empty,
     Block,
     Unit,
+    IfSequence(Cow<'tcx, str>, Applicability),
+    Expr(Cow<'tcx, str>, Applicability),
 }
 
-impl RetReplacement {
+impl<'tcx> RetReplacement<'tcx> {
     fn sugg_help(self) -> &'static str {
         match self {
-            Self::Empty => "remove `return`",
+            Self::Empty | Self::Expr(..) => "remove `return`",
             Self::Block => "replace `return` with an empty block",
             Self::Unit => "replace `return` with a unit value",
+            Self::IfSequence(..) => "remove `return` and wrap the sequence with parentheses",
+        }
+    }
+    fn applicability(&self) -> Option<Applicability> {
+        match self {
+            Self::Expr(_, ap) | Self::IfSequence(_, ap) => Some(*ap),
+            _ => None,
         }
     }
 }
 
-impl ToString for RetReplacement {
+impl<'tcx> ToString for RetReplacement<'tcx> {
     fn to_string(&self) -> String {
-        match *self {
-            Self::Empty => "",
-            Self::Block => "{}",
-            Self::Unit => "()",
+        match self {
+            Self::Empty => String::new(),
+            Self::Block => "{}".to_string(),
+            Self::Unit => "()".to_string(),
+            Self::IfSequence(inner, _) => format!("({inner})"),
+            Self::Expr(inner, _) => inner.to_string(),
         }
-        .to_string()
     }
 }
 
@@ -151,8 +163,8 @@ impl<'tcx> LateLintPass<'tcx> for Return {
         kind: FnKind<'tcx>,
         _: &'tcx FnDecl<'tcx>,
         body: &'tcx Body<'tcx>,
-        _: Span,
-        _: HirId,
+        sp: Span,
+        _: LocalDefId,
     ) {
         match kind {
             FnKind::Closure => {
@@ -166,14 +178,14 @@ impl<'tcx> LateLintPass<'tcx> for Return {
                 check_final_expr(cx, body.value, vec![], replacement);
             },
             FnKind::ItemFn(..) | FnKind::Method(..) => {
-                check_block_return(cx, &body.value.kind, vec![]);
+                check_block_return(cx, &body.value.kind, sp, vec![]);
             },
         }
     }
 }
 
 // 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>) {
+fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>, sp: Span, mut 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);
@@ -183,12 +195,14 @@ fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>,
                     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);
+                    // Remove ending semicolons and any whitespace ' ' in between.
+                    // Without `return`, the suggestion might not compile if the semicolon is retained
+                    if let Some(semi_span) = stmt.span.trim_start(semi_expr.span) {
+                        let semi_span_to_remove =
+                            span_find_starting_semi(cx.sess().source_map(), semi_span.with_hi(sp.hi()));
+                        semi_spans.push(semi_span_to_remove);
                     }
+                    check_final_expr(cx, semi_expr, semi_spans, RetReplacement::Empty);
                 },
                 _ => (),
             }
@@ -201,19 +215,38 @@ fn check_final_expr<'tcx>(
     expr: &'tcx Expr<'tcx>,
     semi_spans: Vec<Span>, /* containing all the places where we would need to remove semicolons if finding an
                             * needless return */
-    replacement: RetReplacement,
+    replacement: RetReplacement<'tcx>,
 ) {
     let peeled_drop_expr = expr.peel_drop_temps();
     match &peeled_drop_expr.kind {
         // simple return is always "bad"
         ExprKind::Ret(ref inner) => {
-            // if desugar of `do yeet`, don't lint
-            if let Some(inner_expr) = inner
-                && let ExprKind::Call(path_expr, _) = inner_expr.kind
-                && let ExprKind::Path(QPath::LangItem(LangItem::TryTraitFromYeet, _, _)) = path_expr.kind
-            {
-                return;
-            }
+            // check if expr return nothing
+            let ret_span = if inner.is_none() && replacement == RetReplacement::Empty {
+                extend_span_to_previous_non_ws(cx, peeled_drop_expr.span)
+            } else {
+                peeled_drop_expr.span
+            };
+
+            let replacement = if let Some(inner_expr) = inner {
+                // if desugar of `do yeet`, don't lint
+                if let ExprKind::Call(path_expr, _) = inner_expr.kind
+                    && let ExprKind::Path(QPath::LangItem(LangItem::TryTraitFromYeet, _, _)) = path_expr.kind
+                {
+                    return;
+                }
+
+                let mut applicability = Applicability::MachineApplicable;
+                let (snippet, _) = snippet_with_context(cx, inner_expr.span, ret_span.ctxt(), "..", &mut applicability);
+                if expr_contains_conjunctive_ifs(inner_expr) {
+                    RetReplacement::IfSequence(snippet, applicability)
+                } else {
+                    RetReplacement::Expr(snippet, applicability)
+                }
+            } else {
+                replacement
+            };
+
             if !cx.tcx.hir().attrs(expr.hir_id).is_empty() {
                 return;
             }
@@ -221,19 +254,13 @@ fn check_final_expr<'tcx>(
             if borrows {
                 return;
             }
-            // check if expr return nothing
-            let ret_span = if inner.is_none() && replacement == RetReplacement::Empty {
-                extend_span_to_previous_non_ws(cx, peeled_drop_expr.span)
-            } else {
-                peeled_drop_expr.span
-            };
 
-            emit_return_lint(cx, ret_span, semi_spans, inner.as_ref().map(|i| i.span), replacement);
+            emit_return_lint(cx, ret_span, semi_spans, replacement);
         },
         ExprKind::If(_, then, else_clause_opt) => {
-            check_block_return(cx, &then.kind, semi_spans.clone());
+            check_block_return(cx, &then.kind, peeled_drop_expr.span, semi_spans.clone());
             if let Some(else_clause) = else_clause_opt {
-                check_block_return(cx, &else_clause.kind, semi_spans);
+                check_block_return(cx, &else_clause.kind, peeled_drop_expr.span, semi_spans);
             }
         },
         // a match expr, check all arms
@@ -246,33 +273,29 @@ fn check_final_expr<'tcx>(
             }
         },
         // if it's a whole block, check it
-        other_expr_kind => check_block_return(cx, other_expr_kind, semi_spans),
+        other_expr_kind => check_block_return(cx, other_expr_kind, peeled_drop_expr.span, semi_spans),
     }
 }
 
-fn emit_return_lint(
-    cx: &LateContext<'_>,
-    ret_span: Span,
-    semi_spans: Vec<Span>,
-    inner_span: Option<Span>,
-    replacement: RetReplacement,
-) {
+fn expr_contains_conjunctive_ifs<'tcx>(expr: &'tcx Expr<'tcx>) -> bool {
+    fn contains_if(expr: &Expr<'_>, on_if: bool) -> bool {
+        match expr.kind {
+            ExprKind::If(..) => on_if,
+            ExprKind::Binary(_, left, right) => contains_if(left, true) || contains_if(right, true),
+            _ => false,
+        }
+    }
+
+    contains_if(expr, false)
+}
+
+fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, semi_spans: Vec<Span>, replacement: RetReplacement<'_>) {
     if ret_span.from_expansion() {
         return;
     }
-    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()
-    };
+    let applicability = replacement.applicability().unwrap_or(Applicability::MachineApplicable);
+    let return_replacement = replacement.to_string();
+    let sugg_help = 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
@@ -288,6 +311,7 @@ fn last_statement_borrows<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>)
             && cx
                 .tcx
                 .fn_sig(def_id)
+                .subst_identity()
                 .skip_binder()
                 .output()
                 .walk()