fix [needless_return] incorrect suggestion when returning if sequence

Author: J-ZhengLi

clippy_lints/src/returns.rs

@@ -69,31 +69,35 @@ declare_clippy_lint! {
     "using a return statement like `return expr;` where an expression would suffice"
 }
 
-#[derive(PartialEq, Eq, Copy, Clone)]
+#[derive(PartialEq, Eq, Clone)]
 enum RetReplacement {
     Empty,
     Block,
     Unit,
+    IfSequence(String),
+    Expr(String),
 }
 
 impl RetReplacement {
     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",
         }
     }
 }
 
 impl ToString for RetReplacement {
     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.clone(),
         }
-        .to_string()
     }
 }
 
@@ -210,28 +214,46 @@ fn check_final_expr<'tcx>(
     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 (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())
+                } else {
+                    RetReplacement::Expr(snippet.to_string())
+                }
+            } else {
+                replacement
+            };
+
             if !cx.tcx.hir().attrs(expr.hir_id).is_empty() {
                 return;
             }
             let borrows = inner.map_or(false, |inner| last_statement_borrows(cx, inner));
             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, peeled_drop_expr.span, semi_spans.clone());
@@ -253,29 +275,21 @@ fn check_final_expr<'tcx>(
     }
 }
 
-fn emit_return_lint(
-    cx: &LateContext<'_>,
-    ret_span: Span,
-    semi_spans: Vec<Span>,
-    inner_span: Option<Span>,
-    replacement: RetReplacement,
-) {
+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 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 = 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

tests/ui/needless_return.fixed

@@ -297,4 +297,8 @@ 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 })
+}
+
 fn main() {}

tests/ui/needless_return.rs

@@ -307,4 +307,8 @@ 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 };
+}
+
 fn main() {}

tests/ui/needless_return.stderr

@@ -418,5 +418,13 @@ LL |         return Err(format!("err!"));
    |
    = help: remove `return`
 
-error: aborting due to 50 previous errors
+error: unneeded `return` statement
+  --> $DIR/needless_return.rs:311:5
+   |
+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