Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'static lifetime suggestion when GAT implied 'static requirement from HRTB #106747

Merged
merged 1 commit into from
Jan 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 113 additions & 4 deletions compiler/rustc_borrowck/src/diagnostics/region_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,13 @@
use rustc_data_structures::fx::FxIndexSet;
use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed, MultiSpan};
use rustc_hir as hir;
use rustc_hir::def::Res::Def;
use rustc_hir::def_id::DefId;
use rustc_hir::intravisit::Visitor;
use rustc_hir::GenericBound::Trait;
use rustc_hir::QPath::Resolved;
use rustc_hir::WherePredicate::BoundPredicate;
use rustc_hir::{PolyTraitRef, TyKind, WhereBoundPredicate};
use rustc_infer::infer::{
error_reporting::nice_region_error::{
self, find_anon_type, find_param_with_region, suggest_adding_lifetime_params,
Expand Down Expand Up @@ -186,6 +191,101 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
false
}

// For generic associated types (GATs) which implied 'static requirement
// from higher-ranked trait bounds (HRTB). Try to locate span of the trait
// and the span which bounded to the trait for adding 'static lifetime suggestion
fn suggest_static_lifetime_for_gat_from_hrtb(
&self,
diag: &mut DiagnosticBuilder<'_, ErrorGuaranteed>,
lower_bound: RegionVid,
) {
let mut suggestions = vec![];
let hir = self.infcx.tcx.hir();

// find generic associated types in the given region 'lower_bound'
let gat_id_and_generics = self
.regioncx
.placeholders_contained_in(lower_bound)
.map(|placeholder| {
if let Some(id) = placeholder.name.get_id()
&& let Some(placeholder_id) = id.as_local()
&& let gat_hir_id = hir.local_def_id_to_hir_id(placeholder_id)
&& let Some(generics_impl) = hir.get_parent(gat_hir_id).generics()
{
Some((gat_hir_id, generics_impl))
} else {
None
}
})
.collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing that you're only using it once afterwards, I don't think you need to collect this first, right? It can remain an iterator. Collecting has a marginal perf cost, but we can avoid it with confidence thanks to the type ownership system :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed. The reason for keeping the code this way I thought was for more readability and ease of debugging.

debug!(?gat_id_and_generics);

// find higher-ranked trait bounds bounded to the generic associated types
let mut hrtb_bounds = vec![];
gat_id_and_generics.iter().flatten().for_each(|(gat_hir_id, generics)| {
for pred in generics.predicates {
let BoundPredicate(
WhereBoundPredicate {
bound_generic_params,
bounds,
..
}) = pred else { continue; };
if bound_generic_params
.iter()
.rfind(|bgp| hir.local_def_id_to_hir_id(bgp.def_id) == *gat_hir_id)
.is_some()
{
for bound in *bounds {
hrtb_bounds.push(bound);
}
}
}
});
debug!(?hrtb_bounds);

hrtb_bounds.iter().for_each(|bound| {
let Trait(PolyTraitRef { trait_ref, span: trait_span, .. }, _) = bound else { return; };
diag.span_note(
*trait_span,
format!("due to current limitations in the borrow checker, this implies a `'static` lifetime")
);
let Some(generics_fn) = hir.get_generics(self.body.source.def_id().expect_local()) else { return; };
let Def(_, trait_res_defid) = trait_ref.path.res else { return; };
debug!(?generics_fn);
generics_fn.predicates.iter().for_each(|predicate| {
let BoundPredicate(
WhereBoundPredicate {
span: bounded_span,
bounded_ty,
bounds,
..
}
) = predicate else { return; };
bounds.iter().for_each(|bd| {
if let Trait(PolyTraitRef { trait_ref: tr_ref, .. }, _) = bd
&& let Def(_, res_defid) = tr_ref.path.res
&& res_defid == trait_res_defid // trait id matches
&& let TyKind::Path(Resolved(_, path)) = bounded_ty.kind
&& let Def(_, defid) = path.res
&& generics_fn.params
.iter()
.rfind(|param| param.def_id.to_def_id() == defid)
.is_some() {
suggestions.push((bounded_span.shrink_to_hi(), format!(" + 'static")));
}
});
});
});
if suggestions.len() > 0 {
suggestions.dedup();
diag.multipart_suggestion_verbose(
format!("consider restricting the type parameter to the `'static` lifetime"),
suggestions,
Applicability::MaybeIncorrect,
);
}
}

/// Produces nice borrowck error diagnostics for all the errors collected in `nll_errors`.
pub(crate) fn report_region_errors(&mut self, nll_errors: RegionErrors<'tcx>) {
// Iterate through all the errors, producing a diagnostic for each one. The diagnostics are
Expand Down Expand Up @@ -223,12 +323,21 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
// to report it; we could probably handle it by
// iterating over the universal regions and reporting
// an error that multiple bounds are required.
self.buffer_error(self.infcx.tcx.sess.create_err(
GenericDoesNotLiveLongEnough {
let mut diag =
self.infcx.tcx.sess.create_err(GenericDoesNotLiveLongEnough {
kind: type_test.generic_kind.to_string(),
span: type_test_span,
},
));
});

// Add notes and suggestions for the case of 'static lifetime
// implied but not specified when a generic associated types
// are from higher-ranked trait bounds
self.suggest_static_lifetime_for_gat_from_hrtb(
&mut diag,
type_test.lower_bound,
);

self.buffer_error(diag);
}
}

Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_borrowck/src/region_infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,14 @@ impl<'tcx> RegionInferenceContext<'tcx> {
self.scc_values.region_value_str(scc)
}

pub(crate) fn placeholders_contained_in<'a>(
&'a self,
r: RegionVid,
) -> impl Iterator<Item = ty::PlaceholderRegion> + 'a {
let scc = self.constraint_sccs.scc(r.to_region_vid());
self.scc_values.placeholders_contained_in(scc)
}

/// Returns access to the value of `r` for debugging purposes.
pub(crate) fn region_universe(&self, r: RegionVid) -> ty::UniverseIndex {
let scc = self.constraint_sccs.scc(r.to_region_vid());
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_middle/src/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,13 @@ impl BoundRegionKind {

None
}

pub fn get_id(&self) -> Option<DefId> {
match *self {
BoundRegionKind::BrNamed(id, _) => return Some(id),
_ => None,
}
}
}

pub trait Article {
Expand Down
10 changes: 10 additions & 0 deletions tests/ui/generic-associated-types/collectivity-regression.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ LL | | // probably should work.
LL | | let _x = x;
LL | | };
| |_____^
|
note: due to current limitations in the borrow checker, this implies a `'static` lifetime
--> $DIR/collectivity-regression.rs:11:16
|
LL | for<'a> T: Get<Value<'a> = ()>,
| ^^^^^^^^^^^^^^^^^^^
help: consider restricting the type parameter to the `'static` lifetime
|
LL | for<'a> T: Get<Value<'a> = ()> + 'static,
| +++++++++

error: aborting due to previous error

43 changes: 43 additions & 0 deletions tests/ui/lifetimes/issue-105507.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// run-rustfix
//
#![allow(warnings)]
struct Wrapper<'a, T: ?Sized>(&'a T);

trait Project {
type Projected<'a> where Self: 'a;
fn project(this: Wrapper<'_, Self>) -> Self::Projected<'_>;
}
trait MyTrait {}
trait ProjectedMyTrait {}

impl<T> Project for Option<T> {
type Projected<'a> = Option<Wrapper<'a, T>> where T: 'a;
fn project(this: Wrapper<'_, Self>) -> Self::Projected<'_> {
this.0.as_ref().map(Wrapper)
}
}

impl<T: MyTrait> MyTrait for Option<Wrapper<'_, T>> {}

impl<T: ProjectedMyTrait> MyTrait for Wrapper<'_, T> {}

impl<T> ProjectedMyTrait for T
where
T: Project,
for<'a> T::Projected<'a>: MyTrait,
//~^ NOTE due to current limitations in the borrow checker, this implies a `'static` lifetime
//~| NOTE due to current limitations in the borrow checker, this implies a `'static` lifetime
{}

fn require_trait<T: MyTrait>(_: T) {}

fn foo<T : MyTrait + 'static + 'static, U : MyTrait + 'static + 'static>(wrap: Wrapper<'_, Option<T>>, wrap1: Wrapper<'_, Option<U>>) {
//~^ HELP consider restricting the type parameter to the `'static` lifetime
//~| HELP consider restricting the type parameter to the `'static` lifetime
require_trait(wrap);
//~^ ERROR `T` does not live long enough
require_trait(wrap1);
//~^ ERROR `U` does not live long enough
}

fn main() {}
43 changes: 43 additions & 0 deletions tests/ui/lifetimes/issue-105507.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// run-rustfix
//
#![allow(warnings)]
struct Wrapper<'a, T: ?Sized>(&'a T);

trait Project {
type Projected<'a> where Self: 'a;
fn project(this: Wrapper<'_, Self>) -> Self::Projected<'_>;
}
trait MyTrait {}
trait ProjectedMyTrait {}

impl<T> Project for Option<T> {
type Projected<'a> = Option<Wrapper<'a, T>> where T: 'a;
fn project(this: Wrapper<'_, Self>) -> Self::Projected<'_> {
this.0.as_ref().map(Wrapper)
}
}

impl<T: MyTrait> MyTrait for Option<Wrapper<'_, T>> {}

impl<T: ProjectedMyTrait> MyTrait for Wrapper<'_, T> {}

impl<T> ProjectedMyTrait for T
where
T: Project,
for<'a> T::Projected<'a>: MyTrait,
//~^ NOTE due to current limitations in the borrow checker, this implies a `'static` lifetime
//~| NOTE due to current limitations in the borrow checker, this implies a `'static` lifetime
{}

fn require_trait<T: MyTrait>(_: T) {}

fn foo<T : MyTrait, U : MyTrait>(wrap: Wrapper<'_, Option<T>>, wrap1: Wrapper<'_, Option<U>>) {
//~^ HELP consider restricting the type parameter to the `'static` lifetime
//~| HELP consider restricting the type parameter to the `'static` lifetime
require_trait(wrap);
//~^ ERROR `T` does not live long enough
require_trait(wrap1);
//~^ ERROR `U` does not live long enough
}

fn main() {}
34 changes: 34 additions & 0 deletions tests/ui/lifetimes/issue-105507.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
error: `T` does not live long enough
--> $DIR/issue-105507.rs:37:5
|
LL | require_trait(wrap);
| ^^^^^^^^^^^^^^^^^^^
|
note: due to current limitations in the borrow checker, this implies a `'static` lifetime
--> $DIR/issue-105507.rs:27:35
|
LL | for<'a> T::Projected<'a>: MyTrait,
| ^^^^^^^
help: consider restricting the type parameter to the `'static` lifetime
|
LL | fn foo<T : MyTrait + 'static, U : MyTrait + 'static>(wrap: Wrapper<'_, Option<T>>, wrap1: Wrapper<'_, Option<U>>) {
| +++++++++ +++++++++

error: `U` does not live long enough
--> $DIR/issue-105507.rs:39:5
|
LL | require_trait(wrap1);
| ^^^^^^^^^^^^^^^^^^^^
|
note: due to current limitations in the borrow checker, this implies a `'static` lifetime
--> $DIR/issue-105507.rs:27:35
|
LL | for<'a> T::Projected<'a>: MyTrait,
| ^^^^^^^
help: consider restricting the type parameter to the `'static` lifetime
|
LL | fn foo<T : MyTrait + 'static, U : MyTrait + 'static>(wrap: Wrapper<'_, Option<T>>, wrap1: Wrapper<'_, Option<U>>) {
| +++++++++ +++++++++

error: aborting due to 2 previous errors