Auto merge of #11627 - y21:issue11616, r=giraffate

[`needless_return_with_question_mark`]: don't lint if never type is used for coercion

Fixes #11616

When we have something like
```rs
let _x: String = {
  return Err(())?;
};
```
we shouldn't suggest removing the `return` because the `!`-ness of `return` is used to coerce the enclosing block to some other type. That will lead to a typeck error without a diverging expression like `return`.

changelog: [`needless_return_with_question_mark`]: don't lint if `return`s never typed-ness is used for coercion

4 files changed, 64 insertions, 3 deletions

diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs
index 8595205691b..d64e931a2f3 100644
--- a/clippy_lints/src/returns.rs
+++ b/clippy_lints/src/returns.rs
@@ -7,10 +7,12 @@ use core::ops::ControlFlow;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::FnKind;
 use rustc_hir::{
-    Block, Body, Expr, ExprKind, FnDecl, ItemKind, LangItem, MatchSource, OwnerNode, PatKind, QPath, Stmt, StmtKind,
+    Block, Body, Expr, ExprKind, FnDecl, HirId, ItemKind, LangItem, MatchSource, Node, OwnerNode, PatKind, QPath, Stmt,
+    StmtKind,
 };
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
+use rustc_middle::ty::adjustment::Adjust;
 use rustc_middle::ty::{self, GenericArgKind, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::def_id::LocalDefId;
@@ -158,6 +160,22 @@ impl<'tcx> ToString for RetReplacement<'tcx> {
 
 declare_lint_pass!(Return => [LET_AND_RETURN, NEEDLESS_RETURN, NEEDLESS_RETURN_WITH_QUESTION_MARK]);
 
+/// Checks if a return statement is "needed" in the middle of a block, or if it can be removed. This
+/// is the case when the enclosing block expression is coerced to some other type, which only works
+/// because of the never-ness of `return` expressions
+fn stmt_needs_never_type(cx: &LateContext<'_>, stmt_hir_id: HirId) -> bool {
+    cx.tcx
+        .hir()
+        .parent_iter(stmt_hir_id)
+        .find_map(|(_, node)| if let Node::Expr(expr) = node { Some(expr) } else { None })
+        .is_some_and(|e| {
+            cx.typeck_results()
+                .expr_adjustments(e)
+                .iter()
+                .any(|adjust| adjust.target != cx.tcx.types.unit && matches!(adjust.kind, Adjust::NeverToAny))
+        })
+}
+
 impl<'tcx> LateLintPass<'tcx> for Return {
     fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
         if !in_external_macro(cx.sess(), stmt.span)
@@ -173,6 +191,7 @@ impl<'tcx> LateLintPass<'tcx> for Return {
             && let [.., final_stmt] = block.stmts
             && final_stmt.hir_id != stmt.hir_id
             && !is_from_proc_macro(cx, expr)
+            && !stmt_needs_never_type(cx, stmt.hir_id)
         {
             span_lint_and_sugg(
                 cx,
diff --git a/tests/ui/needless_return_with_question_mark.fixed b/tests/ui/needless_return_with_question_mark.fixed
index d5932ebcf12..0147c73a94b 100644
--- a/tests/ui/needless_return_with_question_mark.fixed
+++ b/tests/ui/needless_return_with_question_mark.fixed
@@ -5,6 +5,7 @@
     clippy::unit_arg,
     clippy::useless_conversion,
     clippy::diverging_sub_expression,
+    clippy::let_unit_value,
     unused
 )]
 
@@ -59,3 +60,20 @@ fn main() -> Result<(), ()> {
 
     Err(())
 }
+
+fn issue11616() -> Result<(), ()> {
+    let _x: String = {
+        return Err(())?;
+    };
+    let _x: () = {
+        Err(())?;
+        //~^ ERROR: unneeded `return` statement with `?` operator
+    };
+    let _x = match 1 {
+        1 => vec![1, 2],
+        _ => {
+            return Err(())?;
+        },
+    };
+    Ok(())
+}
diff --git a/tests/ui/needless_return_with_question_mark.rs b/tests/ui/needless_return_with_question_mark.rs
index 2485e25f05c..66e1f438f8c 100644
--- a/tests/ui/needless_return_with_question_mark.rs
+++ b/tests/ui/needless_return_with_question_mark.rs
@@ -5,6 +5,7 @@
     clippy::unit_arg,
     clippy::useless_conversion,
     clippy::diverging_sub_expression,
+    clippy::let_unit_value,
     unused
 )]
 
@@ -59,3 +60,20 @@ fn main() -> Result<(), ()> {
 
     Err(())
 }
+
+fn issue11616() -> Result<(), ()> {
+    let _x: String = {
+        return Err(())?;
+    };
+    let _x: () = {
+        return Err(())?;
+        //~^ ERROR: unneeded `return` statement with `?` operator
+    };
+    let _x = match 1 {
+        1 => vec![1, 2],
+        _ => {
+            return Err(())?;
+        },
+    };
+    Ok(())
+}
diff --git a/tests/ui/needless_return_with_question_mark.stderr b/tests/ui/needless_return_with_question_mark.stderr
index 580970a41aa..17aa212ae8d 100644
--- a/tests/ui/needless_return_with_question_mark.stderr
+++ b/tests/ui/needless_return_with_question_mark.stderr
@@ -1,5 +1,5 @@
 error: unneeded `return` statement with `?` operator
-  --> $DIR/needless_return_with_question_mark.rs:28:5
+  --> $DIR/needless_return_with_question_mark.rs:29:5
    |
 LL |     return Err(())?;
    |     ^^^^^^^ help: remove it
@@ -7,5 +7,11 @@ LL |     return Err(())?;
    = note: `-D clippy::needless-return-with-question-mark` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_return_with_question_mark)]`
 
-error: aborting due to previous error
+error: unneeded `return` statement with `?` operator
+  --> $DIR/needless_return_with_question_mark.rs:69:9
+   |
+LL |         return Err(())?;
+   |         ^^^^^^^ help: remove it
+
+error: aborting due to 2 previous errors