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?