Skip to content

Commit

Permalink
Rollup merge of rust-lang#92183 - tmandry:issue-74256, r=estebank
Browse files Browse the repository at this point in the history 
Point at correct argument when async fn output type lifetime disagrees with signature

Fixes most of rust-lang#74256.

## Problems fixed

This PR fixes a couple of related problems in the error reporting code.

### Highlighting the wrong argument

First, the error reporting code was looking at the desugared return type of an `async fn` to decide which parameter to highlight. For example, a function like

```rust
async fn async_fn(self: &Struct, f: &u32) -> &u32
{ f }
```

desugars to

```rust
async fn async_fn<'a, 'b>(self: &'a Struct, f: &'b u32)
-> impl Future<Output = &'a u32> + 'a + 'b
{ f }
```

Since `f: &'b u32` is returned but the output type is `&'a u32`, the error would occur when checking that `'a: 'b`.

The reporting code would look to see if the "offending" lifetime `'b` was included in the return type, and because the code was looking at the desugared future type, it was included. So it defaulted to reporting that the source of the other lifetime `'a` (the `self` type) was the problem, when it was really the type of `f`. (Note that if it had chosen instead to look at `'a` first, it too would have been included in the output type, and it would have arbitrarily reported the error (correctly this time) on the type of `f`.)

Looking at the actual future type isn't useful for this reason; it captures all input lifetimes. Using the written return type for `async fn` solves this problem and results in less confusing error messages for the user.

This isn't a perfect fix, unfortunately; writing the "manually desugared" form of the above function still results in the wrong parameter being highlighted. Looking at the output type of every `impl Future` return type doesn't feel like a very principled approach, though it might work. The problem would remain for function signatures that look like the desugared one above but use different traits. There may be deeper changes required to pinpoint which part of each type is conflicting.

### Lying about await point capture causing lifetime conflicts

The second issue fixed by this PR is the unnecessary complexity in `try_report_anon_anon_conflict`. It turns out that the root cause I suggested in rust-lang#76547 (comment) wasn't really the root cause. Adding special handling to report that a variable was captured over an await point only made the error messages less correct and pointed to a problem other than the one that actually occurred.

Given the above discussion, it's easy to see why: `async fn`s capture all input lifetimes in their return type, so holding an argument across an await point should never cause a lifetime conflict! Removing the special handling simplified the code and improved the error messages (though they still aren't very good!)

## Future work

* Fix error reporting on the "desugared" form of this code
* Get the `suggest_adding_lifetime_params` suggestion firing on these examples
  * cc rust-lang#42703, I think

r? `@estebank`
  • Loading branch information
@matthiaskrgr
matthiaskrgr committed Jan 20, 2022
2 parents 405cf20 + 698631e commit 413f490
Show file tree
Hide file tree
Showing 17 changed files with 244 additions and 350 deletions.
15 changes: 14 additions & 1 deletion compiler/rustc_hir/src/hir.rs
View file
Expand Up @@ -2726,6 +2726,10 @@ pub struct FnHeader {
}

impl FnHeader {
pub fn is_async(&self) -> bool {
matches!(&self.asyncness, IsAsync::Async)
}

pub fn is_const(&self) -> bool {
matches!(&self.constness, Constness::Const)
}
Expand Down Expand Up @@ -3169,7 +3173,7 @@ impl<'hir> Node<'hir> {
}
}

pub fn fn_decl(&self) -> Option<&FnDecl<'hir>> {
pub fn fn_decl(&self) -> Option<&'hir FnDecl<'hir>> {
match self {
Node::TraitItem(TraitItem { kind: TraitItemKind::Fn(fn_sig, _), .. })
| Node::ImplItem(ImplItem { kind: ImplItemKind::Fn(fn_sig, _), .. })
Expand All @@ -3181,6 +3185,15 @@ impl<'hir> Node<'hir> {
}
}

pub fn fn_sig(&self) -> Option<&'hir FnSig<'hir>> {
match self {
Node::TraitItem(TraitItem { kind: TraitItemKind::Fn(fn_sig, _), .. })
| Node::ImplItem(ImplItem { kind: ImplItemKind::Fn(fn_sig, _), .. })
| Node::Item(Item { kind: ItemKind::Fn(fn_sig, _, _), .. }) => Some(fn_sig),
_ => None,
}
}

pub fn body_id(&self) -> Option<BodyId> {
match self {
Node::TraitItem(TraitItem {
Expand Down
31 changes: 19 additions & 12 deletions compiler/rustc_infer/src/infer/error_reporting/mod.rs
View file
Expand Up @@ -65,11 +65,11 @@ use rustc_hir::def_id::DefId;
use rustc_hir::lang_items::LangItem;
use rustc_hir::{Item, ItemKind, Node};
use rustc_middle::dep_graph::DepContext;
use rustc_middle::ty::error::TypeError;
use rustc_middle::ty::{
self,
error::TypeError,
subst::{GenericArgKind, Subst, SubstsRef},
Region, Ty, TyCtxt, TypeFoldable,
Binder, Region, Ty, TyCtxt, TypeFoldable,
};
use rustc_span::{sym, BytePos, DesugaringKind, MultiSpan, Pos, Span};
use rustc_target::spec::abi;
Expand Down Expand Up @@ -1765,7 +1765,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
self.note_error_origin(diag, cause, exp_found, terr);
}

pub fn get_impl_future_output_ty(&self, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
pub fn get_impl_future_output_ty(&self, ty: Ty<'tcx>) -> Option<Binder<'tcx, Ty<'tcx>>> {
if let ty::Opaque(def_id, substs) = ty.kind() {
let future_trait = self.tcx.require_lang_item(LangItem::Future, None);
// Future::Output
Expand All @@ -1775,13 +1775,20 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {

for (predicate, _) in bounds {
let predicate = predicate.subst(self.tcx, substs);
if let ty::PredicateKind::Projection(projection_predicate) =
predicate.kind().skip_binder()
{
if projection_predicate.projection_ty.item_def_id == item_def_id {
// We don't account for multiple `Future::Output = Ty` contraints.
return projection_predicate.term.ty();
}
let output = predicate
.kind()
.map_bound(|kind| match kind {
ty::PredicateKind::Projection(projection_predicate)
if projection_predicate.projection_ty.item_def_id == item_def_id =>
{
projection_predicate.term.ty()
}
_ => None,
})
.transpose();
if output.is_some() {
// We don't account for multiple `Future::Output = Ty` contraints.
return output;
}
}
}
Expand Down Expand Up @@ -1823,8 +1830,8 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}

match (
self.get_impl_future_output_ty(exp_found.expected),
self.get_impl_future_output_ty(exp_found.found),
self.get_impl_future_output_ty(exp_found.expected).map(Binder::skip_binder),
self.get_impl_future_output_ty(exp_found.found).map(Binder::skip_binder),
) {
(Some(exp), Some(found)) if same_type_modulo_infer(exp, found) => match cause.code() {
ObligationCauseCode::IfExpression(box IfExpressionCause { then, .. }) => {
Expand Down
109 changes: 33 additions & 76 deletions compiler/rustc_infer/src/infer/error_reporting/nice_region_error/different_lifetimes.rs
View file
Expand Up @@ -106,90 +106,47 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
None => String::new(),
};

let (span_1, span_2, main_label, span_label, future_return_type) =
match (sup_is_ret_type, sub_is_ret_type) {
(None, None) => {
let (main_label_1, span_label_1) = if ty_sup.hir_id == ty_sub.hir_id {
(
"this type is declared with multiple lifetimes...".to_owned(),
"...but data with one lifetime flows into the other here".to_owned(),
)
} else {
(
"these two types are declared with different lifetimes...".to_owned(),
format!("...but data{} flows{} here", span_label_var1, span_label_var2),
)
};
(ty_sup.span, ty_sub.span, main_label_1, span_label_1, None)
}
debug!(
"try_report_anon_anon_conflict: sub_is_ret_type={:?} sup_is_ret_type={:?}",
sub_is_ret_type, sup_is_ret_type
);

(Some(ret_span), _) => {
let sup_future = self.future_return_type(scope_def_id_sup);
let (return_type, action) = if sup_future.is_some() {
("returned future", "held across an await point")
} else {
("return type", "returned")
};
let mut err = struct_span_err!(self.tcx().sess, span, E0623, "lifetime mismatch");

(
ty_sub.span,
ret_span,
format!(
"this parameter and the {} are declared with different lifetimes...",
return_type
),
format!("...but data{} is {} here", span_label_var1, action),
sup_future,
)
}
(_, Some(ret_span)) => {
let sub_future = self.future_return_type(scope_def_id_sub);
let (return_type, action) = if sub_future.is_some() {
("returned future", "held across an await point")
} else {
("return type", "returned")
};
match (sup_is_ret_type, sub_is_ret_type) {
(ret_capture @ Some(ret_span), _) | (_, ret_capture @ Some(ret_span)) => {
let param_span =
if sup_is_ret_type == ret_capture { ty_sub.span } else { ty_sup.span };

err.span_label(
param_span,
"this parameter and the return type are declared with different lifetimes...",
);
err.span_label(ret_span, "");
err.span_label(span, format!("...but data{} is returned here", span_label_var1));
}

(
(None, None) => {
if ty_sup.hir_id == ty_sub.hir_id {
err.span_label(ty_sup.span, "this type is declared with multiple lifetimes...");
err.span_label(ty_sub.span, "");
err.span_label(span, "...but data with one lifetime flows into the other here");
} else {
err.span_label(
ty_sup.span,
ret_span,
format!(
"this parameter and the {} are declared with different lifetimes...",
return_type
),
format!("...but data{} is {} here", span_label_var1, action),
sub_future,
)
"these two types are declared with different lifetimes...",
);
err.span_label(ty_sub.span, "");
err.span_label(
span,
format!("...but data{} flows{} here", span_label_var1, span_label_var2),
);
}
};

let mut err = struct_span_err!(self.tcx().sess, span, E0623, "lifetime mismatch");

err.span_label(span_1, main_label);
err.span_label(span_2, String::new());
err.span_label(span, span_label);
}
}

self.suggest_adding_lifetime_params(sub, ty_sup, ty_sub, &mut err);

if let Some(t) = future_return_type {
let snip = self
.tcx()
.sess
.source_map()
.span_to_snippet(t.span)
.ok()
.and_then(|s| match (&t.kind, s.as_str()) {
(rustc_hir::TyKind::Tup(&[]), "") => Some("()".to_string()),
(_, "") => None,
_ => Some(s),
})
.unwrap_or_else(|| "{unnamed_type}".to_string());

err.span_label(
t.span,
&format!("this `async fn` implicitly returns an `impl Future<Output = {}>`", snip),
);
}
err.emit();
Some(ErrorReported)
}
Expand Down
19 changes: 6 additions & 13 deletions compiler/rustc_infer/src/infer/error_reporting/nice_region_error/find_anon_type.rs
View file
@@ -1,6 +1,5 @@
use rustc_hir as hir;
use rustc_hir::intravisit::{self, Visitor};
use rustc_hir::Node;
use rustc_middle::hir::map::Map;
use rustc_middle::hir::nested_filter;
use rustc_middle::middle::resolve_lifetime as rl;
Expand All @@ -25,25 +24,19 @@ pub(crate) fn find_anon_type<'tcx>(
tcx: TyCtxt<'tcx>,
region: Region<'tcx>,
br: &ty::BoundRegionKind,
) -> Option<(&'tcx hir::Ty<'tcx>, &'tcx hir::FnDecl<'tcx>)> {
) -> Option<(&'tcx hir::Ty<'tcx>, &'tcx hir::FnSig<'tcx>)> {
if let Some(anon_reg) = tcx.is_suitable_region(region) {
let hir_id = tcx.hir().local_def_id_to_hir_id(anon_reg.def_id);
let fndecl = match tcx.hir().get(hir_id) {
Node::Item(&hir::Item { kind: hir::ItemKind::Fn(ref m, ..), .. })
| Node::TraitItem(&hir::TraitItem {
kind: hir::TraitItemKind::Fn(ref m, ..), ..
})
| Node::ImplItem(&hir::ImplItem { kind: hir::ImplItemKind::Fn(ref m, ..), .. }) => {
&m.decl
}
_ => return None,
let Some(fn_sig) = tcx.hir().get(hir_id).fn_sig() else {
return None
};

fndecl
fn_sig
.decl
.inputs
.iter()
.find_map(|arg| find_component_for_bound_region(tcx, arg, br))
.map(|ty| (ty, &**fndecl))
.map(|ty| (ty, fn_sig))
} else {
None
}
Expand Down
91 changes: 26 additions & 65 deletions compiler/rustc_infer/src/infer/error_reporting/nice_region_error/util.rs
View file
Expand Up @@ -4,7 +4,7 @@
use crate::infer::error_reporting::nice_region_error::NiceRegionError;
use rustc_hir as hir;
use rustc_hir::def_id::LocalDefId;
use rustc_middle::ty::{self, DefIdTree, Region, Ty};
use rustc_middle::ty::{self, Binder, DefIdTree, Region, Ty, TypeFoldable};
use rustc_span::Span;

/// Information about the anonymous region we are searching for.
Expand Down Expand Up @@ -94,81 +94,42 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
})
}

pub(super) fn future_return_type(
&self,
local_def_id: LocalDefId,
) -> Option<&rustc_hir::Ty<'_>> {
if let Some(hir::IsAsync::Async) = self.asyncness(local_def_id) {
if let rustc_middle::ty::Opaque(def_id, _) =
self.tcx().type_of(local_def_id).fn_sig(self.tcx()).output().skip_binder().kind()
{
match self.tcx().hir().get_if_local(*def_id) {
Some(hir::Node::Item(hir::Item {
kind:
hir::ItemKind::OpaqueTy(hir::OpaqueTy {
bounds,
origin: hir::OpaqueTyOrigin::AsyncFn(..),
..
}),
..
})) => {
for b in bounds.iter() {
if let hir::GenericBound::LangItemTrait(
hir::LangItem::Future,
_span,
_hir_id,
generic_args,
) = b
{
for type_binding in generic_args.bindings.iter() {
if type_binding.ident.name == rustc_span::sym::Output {
if let hir::TypeBindingKind::Equality {
term: hir::Term::Ty(ty),
} = type_binding.kind
{
return Some(ty);
}
}
}
}
}
}
_ => {}
}
}
}
None
}

pub(super) fn asyncness(&self, local_def_id: LocalDefId) -> Option<hir::IsAsync> {
// similar to the asyncness fn in rustc_ty_utils::ty
let hir_id = self.tcx().hir().local_def_id_to_hir_id(local_def_id);
let node = self.tcx().hir().get(hir_id);
let fn_kind = node.fn_kind()?;
Some(fn_kind.asyncness())
}

// Here, we check for the case where the anonymous region
// is in the return type.
// is in the return type as written by the user.
// FIXME(#42703) - Need to handle certain cases here.
pub(super) fn is_return_type_anon(
&self,
scope_def_id: LocalDefId,
br: ty::BoundRegionKind,
decl: &hir::FnDecl<'_>,
hir_sig: &hir::FnSig<'_>,
) -> Option<Span> {
let ret_ty = self.tcx().type_of(scope_def_id);
if let ty::FnDef(_, _) = ret_ty.kind() {
let sig = ret_ty.fn_sig(self.tcx());
let late_bound_regions =
self.tcx().collect_referenced_late_bound_regions(&sig.output());
if late_bound_regions.iter().any(|r| *r == br) {
return Some(decl.output.span());
}
let fn_ty = self.tcx().type_of(scope_def_id);
if let ty::FnDef(_, _) = fn_ty.kind() {
let ret_ty = fn_ty.fn_sig(self.tcx()).output();
let span = hir_sig.decl.output.span();
let future_output = if hir_sig.header.is_async() {
ret_ty.map_bound(|ty| self.infcx.get_impl_future_output_ty(ty)).transpose()
} else {
None
};
return match future_output {
Some(output) if self.includes_region(output, br) => Some(span),
None if self.includes_region(ret_ty, br) => Some(span),
_ => None,
};
}
None
}

fn includes_region(
&self,
ty: Binder<'tcx, impl TypeFoldable<'tcx>>,
region: ty::BoundRegionKind,
) -> bool {
let late_bound_regions = self.tcx().collect_referenced_late_bound_regions(&ty);
late_bound_regions.iter().any(|r| *r == region)
}

// Here we check for the case where anonymous region
// corresponds to self and if yes, we display E0312.
// FIXME(#42700) - Need to format self properly to
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_typeck/src/check/expr.rs
View file
Expand Up @@ -1909,7 +1909,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
_ => return,
};
let mut add_label = true;
if let ty::Adt(def, _) = output_ty.kind() {
if let ty::Adt(def, _) = output_ty.skip_binder().kind() {
// no field access on enum type
if !def.is_enum() {
if def
Expand Down

0 comments on commit 413f490

Please sign in to comment.