WF checks on closure arguments and improved type-test promotion. - #151510
WF checks on closure arguments and improved type-test promotion.#151510LorrensP-2158466 wants to merge 3 commits into
Conversation
rustbot
added
S-waiting-on-review
| error: lifetime may not live long enough | ||
| --> $DIR/check-wf-of-closure-args.rs:13:41 | ||
| | | ||
| LL | let _: for<'x> fn(MyTy<&'x str>) = |x| wf(x); | ||
| | ^ | ||
| | | | ||
| | has type `MyTy<&'1 str>` | ||
| | requires that `'1` must outlive `'static` | ||
|
|
||
| error: aborting due to 2 previous errors |
There was a problem hiding this comment.
I find that the error message is a bit worse now, compared to the old:
error[E0521]: borrowed data escapes outside of closure
--> src/lib.rs:10:44
|
10 | let _: for<'x> fn(MyTy<&'x str>) = |x| wf(x); // FAIL
| - ^^^^^
| | |
| | `x` escapes the closure body here
| | argument requires that `'1` must outlive `'static`
| `x` is a reference that is only valid in the closure body
| has type `MyTy<&'1 str>`
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
CI x86_64-gnu-tools failed with an error I don't understand: |
615d765 to
734d270
Compare
|
@bors try |
This comment has been minimized.
This comment has been minimized.
WF checks on closure arguments.
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
This comment has been minimized.
This comment has been minimized.
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
craterbot
added
S-waiting-on-review
|
Why was the |
|
@LorrensP-2158466 can you look into how to fix |
|
@lcnr Will do, both! |
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
WF checks on closure arguments.
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
@LorrensP-2158466 please update the FCP proposal for this PR (could wait until after the crater run, but don't expect anything surprising here) |
|
That will be a rather big FCP. I'll try to merge the 2 as best as I can, if you find the need to delete some details, go ahead!. |
LorrensP-2158466
changed the title
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
This comment has been minimized.
This comment has been minimized.
Done by removing the function that skipped the WF check on user types (used to skip it on closure args) and using the default one. add tests of issue + bless/change other tests
Fix in question: We only propagate `T: 'ub` requirements when 'ub actually outlives the lower bounds of the type test. If none of the non-local upper bounds outlive it, then we propagate all of them. + filter redundant requirements through unit propagation & bless test
|
rebased to fix conflict and updated FCP. Now waiting on crater so that i can finish the FCP. |
5581cea to
993b2c9
Compare
|
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. |
|
🎉 Experiment
Footnotes
|
craterbot
added
S-waiting-on-review
|
Obligatory, never did a crater triage, so excuse me if this is done wrong 😅, below is the detailed triage, I'll give a reducer that reports the same kind of error. So the root 9 regressions seem to be expected, they are a result of 2 crates that fail to compile because of the following: trait Bound<'a> {}
impl<'a> Bound<'a> for &'a str {}
struct Container<'a, K>
where
K: Bound<'a>,
{
_d: &'a [u8],
_k: std::marker::PhantomData<K>,
}
fn main() {
let _closure = |_: &Container<&str>| {};
}With this pr, it fails like so: While it succeeds on nightly: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=7ad2eb0611bb068c3df9db34a9fa7660 I don't know enough of this part of the compiler to correctly say this should compile or not. What do you say @lcnr? Detailed Triage (Click to expand)
|
|
So yeah, the compiler creates 3 regions: And does not "infer" that Though, I don't know if the compiler should infer it, and I don't know how to make it compile by adding explicit lifetimes. |
|
Also, with fn main() {
let _closure = for<'a, 'd> |_: &'a Container<'d, &'d str>| -> () {};
} |
|
does the following also break? trait Bound<'a> {}
impl<'a> Bound<'a> for &'a str {}
struct Container<'a, K>
where
K: Bound<'a>,
{
_d: &'a [u8],
_k: std::marker::PhantomData<K>,
}
fn main() {
let _closure = |_: Container<&str>| {};
}
There is no real implied bound that'd make that compile :/ because the fact that trait Bound<'a> {}
impl<'a> Bound<'a> for &'a str {}
struct Container<'a, K: Bound<'a>>(std::marker::PhantomData<(&'a (), K)>);
fn main() {
let _ = |x: Container<'_, &str>| -> Container<'_, &str> { x };
}already errors with the following on stable error: lifetime may not live long enough
--> src/main.rs:5:63
|
5 | let _ = |x: Container<'_, &str>| -> Container<'_, &str> { x };
| - ------------------- ^ returning this value requires that `'1` must outlive `'2`
| | |
| | return type of closure is Container<'2, &str>
| has type `Container<'1, &str>`so does this snippet if we make the lifetimes invariant trait Bound<'a> {}
impl<'a> Bound<'a> for &'a str {}
struct Container<'a, K: Bound<'a>>(std::marker::PhantomData<*mut (&'a (), K)>);
fn main() {
let _ = |x: Container<'_, &str>| drop(x);
}damn, this sucks xd |
|
you can work around this with trait Bound<'a> {}
impl<'a> Bound<'a> for &'a str {}
struct Container<'a, K: Bound<'a>>(std::marker::PhantomData<*mut (&'a (), K)>);
fn id<F: for<'a> FnOnce(Container<'a, &'a str>)>(f: F) -> F { f }
fn main() {
let _ = id(|x: Container<'_, &str>| drop(x)); // compiles
} |
lcnr
added
I-types-nominated
It does :(
it does indeed. |
lcnr
removed
the
S-waiting-on-review
9 participants
View all comments
Fixes #104478.
Fixes #154267.
Enables WF checks on the arguments of a closure and improves type-test promotion.
The now removed function
ascribe_user_type_skip_wfmentioned that skipping WF was done due to backwards compatibility reasons.FCP below explains the type-test changes.
FCP: WF checks on closure arguments and better type-test propagation
Summary
On stable, we only perform WF-checks on closure arguments when that argument is actually used. This causes non WF code to still compile.
This was necessary as properly checking them for WF caused breakage due to a weakness of closure requirement propagation #104477, see #104478. As we've fixed #104477 in #148329 and in this PR, the remaining breakage is now all intended.
This change cause compilation errors when a used type is not WF. This is an intentional breakaging change.
Example
Consider the example from the first linked issue of this PR.
Currently, on stable, this fails only because of the second closure:
We don't emit an error for the first closure because the argument is not used in the closure body, even though it is also not WF.
With this PR we now error for both closures:
Improved Type Test Promotion
When only enabling WF-checks on closure arguments, a regression was observed when compiling
jobsteal: See the crater triage by @theemathas #151510 (comment).This regression and possible others can be resolved by immproving type-test promotion, this is the second part of the PR. We've already changed closure requirement propagation for region outlive constraints, see #148329 for background here.
For a constraint like
T: 'a,Tis promoted first, replacing all free regions inTwith'staticor equal region parameters, producing a constraint subject, free of closure-local regions - failing if any region has no such replacement.'ais promoted to the caller's context, we then find non-local universal regions that'aoutlives. On stable, we propagateT: 'ubfor all non-local upper bounds of'a.These constraints are too conservative. We find some universal regions
'urin the SCC of'a, then for each of those'urwe find their non-local upper bounds and propagateT: 'ub. Though, we actually need only oneT: 'ubto proveT: 'ur. However, the compiler does not support OR constraints (e.g.T: 'ub1ORT: '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
'aand we minimize the amount of OR requirements we propagate to the closure creator.Example for Universal Regions Minization
Consider this program:
It currently fails to compile and gives the following error:
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:These are
'aand'c, then for each of those regions, we find their non-local upper bounds'uband propagateT: 'ub:'a, this is just'a->T: 'a'c, these are'aand'b->T: 'aORT: 'bT: 'aholds in the parent. We also propagateT: 'bon stable, resulting in an error.This P 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 propagatingT: 'b.The reason we can do this "minimization" is somewhat trivial. If we need to prove
T: '1andT: '2, but'1: '2holds, there is no reason to proveT: '2as well.Example for OR Requirement minimization
Say we introduce another type test on the first example:
This
T: 'ctype-test now introduces an implicit OR requirement: the non-local upper bounds of'care['a, 'b], thus propagatingT: 'a OR T: 'bas two standalone requirements, both of which should hold. Even though our example shows thatconstrainalready requiresT: 'a, makingT: 'bredundant.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: 'afrom type testT: 'a.R2:T: 'a OR T: 'bfrom type testT: 'c.With full requirement
R1 AND R2represented 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 removingR2, because it containsR1.Note that we can be even smarter during this removal process, say we had following OR requirements:
R1:T: 'aR2:T: 'b OR T: 'cWith full requirement
R1 AND R2and assumption'a: 'b. This assumption would allow us to removeR2, becauseT: 'aimpliesT: 'bthrough the assumption.Reference
The specifics of WF checking is not documented in the 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.
Coverage
See the tests added in this PR for things which now (don't) compile.
Outstanding bugs WF
There is a related unsoundness in #151637, which is not fixed by this PR. This is due to closures using implied bounds only involving external regions and needs to be fixed separately. See the WIP work in #153027.
Outstanding bugs type-test Promotion
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.
Breaking changes
This change is intentionally breaking: (TODO after crater).
History
WF checks on closure arguments was left as future work in #101947 and tracked in #104478.
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.
Open items
Rustc Dev Guide should be updated: type-outlive constraints.
r? @lcnr