make closures act like MaybeDangling - #160745
Conversation
|
cc @rust-lang/miri Some changes occurred to the CTFE machinery Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
|
r? @clarfonthey rustbot has assigned @clarfonthey. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rfcbot merge opsem |
|
@RalfJung has proposed to merge this. The next step is review by the rest of the tagged team members: 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. |
This comment has been minimized.
This comment has been minimized.
75383ed to
932a86d
Compare
This comment has been minimized.
This comment has been minimized.
932a86d to
a13be5a
Compare
This comment has been minimized.
This comment has been minimized.
a13be5a to
e3c49f3
Compare
e3c49f3 to
d919fe5
Compare
|
r? compiler This isn't really a libs change. |
|
@rustbot reviewed
But in the face of std getting it wrong1, and of unpin generators hitting another very subtle case of not being logically pinned but still needing to be (logically) wrapped in Furthermore, I strongly suspect that giving closures Footnotes
|
|
I almost forgot the strongest rationale: there's the really subtle case where you might think you're capturing a pointer but actually capture a (potentially mutable) reference instead because of how place capture rules work. fn invoke(f: impl FnOnce()) {
f()
}
fn main() {
let p = Box::leak(Box::new(0i32));
invoke(|| {
drop(unsafe { Box::from_raw(&raw mut *p) });
});
}Note: I could've sworn that having The remaining instance of this style of footgun is much less potent than the one I recall existing previously, but combined with the other evidence that retagging closures' captures when retagging the |
That would not help. The problem is the retag that happens when we move the generator. At that point it is not behind any kind of reference so no amount of UnsafeCell/UnsafeUnpin makes a difference.
The example in the OP actually does get |
This makes closures (and types like them: coroutines and coroutine closures) act like MaybeDangling. This means that the aliasing model will entirely ignore references and
Boxes passed around as closure captures, removing a pretty subtle footgun that has already caused multiple soundness issues:Here's an example of code that no longer has UB under this PR:
Basically, what we are establishing here is that immediately invoking a closure should be (almost) equivalent to just inlining its body. (There is still a caveat here in that if you capture things that violate their validity invariant, the inlined body might not care but immediately invoking the closure will. But at least for all the subtle questions around aliasing, the two will be equivalent under this PR.)
Overall I think the fact that moving a closure / generator will alter its contents (by retagging) is just a bit too subtle. It's already subtle for "normal" types but there at least one can see the type with its fields. For closures, that's all entirely implicit. At the same time, the benefit we get from this at the moment is tiny -- we can only actually tell LLVM about these references if the closure/generator has scalar / scalar-pair representation, which can only happen when it captures at most 2 scalar values.
This PR just implements the semantics without updating any docs. I am not sure where we'd document this, given our general lack of documentation around the aliasing model. Still we should t-opsem FCP this PR to ensure we have team consensus for not retagging or requiring reference dereferenceability inside closoures and closure-like types (and then we can involve lang if/when we start making official promises about this).
On the implementation side, I realized this by introducing the notion of "maybe-dangling-like" types, so that the semantics is not hard-coded specifically to
MaybeDangling. This also lets us simplifyManuallyDrop, reducing its field nesting a bit, which should help with some of the query limit issues people encountered when we added the extra field nesting. It also means generators get the desired semantics without increasing their field nesting. Cc @WaffleLapkinFixes #159443