Auto merge of #12021 - PartiallyTyped:11982, r=flip1995

FP: `needless_return_with_question_mark` with implicit Error Conversion

Return with a question mark was triggered in situations where the `?` desuraging was performing error conversion via `Into`/`From`.

The desugared `?` produces a match over an expression with type `std::ops::ControlFlow<B,C>` with `B:Result<Infallible, E:Error>` and `C:Result<_, E':Error>`, and the arms perform the conversion. The patch adds another check in the lint that checks that `E == E'`. If `E == E'`, then the `?` is indeed unnecessary.

changelog: False Positive: [`needless_return_with_question_mark`] when implicit Error Conversion occurs.

fixes: #11982

Author: bors

clippy_lints/src/returns.rs

@@ -2,10 +2,14 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then, span_lin
 use clippy_utils::source::{snippet_opt, snippet_with_context};
 use clippy_utils::sugg::has_enclosing_paren;
 use clippy_utils::visitors::{for_each_expr_with_closures, Descend};
-use clippy_utils::{fn_def_id, is_from_proc_macro, is_inside_let_else, path_to_local_id, span_find_starting_semi};
+use clippy_utils::{
+    fn_def_id, is_from_proc_macro, is_inside_let_else, is_res_lang_ctor, path_res, path_to_local_id,
+    span_find_starting_semi,
+};
 use core::ops::ControlFlow;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::FnKind;
+use rustc_hir::LangItem::ResultErr;
 use rustc_hir::{
     Block, Body, Expr, ExprKind, FnDecl, HirId, ItemKind, LangItem, MatchSource, Node, OwnerNode, PatKind, QPath, Stmt,
     StmtKind,
@@ -182,7 +186,15 @@ impl<'tcx> LateLintPass<'tcx> for Return {
         if !in_external_macro(cx.sess(), stmt.span)
             && let StmtKind::Semi(expr) = stmt.kind
             && let ExprKind::Ret(Some(ret)) = expr.kind
-            && let ExprKind::Match(.., MatchSource::TryDesugar(_)) = ret.kind
+            // return Err(...)? desugars to a match
+            // over a Err(...).branch()
+            // which breaks down to a branch call, with the callee being
+            // the constructor of the Err variant
+            && let ExprKind::Match(maybe_cons, _, MatchSource::TryDesugar(_)) = ret.kind
+            && let ExprKind::Call(_, [maybe_result_err]) = maybe_cons.kind
+            && let ExprKind::Call(maybe_constr, _) = maybe_result_err.kind
+            && is_res_lang_ctor(cx, path_res(cx, maybe_constr), ResultErr)
+
             // Ensure this is not the final stmt, otherwise removing it would cause a compile error
             && let OwnerNode::Item(item) = cx.tcx.hir().owner(cx.tcx.hir().get_parent_item(expr.hir_id))
             && let ItemKind::Fn(_, _, body) = item.kind

tests/ui/needless_return_with_question_mark.fixed

@@ -77,3 +77,53 @@ fn issue11616() -> Result<(), ()> {
     };
     Ok(())
 }
+
+fn issue11982() {
+    mod bar {
+        pub struct Error;
+        pub fn foo(_: bool) -> Result<(), Error> {
+            Ok(())
+        }
+    }
+
+    pub struct Error;
+
+    impl From<bar::Error> for Error {
+        fn from(_: bar::Error) -> Self {
+            Error
+        }
+    }
+
+    fn foo(ok: bool) -> Result<(), Error> {
+        if !ok {
+            return bar::foo(ok).map(|_| Ok::<(), Error>(()))?;
+        };
+        Ok(())
+    }
+}
+
+fn issue11982_no_conversion() {
+    mod bar {
+        pub struct Error;
+        pub fn foo(_: bool) -> Result<(), Error> {
+            Ok(())
+        }
+    }
+
+    fn foo(ok: bool) -> Result<(), bar::Error> {
+        if !ok {
+            return bar::foo(ok).map(|_| Ok::<(), bar::Error>(()))?;
+        };
+        Ok(())
+    }
+}
+
+fn general_return() {
+    fn foo(ok: bool) -> Result<(), ()> {
+        let bar = Result::Ok(Result::<(), ()>::Ok(()));
+        if !ok {
+            return bar?;
+        };
+        Ok(())
+    }
+}

tests/ui/needless_return_with_question_mark.rs

@@ -77,3 +77,53 @@ fn issue11616() -> Result<(), ()> {
     };
     Ok(())
 }
+
+fn issue11982() {
+    mod bar {
+        pub struct Error;
+        pub fn foo(_: bool) -> Result<(), Error> {
+            Ok(())
+        }
+    }
+
+    pub struct Error;
+
+    impl From<bar::Error> for Error {
+        fn from(_: bar::Error) -> Self {
+            Error
+        }
+    }
+
+    fn foo(ok: bool) -> Result<(), Error> {
+        if !ok {
+            return bar::foo(ok).map(|_| Ok::<(), Error>(()))?;
+        };
+        Ok(())
+    }
+}
+
+fn issue11982_no_conversion() {
+    mod bar {
+        pub struct Error;
+        pub fn foo(_: bool) -> Result<(), Error> {
+            Ok(())
+        }
+    }
+
+    fn foo(ok: bool) -> Result<(), bar::Error> {
+        if !ok {
+            return bar::foo(ok).map(|_| Ok::<(), bar::Error>(()))?;
+        };
+        Ok(())
+    }
+}
+
+fn general_return() {
+    fn foo(ok: bool) -> Result<(), ()> {
+        let bar = Result::Ok(Result::<(), ()>::Ok(()));
+        if !ok {
+            return bar?;
+        };
+        Ok(())
+    }
+}