add some adjustment regarding review suggestion

4 files changed, 61 insertions, 35 deletions

diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs
index 84bcef856d0..f0d7dd23a67 100644
--- a/clippy_lints/src/returns.rs
+++ b/clippy_lints/src/returns.rs
@@ -14,6 +14,7 @@ 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
@@ -70,33 +71,39 @@ declare_clippy_lint! {
 }
 
 #[derive(PartialEq, Eq, Clone)]
-enum RetReplacement {
+enum RetReplacement<'tcx> {
     Empty,
     Block,
     Unit,
-    IfSequence(String),
-    Expr(String),
+    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 | Self::Expr(_) => "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",
+            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 => String::new(),
             Self::Block => "{}".to_string(),
             Self::Unit => "()".to_string(),
-            Self::IfSequence(inner) => format!("({inner})"),
-            Self::Expr(inner) => inner.clone(),
+            Self::IfSequence(inner, _) => format!("({inner})"),
+            Self::Expr(inner, _) => inner.to_string(),
         }
     }
 }
@@ -208,7 +215,7 @@ 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 {
@@ -229,17 +236,12 @@ fn check_final_expr<'tcx>(
                     return;
                 }
 
-                let (snippet, _) = snippet_with_context(
-                    cx,
-                    inner_expr.span,
-                    ret_span.ctxt(),
-                    "..",
-                    &mut Applicability::MachineApplicable,
-                );
-                if expr_contains_if(inner_expr) {
-                    RetReplacement::IfSequence(snippet.to_string())
+                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.to_string())
+                    RetReplacement::Expr(snippet, applicability)
                 }
             } else {
                 replacement
@@ -275,19 +277,23 @@ fn check_final_expr<'tcx>(
     }
 }
 
-fn expr_contains_if<'tcx>(expr: &'tcx Expr<'tcx>) -> bool {
-    match expr.kind {
-        ExprKind::If(..) => true,
-        ExprKind::Binary(_, left, right) => expr_contains_if(left) || expr_contains_if(right),
-        _ => false,
+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) {
+fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, semi_spans: Vec<Span>, replacement: RetReplacement<'_>) {
     if ret_span.from_expansion() {
         return;
     }
-    let applicability = Applicability::MachineApplicable;
+    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| {
diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed
index c77554fb47b..0f525dd294c 100644
--- a/tests/ui/needless_return.fixed
+++ b/tests/ui/needless_return.fixed
@@ -297,8 +297,14 @@ fn issue10051() -> Result<String, String> {
     }
 }
 
-fn issue10049(b1: bool, b2: bool, b3: bool) -> u32 {
-    (if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 })
+mod issue10049 {
+    fn single() -> u32 {
+        if true { 1 } else { 2 }
+    }
+
+    fn multiple(b1: bool, b2: bool, b3: bool) -> u32 {
+        (if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 })
+    }
 }
 
 fn main() {}
diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs
index 8fed64ac6e3..a1db8375d95 100644
--- a/tests/ui/needless_return.rs
+++ b/tests/ui/needless_return.rs
@@ -307,8 +307,14 @@ fn issue10051() -> Result<String, String> {
     }
 }
 
-fn issue10049(b1: bool, b2: bool, b3: bool) -> u32 {
-    return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 };
+mod issue10049 {
+    fn single() -> u32 {
+        return if true { 1 } else { 2 };
+    }
+
+    fn multiple(b1: bool, b2: bool, b3: bool) -> u32 {
+        return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 };
+    }
 }
 
 fn main() {}
diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr
index 18edbce2f44..87d0cd3e14c 100644
--- a/tests/ui/needless_return.stderr
+++ b/tests/ui/needless_return.stderr
@@ -419,12 +419,20 @@ LL |         return Err(format!("err!"));
    = help: remove `return`
 
 error: unneeded `return` statement
-  --> $DIR/needless_return.rs:311:5
+  --> $DIR/needless_return.rs:312:9
    |
-LL |     return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 };
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |         return if true { 1 } else { 2 };
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: remove `return`
+
+error: unneeded `return` statement
+  --> $DIR/needless_return.rs:316:9
+   |
+LL |         return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 };
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: remove `return` and wrap the sequence with parentheses
 
-error: aborting due to 51 previous errors
+error: aborting due to 52 previous errors