Do not check outlives when explicit bound contains escaping regions

[Ekleog]

This makes the code similar to the handling of ty::Ref in
src/librustc/ty/wf.rs, except checking for non-escaping-regions
somewhere down the call chain instead of at the root.

Fixes #53548

Note: I have literally 0 confidence that this fix is actually correct, as I don't really understand all the surrounding code. However, it appears to break no test, and to pass the added test, equivalent to the one in #53548. Maybe the root cause of the issue is somewhere else, though.

Hope this helps! :)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[petrochenkov]

r? @nikomatsakis

[TimNN]

Ping from triage @nikomatsakis / @rust-lang/compiler: This PR requires your review.

[Ekleog]

@nikomatsakis If you feel that there should be a debug_assert!(!region.has_escaping_regions()) instead of this current && !region.has_escaping_regions(), I can try to investigate more into how the region argument actually ends up having escaping regions.

I'm currently assuming the change is likely not-too-bad because it's similar to the handling of ty::Ref, but I'm not the one who actually wrote the surrounding function :)

[nikomatsakis]

@Ekleog sorry I've not gotten back to you here :( let me add this to my to-do list. I took a look a few times but it's hard to remember the full context here.

[TimNN]

Ping from triage @nikomatsakis / @rust-lang/compiler: This PR requires your review.

[bors]

☔ The latest upstream changes (presumably #55330) made this pull request unmergeable. Please resolve the merge conflicts.

[pnkfelix]

r? @pnkfelix

[pnkfelix]

Wow I/we really failed to address this fast enough:

• The core of this PR uses a method that ... no longer exists?
• Also the Pin API changed a bit since this regression test was written.

Still the bug persists, as demonstrated on this playpen.

[pnkfelix]

(ah the current code calls a data.has_escaping_bound_vars; that may well work for the region as well...)

[pnkfelix]

Also here is a more reduced test case (play) which still needs generators (and yield) but does not use the await! macro nor the Pin trait stuff:

#![feature(async_await, futures_api, generators)]

// A trait with 'static bound
trait Trait: 'static {}

// The actual reproducer, requires that Trait is 'static and a trait
// object to it is captured in a closure for successful reproduction.
async fn foo(b: Box<Trait + 'static>) -> () {
    let _bar = move || { b; () };
    yield
}

pub fn main() {}

[pnkfelix]

Also, the example I just gave will actually ICE a debug-build of rustc, because of this debug_assert!:

rust/src/librustc/ty/sty.rs

Lines 788 to 792 in 0195812

	pub fn dummy<'tcx>(value: T) -> Binder<T>
	where T: TypeFoldable<'tcx>
	{
	debug_assert!(!value.has_escaping_bound_vars());
	Binder(value)

This lends credence to the idea that the proposed check, or something like it, may belong in:

rust/src/librustc/ty/wf.rs

Lines 493 to 503 in 0195812

	if !data.has_escaping_bound_vars() {
	let implicit_bounds =
	object_region_bounds(self.infcx.tcx, data);
	let explicit_bound = region;
	self.out.reserve(implicit_bounds.len());
	for implicit_bound in implicit_bounds {
	let cause = self.cause(traits::ObjectTypeBound(ty, explicit_bound));
	let outlives = ty::Binder::dummy(
	ty::OutlivesPredicate(explicit_bound, implicit_bound));

But I want to review RFC 1214 and the comment above this code carefully before approving/implementing such a change.

The comment in particular says that the region bound in a trait object is a so-called "master bound" that implies that bounds from other traits are all met. So ... it seems plausible to sidestep pushing new obligations in this case ...

but on the other hand, the interaction with generators troubles me. There may be some constraint imposed by generators that I am overlooking in my naive review of the stack trace and debug log.

[pnkfelix]

If I understand the debug logs correctly, the place we are ICE'ing on debug builds is when we attempt to evaluate a predicate as follows:

rustc::traits::util: super_predicates: data=Binder(TraitPredicate(<FreshTy(0) as Trait>)) predicates=[Binder(OutlivesPredicate(FreshTy(0), ReStatic))]

where FreshTy(0), IIUC, is a (the?) witness for the generator.

And then we are ICE'ing because that witness holds escaping bound variables.

Is that a sign of a bug elsewhere? Should the witness have escaping bound variables in this scenario?

[pnkfelix]

For example, another potential route for "fixing" the bug here might be to change this code here:

rust/src/librustc_typeck/check/generator_interior.rs

Lines 121 to 129 in 0195812

	// Replace all regions inside the generator interior with late bound regions
	// Note that each region slot in the types gets a new fresh late bound region,
	// which means that none of the regions inside relate to any other, even if
	// typeck had previously found constraints that would cause them to be related.
	let mut counter = 0;
	let type_list = fcx.tcx.fold_regions(&type_list, &mut false, |_, current_depth| {
	counter += 1;
	fcx.tcx.mk_region(ty::ReLateBound(current_depth, ty::BrAnon(counter)))
	});

so that, as a special case, it does not replace ReStatic with an ReLateBound region, but instead just leaves ReStatic as ReStatic...

(i don't understand what the generator_interior code is doing though, so its possible that replacing with ReLateBound remains the correct thing...)

---

Update: I didn't read the debug log carefully enough. The suggestion in this comment on its own would not resolve this issue, because we have already lost the ReStatic in the type of the generator itself before we start the code that maps the regions to ReLateBound...

[pnkfelix]

The code that does the ReLateBound replacement comes from PR #45337; i.e., it has been there since the beginning of immovable generators; see e.g. discussion here #45337 (review) and here #45337 (comment)

I'm still not clear on whether it needs to do its replacement so aggressively. Ignoring ReStatic during the replacement seems reasonable to me. (It may even be reasonable to also ignore ReEarlyBound regions too...)

[pnkfelix]

Potentially useful info: even this suffices to reproduce the ICE in the debug build of rustc:

#![feature(async_await, futures_api, generators)]

// A trait with 'static bound
trait MyTrait: 'static {}

async fn foo(b: Box<MyTrait + 'static>) -> () {
    let _bar = b;
    yield
}

pub fn main() {}

[pnkfelix]

Wait a minute, what is this:

rust/src/librustc_typeck/astconv.rs

Lines 102 to 105 in 0195812

	/// Dummy type used for the `Self` of a `TraitRef` created for converting
	/// a trait object, and which gets removed in `ExistentialTraitRef`.
	/// This type must not appear anywhere in other converted types.
	const TRAIT_OBJECT_DUMMY_SELF: ty::TyKind<'static> = ty::Infer(ty::FreshTy(0));

That is: should I not even be seeing FreshTy(0) in the structure of the generator interior type here?

---

Update: ARGH well these combinations of hacks are actively impeding the utility of debug logs:

rust/src/librustc/util/ppaux.rs

Lines 609 to 614 in 0195812

	('tcx) &'tcx ty::List<ty::ExistentialPredicate<'tcx>>, (self, f, cx) {
	display {
	// Generate the main trait ref, including associated types.
	ty::tls::with(|tcx| {
	// Use a type that can't appear in defaults of type parameters.
	let dummy_self = tcx.mk_infer(ty::FreshTy(0));

---

Update 2: Okay, one quick hack later to use unique numbers at each FreshTy, and I have now determined that the FreshTy in question is arising from:

rust/src/librustc/ty/wf.rs

Lines 523 to 526 in 0195812

	// Since we don't actually *know* the self type for an object,
	// this "open(err)" serves as a kind of dummy standin -- basically
	// a placeholder type.
	let open_ty = tcx.mk_infer(ty::FreshTy(0));

which is entirely unsurprising in hindsight, but good to confirm.

[XAMPPRocky]

Triage; @Ekleog Hello, have you been able to get back to this PR?

[Ekleog]

@Aaronepower Not yet, sorry. I've had to move a bit to other things, will try to come back to it when I find some time… but likely not in the month to come, even though I was more hopeful when reading @pnkfelix's comments :/

If someone wants to take this over, by all means feel free! Otherwise I'll try to do it hopefully some time next year.

[XAMPPRocky]

@Ekleog Okay I'm going to close this PR. Feel free to reopen it once you are able to!

Anyone who wants to continue should open a new PR referencing this one.