Auto merge of rust-lang#123812 - compiler-errors:additional-fixes, r=fmease

Follow-up fixes to `report_return_mismatched_types`

Some renames, simplifications, fixes, etc. Follow-ups to rust-lang#123804. I don't think it totally disentangles this code, but it does remove some of the worst offenders on the "I am so confused" scale (e.g. `get_node_fn_decl`).

Author: bors

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

@@ -992,7 +992,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
             }
         }
 
-        if look_at_return && hir.get_return_block(closure_id).is_some() {
+        if look_at_return && hir.get_fn_id_for_return_block(closure_id).is_some() {
             // ...otherwise we are probably in the tail expression of the function, point at the
             // return type.
             match self.infcx.tcx.hir_node_by_def_id(hir.get_parent_item(fn_call_id).def_id) {

compiler/rustc_hir_typeck/src/coercion.rs

@@ -1591,31 +1591,28 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
                         err.span_label(cause.span, "return type is not `()`");
                     }
                     ObligationCauseCode::BlockTailExpression(blk_id, ..) => {
-                        let parent_id = fcx.tcx.parent_hir_id(blk_id);
                         err = self.report_return_mismatched_types(
                             cause,
                             expected,
                             found,
                             coercion_error,
                             fcx,
-                            parent_id,
+                            blk_id,
                             expression,
-                            Some(blk_id),
                         );
                         if !fcx.tcx.features().unsized_locals {
                             unsized_return = self.is_return_ty_definitely_unsized(fcx);
                         }
                     }
-                    ObligationCauseCode::ReturnValue(id) => {
+                    ObligationCauseCode::ReturnValue(return_expr_id) => {
                         err = self.report_return_mismatched_types(
                             cause,
                             expected,
                             found,
                             coercion_error,
                             fcx,
-                            id,
+                            return_expr_id,
                             expression,
-                            None,
                         );
                         if !fcx.tcx.features().unsized_locals {
                             unsized_return = self.is_return_ty_definitely_unsized(fcx);
@@ -1809,13 +1806,14 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
         found: Ty<'tcx>,
         ty_err: TypeError<'tcx>,
         fcx: &FnCtxt<'a, 'tcx>,
-        id: hir::HirId,
+        block_or_return_id: hir::HirId,
         expression: Option<&'tcx hir::Expr<'tcx>>,
-        blk_id: Option<hir::HirId>,
     ) -> Diag<'a> {
         let mut err = fcx.err_ctxt().report_mismatched_types(cause, expected, found, ty_err);
 
-        let parent_id = fcx.tcx.parent_hir_id(id);
+        let due_to_block = matches!(fcx.tcx.hir_node(block_or_return_id), hir::Node::Block(..));
+
+        let parent_id = fcx.tcx.parent_hir_id(block_or_return_id);
         let parent = fcx.tcx.hir_node(parent_id);
         if let Some(expr) = expression
             && let hir::Node::Expr(hir::Expr {
@@ -1829,72 +1827,64 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
         // Verify that this is a tail expression of a function, otherwise the
         // label pointing out the cause for the type coercion will be wrong
         // as prior return coercions would not be relevant (#57664).
-        let fn_decl = if let (Some(expr), Some(blk_id)) = (expression, blk_id) {
+        if let Some(expr) = expression
+            && due_to_block
+        {
             fcx.suggest_missing_semicolon(&mut err, expr, expected, false);
-            let pointing_at_return_type =
-                fcx.suggest_mismatched_types_on_tail(&mut err, expr, expected, found, blk_id);
-            if let (Some(cond_expr), true, false) = (
-                fcx.tcx.hir().get_if_cause(expr.hir_id),
-                expected.is_unit(),
-                pointing_at_return_type,
-            )
+            let pointing_at_return_type = fcx.suggest_mismatched_types_on_tail(
+                &mut err,
+                expr,
+                expected,
+                found,
+                block_or_return_id,
+            );
+            if let Some(cond_expr) = fcx.tcx.hir().get_if_cause(expr.hir_id)
+                && expected.is_unit()
+                && !pointing_at_return_type
                 // If the block is from an external macro or try (`?`) desugaring, then
                 // do not suggest adding a semicolon, because there's nowhere to put it.
                 // See issues #81943 and #87051.
                 && matches!(
                     cond_expr.span.desugaring_kind(),
                     None | Some(DesugaringKind::WhileLoop)
-                ) && !in_external_macro(fcx.tcx.sess, cond_expr.span)
-                    && !matches!(
-                        cond_expr.kind,
-                        hir::ExprKind::Match(.., hir::MatchSource::TryDesugar(_))
-                    )
+                )
+                && !in_external_macro(fcx.tcx.sess, cond_expr.span)
+                && !matches!(
+                    cond_expr.kind,
+                    hir::ExprKind::Match(.., hir::MatchSource::TryDesugar(_))
+                )
             {
                 err.span_label(cond_expr.span, "expected this to be `()`");
                 if expr.can_have_side_effects() {
                     fcx.suggest_semicolon_at_end(cond_expr.span, &mut err);
                 }
             }
-            fcx.get_node_fn_decl(parent)
-                .map(|(fn_id, fn_decl, _, is_main)| (fn_id, fn_decl, is_main))
-        } else {
-            fcx.get_fn_decl(parent_id)
         };
 
-        if let Some((fn_id, fn_decl, can_suggest)) = fn_decl {
-            if blk_id.is_none() {
-                fcx.suggest_missing_return_type(
-                    &mut err,
-                    fn_decl,
-                    expected,
-                    found,
-                    can_suggest,
-                    fn_id,
-                );
-            }
-        }
-
-        let mut parent_id = fcx.tcx.hir().get_parent_item(id).def_id;
-        let mut parent_item = fcx.tcx.hir_node_by_def_id(parent_id);
-        // When suggesting return, we need to account for closures and async blocks, not just items.
-        for (_, node) in fcx.tcx.hir().parent_iter(id) {
-            match node {
-                hir::Node::Expr(&hir::Expr {
-                    kind: hir::ExprKind::Closure(hir::Closure { def_id, .. }),
-                    ..
-                }) => {
-                    parent_item = node;
-                    parent_id = *def_id;
-                    break;
-                }
-                hir::Node::Item(_) | hir::Node::TraitItem(_) | hir::Node::ImplItem(_) => break,
-                _ => {}
-            }
+        // If this is due to an explicit `return`, suggest adding a return type.
+        if let Some((fn_id, fn_decl, can_suggest)) = fcx.get_fn_decl(parent_id)
+            && !due_to_block
+        {
+            fcx.suggest_missing_return_type(&mut err, fn_decl, expected, found, can_suggest, fn_id);
         }
 
-        if let (Some(expr), Some(_), Some(fn_decl)) = (expression, blk_id, parent_item.fn_decl()) {
+        // If this is due to a block, then maybe we forgot a `return`/`break`.
+        if due_to_block
+            && let Some(expr) = expression
+            && let Some((parent_fn_decl, parent_id)) = fcx
+                .tcx
+                .hir()
+                .parent_iter(block_or_return_id)
+                .find_map(|(_, node)| Some((node.fn_decl()?, node.associated_body()?.0)))
+        {
             fcx.suggest_missing_break_or_return_expr(
-                &mut err, expr, fn_decl, expected, found, id, parent_id,
+                &mut err,
+                expr,
+                parent_fn_decl,
+                expected,
+                found,
+                block_or_return_id,
+                parent_id,
             );
         }
 

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

@@ -33,7 +33,7 @@ use rustc_middle::{bug, span_bug};
 use rustc_session::lint;
 use rustc_span::def_id::LocalDefId;
 use rustc_span::hygiene::DesugaringKind;
-use rustc_span::symbol::{kw, sym, Ident};
+use rustc_span::symbol::{kw, sym};
 use rustc_span::Span;
 use rustc_target::abi::FieldIdx;
 use rustc_trait_selection::traits::error_reporting::TypeErrCtxtExt as _;
@@ -867,76 +867,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         )
     }
 
-    /// Given a function `Node`, return its  `HirId` and `FnDecl` if it exists. Given a closure
-    /// that is the child of a function, return that function's `HirId` and `FnDecl` instead.
-    /// This may seem confusing at first, but this is used in diagnostics for `async fn`,
-    /// for example, where most of the type checking actually happens within a nested closure,
-    /// but we often want access to the parent function's signature.
-    ///
-    /// Otherwise, return false.
-    pub(crate) fn get_node_fn_decl(
-        &self,
-        node: Node<'tcx>,
-    ) -> Option<(LocalDefId, &'tcx hir::FnDecl<'tcx>, Ident, bool)> {
-        match node {
-            Node::Item(&hir::Item {
-                ident,
-                kind: hir::ItemKind::Fn(ref sig, ..),
-                owner_id,
-                ..
-            }) => {
-                // This is less than ideal, it will not suggest a return type span on any
-                // method called `main`, regardless of whether it is actually the entry point,
-                // but it will still present it as the reason for the expected type.
-                Some((owner_id.def_id, &sig.decl, ident, ident.name != sym::main))
-            }
-            Node::TraitItem(&hir::TraitItem {
-                ident,
-                kind: hir::TraitItemKind::Fn(ref sig, ..),
-                owner_id,
-                ..
-            }) => Some((owner_id.def_id, &sig.decl, ident, true)),
-            Node::ImplItem(&hir::ImplItem {
-                ident,
-                kind: hir::ImplItemKind::Fn(ref sig, ..),
-                owner_id,
-                ..
-            }) => Some((owner_id.def_id, &sig.decl, ident, false)),
-            Node::Expr(&hir::Expr {
-                hir_id,
-                kind:
-                    hir::ExprKind::Closure(hir::Closure {
-                        kind: hir::ClosureKind::Coroutine(..), ..
-                    }),
-                ..
-            }) => {
-                let (ident, sig, owner_id) = match self.tcx.parent_hir_node(hir_id) {
-                    Node::Item(&hir::Item {
-                        ident,
-                        kind: hir::ItemKind::Fn(ref sig, ..),
-                        owner_id,
-                        ..
-                    }) => (ident, sig, owner_id),
-                    Node::TraitItem(&hir::TraitItem {
-                        ident,
-                        kind: hir::TraitItemKind::Fn(ref sig, ..),
-                        owner_id,
-                        ..
-                    }) => (ident, sig, owner_id),
-                    Node::ImplItem(&hir::ImplItem {
-                        ident,
-                        kind: hir::ImplItemKind::Fn(ref sig, ..),
-                        owner_id,
-                        ..
-                    }) => (ident, sig, owner_id),
-                    _ => return None,
-                };
-                Some((owner_id.def_id, &sig.decl, ident, ident.name != sym::main))
-            }
-            _ => None,
-        }
-    }
-
     /// Given a `HirId`, return the `HirId` of the enclosing function, its `FnDecl`, and whether a
     /// suggestion can be made, `None` otherwise.
     pub fn get_fn_decl(
@@ -945,10 +875,73 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     ) -> Option<(LocalDefId, &'tcx hir::FnDecl<'tcx>, bool)> {
         // Get enclosing Fn, if it is a function or a trait method, unless there's a `loop` or
         // `while` before reaching it, as block tail returns are not available in them.
-        self.tcx.hir().get_return_block(blk_id).and_then(|blk_id| {
-            let parent = self.tcx.hir_node(blk_id);
-            self.get_node_fn_decl(parent)
-                .map(|(fn_id, fn_decl, _, is_main)| (fn_id, fn_decl, is_main))
+        self.tcx.hir().get_fn_id_for_return_block(blk_id).and_then(|item_id| {
+            match self.tcx.hir_node(item_id) {
+                Node::Item(&hir::Item {
+                    ident,
+                    kind: hir::ItemKind::Fn(ref sig, ..),
+                    owner_id,
+                    ..
+                }) => {
+                    // This is less than ideal, it will not suggest a return type span on any
+                    // method called `main`, regardless of whether it is actually the entry point,
+                    // but it will still present it as the reason for the expected type.
+                    Some((owner_id.def_id, sig.decl, ident.name != sym::main))
+                }
+                Node::TraitItem(&hir::TraitItem {
+                    kind: hir::TraitItemKind::Fn(ref sig, ..),
+                    owner_id,
+                    ..
+                }) => Some((owner_id.def_id, sig.decl, true)),
+                // FIXME: Suggestable if this is not a trait implementation
+                Node::ImplItem(&hir::ImplItem {
+                    kind: hir::ImplItemKind::Fn(ref sig, ..),
+                    owner_id,
+                    ..
+                }) => Some((owner_id.def_id, sig.decl, false)),
+                Node::Expr(&hir::Expr {
+                    hir_id,
+                    kind: hir::ExprKind::Closure(&hir::Closure { def_id, kind, fn_decl, .. }),
+                    ..
+                }) => {
+                    match kind {
+                        hir::ClosureKind::CoroutineClosure(_) => {
+                            // FIXME(async_closures): Implement this.
+                            return None;
+                        }
+                        hir::ClosureKind::Closure => Some((def_id, fn_decl, true)),
+                        hir::ClosureKind::Coroutine(hir::CoroutineKind::Desugared(
+                            _,
+                            hir::CoroutineSource::Fn,
+                        )) => {
+                            let (ident, sig, owner_id) = match self.tcx.parent_hir_node(hir_id) {
+                                Node::Item(&hir::Item {
+                                    ident,
+                                    kind: hir::ItemKind::Fn(ref sig, ..),
+                                    owner_id,
+                                    ..
+                                }) => (ident, sig, owner_id),
+                                Node::TraitItem(&hir::TraitItem {
+                                    ident,
+                                    kind: hir::TraitItemKind::Fn(ref sig, ..),
+                                    owner_id,
+                                    ..
+                                }) => (ident, sig, owner_id),
+                                Node::ImplItem(&hir::ImplItem {
+                                    ident,
+                                    kind: hir::ImplItemKind::Fn(ref sig, ..),
+                                    owner_id,
+                                    ..
+                                }) => (ident, sig, owner_id),
+                                _ => return None,
+                            };
+                            Some((owner_id.def_id, sig.decl, ident.name != sym::main))
+                        }
+                        _ => None,
+                    }
+                }
+                _ => None,
+            }
         })
     }
 

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

@@ -1774,15 +1774,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                     // that highlight errors inline.
                     let mut sp = blk.span;
                     let mut fn_span = None;
-                    if let Some((decl, ident)) = self.get_parent_fn_decl(blk.hir_id) {
+                    if let Some((fn_def_id, decl, _)) = self.get_fn_decl(blk.hir_id) {
                         let ret_sp = decl.output.span();
                         if let Some(block_sp) = self.parent_item_span(blk.hir_id) {
                             // HACK: on some cases (`ui/liveness/liveness-issue-2163.rs`) the
                             // output would otherwise be incorrect and even misleading. Make sure
                             // the span we're aiming at correspond to a `fn` body.
                             if block_sp == blk.span {
                                 sp = ret_sp;
-                                fn_span = Some(ident.span);
+                                fn_span = self.tcx.def_ident_span(fn_def_id);
                             }
                         }
                     }
@@ -1897,15 +1897,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         None
     }
 
-    /// Given a function block's `HirId`, returns its `FnDecl` if it exists, or `None` otherwise.
-    pub(crate) fn get_parent_fn_decl(
-        &self,
-        blk_id: HirId,
-    ) -> Option<(&'tcx hir::FnDecl<'tcx>, Ident)> {
-        let parent = self.tcx.hir_node_by_def_id(self.tcx.hir().get_parent_item(blk_id).def_id);
-        self.get_node_fn_decl(parent).map(|(_, fn_decl, ident, _)| (fn_decl, ident))
-    }
-
     /// If `expr` is a `match` expression that has only one non-`!` arm, use that arm's tail
     /// expression's `Span`, otherwise return `expr.span`. This is done to give better errors
     /// when given code like the following:

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

@@ -800,6 +800,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         can_suggest: bool,
         fn_id: LocalDefId,
     ) -> bool {
+        // Can't suggest `->` on a block-like coroutine
+        if let Some(hir::CoroutineKind::Desugared(_, hir::CoroutineSource::Block)) =
+            self.tcx.coroutine_kind(fn_id)
+        {
+            return false;
+        }
+
         let found =
             self.resolve_numeric_literals_with_default(self.resolve_vars_if_possible(found));
         // Only suggest changing the return type for methods that
@@ -1909,7 +1916,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         let returned = matches!(
             self.tcx.parent_hir_node(expr.hir_id),
             hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Ret(_), .. })
-        ) || map.get_return_block(expr.hir_id).is_some();
+        ) || map.get_fn_id_for_return_block(expr.hir_id).is_some();
         if returned
             && let ty::Adt(e, args_e) = expected.kind()
             && let ty::Adt(f, args_f) = found.kind()