Skip to content

Commit

Permalink
Auto merge of #123812 - compiler-errors:additional-fixes, r=fmease
Browse files Browse the repository at this point in the history 
Follow-up fixes to `report_return_mismatched_types`

Some renames, simplifications, fixes, etc. Follow-ups to #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`).
  • Loading branch information
@bors
bors committed May 21, 2024
2 parents 1d0e4af + a2a6fe7 commit e875391
Show file tree
Hide file tree
Showing 12 changed files with 170 additions and 173 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
104 changes: 47 additions & 57 deletions compiler/rustc_hir_typeck/src/coercion.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
);
}

Expand Down
143 changes: 68 additions & 75 deletions compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 _;
Expand Down Expand Up @@ -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(
Expand All @@ -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,
}
})
}

Expand Down
13 changes: 2 additions & 11 deletions compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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:
Expand Down
9 changes: 8 additions & 1 deletion compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
Loading

0 comments on commit e875391

Please sign in to comment.