Skip to content

Commit

Permalink
Rollup merge of rust-lang#96385 - marmeladema:nll-fix-trait-lifetime-…
Browse files Browse the repository at this point in the history
…bound-suggestions, r=jackh726

Recover most `impl Trait` and `dyn Trait` lifetime bound suggestions under NLL

This is done by replacing the duplicated (and very partial) implementation from borrowck with one inspsired from `NiceRegionError::try_report_static_impl_trait` and by re-using `suggest_new_region_bound`.

Fixes rust-lang#96277

r? `@jackh726`
  • Loading branch information
Dylan-DPC authored Apr 26, 2022
2 parents 27195ed + e1b074a commit 7ddf9f4
Showing 14 changed files with 244 additions and 148 deletions.
119 changes: 42 additions & 77 deletions compiler/rustc_borrowck/src/diagnostics/region_errors.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
//! Error reporting machinery for lifetime errors.
use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed};
use rustc_errors::{Diagnostic, DiagnosticBuilder, ErrorGuaranteed};
use rustc_infer::infer::{
error_reporting::nice_region_error::NiceRegionError,
error_reporting::unexpected_hidden_region_diagnostic, NllRegionVariableOrigin,
RelateParamBound,
error_reporting::nice_region_error::{self, find_param_with_region, NiceRegionError},
error_reporting::unexpected_hidden_region_diagnostic,
NllRegionVariableOrigin, RelateParamBound,
};
use rustc_middle::hir::place::PlaceBase;
use rustc_middle::mir::{ConstraintCategory, ReturnConstraint};
use rustc_middle::ty::subst::{InternalSubsts, Subst};
use rustc_middle::ty::subst::InternalSubsts;
use rustc_middle::ty::{self, RegionVid, Ty};
use rustc_span::symbol::{kw, sym};
use rustc_span::{BytePos, Span};
use rustc_span::symbol::sym;
use rustc_span::Span;

use crate::borrowck_errors;

@@ -651,82 +651,47 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
fr_name: RegionName,
outlived_fr: RegionVid,
) {
if let (Some(f), Some(ty::ReStatic)) =
(self.to_error_region(fr), self.to_error_region(outlived_fr).as_deref())
if let (Some(f), Some(outlived_f)) =
(self.to_error_region(fr), self.to_error_region(outlived_fr))
{
if let Some(&ty::Opaque(did, substs)) = self
if *outlived_f != ty::ReStatic {
return;
}

let fn_returns = self
.infcx
.tcx
.is_suitable_region(f)
.map(|r| r.def_id)
.and_then(|id| self.infcx.tcx.return_type_impl_trait(id))
.map(|(ty, _)| ty.kind())
{
// Check whether or not the impl trait return type is intended to capture
// data with the static lifetime.
//
// eg. check for `impl Trait + 'static` instead of `impl Trait`.
let has_static_predicate = {
let bounds = self.infcx.tcx.explicit_item_bounds(did);

let mut found = false;
for (bound, _) in bounds {
if let ty::PredicateKind::TypeOutlives(ty::OutlivesPredicate(_, r)) =
bound.kind().skip_binder()
{
let r = r.subst(self.infcx.tcx, substs);
if r.is_static() {
found = true;
break;
} else {
// If there's already a lifetime bound, don't
// suggest anything.
return;
}
}
}

found
};
.map(|r| self.infcx.tcx.return_type_impl_or_dyn_traits(r.def_id))
.unwrap_or_default();

debug!(
"add_static_impl_trait_suggestion: has_static_predicate={:?}",
has_static_predicate
);
let static_str = kw::StaticLifetime;
// If there is a static predicate, then the only sensible suggestion is to replace
// fr with `'static`.
if has_static_predicate {
diag.help(&format!("consider replacing `{fr_name}` with `{static_str}`"));
} else {
// Otherwise, we should suggest adding a constraint on the return type.
let span = self.infcx.tcx.def_span(did);
if let Ok(snippet) = self.infcx.tcx.sess.source_map().span_to_snippet(span) {
let suggestable_fr_name = if fr_name.was_named() {
fr_name.to_string()
} else {
"'_".to_string()
};
let span = if snippet.ends_with(';') {
// `type X = impl Trait;`
span.with_hi(span.hi() - BytePos(1))
} else {
span
};
let suggestion = format!(" + {suggestable_fr_name}");
let span = span.shrink_to_hi();
diag.span_suggestion(
span,
&format!(
"to allow this `impl Trait` to capture borrowed data with lifetime \
`{fr_name}`, add `{suggestable_fr_name}` as a bound",
),
suggestion,
Applicability::MachineApplicable,
);
}
}
if fn_returns.is_empty() {
return;
}

let param = if let Some(param) = find_param_with_region(self.infcx.tcx, f, outlived_f) {
param
} else {
return;
};

let lifetime = if f.has_name() { fr_name.to_string() } else { "'_".to_string() };

let arg = match param.param.pat.simple_ident() {
Some(simple_ident) => format!("argument `{}`", simple_ident),
None => "the argument".to_string(),
};
let captures = format!("captures data from {}", arg);

return nice_region_error::suggest_new_region_bound(
self.infcx.tcx,
diag,
fn_returns,
lifetime,
Some(arg),
captures,
Some((param.param_ty_span, param.param_ty.to_string())),
);
}
}
}
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@ mod trait_impl_difference;
mod util;

pub use static_impl_trait::suggest_new_region_bound;
pub use util::find_param_with_region;

impl<'cx, 'tcx> InferCtxt<'cx, 'tcx> {
pub fn try_report_nice_region_error(&self, error: &RegionResolutionError<'tcx>) -> bool {
Original file line number Diff line number Diff line change
@@ -2,14 +2,15 @@
//! anonymous regions.
use crate::infer::error_reporting::nice_region_error::NiceRegionError;
use crate::infer::TyCtxt;
use rustc_hir as hir;
use rustc_hir::def_id::LocalDefId;
use rustc_middle::ty::{self, Binder, DefIdTree, Region, Ty, TypeFoldable};
use rustc_span::Span;

/// Information about the anonymous region we are searching for.
#[derive(Debug)]
pub(super) struct AnonymousParamInfo<'tcx> {
pub struct AnonymousParamInfo<'tcx> {
/// The parameter corresponding to the anonymous region.
pub param: &'tcx hir::Param<'tcx>,
/// The type corresponding to the anonymous region parameter.
@@ -22,76 +23,83 @@ pub(super) struct AnonymousParamInfo<'tcx> {
pub is_first: bool,
}

// This method walks the Type of the function body parameters using
// `fold_regions()` function and returns the
// &hir::Param of the function parameter corresponding to the anonymous
// region and the Ty corresponding to the named region.
// Currently only the case where the function declaration consists of
// one named region and one anonymous region is handled.
// Consider the example `fn foo<'a>(x: &'a i32, y: &i32) -> &'a i32`
// Here, we would return the hir::Param for y, we return the type &'a
// i32, which is the type of y but with the anonymous region replaced
// with 'a, the corresponding bound region and is_first which is true if
// the hir::Param is the first parameter in the function declaration.
pub fn find_param_with_region<'tcx>(
tcx: TyCtxt<'tcx>,
anon_region: Region<'tcx>,
replace_region: Region<'tcx>,
) -> Option<AnonymousParamInfo<'tcx>> {
let (id, bound_region) = match *anon_region {
ty::ReFree(ref free_region) => (free_region.scope, free_region.bound_region),
ty::ReEarlyBound(ebr) => {
(tcx.parent(ebr.def_id).unwrap(), ty::BoundRegionKind::BrNamed(ebr.def_id, ebr.name))
}
_ => return None, // not a free region
};

let hir = &tcx.hir();
let hir_id = hir.local_def_id_to_hir_id(id.as_local()?);
let body_id = hir.maybe_body_owned_by(hir_id)?;
let body = hir.body(body_id);
let owner_id = hir.body_owner(body_id);
let fn_decl = hir.fn_decl_by_hir_id(owner_id).unwrap();
let poly_fn_sig = tcx.fn_sig(id);
let fn_sig = tcx.liberate_late_bound_regions(id, poly_fn_sig);
body.params
.iter()
.take(if fn_sig.c_variadic {
fn_sig.inputs().len()
} else {
assert_eq!(fn_sig.inputs().len(), body.params.len());
body.params.len()
})
.enumerate()
.find_map(|(index, param)| {
// May return None; sometimes the tables are not yet populated.
let ty = fn_sig.inputs()[index];
let mut found_anon_region = false;
let new_param_ty = tcx.fold_regions(ty, &mut false, |r, _| {
if r == anon_region {
found_anon_region = true;
replace_region
} else {
r
}
});
if found_anon_region {
let ty_hir_id = fn_decl.inputs[index].hir_id;
let param_ty_span = hir.span(ty_hir_id);
let is_first = index == 0;
Some(AnonymousParamInfo {
param,
param_ty: new_param_ty,
param_ty_span,
bound_region,
is_first,
})
} else {
None
}
})
}

impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
// This method walks the Type of the function body parameters using
// `fold_regions()` function and returns the
// &hir::Param of the function parameter corresponding to the anonymous
// region and the Ty corresponding to the named region.
// Currently only the case where the function declaration consists of
// one named region and one anonymous region is handled.
// Consider the example `fn foo<'a>(x: &'a i32, y: &i32) -> &'a i32`
// Here, we would return the hir::Param for y, we return the type &'a
// i32, which is the type of y but with the anonymous region replaced
// with 'a, the corresponding bound region and is_first which is true if
// the hir::Param is the first parameter in the function declaration.
pub(super) fn find_param_with_region(
&self,
anon_region: Region<'tcx>,
replace_region: Region<'tcx>,
) -> Option<AnonymousParamInfo<'_>> {
let (id, bound_region) = match *anon_region {
ty::ReFree(ref free_region) => (free_region.scope, free_region.bound_region),
ty::ReEarlyBound(ebr) => (
self.tcx().parent(ebr.def_id).unwrap(),
ty::BoundRegionKind::BrNamed(ebr.def_id, ebr.name),
),
_ => return None, // not a free region
};

let hir = &self.tcx().hir();
let hir_id = hir.local_def_id_to_hir_id(id.as_local()?);
let body_id = hir.maybe_body_owned_by(hir_id)?;
let body = hir.body(body_id);
let owner_id = hir.body_owner(body_id);
let fn_decl = hir.fn_decl_by_hir_id(owner_id).unwrap();
let poly_fn_sig = self.tcx().fn_sig(id);
let fn_sig = self.tcx().liberate_late_bound_regions(id, poly_fn_sig);
body.params
.iter()
.take(if fn_sig.c_variadic {
fn_sig.inputs().len()
} else {
assert_eq!(fn_sig.inputs().len(), body.params.len());
body.params.len()
})
.enumerate()
.find_map(|(index, param)| {
// May return None; sometimes the tables are not yet populated.
let ty = fn_sig.inputs()[index];
let mut found_anon_region = false;
let new_param_ty = self.tcx().fold_regions(ty, &mut false, |r, _| {
if r == anon_region {
found_anon_region = true;
replace_region
} else {
r
}
});
if found_anon_region {
let ty_hir_id = fn_decl.inputs[index].hir_id;
let param_ty_span = hir.span(ty_hir_id);
let is_first = index == 0;
Some(AnonymousParamInfo {
param,
param_ty: new_param_ty,
param_ty_span,
bound_region,
is_first,
})
} else {
None
}
})
find_param_with_region(self.tcx(), anon_region, replace_region)
}

// Here, we check for the case where the anonymous region
6 changes: 5 additions & 1 deletion src/test/ui/impl-trait/issues/issue-88236-2.nll.stderr
Original file line number Diff line number Diff line change
@@ -24,10 +24,14 @@ LL | fn make_bad_impl<'b>(x: &'b ()) -> impl for<'a> Hrtb<'a, Assoc = impl Send
LL | x
| ^ returning this value requires that `'b` must outlive `'static`
|
help: to allow this `impl Trait` to capture borrowed data with lifetime `'b`, add `'b` as a bound
help: to declare that the `impl Trait` captures data from argument `x`, you can add an explicit `'b` lifetime bound
|
LL | fn make_bad_impl<'b>(x: &'b ()) -> impl for<'a> Hrtb<'a, Assoc = impl Send + 'a> + 'b {
| ++++
help: to declare that the `impl Trait` captures data from argument `x`, you can add an explicit `'b` lifetime bound
|
LL | fn make_bad_impl<'b>(x: &'b ()) -> impl for<'a> Hrtb<'a, Assoc = impl Send + 'a + 'b> {
| ++++

error: implementation of `Hrtb` is not general enough
--> $DIR/issue-88236-2.rs:20:5
Original file line number Diff line number Diff line change
@@ -32,15 +32,29 @@ LL | fn elided2(x: &i32) -> impl Copy + 'static { x }
| |
| let's call the lifetime of this reference `'1`
|
= help: consider replacing `'1` with `'static`
help: consider changing the `impl Trait`'s explicit `'static` bound to the lifetime of argument `x`
|
LL | fn elided2(x: &i32) -> impl Copy + '_ { x }
| ~~
help: alternatively, add an explicit `'static` bound to this reference
|
LL | fn elided2(x: &'static i32) -> impl Copy + 'static { x }
| ~~~~~~~~~~~~

error: lifetime may not live long enough
--> $DIR/must_outlive_least_region_or_bound.rs:11:55
|
LL | fn explicit2<'a>(x: &'a i32) -> impl Copy + 'static { x }
| -- lifetime `'a` defined here ^ returning this value requires that `'a` must outlive `'static`
|
= help: consider replacing `'a` with `'static`
help: consider changing the `impl Trait`'s explicit `'static` bound to the lifetime of argument `x`
|
LL | fn explicit2<'a>(x: &'a i32) -> impl Copy + 'a { x }
| ~~
help: alternatively, add an explicit `'static` bound to this reference
|
LL | fn explicit2<'a>(x: &'static i32) -> impl Copy + 'static { x }
| ~~~~~~~~~~~~

error[E0621]: explicit lifetime required in the type of `x`
--> $DIR/must_outlive_least_region_or_bound.rs:13:41
@@ -57,14 +71,30 @@ LL | fn elided5(x: &i32) -> (Box<dyn Debug>, impl Debug) { (Box::new(x), x) }
| - ^^^^^^^^^^^^^^^^ returning this value requires that `'1` must outlive `'static`
| |
| let's call the lifetime of this reference `'1`
|
help: to declare that the trait object captures data from argument `x`, you can add an explicit `'_` lifetime bound
|
LL | fn elided5(x: &i32) -> (Box<dyn Debug + '_>, impl Debug) { (Box::new(x), x) }
| ++++
help: to declare that the `impl Trait` captures data from argument `x`, you can add an explicit `'_` lifetime bound
|
LL | fn elided5(x: &i32) -> (Box<dyn Debug>, impl Debug + '_) { (Box::new(x), x) }
| ++++

error: lifetime may not live long enough
--> $DIR/must_outlive_least_region_or_bound.rs:29:69
|
LL | fn with_bound<'a>(x: &'a i32) -> impl LifetimeTrait<'a> + 'static { x }
| -- lifetime `'a` defined here ^ returning this value requires that `'a` must outlive `'static`
|
= help: consider replacing `'a` with `'static`
help: consider changing the `impl Trait`'s explicit `'static` bound to the lifetime of argument `x`
|
LL | fn with_bound<'a>(x: &'a i32) -> impl LifetimeTrait<'a> + 'a { x }
| ~~
help: alternatively, add an explicit `'static` bound to this reference
|
LL | fn with_bound<'a>(x: &'static i32) -> impl LifetimeTrait<'a> + 'static { x }
| ~~~~~~~~~~~~

error[E0700]: hidden type for `impl Trait` captures lifetime that does not appear in bounds
--> $DIR/must_outlive_least_region_or_bound.rs:34:5
Loading

0 comments on commit 7ddf9f4

Please sign in to comment.