Consider alias bounds when computing liveness in NLL (and opaque captures)

[compiler-errors]

Background

Right now, liveness analysis in NLL is a bit simplistic. It simply walks through all of the regions of a type and marks them as being live at points. This is problematic in the case of aliases, since it requires that we mark all of the regions in their args¹ as live, leading to bugs like #42940.

In reality, we may be able to deduce that fewer regions are allowed to be present in the projected type (or "hidden type" for opaques) via item bounds or where clauses, and therefore ideally, we should be able to soundly require fewer regions to be live in the alias.

For example:

trait Captures<'a> {}
impl<T> Captures<'_> for T {}

fn capture<'o>(_: &'o mut ()) -> impl Sized + Captures<'o> + 'static {}

fn test_two_mut(mut x: ()) {
    let _f1 = capture(&mut x);
    let _f2 = capture(&mut x);
    //~^ ERROR cannot borrow `x` as mutable more than once at a time
}

In the example above, we should be able to deduce from the 'static bound on capture's opaque that even though 'o is a captured region, it can never show up in the opaque's hidden type, and can soundly be ignored for liveness purposes.

The Fix

We apply a simple version of RFC 1214's OutlivesProjectionEnv and OutlivesProjectionTraitDef rules to NLL's make_all_regions_live computation.

Specifically, when we encounter an alias type, we:

1. Look for a unique outlives bound in the param-env or item bounds for that alias. If there is more than one unique region, bail, unless any of the outlives bound's regions is 'static, and in that case, prefer 'static. If we find such a unique region, we can mark that outlives region as live and skip walking through the args of the opaque.
2. Otherwise, walk through the alias's args recursively, as we do today.

Additionally: Opaque Hidden Types

A similar bug comes about when walking through the hidden type of an opaque to validate it doesn't capture lifetimes that aren't part of that opaque. For example:

trait Captures<'a> {}
impl<T> Captures<'_> for T {}

fn a() -> impl Sized + 'static {
    b(&vec![])
}

fn b<'o>(_: &'o Vec<i32>) -> impl Sized + Captures<'o> + 'static {}

The hidden type of a::{opaque} is inferred to be b::<'?1>::{opaque}, where '?1 is a region local to the body of a. However, the 'static bound on b's opaque should allow us to deduce that the hidden type will never mention that region '?1, and we can ignore it (and replace it with ReErased in b's opaque's hidden type). Similarly to liveness, we don't currently do anything other than recurse through aliases for their lifetime components.

This PR ends up using the same region visitor as liveness for opaque type region captures.

Limitations

This approach has some limitations. Specifically, since liveness doesn't use the same type-test logic as outlives bounds do, we can't really try several options when we're faced with a choice.

If we encounter two unique outlives regions in the param-env or bounds, we simply fall back to walking the opaque via its args. I expect this to be mostly mitigated by the special treatment of 'static, and can be fixed in a forwards-compatible by a more sophisticated analysis in the future.

Read more

Context: #42940 (comment)
More context: #115822 (comment)

Fixes #42940

Footnotes

1. except for bivariant region args in opaques, which will become less relevant when we move onto edition 2024 capture semantics for opaques. ↩

[rustbot]

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

[nikomatsakis]

I believe this is correct -- I do have one question (behavior with elaborated bounds).

[nikomatsakis]

What happens with impl for<'a> 'a ? I guess we probably don't accept that. Related question, does this take into account elaborated bounds? (In that case, you could have impl for<'a> Trait<'a> where trait Trait<'a>: 'a, for example.)

[compiler-errors]

Related question, does this take into account elaborated bounds? (In that case, you could have impl for<'a> Trait<'a> where trait Trait<'a>: 'a, for example.)

Yes, we elaborate bounds. It acts like a for<'a> T: 'a bound, so it'll be treated like T: 'static due to this code:

rust/compiler/rustc_infer/src/infer/outlives/test_type_match.rs

Lines 51 to 60 in 8e47113

	if let ty::RegionKind::ReLateBound(depth, br) = verify_if_eq.bound.kind() {
	assert!(depth == ty::INNERMOST);
	match m.map.get(&br) {
	Some(&r) => Some(r),
	None => {
	// If there is no mapping, then this region is unconstrained.
	// In that case, we escalate to `'static`.
	Some(tcx.lifetimes.re_static)
	}
	}

What happens with impl for<'a> 'a ? I guess we probably don't accept that.

We don't accept that in the grammar, but yeah, you can get it implied like above.

[nikomatsakis]

in general we could pick the "maximum" one, but that's ok, static is a good choice

[compiler-errors]

Yeah, I agree, but we have no way (afaict) at this point to query what the maximum one is.

[danielhenrymantilla]

Tangentially, could we start linting on multiple + 'lt bounds on impl Trait-aliases? It's always a mistake, one which beginners may make, or even people mislead by the rustc diagnostic which (in the single lifetime case) suggests + 'lt instead of + Captures<'lt>:

warning: multiple lifetime bounds
 --> src/lib.rs:5:41
  |
5 | fn foo<'a, 'b>(it: (&'a (), &'b ())) -> impl 'a + 'b + Trait {
  |                                              ^^^^^^^
help: introduce a new lifetime `'c` and use it in the `impl` type
5 | fn foo<'a, 'b, 'c>(it: (&'a (), &'b ())) -> impl 'c + Trait {
                 ++++                                ++
note: you will then need to bound `'c` w.r.t. `'a` and `'b` accordingly

[compiler-errors]

@danielhenrymantilla: We could lint such a case, which would help for the case of several explicit outlives choices. Consider outlives that are deduced from supertrait bounds tho:

trait Outlives<'a>: 'a {}

and some type like impl Outlives<'a> + 'b. We currently have to choose between 'a and 'b here too. I wouldn't want to make such a lint trigger here because it's possible that the user doesn't even have a choice over the outlives bounds that are a consequence of elaborating Outlives.

---

I'd rather this code just be smart and understand how to detect a maximal region, but leave that for future work.

[nikomatsakis]

@compiler-errors we do have a way to query for the maximum, or we did at one point, but we can discuss it separately.

[compiler-errors]

@rfcbot fcp merge

see description for motivation and approach

[rfcbot]

Team member @compiler-errors has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @aliemjay
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

Concerns:

• unsound-lifetime-erasure-of-hidden-types (Consider alias bounds when computing liveness in NLL (and opaque captures) #116040 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@rfcbot reviewed

[aliemjay]

@bors try

[bors]

⌛ Trying commit 1b88ace with merge b86657a...

[bors]

☀️ Try build successful - checks-actions
Build commit: b86657a (b86657a6fca2ea2300726976d0dc67ae11722a88)

[aliemjay]

1. Only enable these capture semantics when we capture all in-scope lifetimes, presumably under the same feature gate as Lifetime Capture Rules 2024 rfcs#3498 and for RPITITs (which have these always-capture semantics by default).

rust-lang/rfcs#3498 is not a solution. You can always reproduce it with TAIT:

// ...
type Opaque<'a> = impl Sized + 'a;
fn step2<'a, 'b: 'a>() -> Opaque<'a> {
    step1::<'a, 'b>() // hidden type: MyTy<'a, 'erased>
}
// ...

[compiler-errors]

Well I guess that TAIT example precludes (2.) as well. Without the notion of an existential region or some other way of making ReErased fail its region obligations rather than silently swallow them, I guess there's no way to make this work out for opaque captures. That fucking sucks, I guess.

[nikomatsakis]

Hold up. I'm a bit confused about what's going on here. Maybe I didn't read the PR closely enough! Are we introducing erased regions here where they didn't exist before?

[nikomatsakis]

Specifically, this step is confusing to me:

fn step2<'a, 'b: 'a>() -> impl Sized + 'a {
    step1::<'a, 'b>() // hidden type: MyTy<'a, 'erased>
}

Here, I would expect the hidden type to actually be something like OpaqueFromStep1<'a, 'b>, and then I would expect an error because 'b is not an allowed type parameter (and OpaqueFromStep1 is invariant with respect to 'b).

If we were to reveal the hidden type, I still would expect it to be MyTy<'a, 'b>; which would be an error, or else be converted (through variance) to MyTy<'a, 'a>.

In other words, I don't understand how and why 'erased is getting introduced into hidden types ever -- what am I missing?

[nikomatsakis]

I talked to @compiler-errors a bit offline and we agreed that

• the scheme I describe above, where we do not erase regions from the type but rather error if disallowed regions are included, would be sound, but it would also lead to some examples that we wanted to accept not being accepted
• the fix for that would be existentials (cc Tracking issue for existential lifetimes #60670), as @compiler-errors suggested -- interesting question is how they can be implemented and thought about (I feel happier having a-mir-formality to use for that)

I'm still a bit unclear on just what portion of the erasure here is new to this PR, I'll have to review the code, or maybe it's an interaction with existing code?

[nikomatsakis]

In reviewing #112905, we realized a potential concern here. I'll quote from the big comment I just left on that PR:

As a side note, we dug into the "dyn-transmute" mechanism here, which is an important interaction I was not aware of. The key point seems to be that if you have "semantic 'static bounds", meaning that T<'a, 'b>: 'static if it has no reachable references (even though T<'a, 'b> may still have free regions), then typeid+dyn allows you to change 'a and 'b arbitrarily. This is unsound here because the closure's body required a relationship between those lifetimes that was being lost. In general, I would like to adopt semantic 'static bounds (and e.g. #116040 is an instance of that), so this is concerning -- however, it may be that semantic 'static bounds are still sound so long as we don't lose the where-clauses like this. I'm going to leave a separate comment on that PR though to call more attention to this interaction.

I was aware before the relationship to semantic 'static bounds, but I hadn't realized the interaction with dyn any, which makes it particularly salient. At the moment, I cannot see how this causes unsoundness if all where-clauses etc are properly tracked, but @aliemjay you seem to be more creative than I.

(To be clear, I am assuming here that we fix this weird loophole around 'erased, which effectively can cause where-clauses to be lost in another way.)

[nikomatsakis]

Let me write out another concern. This is one that I was already thinking about, but I'd like to write it out and call attention to it more explicitly, maybe my reasoning is flawed. RFC #1214 was the point where we moved to syntactic outlives instead of semantic outlives, and the motivation there was to ensure that the outlives rule which says Alias<T0...Tn>: 'x if T0...Tn: x. The concern was that impls like this would be unsound without a more syntactic rule...

impl<'a> MyTrait for fn(&'a u32) {
    type Elem = &'a u32;
}

...because we could conclude that <fn(&'a u32) as MyTrait>::Elem: 'static even though it is &'a u32. (Similar impl<T> MyTrait for fn(T) { type Elem = T; }.) Bad.

So, my concern with adopting semantic outlives here is that maybe there is some way to leverage a captured lifetime in an impl. e.g. if this were allowed...

type Foo<'a> = impl 'static;

impl<'a> MyTrait for Foo<'a> {
    type Elem = &'a u32;
}

...then this would be bad. However, we don't consider lifetimes used in opaques to be constrained:

error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates
 --> src/main.rs:6:6
  |
6 | impl<'a> MyTrait for Foo<'a> {
  |      ^^ unconstrained lifetime parameter

So I feel fine about this, but again maybe somebody else can see a problem I don't.

[compiler-errors]

I'm going to close this and reopen it with just the liveness changes, and not the changes having to do with opaque captures (which requires a faithful representation of existential regions, or else we'll be unsound). I will start a new FCP. In the mean time, I've committed some of the tests here: #116730