Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 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
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,11 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
self.region_bound_pairs
.insert(ty::OutlivesPredicate(GenericKind::Projection(projection_b), r_a));
}

OutlivesBound::RegionSubOpaque(r_a, def_id, substs) => {
self.region_bound_pairs
.insert(ty::OutlivesPredicate(GenericKind::Opaque(def_id, substs), r_a));
}
}
}
}
Expand Down
13 changes: 12 additions & 1 deletion compiler/rustc_infer/src/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2314,7 +2314,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
&self,
generic_param_scope: LocalDefId,
span: Span,
origin: Option<SubregionOrigin<'tcx>>,
mut origin: Option<SubregionOrigin<'tcx>>,
bound_kind: GenericKind<'tcx>,
sub: Region<'tcx>,
) -> DiagnosticBuilder<'a, ErrorGuaranteed> {
Expand Down Expand Up @@ -2349,6 +2349,14 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
None
}
}
GenericKind::Opaque(def_id, _substs) => {
// Avoid emitting a `... so that the type` message at the error site.
// It would be out of order for return position impl trait
origin = None;
// Make sure the lifetime suggestion is on the RPIT instead of proposing
// to add a bound for opaque types (which isn't possible)
Some((self.tcx.def_span(def_id).shrink_to_hi(), true))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This would suggest making changes at the opaque type's definition site and the suggested lifetime probably cannot be named in that scope.

I still think removing the need_infer hack for opaque types in https://github.com/rust-lang/rust/pull/95474/files#r880479741 is the way to go. After all it's just a diagnostics hack for projection types only.

for example:

// Now we suggest adding `+ 'static` to the RPIT here.
fn opaque<T>(_: T) -> impl Sized {}

// ... but I think we should suggest `T: 'static` here similar to the current stable.
fn test<T>(t: T) -> impl Sized + 'static {
    opaque(t)
}

}
_ => None,
};

Expand All @@ -2374,6 +2382,9 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
let labeled_user_string = match bound_kind {
GenericKind::Param(ref p) => format!("the parameter type `{}`", p),
GenericKind::Projection(ref p) => format!("the associated type `{}`", p),
GenericKind::Opaque(def_id, substs) => {
format!("the opaque type `{}`", self.tcx.def_path_str_with_substs(def_id, substs))
}
};

if let Some(SubregionOrigin::CompareImplItemObligation {
Expand Down
17 changes: 15 additions & 2 deletions compiler/rustc_infer/src/infer/outlives/components.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@
// RFC for reference.

use rustc_data_structures::sso::SsoHashSet;
use rustc_hir::def_id::DefId;
use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitable};
use rustc_middle::ty::{self, SubstsRef, Ty, TyCtxt, TypeVisitable};
use smallvec::{smallvec, SmallVec};

#[derive(Debug)]
Expand Down Expand Up @@ -45,6 +46,8 @@ pub enum Component<'tcx> {
// them. This gives us room to improve the regionck reasoning in
// the future without breaking backwards compat.
EscapingProjection(Vec<Component<'tcx>>),

Opaque(DefId, SubstsRef<'tcx>),
}

/// Push onto `out` all the things that must outlive `'a` for the condition
Expand Down Expand Up @@ -120,6 +123,17 @@ fn compute_components<'tcx>(
out.push(Component::Param(p));
}

// Ignore lifetimes found in opaque types. Opaque types can
// have lifetimes in their substs which their hidden type doesn't
// actually use. If we inferred that an opaque type is outlived by
// its parameter lifetimes, then we could prove that any lifetime
// outlives any other lifetime, which is unsound.
// See https://github.com/rust-lang/rust/issues/84305 for
// more details.
ty::Opaque(def_id, substs) => {
out.push(Component::Opaque(def_id, substs));
},

// For projections, we prefer to generate an obligation like
// `<P0 as Trait<P1...Pn>>::Foo: 'a`, because this gives the
// regionck more ways to prove that it holds. However,
Expand Down Expand Up @@ -168,7 +182,6 @@ fn compute_components<'tcx>(
ty::Float(..) | // OutlivesScalar
ty::Never | // ...
ty::Adt(..) | // OutlivesNominalType
ty::Opaque(..) | // OutlivesNominalType (ish)
ty::Foreign(..) | // OutlivesNominalType
ty::Str | // OutlivesScalar (ish)
ty::Slice(..) | // ...
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_infer/src/infer/outlives/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ impl<'a, 'tcx> OutlivesEnvironmentBuilder<'tcx> {
self.region_bound_pairs
.insert(ty::OutlivesPredicate(GenericKind::Projection(projection_b), r_a));
}
OutlivesBound::RegionSubOpaque(r_a, def_id, substs) => {
self.region_bound_pairs
.insert(ty::OutlivesPredicate(GenericKind::Opaque(def_id, substs), r_a));
}
OutlivesBound::RegionSubRegion(r_a, r_b) => {
if let (ReEarlyBound(_) | ReFree(_), ReVar(vid_b)) = (r_a.kind(), r_b.kind()) {
infcx
Expand Down
139 changes: 99 additions & 40 deletions compiler/rustc_infer/src/infer/outlives/obligations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,11 @@ use crate::infer::{
};
use crate::traits::{ObligationCause, ObligationCauseCode};
use rustc_data_structures::undo_log::UndoLogs;
use rustc_hir::def_id::DefId;
use rustc_hir::def_id::LocalDefId;
use rustc_middle::mir::ConstraintCategory;
use rustc_middle::ty::subst::GenericArgKind;
use rustc_middle::ty::{self, Region, Ty, TyCtxt, TypeVisitable};
use rustc_middle::ty::{self, Region, SubstsRef, Ty, TyCtxt, TypeVisitable};
use smallvec::smallvec;

impl<'cx, 'tcx> InferCtxt<'cx, 'tcx> {
Expand Down Expand Up @@ -283,6 +284,9 @@ where
Component::Param(param_ty) => {
self.param_ty_must_outlive(origin, region, *param_ty);
}
Component::Opaque(def_id, substs) => {
self.opaque_must_outlive(*def_id, substs, origin, region)
}
Component::Projection(projection_ty) => {
self.projection_must_outlive(origin, region, *projection_ty);
}
Expand Down Expand Up @@ -314,17 +318,66 @@ where
);

let generic = GenericKind::Param(param_ty);
let verify_bound = self.verify_bound.generic_bound(generic);
let verify_bound = self.verify_bound.param_bound(param_ty);
self.delegate.push_verify(origin, generic, region, verify_bound);
}

#[instrument(level = "debug", skip(self))]
fn opaque_must_outlive(
&mut self,
def_id: DefId,
substs: SubstsRef<'tcx>,
origin: infer::SubregionOrigin<'tcx>,
region: ty::Region<'tcx>,
) {
self.generic_must_outlive(
origin,
region,
GenericKind::Opaque(def_id, substs),
def_id,
substs,
|ty| match *ty.kind() {
ty::Opaque(def_id, substs) => (def_id, substs),
_ => bug!("expected only projection types from env, not {:?}", ty),
},
);
}

#[instrument(level = "debug", skip(self))]
fn projection_must_outlive(
&mut self,
origin: infer::SubregionOrigin<'tcx>,
region: ty::Region<'tcx>,
projection_ty: ty::ProjectionTy<'tcx>,
) {
self.generic_must_outlive(
origin,
region,
GenericKind::Projection(projection_ty),
projection_ty.item_def_id,
projection_ty.substs,
|ty| match ty.kind() {
ty::Projection(projection_ty) => (projection_ty.item_def_id, projection_ty.substs),
_ => bug!("expected only projection types from env, not {:?}", ty),
},
);
}

#[instrument(level = "debug", skip(self, filter))]
fn generic_must_outlive(
&mut self,
origin: infer::SubregionOrigin<'tcx>,
region: ty::Region<'tcx>,
generic: GenericKind<'tcx>,
def_id: DefId,
substs: SubstsRef<'tcx>,
filter: impl Fn(Ty<'tcx>) -> (DefId, SubstsRef<'tcx>),
) {
Comment thread
oli-obk marked this conversation as resolved.
Outdated
// An optimization for a common case with opaque types.
if substs.is_empty() {
return;
}

// This case is thorny for inference. The fundamental problem is
// that there are many cases where we have choice, and inference
// doesn't like choice (the current region inference in
Expand All @@ -343,16 +396,15 @@ where
// These are guaranteed to apply, no matter the inference
// results.
let trait_bounds: Vec<_> =
self.verify_bound.projection_declared_bounds_from_trait(projection_ty).collect();
self.verify_bound.declared_region_bounds(def_id, substs).collect();

debug!(?trait_bounds);

// Compute the bounds we can derive from the environment. This
// is an "approximate" match -- in some cases, these bounds
// may not apply.
let mut approx_env_bounds =
self.verify_bound.projection_approx_declared_bounds_from_env(projection_ty);
debug!("projection_must_outlive: approx_env_bounds={:?}", approx_env_bounds);
let mut approx_env_bounds = self.verify_bound.approx_declared_bounds_from_env(generic);
debug!(?approx_env_bounds);

// Remove outlives bounds that we get from the environment but
// which are also deducible from the trait. This arises (cc
Expand All @@ -366,14 +418,8 @@ where
// If the declaration is `trait Trait<'b> { type Item: 'b; }`, then `projection_declared_bounds_from_trait`
// will be invoked with `['b => ^1]` and so we will get `^1` returned.
let bound = bound_outlives.skip_binder();
match *bound.0.kind() {
ty::Projection(projection_ty) => self
.verify_bound
.projection_declared_bounds_from_trait(projection_ty)
.all(|r| r != bound.1),

_ => panic!("expected only projection types from env, not {:?}", bound.0),
}
let (def_id, substs) = filter(bound.0);
self.verify_bound.declared_region_bounds(def_id, substs).all(|r| r != bound.1)
});

// If declared bounds list is empty, the only applicable rule is
Expand All @@ -390,29 +436,11 @@ where
// the problem is to add `T: 'r`, which isn't true. So, if there are no
// inference variables, we use a verify constraint instead of adding
// edges, which winds up enforcing the same condition.
let needs_infer = projection_ty.needs_infer();
let needs_infer = substs.needs_infer();
if approx_env_bounds.is_empty() && trait_bounds.is_empty() && needs_infer {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The rationale for && need_infer in the previous comment doesn't seem to apply to RPITs, because the type cannot be named anyway, so the user will eventually need to specify the bounds on the components rater than on the opaque type.

This indeed causes diagnostics regression in this case:

fn id<T>(_: T) -> impl Sized {}
fn test<T>(t: T) -> impl Sized + 'static {
    id(t)
    //^ ERROR the opaque type `id<T>::{opaque#0}` may not live long enough
}

But removing needs_infer condition for RPITs may not be a good solution either since it would prevent us from adding useful suggestions like adding explicit lifetime bounds to the opaque type (I guess!). Maybe we can improve diagnostics by some other way, at least by removing the cryptic id<T>::{opaque#0}.

This is problem for TAIT as well because it shows the same cryptic error message, but I think here we want to suggest the bound OpaqueTy<T>: 'static rather than T: 'static.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that the problem is a diagnostic regression, can't this be handled on the other side, when we emit the diagnostic? Teaching the machinery that suggests constraining where clauses locally to also look at functions that return impl Trait's should be possible, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that TAITs will be nameable, so, we do need to keep this path

Copy link
Copy Markdown
Contributor

@aliemjay aliemjay Sep 23, 2022

Choose a reason for hiding this comment

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

I'd leave it as it is for TAIT as well. I prefer suggesting T: 'static than OpaqueTy<T>: 'static. It's true that the later is more general but they're not really different unless T appears unconstrained in the hidden type, which should be a rare use case.

IMO, it's a cursed hack even for projection types. This is a diagnostic issue and should not be dealt with here. The diagnostics machinery should be smarter to suggest the two possible solutions.

debug!("projection_must_outlive: no declared bounds");

let constraint = origin.to_constraint_category();
for k in projection_ty.substs {
match k.unpack() {
GenericArgKind::Lifetime(lt) => {
self.delegate.push_sub_region_constraint(
origin.clone(),
region,
lt,
constraint,
);
}
GenericArgKind::Type(ty) => {
self.type_must_outlive(origin.clone(), ty, region, constraint);
}
GenericArgKind::Const(_) => {
// Const parameters don't impose constraints.
}
}
}
debug!("no declared bounds");

self.substs_must_outlive(substs, origin, region);

return;
}
Expand Down Expand Up @@ -442,8 +470,8 @@ where
.all(|b| b == Some(trait_bounds[0]))
{
let unique_bound = trait_bounds[0];
debug!("projection_must_outlive: unique trait bound = {:?}", unique_bound);
debug!("projection_must_outlive: unique declared bound appears in trait ref");
debug!(?unique_bound);
debug!("unique declared bound appears in trait ref");
let category = origin.to_constraint_category();
self.delegate.push_sub_region_constraint(origin, region, unique_bound, category);
return;
Expand All @@ -454,11 +482,42 @@ where
// projection outlive; in some cases, this may add insufficient
// edges into the inference graph, leading to inference failures
// even though a satisfactory solution exists.
let generic = GenericKind::Projection(projection_ty);
let verify_bound = self.verify_bound.generic_bound(generic);
let verify_bound = self.verify_bound.projection_opaque_bounds(
generic,
def_id,
substs,
&mut Default::default(),
);
debug!("projection_must_outlive: pushing {:?}", verify_bound);
self.delegate.push_verify(origin, generic, region, verify_bound);
}

fn substs_must_outlive(
&mut self,
substs: SubstsRef<'tcx>,
origin: infer::SubregionOrigin<'tcx>,
region: ty::Region<'tcx>,
) {
let constraint = origin.to_constraint_category();
for k in substs {
match k.unpack() {
GenericArgKind::Lifetime(lt) => {
self.delegate.push_sub_region_constraint(
origin.clone(),
region,
lt,
constraint,
);
}
GenericArgKind::Type(ty) => {
self.type_must_outlive(origin.clone(), ty, region, constraint);
}
GenericArgKind::Const(_) => {
// Const parameters don't impose constraints.
}
}
}
}
}

impl<'cx, 'tcx> TypeOutlivesDelegate<'tcx> for &'cx InferCtxt<'cx, 'tcx> {
Expand Down
Loading