Better closure requirement propagation V2

[LorrensP-2158466]

View all comments

Fixes #154267
Unblocks #151510.

Summary

Background

While we borrowck closures and their parent body in the same query to handle opaque types, each body is otherwise handled separately. When checking closures (and const blocks), we don't know the region constraints of the parent body, so we don't know anything about non-late bound regions of the closure.

These regions are represented as external universal regions of the closure, e.g. in

fn foo<'a>(mut x: &'a str) {
    let upvar = "hi there";
    let _: Box<dyn FnOnce(&mut &'a str) -> &'a str> = Box::new(|s| {
        let prev = *s;
        *s = upvar;
        prev
    });
}

When borrow checking the closure, it's signature is for<'anon> fn(&'anon &'ext0 str) -> &'ext1 str) and it has an upvar of type &'ext2 str.

Because of its signature it can assume that 'ext0: 'anon holds. It does not know anything else about its external regions. Notably even though the expected signature will use 'a twice, we still use two separate external regions here.

Borrow checking the closure now requires 'ext0: 'ext1 because of the return statement and 'ext2: 'ext0 because of the assignment. We don't know whether these outlive requirements hold and we propagate them to the parent function instead.

If both regions we have to propagate are already external, that's easy. However, we may also have to handle constraints which reference regions internal to the closure. In this case, we can't propagate the constraint to the caller, as the caller cannot name these regions of the closure. In these cases we propagate constrains which imply the constraint we actually have to prove.

E.g. in the above example if the closure ended up requiring 'ext2: 'anon we can't propagate this to the caller directly. However, as we know that 'ext0: 'anon holds, we can instead propagate 'ext2: 'ext0 which then transitively implies 'ext2: 'anon.

Given longer_fr: shorter_fr we need to propagate at least one constraint fr-: shorter_fr+ for which the closure knows that longer_fr: longer_fr- and shorter_fr+: 'shorter_fr hold.

The behavior on stable

For a constraint like T: 'a, T is promoted first, replacing all free regions in T with 'static or equal region parameters, producing a constraint subject, free of closure-local regions - failing if any region has no such replacement.'a is promoted to the caller's context, we then find non-local universal regions that 'a outlives. Then for each such region 'ub, we propagate T: 'ub.

[!note] These constraints are to conservative:
We find some universal regions 'ur in the SCC of 'a, then for each of those 'ur we find their non-local upper bounds and propagate T: 'ub. Though, we actually need only one T: 'ub to prove T: 'ur. However, the compiler does not support OR constraints (e.g. T: 'ub1 OR T: 'ub2), so we require them all.

The behavior of this PR

There are 2 changes made in this PR relevant to the current behaviour on stable: We minimize the amount of universal regions found for 'a and we minimize the amount of OR requirements we propagate to the closure creator.

Example for Universal Regions Minization

Consider this program:

struct Arg<'a: 'c, 'b: 'c, 'c, T> {
    field: *mut (&'a (), &'b (), &'c (), T),
}

impl<'a, 'b, 'c, T> Arg<'a, 'b, 'c, T> {
    fn constrain(self)
    where
        T: 'a,
    {
    }
}

fn takes_closure<'a, 'b, T>(
    _: impl for<'c> FnOnce(Arg<'a, 'b, 'c, T>)
) {}

fn error<'a, 'b, T: 'a>() {
    takes_closure::<'a, 'b, T>(|arg| arg.constrain());
}

It currently fails to compile and gives the following error:

error[E0309]: the parameter type `T` may not live long enough
  --> src/lib.rs:20:38
   |
19 | fn error<'a, 'b, T: 'a>() {
   |              -- the parameter type `T` must be valid for the lifetime `'b` as defined here...
20 |     takes_closure::<'a, 'b, T>(|arg| arg.constrain());
   |                                      ^^^^^^^^^^^^^^^ ...so that the type `T` will meet its required lifetime bounds
   |
help: consider adding an explicit lifetime bound
   |
19 | fn error<'a, 'b, T: 'a + 'b>() {
   |                        ++++

The closure tries to prove T: 'a, but it can't, so it tries to promote it to the creator.
On stable, fn try_promote_type_test finds all universal regions in the constraint graph for the lower bound 'a:

let r_scc = self.constraint_sccs.scc(lower_bound);
// ...
for ur in self.scc_values.universal_regions_outlived_by(r_scc) {

These are 'a and 'c, then for each of those regions, we find their non-local upper bounds 'ub and propagate T: 'ub:

• for 'a, this is just 'a -> T: 'a
• for 'c, these are 'a and 'b -> T: 'a OR T: 'b

Clearly, there is no outlives relation between 'a and 'b that requires T: 'b to prove T: 'a.

This pr first minimizes the regions returned by self.scc_values.universal_regions_outlived_by(r_scc) by removing any region already outlived by another region in the set. For the example above we would get ['a, 'c], which we would be minimized to ['a], because 'c: 'a. Thus never propagating T: 'b.

The reason we can do this "minimization" is somewhat trivial. If we need to prove T: '1 and T: '2, but '1: '2 holds, there is no reason to prove T: '2 as well.

Example for OR Requirement minimization

Say we introduce another type test on the first example:

struct Arg<'a: 'c, 'b: 'c, 'c, T> {
    field: *mut (&'a (), &'b (), &'c (), T),
}

impl<'a, 'b, 'c, T> Arg<'a, 'b, 'c, T> {
    fn constrain(self)
    where
        T: 'a,
        T: 'c,
    {
    }
}

fn takes_closure<'a, 'b, T>(_: impl for<'c> FnOnce(Arg<'a, 'b, 'c, T>)) {}

// We have `'a: 'c` and `'b: 'c`, requiring `T: 'a` in `constrain` should not need
// `T: 'b` here.
fn error<'a, 'b, T: 'a>() {
    takes_closure::<'a, 'b, T>(|arg| arg.constrain());
}

This T: 'c type-test now introduces an implicit OR requirement: the non-local upper bounds of 'c are ['a, 'b], thus propagating T: 'a OR T: 'b as two standalone requirements, both of which should hold. Even though our example shows that constrain already requires T: 'a, making T: 'b redundant.

This PR changes the way these requirements are propagated, essentially propagating a list of requirements for each univeral regions. This list is seen as a collection of OR requirements and if we have multiple such lists, we require all of them to hold. In our above example this would result in 2 OR requirements:

• R1: T: 'a from type test T: 'a.
• R2: T: 'a OR T: 'b from type test T: 'c.

With full requirement R1 AND R2 represented by [R1, R2]. We then loop over these OR requirements and collect all "unit" requirements (R1 above). We then loop again over the OR requirements and remove every OR requirement which contains at least one unit requirement. For our example, this would result in removing R2, because it contains R1.

Note that we can be even smarter during this removal process, say we had following OR requirements:

• R1: T: 'a
• R2: T: 'b OR T: 'c

With full requirement R1 AND R2 and assumption 'a: 'b. This assumption would allow us to remove R2, because T: 'a implies T: 'b through the assumption.

Design

Reference

We currently don't document borrowck in the reference. Once we do this, this is PR requires a localized change to how we propagate requirements from nested bodies.

Other

RFC history, Answers to unresolved questions, Post-RFC changes, Key points, Nightly extensions, Doors closed, Feedback, Call for testing, Nightly use: n/a

Implementation n/a see summary

Major parts n/a

Coverage

All examples shown above are covered as tests in this PR.

Outstanding bugs

We sometimes still propagate multiple constraints even though a single one would be sufficient, potentially causing unnecessary errors.

To avoid any such errors here, we'd probably have to support OR constraints. There may also be some future improvements to closure constraint propagation, but 🤷 we want to avoid introducing non-determinstic or surprising behavior here.

Outstanding FIXMEs n/a

Tool changes n/a

Breaking changes n/a

Type system, opsem

Compile-time checks n/a

Type system rules n/a

Sound by default? n/a

Breaks the AM? n/a

Common interactions

Temporaries n/a

Drop order n/a

Pre-expansion / post-expansion n/a

Edition hygiene n/a

SemVer implications n/a

Exposing other features n/a

History

Propagating closure requirements was originally implemented by @nikomatsakis in #46509 and then improved by @matthewjasper in #58347.
It was then improved by @LorrensP-2158466 in #148329.

Acknowledgments

Open items

Rustc Dev Guide should be updated: type-outlive constraints.

r? @lcnr

[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!

[LorrensP-2158466]

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!

Yeah, nevermind, the 2-phase way is much easier to implement and is more logical (at least to me :)). When trying to define the problem it reminded me of Unit Propagation. Basically given a boolean expression in conjunctive normal form (i.e boolean expressions with AND operators between them), try to simplify the expressions by "propagating unit expressions".

Filtering these OR requirements in conjunctive normal form can be reduced to this Unit Propagation problem, but it is simpler, the subexpressions only contain OR operators, so the full expression is true if at least one unit is true. Thus, Given your example:

• we first propagate: T: 'ext1 OR T: 'ext2, call this R1
• we then propagate: T: 'ext1, call this R2

Then the full requirement (expression) is R1 AND R2; we can remove R1 because the unit R2 is contained within it and if R2 is true, T2 will be true as well.

This can be done in 1 pass, as the code does. And it compiles your harder example successfully.

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[lcnr]

could go even stronger, if you have any unit clause T: 'a and later have some T: 'b || T: 'c with an assumption 'a: 'b we can also drop that requirement, might be cool to write a test for that

View changes since the review

[LorrensP-2158466]

Oh yeah, that's true.

Implementing it is straightforward (i think for now), but actually creating a test is quite hard. Just extending your example didn't produce errors that i need, i am still bad at this 😅 .

[LorrensP-2158466]

Nevermind, I think I got it, just had to draw it out first, that helped.

I made an extra commit with the change and test.

[lcnr]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8d9f715 (8d9f715db1e34b77c6b42c1b43fed62f601743d9, parent: 2f43fe430374fce8c75d477bdb78715d5c0349c4)

[rust-timer]

Finished benchmarking commit (8d9f715): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (secondary -7.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-7.6%	[-7.6%, -7.6%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.9%, 2.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	-	-	0

Bootstrap: 487.764s -> 489.197s (0.29%)
Artifact size: 393.48 MiB -> 393.49 MiB (0.00%)

[lcnr]

lgtm 😊 gj on writing the test here

would u be up to write the FCP proposal for this

View changes since this review

[LorrensP-2158466]

Gladly!

[lcnr]

With an FCP proposal written by @LorrensP-2158466

@rfcbot fcp merge types

[rust-rfcbot]

@lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @oli-obk
• @spastorino

No concerns currently listed.

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.