Better closure requirement propagation V2

[LorrensP-2158466]

Fixes #154267
Unblocks #151510.

We only propagate T: 'ub requirements when 'ub actually outlives the lower bound of the type test. If none of the non-local upper bounds outlive the lower bound, then we propagate all of them.

r? @lcnr

[lcnr]

outdated comment

[lcnr]

that's still odd and the test feels confusing/easier to fix than the one I wrote 🤔

In your test we have

'a: 'c
'b: 'c
T: 'a

We should clearly not propagate T: 'c and 'c: 'a. It's weird that we look at at universal_regions_outlived_by at all. Why are we not directly looking at the non_local_upper_bounds of lower_bound?

At least only look at the smallest such universal region

[LorrensP-2158466]

We should clearly not propagate T: 'c and 'c: 'a

And that doesn't happen, I don't know what you mean with this.

It's weird that we look at at universal_regions_outlived_by at all. Why are we not directly looking at the non_local_upper_bounds of lower_bound

The function comment:

    /// Once we've promoted T, we have to "promote" `'X` to some region
    /// that is "external" to the closure. Generally speaking, a region
    /// may be the union of some points in the closure body as well as
    /// various free lifetimes. We can ignore the points in the closure
    /// body: if the type T can be expressed in terms of external regions,
    /// we know it outlives the points in the closure body. That
    /// just leaves the free regions.

Otherwise I have no clue, dev-guide didn't give a reason. If I try to do non_local_upper_bounds of lower_bound the compiler panics:

thread 'rustc' (6493769) panicked at compiler/rustc_borrowck/src/type_check/free_region_relations.rs:113:9:
assertion failed: self.universal_regions.is_universal_region(fr0)

At least only look at the smallest such universal region

How is this possible?

[lcnr]

We should clearly not propagate T: 'c and 'c: 'a

on stable we propagate T: 'c as we separately require/know 'c: 'a, do we not? I guess we don't propagate 'c: 'a

[lcnr]

If I try to do non_local_upper_bounds of lower_bound the compiler panics:

thread 'rustc' (6493769) panicked at compiler/rustc_borrowck/src/type_check/free_region_relations.rs:113:9:
assertion failed: self.universal_regions.is_universal_region(fr0)

At least only look at the smallest such universal region

How is this possible?

I guess we currently don't have a function to just do that right away, unsure.

We could call universal_regions_outlived_by and then manually filter the universals to ones which are not outlived by any other of the returned regions.

There's ReverseSccGraph.upper_bounds which seems useful?

How is this possible?

I guess the core answer here is

• we have a way to get all universal regions which are required universal_regions_outlived_by
• and then have a way to test whether universal_regions outlive each other

so this is definitely possible, worst case we have to reimpl it

[LorrensP-2158466]

on stable we propagate T: 'c as we separately require/know 'c: 'a, do we not? I guess we don't propagate 'c: 'a

I have feeling that you are mixing up 'a: 'c with 'c: 'a, because I don't see why the compiler might do this. [In this example] Clearly we should never propogate the latter.
Propagating T: 'c is wrong, and does currently happen on nightly/stable.

Also, I tried your suggestion, though I couldn't get access to ReverseSccGraph.upper_bounds, so i did this instead:

debug(?universal_regions);
let minimal_universal_regions: Vec<_> = universal_regions
    .iter()
    .copied()
    .filter(|&a| {
        !universal_regions
            .iter()
            .copied()
            .any(|b| b != a && self.universal_region_relations.outlives(b, a))
    })
    .collect();
debug(?minimal_universal_regions);
let universal_regions = if minimal_universal_regions.is_empty() {
    universal_regions
} else {
    minimal_universal_regions
};
debug(?universal_regions);

It fixes the test of this PR, but your "harder" test still miscompiles. Because when doing the type test T:'a, this happens:

0ms DEBUG rustc_borrowck::region_infer universal_regions=['?2, '?5]
0ms DEBUG rustc_borrowck::region_infer minimal_universal_regions=['?2]
0ms DEBUG rustc_borrowck::region_infer universal_regions=['?2]
0ms DEBUG rustc_borrowck::region_infer universal_region_outlived_by ur='?2
0ms DEBUG rustc_borrowck::type_check::free_region_relations non_local_upper_bound(fr='?2)
0ms DEBUG rustc_borrowck::type_check::free_region_relations non_local_bound: external_parents=['?2]
0ms DEBUG rustc_borrowck::region_infer non_local_ub=['?2]
0ms DEBUG rustc_borrowck::region_infer adding closure requirement, requirement=ClosureOutlivesRequirement { subject: Ty(ClosureOutlivesSubjectTy { inner: T/#1 }), outlived_free_region: '?2, blame_span: lcnr_test.rs:19:38: 19:53 (#0), category: Boring }

?2 = 'a, and ?5 = 'c.
As you can see, we propagate: T: 'a.

And for T: 'c, this happens:

0ms DEBUG rustc_borrowck::region_infer universal_regions=['?5]
 0ms DEBUG rustc_borrowck::region_infer minimal_universal_regions=['?5]
 0ms DEBUG rustc_borrowck::region_infer universal_regions=['?5]
 0ms DEBUG rustc_borrowck::region_infer universal_region_outlived_by ur='?5
 0ms DEBUG rustc_borrowck::type_check::free_region_relations non_local_upper_bound(fr='?5)
 0ms DEBUG rustc_borrowck::type_check::free_region_relations non_local_bound: external_parents=['?3, '?2]
 0ms DEBUG rustc_borrowck::region_infer non_local_ub=['?3, '?2]
 0ms DEBUG rustc_borrowck::region_infer adding closure requirement, requirement=ClosureOutlivesRequirement { subject: Ty(ClosureOutlivesSubjectTy { inner: T/#1 }), outlived_free_region: '?3, blame_span: lcnr_test.rs:19:38: 19:53 (#0), category: Boring }
 0ms DEBUG rustc_borrowck::region_infer adding closure requirement, requirement=ClosureOutlivesRequirement { subject: Ty(ClosureOutlivesSubjectTy { inner: T/#1 }), outlived_free_region: '?2, blame_span: lcnr_test.rs:19:38: 19:53 (#0), category: Boring }

?2 = 'a, ?3 = 'b and?5 = 'c.

So it's still thinking that to prove T: 'c, we need both T: 'a and T: 'c to hold true, because the type test is looked at in "isolation". That's why I asked in zulip that maybe we need "OR" constraints here.

[LorrensP-2158466]

I do still have a feeling that I am not completely understanding what you think is the correct fix.
How I understand the current issue: When type testing T: 'c, we don't actually know that we are already (or will) require T': a, so the only way to prove that T: 'c is that T should outlive either 'a or 'b, right?

[lcnr]

on computing minimal_universal_regions sgtm, though I'd use !a_outlives_b && b_outlives_a and then minimal_universal_regions should never be empty 🤔

[LorrensP-2158466]

Alright, thanks!

[lcnr]

I do still have a feeling that I am not completely understanding what you think is the correct fix. How I understand the current issue: When type testing T: 'c, we don't actually know that we are already (or will) require T': a, so the only way to prove that T: 'c is that T should outlive either 'a or 'b, right?

Yeah, let me write down my thoughts here. There are actually multiple things here. We can prove T: 'local by propagating a constraint T: 'ext where we know/separately require 'ext: 'local

What we currently do on stable and what I didn't even think of, is that if we have 'ext1: 'local, 'ext2: 'local, 'ext2: 'ext1 we actually propagate both T: 'ext1 and T: 'ext2. We should pick the minimal external regions here to propagate the weakest constraint which still implies T: 'local. In this case that is T: 'ext1.

Separately, if we have 'ext1: 'local and 'ext2: 'local without any outlives relation between 'ext1 and 'ext2 we'd need to prove T: 'ext1 OR T: 'ext2 and there isn't an obvious better choice. We do not support propagating OR constraints, so we currently just propagate both.

Now, if we know that we'll separately propagate T: 'ext1 anyways, there is no need to also propagate T: 'ext2. The question is: how does propagating type tests know that one of the constraints in the OR is already required or guaranteed to hold.

We do have access to self.type_tests here, so you could go through the list of type tests and check whether these already contain some T: 'ext1 constraint.

Separately, we could handle type tests as a two step pass. First collect all type tests which have a single possible candidate, then go through all the ones with multiple options and check whether one of the options is already part of the list of definitely required type tests.

Could also do that separately/not handle that part for now 🤷

[LorrensP-2158466]

Thanks for the explanation, i understand now.

Yeah, let me write down my thoughts here. There are actually multiple things here. We can prove T: 'local by propagating a constraint T: 'ext where we know/separately require 'ext: 'local

What we currently do on stable and what I didn't even think of, is that if we have 'ext1: 'local, 'ext2: 'local, 'ext2: 'ext1 we actually propagate both T: 'ext1 and T: 'ext2. We should pick the minimal external regions here to propagate the weakest constraint which still implies T: 'local. In this case that is T: 'ext1.

I am with you here, that's what this PR is currently doing right?

Separately, if we have 'ext1: 'local and 'ext2: 'local without any outlives relation between 'ext1 and 'ext2 we'd need to prove T: 'ext1 OR T: 'ext2 and there isn't an obvious better choice. We do not support propagating OR constraints, so we currently just propagate both.

Now, if we know that we'll separately propagate T: 'ext1 anyways, there is no need to also propagate T: 'ext2. The question is: how does propagating type tests know that one of the constraints in the OR is already required or guaranteed to hold.

Yes right, I was thinking in the same direction, this is the part where i am stuck because a clean solution didn't come to me. I only came up with the following myself:

We do have access to self.type_tests here, so you could go through the list of type tests and check whether these already contain some T: 'ext1 constraint.

But this seems a bit unnatural to me, i prefer the other one:

Separately, we could handle type tests as a two step pass. First collect all type tests which have a single possible candidate, then go through all the ones with multiple options and check whether one of the options is already part of the list of definitely required type tests.

I find that this equates to finding the "minimal required" type tests. Which we seem to resort to in these closure propagation pr's.

Could also do that separately/not handle that part for now 🤷

I will try both solutions anyway, you never know :). i'll create separate branches in my fork and create a channel on zullip to discuss them. If you want it done in another way, lmk!