mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Rollup merge of #76468 - SNCPlay42:lifetime-names, r=Mark-Simulacrum

Browse Source 
Improve lifetime name annotations for closures & async functions

* Don't refer to async functions as "generators" in error output
* Where possible, emit annotations pointing exactly at the `&` in the return type of closures (when they have explicit return types) and async functions, like we do for arguments.
Addresses #74072, but I wouldn't call that *closed* until annotations are identical for async and non-async functions.
* Emit a better annotation when the lifetime doesn't appear in the full name type, which currently happens for opaque types like `impl Future`. Addresses #74497, but further improves could probably be made (why *doesn't* it appear in the type as `impl Future + '1`?)
This is included in the same PR because the changes to `give_name_if_anonymous_region_appears_in_output` would introduce ICE otherwise (it would return `None` in cases where it didn't previously, which then gets `unwrap`ped)
This commit is contained in:
Dylan DPC 2020-11-09 19:06:39 +01:00 committed by GitHub
commit 99f16e637b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 282 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

204
compiler/rustc_mir/src/borrow_check/diagnostics/region_name.rs
View File

@ -6,8 +6,8 @@
use rustc_middle::ty::print::RegionHighlightMode;
use rustc_middle::ty::subst::{GenericArgKind, SubstsRef};
use rustc_middle::ty::{self, RegionVid, Ty};
use rustc_span::symbol::kw;
use rustc_span::{symbol::Symbol, Span, DUMMY_SP};
use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_span::{Span, DUMMY_SP};
use crate::borrow_check::{nll::ToRegionVid, universal_regions::DefiningTy, MirBorrowckCtxt};
@ -39,7 +39,7 @@
/// The region corresponding to a closure upvar.
AnonRegionFromUpvar(Span, String),
/// The region corresponding to the return type of a closure.
AnonRegionFromOutput(Span, String, String),
AnonRegionFromOutput(RegionNameHighlight, String),
/// The region from a type yielded by a generator.
AnonRegionFromYieldTy(Span, String),
/// An anonymous region from an async fn.
@ -57,6 +57,10 @@
/// The anonymous region corresponds to a region where the type annotation is completely missing
/// from the code, e.g. in a closure arguments `|x| { ... }`, where `x` is a reference.
CannotMatchHirTy(Span, String),
/// The anonymous region corresponds to a region where the type annotation is completely missing
/// from the code, and *even if* we print out the full name of the type, the region name won't
/// be included. This currently occurs for opaque types like `impl Future`.
Occluded(Span, String),
}
impl RegionName {
@ -81,13 +85,14 @@ impl RegionName {
| RegionNameSource::NamedFreeRegion(span)
| RegionNameSource::SynthesizedFreeEnvRegion(span, _)
| RegionNameSource::AnonRegionFromUpvar(span, _)
| RegionNameSource::AnonRegionFromOutput(span, _, _)
| RegionNameSource::AnonRegionFromYieldTy(span, _)
| RegionNameSource::AnonRegionFromAsyncFn(span) => Some(span),
RegionNameSource::AnonRegionFromArgument(ref highlight) => match *highlight {
RegionNameSource::AnonRegionFromArgument(ref highlight)
| RegionNameSource::AnonRegionFromOutput(ref highlight, _) => match *highlight {
RegionNameHighlight::MatchedHirTy(span)
| RegionNameHighlight::MatchedAdtAndSegment(span)
| RegionNameHighlight::CannotMatchHirTy(span, _) => Some(span),
| RegionNameHighlight::CannotMatchHirTy(span, _)
| RegionNameHighlight::Occluded(span, _) => Some(span),
},
}
}
@ -112,6 +117,7 @@ impl RegionName {
diag.span_label(*span, format!("has type `{}`", type_name));
}
RegionNameSource::AnonRegionFromArgument(RegionNameHighlight::MatchedHirTy(span))
| RegionNameSource::AnonRegionFromOutput(RegionNameHighlight::MatchedHirTy(span), _)
| RegionNameSource::AnonRegionFromAsyncFn(span) => {
diag.span_label(
*span,
@ -120,16 +126,44 @@ impl RegionName {
}
RegionNameSource::AnonRegionFromArgument(
RegionNameHighlight::MatchedAdtAndSegment(span),
)
| RegionNameSource::AnonRegionFromOutput(
RegionNameHighlight::MatchedAdtAndSegment(span),
_,
) => {
diag.span_label(*span, format!("let's call this `{}`", self));
}
RegionNameSource::AnonRegionFromArgument(RegionNameHighlight::Occluded(
span,
type_name,
)) => {
diag.span_label(
*span,
format!("lifetime `{}` appears in the type {}", self, type_name),
);
}
RegionNameSource::AnonRegionFromOutput(
RegionNameHighlight::Occluded(span, type_name),
mir_description,
) => {
diag.span_label(
*span,
format!(
"return type{} `{}` contains a lifetime `{}`",
mir_description, type_name, self
),
);
}
RegionNameSource::AnonRegionFromUpvar(span, upvar_name) => {
diag.span_label(
*span,
format!("lifetime `{}` appears in the type of `{}`", self, upvar_name),
);
}
RegionNameSource::AnonRegionFromOutput(span, mir_description, type_name) => {
RegionNameSource::AnonRegionFromOutput(
RegionNameHighlight::CannotMatchHirTy(span, type_name),
mir_description,
) => {
diag.span_label(*span, format!("return type{} is {}", mir_description, type_name));
}
RegionNameSource::AnonRegionFromYieldTy(span, type_name) => {
@ -349,19 +383,21 @@ fn give_name_if_anonymous_region_appears_in_arguments(
argument_index,
);
self.get_argument_hir_ty_for_highlighting(argument_index)
let highlight = self
.get_argument_hir_ty_for_highlighting(argument_index)
.and_then(|arg_hir_ty| self.highlight_if_we_can_match_hir_ty(fr, arg_ty, arg_hir_ty))
.or_else(|| {
.unwrap_or_else(|| {
// `highlight_if_we_cannot_match_hir_ty` needs to know the number we will give to
// the anonymous region. If it succeeds, the `synthesize_region_name` call below
// will increment the counter, "reserving" the number we just used.
let counter = *self.next_region_name.try_borrow().unwrap();
self.highlight_if_we_cannot_match_hir_ty(fr, arg_ty, span, counter)
})
.map(|highlight| RegionName {
name: self.synthesize_region_name(),
source: RegionNameSource::AnonRegionFromArgument(highlight),
})
});
Some(RegionName {
name: self.synthesize_region_name(),
source: RegionNameSource::AnonRegionFromArgument(highlight),
})
}
fn get_argument_hir_ty_for_highlighting(
@ -399,7 +435,7 @@ fn highlight_if_we_cannot_match_hir_ty(
ty: Ty<'tcx>,
span: Span,
counter: usize,
) -> Option<RegionNameHighlight> {
) -> RegionNameHighlight {
let mut highlight = RegionHighlightMode::default();
highlight.highlighting_region_vid(needle_fr, counter);
let type_name =
@ -411,9 +447,9 @@ fn highlight_if_we_cannot_match_hir_ty(
);
if type_name.find(&format!("'{}", counter)).is_some() {
// Only add a label if we can confirm that a region was labelled.
Some(RegionNameHighlight::CannotMatchHirTy(span, type_name))
RegionNameHighlight::CannotMatchHirTy(span, type_name)
} else {
None
RegionNameHighlight::Occluded(span, type_name)
}
}
@ -643,6 +679,7 @@ fn give_name_if_anonymous_region_appears_in_upvars(&self, fr: RegionVid) -> Opti
/// or be early bound (named, not in argument).
fn give_name_if_anonymous_region_appears_in_output(&self, fr: RegionVid) -> Option<RegionName> {
let tcx = self.infcx.tcx;
let hir = tcx.hir();
let return_ty = self.regioncx.universal_regions().unnormalized_output_ty;
debug!("give_name_if_anonymous_region_appears_in_output: return_ty = {:?}", return_ty);
@ -650,42 +687,123 @@ fn give_name_if_anonymous_region_appears_in_output(&self, fr: RegionVid) -> Opti
return None;
}
let mut highlight = RegionHighlightMode::default();
highlight.highlighting_region_vid(fr, *self.next_region_name.try_borrow().unwrap());
let type_name =
self.infcx.extract_inference_diagnostics_data(return_ty.into(), Some(highlight)).name;
let mir_hir_id = self.mir_hir_id();
let (return_span, mir_description) = match tcx.hir().get(self.mir_hir_id()) {
let (return_span, mir_description, hir_ty) = match hir.get(mir_hir_id) {
hir::Node::Expr(hir::Expr {
kind: hir::ExprKind::Closure(_, return_ty, _, span, gen_move),
kind: hir::ExprKind::Closure(_, return_ty, body_id, span, _),
..
}) => (
match return_ty.output {
hir::FnRetTy::DefaultReturn(_) => tcx.sess.source_map().end_point(*span),
hir::FnRetTy::Return(_) => return_ty.output.span(),
},
if gen_move.is_some() { " of generator" } else { " of closure" },
),
hir::Node::ImplItem(hir::ImplItem {
kind: hir::ImplItemKind::Fn(method_sig, _),
..
}) => (method_sig.decl.output.span(), ""),
_ => (self.body.span, ""),
}) => {
let (mut span, mut hir_ty) = match return_ty.output {
hir::FnRetTy::DefaultReturn(_) => {
(tcx.sess.source_map().end_point(*span), None)
}
hir::FnRetTy::Return(hir_ty) => (return_ty.output.span(), Some(hir_ty)),
};
let mir_description = match hir.body(*body_id).generator_kind {
Some(hir::GeneratorKind::Async(gen)) => match gen {
hir::AsyncGeneratorKind::Block => " of async block",
hir::AsyncGeneratorKind::Closure => " of async closure",
hir::AsyncGeneratorKind::Fn => {
let parent_item = hir.get(hir.get_parent_item(mir_hir_id));
let output = &parent_item
.fn_decl()
.expect("generator lowered from async fn should be in fn")
.output;
span = output.span();
if let hir::FnRetTy::Return(ret) = output {
hir_ty = Some(self.get_future_inner_return_ty(*ret));
}
" of async function"
}
},
Some(hir::GeneratorKind::Gen) => " of generator",
None => " of closure",
};
(span, mir_description, hir_ty)
}
node => match node.fn_decl() {
Some(fn_decl) => {
let hir_ty = match fn_decl.output {
hir::FnRetTy::DefaultReturn(_) => None,
hir::FnRetTy::Return(ty) => Some(ty),
};
(fn_decl.output.span(), "", hir_ty)
}
None => (self.body.span, "", None),
},
};
let highlight = hir_ty
.and_then(|hir_ty| self.highlight_if_we_can_match_hir_ty(fr, return_ty, hir_ty))
.unwrap_or_else(|| {
// `highlight_if_we_cannot_match_hir_ty` needs to know the number we will give to
// the anonymous region. If it succeeds, the `synthesize_region_name` call below
// will increment the counter, "reserving" the number we just used.
let counter = *self.next_region_name.try_borrow().unwrap();
self.highlight_if_we_cannot_match_hir_ty(fr, return_ty, return_span, counter)
});
Some(RegionName {
// This counter value will already have been used, so this function will increment it
// so the next value will be used next and return the region name that would have been
// used.
name: self.synthesize_region_name(),
source: RegionNameSource::AnonRegionFromOutput(
return_span,
mir_description.to_string(),
type_name,
),
source: RegionNameSource::AnonRegionFromOutput(highlight, mir_description.to_string()),
})
}
/// From the [`hir::Ty`] of an async function's lowered return type,
/// retrieve the `hir::Ty` representing the type the user originally wrote.
///
/// e.g. given the function:
///
/// ```
/// async fn foo() -> i32 {}
/// ```
///
/// this function, given the lowered return type of `foo`, an [`OpaqueDef`] that implements `Future<Output=i32>`,
/// returns the `i32`.
///
/// [`OpaqueDef`]: hir::TyKind::OpaqueDef
fn get_future_inner_return_ty(&self, hir_ty: &'tcx hir::Ty<'tcx>) -> &'tcx hir::Ty<'tcx> {
let hir = self.infcx.tcx.hir();
if let hir::TyKind::OpaqueDef(id, _) = hir_ty.kind {
let opaque_ty = hir.item(id.id);
if let hir::ItemKind::OpaqueTy(hir::OpaqueTy {
bounds:
[hir::GenericBound::LangItemTrait(
hir::LangItem::Future,
_,
_,
hir::GenericArgs {
bindings:
[hir::TypeBinding {
ident: Ident { name: sym::Output, .. },
kind: hir::TypeBindingKind::Equality { ty },
..
}],
..
},
)],
..
}) = opaque_ty.kind
{
ty
} else {
span_bug!(
hir_ty.span,
"bounds from lowered return type of async fn did not match expected format: {:?}",
opaque_ty
);
}
} else {
span_bug!(
hir_ty.span,
"lowered return type of async fn is not OpaqueDef: {:?}",
hir_ty
);
}
}
fn give_name_if_anonymous_region_appears_in_yield_ty(
&self,
fr: RegionVid,

37
src/test/ui/async-await/issue-74072-lifetime-name-annotations.rs Normal file
View File

@ -0,0 +1,37 @@
// edition:2018
#![feature(async_closure)]
use std::future::Future;
// test the quality of annotations giving lifetimes names (`'1`) when async constructs are involved
pub async fn async_fn(x: &mut i32) -> &i32 {
let y = &*x;
*x += 1; //~ ERROR cannot assign to `*x` because it is borrowed
y
}
pub fn async_closure(x: &mut i32) -> impl Future<Output=&i32> {
(async move || {
let y = &*x;
*x += 1; //~ ERROR cannot assign to `*x` because it is borrowed
y
})()
}
pub fn async_closure_explicit_return_type(x: &mut i32) -> impl Future<Output=&i32> {
(async move || -> &i32 {
let y = &*x;
*x += 1; //~ ERROR cannot assign to `*x` because it is borrowed
y
})()
}
pub fn async_block(x: &mut i32) -> impl Future<Output=&i32> {
async move {
let y = &*x;
*x += 1; //~ ERROR cannot assign to `*x` because it is borrowed
y
}
}
fn main() {}

51
src/test/ui/async-await/issue-74072-lifetime-name-annotations.stderr Normal file
View File

@ -0,0 +1,51 @@
error[E0506]: cannot assign to `*x` because it is borrowed
--> $DIR/issue-74072-lifetime-name-annotations.rs:9:5
|
LL | pub async fn async_fn(x: &mut i32) -> &i32 {
| - let's call the lifetime of this reference `'1`
LL | let y = &*x;
| --- borrow of `*x` occurs here
LL | *x += 1;
| ^^^^^^^ assignment to borrowed `*x` occurs here
LL | y
| - returning this value requires that `*x` is borrowed for `'1`
error[E0506]: cannot assign to `*x` because it is borrowed
--> $DIR/issue-74072-lifetime-name-annotations.rs:16:9
|
LL | let y = &*x;
| --- borrow of `*x` occurs here
LL | *x += 1;
| ^^^^^^^ assignment to borrowed `*x` occurs here
LL | y
| - returning this value requires that `*x` is borrowed for `'1`
LL | })()
| - return type of async closure is &'1 i32
error[E0506]: cannot assign to `*x` because it is borrowed
--> $DIR/issue-74072-lifetime-name-annotations.rs:24:9
|
LL | (async move || -> &i32 {
| - let's call the lifetime of this reference `'1`
LL | let y = &*x;
| --- borrow of `*x` occurs here
LL | *x += 1;
| ^^^^^^^ assignment to borrowed `*x` occurs here
LL | y
| - returning this value requires that `*x` is borrowed for `'1`
error[E0506]: cannot assign to `*x` because it is borrowed
--> $DIR/issue-74072-lifetime-name-annotations.rs:32:9
|
LL | let y = &*x;
| --- borrow of `*x` occurs here
LL | *x += 1;
| ^^^^^^^ assignment to borrowed `*x` occurs here
LL | y
| - returning this value requires that `*x` is borrowed for `'1`
LL | }
| - return type of async block is &'1 i32
error: aborting due to 4 previous errors
For more information about this error, try `rustc --explain E0506`.

19
src/test/ui/async-await/issue-74497-lifetime-in-opaque.rs Normal file
View File

@ -0,0 +1,19 @@
// edition:2018
// test that names give to anonymous lifetimes in opaque types like `impl Future` are correctly
// introduced in error messages
use std::future::Future;
pub async fn foo<F, T>(_: F)
where
F: Fn(&u8) -> T,
T: Future<Output = ()>,
{
}
pub async fn bar(_: &u8) {}
fn main() {
let _ = foo(|x| bar(x)); //~ ERROR lifetime may not live long enough
}

11
src/test/ui/async-await/issue-74497-lifetime-in-opaque.stderr Normal file
View File

@ -0,0 +1,11 @@
error: lifetime may not live long enough
--> $DIR/issue-74497-lifetime-in-opaque.rs:18:21
|
LL | let _ = foo(|x| bar(x));
| -- ^^^^^^ returning this value requires that `'1` must outlive `'2`
| ||
| |return type of closure `impl Future` contains a lifetime `'2`
| has type `&'1 u8`
error: aborting due to previous error

2
src/test/ui/borrowck/issue-53432-nested-closure-outlives-borrowed-value.stderr
View File

@ -4,7 +4,7 @@ error: lifetime may not live long enough
LL | let _action = move || {
| -------
| | |
| | return type of closure is [closure@$DIR/issue-53432-nested-closure-outlives-borrowed-value.rs:4:9: 4:15]
| | return type of closure `[closure@$DIR/issue-53432-nested-closure-outlives-borrowed-value.rs:4:9: 4:15]` contains a lifetime `'2`
| lifetime `'1` represents this closure's body
LL | || f() // The `nested` closure
| ^^^^^^ returning this value requires that `'1` must outlive `'2`

4
src/test/ui/nll/issue-58053.stderr
View File

@ -2,9 +2,9 @@ error: lifetime may not live long enough
--> $DIR/issue-58053.rs:6:33
|
LL | let f = |x: &i32| -> &i32 { x };
| - ---- ^ returning this value requires that `'1` must outlive `'2`
| - - ^ returning this value requires that `'1` must outlive `'2`
| | |
| | return type of closure is &'2 i32
| | let's call the lifetime of this reference `'2`
| let's call the lifetime of this reference `'1`
error: lifetime may not live long enough