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

fix multiple issues when promoting type-test subject #108691

Merged
merged 6 commits into from
Mar 7, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
35 changes: 14 additions & 21 deletions compiler/rustc_borrowck/src/region_infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ use rustc_infer::infer::outlives::test_type_match;
use rustc_infer::infer::region_constraints::{GenericKind, VarInfos, VerifyBound, VerifyIfEq};
use rustc_infer::infer::{InferCtxt, NllRegionVariableOrigin, RegionVariableOrigin};
use rustc_middle::mir::{
Body, ClosureOutlivesRequirement, ClosureOutlivesSubject, ClosureRegionRequirements,
ConstraintCategory, Local, Location, ReturnConstraint, TerminatorKind,
Body, ClosureOutlivesRequirement, ClosureOutlivesSubject, ClosureOutlivesSubjectTy,
ClosureRegionRequirements, ConstraintCategory, Local, Location, ReturnConstraint,
TerminatorKind,
};
use rustc_middle::traits::ObligationCause;
use rustc_middle::traits::ObligationCauseCode;
Expand Down Expand Up @@ -1084,18 +1085,10 @@ impl<'tcx> RegionInferenceContext<'tcx> {
true
}

/// When we promote a type test `T: 'r`, we have to convert the
/// type `T` into something we can store in a query result (so
/// something allocated for `'tcx`). This is problematic if `ty`
/// contains regions. During the course of NLL region checking, we
/// will have replaced all of those regions with fresh inference
/// variables. To create a test subject, we want to replace those
/// inference variables with some region from the closure
/// signature -- this is not always possible, so this is a
/// fallible process. Presuming we do find a suitable region, we
/// will use it's *external name*, which will be a `RegionKind`
/// variant that can be used in query responses such as
/// `ReEarlyBound`.
/// When we promote a type test `T: 'r`, we have to replace all region
/// variables in the type `T` with an equal universal region from the
/// closure signature.
/// This is not always possible, so this is a fallible process.
#[instrument(level = "debug", skip(self, infcx))]
fn try_promote_type_test_subject(
&self,
Expand Down Expand Up @@ -1144,22 +1137,22 @@ impl<'tcx> RegionInferenceContext<'tcx> {
// find an equivalent.
let upper_bound = self.non_local_universal_upper_bound(region_vid);
if self.region_contains(region_vid, upper_bound) {
self.definitions[upper_bound].external_name.unwrap_or(r)
tcx.mk_re_var(upper_bound)
} else {
// In the case of a failure, use a `ReVar` result. This will
// cause the `needs_infer` later on to return `None`.
r
// In the case of a failure, use `ReErased`. We will eventually
// return `None` in this case.
tcx.lifetimes.re_erased
}
});

debug!("try_promote_type_test_subject: folded ty = {:?}", ty);

// `needs_infer` will only be true if we failed to promote some region.
if ty.needs_infer() {
// This will be true if we failed to promote some region.
if ty.has_erased_regions() {
return None;
}

Some(ClosureOutlivesSubject::Ty(ty))
Some(ClosureOutlivesSubject::Ty(ClosureOutlivesSubjectTy::new(tcx, ty)))
}

/// Given some universal or existential region `r`, finds a
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,9 @@ impl<'a, 'tcx> ConstraintConversion<'a, 'tcx> {
let outlived_region = closure_mapping[outlives_requirement.outlived_free_region];
let subject = match outlives_requirement.subject {
ClosureOutlivesSubject::Region(re) => closure_mapping[re].into(),
ClosureOutlivesSubject::Ty(ty) => ty.into(),
ClosureOutlivesSubject::Ty(subject_ty) => {
subject_ty.instantiate(self.tcx, |vid| closure_mapping[vid]).into()
}
};

self.category = outlives_requirement.category;
Expand Down
57 changes: 45 additions & 12 deletions compiler/rustc_middle/src/mir/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_errors::ErrorGuaranteed;
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_index::bit_set::BitMatrix;
use rustc_index::vec::IndexVec;
use rustc_index::vec::{Idx, IndexVec};
use rustc_span::Span;
use rustc_target::abi::VariantIdx;
use smallvec::SmallVec;
Expand Down Expand Up @@ -289,13 +289,6 @@ pub struct ConstQualifs {
/// instance of the closure is created, the corresponding free regions
/// can be extracted from its type and constrained to have the given
/// outlives relationship.
///
/// In some cases, we have to record outlives requirements between types and
/// regions as well. In that case, if those types include any regions, those
/// regions are recorded using their external names (`ReStatic`,
/// `ReEarlyBound`, `ReFree`). We use these because in a query response we
/// cannot use `ReVar` (which is what we use internally within the rest of the
/// NLL code).
#[derive(Clone, Debug, TyEncodable, TyDecodable, HashStable)]
pub struct ClosureRegionRequirements<'tcx> {
/// The number of external regions defined on the closure. In our
Expand Down Expand Up @@ -392,16 +385,56 @@ pub enum ClosureOutlivesSubject<'tcx> {
/// Subject is a type, typically a type parameter, but could also
/// be a projection. Indicates a requirement like `T: 'a` being
/// passed to the caller, where the type here is `T`.
///
/// The type here is guaranteed not to contain any free regions at
/// present.
Ty(Ty<'tcx>),
Ty(ClosureOutlivesSubjectTy<'tcx>),

/// Subject is a free region from the closure. Indicates a requirement
/// like `'a: 'b` being passed to the caller; the region here is `'a`.
Region(ty::RegionVid),
}

/// Represents a `ty::Ty` for use in [`ClosureOutlivesSubject`].
///
/// This indirection is necessary because the type may include `ReVar` regions,
/// which is what we use internally within NLL code,
/// and we can't use `ReVar`s in a query response.
#[derive(Copy, Clone, Debug, TyEncodable, TyDecodable, HashStable)]
pub struct ClosureOutlivesSubjectTy<'tcx> {
inner: Ty<'tcx>,
}

impl<'tcx> ClosureOutlivesSubjectTy<'tcx> {
// All regions of `ty` must be of kind `ReVar`
// and must point to an early-bound region in the closure's signature.
pub fn new(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Self {
let inner = tcx.fold_regions(ty, |r, depth| match r.kind() {
ty::ReVar(vid) => {
let br = ty::BoundRegion {
var: ty::BoundVar::new(vid.index()),
kind: ty::BrAnon(0u32, None),
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't matter too much, but this should be vid.index() instead of 0, shouldn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I changed it in 97381d2.

};
tcx.mk_re_late_bound(depth, br)
}
_ => bug!("unexpected region in ClosureOutlivesSubjectTy: {r:?}"),
});

Self { inner }
}

pub fn instantiate(
self,
tcx: TyCtxt<'tcx>,
mut map: impl FnMut(ty::RegionVid) -> ty::Region<'tcx>,
) -> Ty<'tcx> {
tcx.fold_regions(self.inner, |r, depth| match r.kind() {
ty::ReLateBound(debruijn, br) => {
debug_assert_eq!(debruijn, depth);
map(ty::RegionVid::new(br.var.index()))
}
_ => bug!("unexpected region {r:?}"),
})
}
}

Copy link
Member

Choose a reason for hiding this comment

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

I don't really like this hack. Trying to think of a better alternative though.

I think it would be better to "formalize" this in some sense by having some way to have explicit "binders" of the captured regions, Or expanding on the concept of external_name a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it a hard-and-fast rule that ty::Binder is the only binder for ReLateBound regions? I don't think so -- we already have Canonical<T> which serves as a binder too. The only limitation is that the type can't implement TypeFoldable/Visitable because these only support ty::Binder.

I tweaked it a bit in a later commit to make it more clear.

Copy link
Member

Choose a reason for hiding this comment

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

I think the concern for me is less about Binder, and more so that we aren't being explicit about the "bound variables" contained within (and what they mean to those that receive the "canonical" form).

What seems better to me is some more explicit "closure nameable" set of "bound vars" that this has access to. When borrow checking the context around the closure, the mapping between the "real" variables and the ones available to the closure is clear (maybe even trivial), but even if not in that context, it's still "just a set of bound vars".

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. This binder needs to store more context, but this is preexisting problem - the parent struct ClosureOutlivesRequirements is already littered by RegionVids with no context. Actually this is exactly the reason I haven't made this binder type more generic like RegionVarBinder<T> and made it explicit in the docs that it should be used in the context of ClosureOutlivesRequirements. The RegionVids there are already used to index into the early-bound regions of the closure type.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, definitely a preexisting problem that's just more pointy now. I think we can land, but we should chat about a followup for this (and if it's something you want to pursue or something we should offload to someone else).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to work on a followup PR. Let me open an issue to track this..

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to work on a followup PR. Let me open an issue to track this..

did this happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops I missed that. I've opened #111310.

/// The constituent parts of a mir constant of kind ADT or array.
#[derive(Copy, Clone, Debug, HashStable)]
pub struct DestructuredConstant<'tcx> {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// chek-fail
// known-bug: #108639

trait Trait {
type Item<'a>: 'a;
}

fn assert_static<T: 'static>(_: T) {}
fn relate<T>(_: T, _: T) {}

fn test_args<I: Trait>() {
let closure = |a, b| {
relate(&a, b);
assert_static(a);
};
closure(None::<I::Item<'_>>, &None::<I::Item<'_>>);
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0310]: the associated type `<I as Trait>::Item<'_>` may not live long enough
--> $DIR/type-test-subject-non-trivial-region.rs:14:9
|
LL | assert_static(a);
| ^^^^^^^^^^^^^^^^
|
= help: consider adding an explicit lifetime bound `<I as Trait>::Item<'_>: 'static`...
= note: ...so that the type `<I as Trait>::Item<'_>` will meet its required lifetime bounds

error: aborting due to previous error

For more information about this error, try `rustc --explain E0310`.
18 changes: 18 additions & 0 deletions tests/ui/nll/closure-requirements/type-test-subject-opaque-1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Regression test for #107426.
// check-pass

use std::marker::PhantomData;
#[derive(Clone, Copy)]
pub struct Scope<'a>(&'a PhantomData<&'a mut &'a ()>);
fn event<'a, F: FnMut() + 'a>(_: Scope<'a>, _: F) {}
fn make_fn<'a>(_: Scope<'a>) -> impl Fn() + Copy + 'a {
|| {}
}

fn foo(cx: Scope) {
let open_toggle = make_fn(cx);

|| event(cx, open_toggle);
}

fn main() {}
17 changes: 17 additions & 0 deletions tests/ui/nll/closure-requirements/type-test-subject-opaque-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// check-fail
// known-bug: #107516

fn iter1<'a: 'a>() -> impl Iterator<Item = &'static str> {
None.into_iter()
}

fn iter2<'a>() -> impl Iterator<Item = &'a str> {
None.into_iter()
}

struct Bivar<'a, I: Iterator<Item = &'a str> + 'a>(I);

fn main() {
let _ = || Bivar(iter1());
let _ = || Bivar(iter2());
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
error[E0310]: the opaque type `iter1<'_>::{opaque#0}` may not live long enough
--> $DIR/type-test-subject-opaque-2.rs:15:16
|
LL | let _ = || Bivar(iter1());
| ^^^^^^^^^^^^^^
|
= help: consider adding an explicit lifetime bound `iter1<'_>::{opaque#0}: 'static`...
= note: ...so that the type `impl Iterator<Item = &'static str>` will meet its required lifetime bounds

error: `iter2<'_>::{opaque#0}<'_>` does not live long enough
--> $DIR/type-test-subject-opaque-2.rs:16:16
|
LL | let _ = || Bivar(iter2());
| ^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0310`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// See #108635 for description.
// check-pass

trait Trait {
type Item<'a>: 'a;
}

fn assert_static<T: 'static>(_: T) {}

fn test_args<I: Trait>() {
let closure = |a, _b| assert_static(a);

closure(None::<I::Item<'_>>, &None::<I::Item<'_>>);
}

fn test_upvars<I: Trait>() {
let upvars = (None::<I::Item<'_>>, &None::<I::Item<'_>>);
let _closure = || {
let (a, _b) = upvars;
assert_static(a);
};
}

fn main() {}