Tracking issue for Pin APIs (RFC 2349)
withoutboats opened this issue · 211 comments
Tracking issue for rust-lang/rfcs#2349
Blocking stabilization:
- Implementation (PR #49058)
- Documentation
Unresolved questions:
- Should we provide stronger guarantees around leaking
!Unpindata?
Edit: Summary comment: #49150 (comment) (in the hidden-by-default part)
I now notice that stack pinning is not part of the RFC, @withoutboats are you planning on releasing a crate for this, or should I just copy the example code into my crate that needs it?
@Nemo157 You should copy it and report your experience!
The unresolved question about leaking Unpin data relates to this. That API is unsound if we say you cannot overwrite Unpin data in a Pin unless the destructor runs, as @cramertj requested. There are other, less ergonomic, stack pinning APIs that do work for this. Its unclear what the right choice here is - is the ergonomic stack pinning API more useful or is the extra guarantee about leaking more useful?
One thing I'll note is that the stack pinning was not sufficient for things like Future::poll inside the await! macro, because it didn't allow us to poll in a loop. I'd be interested if you run into those issues, and how/if you solve them if you do.
My current usage is pretty trivial, a single-threaded executor running a single StableFuture without spawning support. Switching to an API like @cramertj suggests would work fine with this. I have wondered how to extend this to allow spawning multiple StableFutures, but at least with my current project that's not necessary.
Just tried to experiment with the API. Looks like the following (suggested by RFC) definition of Future is no longer object-safe?
trait Future {
type Item;
type Error;
fn poll(self: Pin<Self>, cx: &mut task::Context) -> Poll<Self::Item, Self::Error>;
}Nevermind. Found a note about a plan to make arbitrary_self_types object-safe.
One thing I'll note is that the stack pinning was not sufficient for things like Future::poll inside the await! macro, because it didn't allow us to poll in a loop.
Could you elaborate on that?
@cramertj That sounds like a restriction of the Pin API, not of the stack pinning API?
@RalfJung Yes, that's correct. However, PinBox can be reborrowed as Pin, while the stack-pinned type cannot (one borrow as Pin creates a borrow for the entire lifetime of the stack type).
Given a Pin, I can borrow it as &mut Pin and then use Pin::borrow -- that's a form of reborrowing. I take it that's not the kind of reborowing you are talking about?
@RalfJung No-- methods like Future::poll were planned to take self: Pin<Self>, rather than self: &mut Pin<Self> (which isn't a valid self type since it isn't Deref<item = Self> -- it's Deref<Item = Pin<Self>>).
It might be the case that we could get this to work with Pin::borrow actually. I'm not sure.
@cramertj I did not suggest to call poll on x: &mut Pin<Self>; I thought of x.borrow().poll().
I'll try and remember to post an example of some of the stuff I'm doing with Pins next week, as far as I can tell reborrowing works perfectly. I have a pinned version of the futures::io::AsyncRead trait along with working adaptors like fn read_exact<'a, 'b, R: PinRead + 'a>(read: Pin<'a, R>, buf: &'b [u8]) -> impl StableFuture + 'a + 'b and I'm able to work this into a relatively complex StableFuture that's just stack pinned at the top level.
Here's the full example of what I'm using for reading:
pub trait Read {
type Error;
fn poll_read(
self: Pin<Self>,
cx: &mut task::Context,
buf: &mut [u8],
) -> Poll<usize, Self::Error>;
}
pub fn read_exact<'a, 'b: 'a, R: Read + 'a>(
mut this: Pin<'a, R>,
buf: &'b mut [u8],
) -> impl StableFuture<Item = (), Error = Error<R::Error>>
+ Captures<'a>
+ Captures<'b> {
async_block_pinned! {
let mut position = 0;
while position < buf.len() {
let amount = await!(poll_fn(|cx| {
Pin::borrow(&mut this).poll_read(cx, &mut buf[position..])
}))?;
position += amount;
if amount == 0 {
Err(Error::UnexpectedEof)?;
}
}
Ok(())
}
}This is slightly annoying as you have to pass instances through everywhere as Pin and use Pin::borrow whenever you call functions on them.
#[async]
fn foo<'a, R>(source: Pin<'a, R>) -> Result<!, Error> where R: Read + 'a {
loop {
let mut buffer = [0; 8];
await!(read_exact(Pin::borrow(&mut source), &mut buffer[..]));
// do something with buffer
}
}I just had a thought that I could impl<'a, R> Read for Pin<'a R> where R: Read + 'a to workaround having to pass values as Pin<'a, R> everywhere, instead I could use fn foo<R>(source: R) where R: Read + Unpin. Unfortunately that fails because Pin<'a, R>: !Unpin, I think it's safe to add an unsafe impl<'a, T> Unpin for Pin<'a, T> {} since the pin itself is just a reference and the data behind it is still pinned.
Concern: It seems likely that we want most types in libstd to implement Unpin unconditionally, even if their type parameters are not Pin. Examples are Vec, VecDeque, Box, Cell, RefCell, Mutex, RwLock, Rc, Arc. I expect most crates will not think about pinning at all, and hence only have their types be Unpin if all their fields are Unpin. That's a sound choice, but it leads to unnecessarily weak interfaces.
Will this solve itself if we make sure to implement Unpin for all libstd pointer types (maybe even including raw pointers) and UnsafeCell? Is that something we will want to do?
Will this solve itself if we make sure to implement Unpin for all libstd pointer types (maybe even including raw pointers) and UnsafeCell? Is that something we will want to do?
Yes, it seems like the same situation as Send to me.
A new question just occurred to me: When are Pin and PinBox Send? Right now, the auto trait mechanism makes them Send whenever T is Send. There is no a priori reason to do that; just like types in the shared typestate have their own marker trait for sendability (called Sync), we could make a marker trait saying when Pin<T> is Send, e.g. PinSend. In principle, it is possible to write types that are Send but not PinSend and vice versa.
@RalfJung Pin is Send when &mut T is Send. PinBox is Send when Box<T> is Send. I see no reason for them to be different.
Well, just like some types are Send but not Sync, you could have a type relying on "Once this method is called with Pin<Self>, I can rely on never being moved to another thread". For example, this could give rise to futures that can be sent around before being started for the first time, but then have to remain in one thread (much like they can be moved around before being started, but then have to remain pinned). I'm not sure if I can come up with a convincing example, maybe something about a future that uses thread-local storage?
I just hit the lifetime issue mentioned by @Diggsey. I believe Pin<Option<T>> -> Option<Pin<T>> should be a safe operation but it doesn't appear possible to implement even using the current unsafe APIs, let alone what sort of API would be needed to make this safe code:
trait OptionAsPin<T> {
fn as_pin<'a>(self: Pin<'a, Self>) -> Option<Pin<'a, T>>;
}
impl<T> OptionAsPin<T> for Option<T> {
fn as_pin<'a>(self: Pin<'a, Self>) -> Option<Pin<'a, T>> {
match *unsafe { Pin::get_mut(&mut self) } {
Some(ref mut item) => Some(unsafe { Pin::new_unchecked(item) }),
None => None,
}
}
}(It's possible to workaround using transmute to force the lifetimes, but that makes me feel far too icky).
I'd like to add an unresolved question: Should we add back a shared pinned reference type? I think the answer is yes. See this post with discussion for further details.
I just read that futures 0.2 is not as final as I thought it was, so maybe it is still possible after all to rename Pin back to PinMut and add a shared version.
@RalfJung I read your blog post again more thoroughly to understand the changes you propose.
I think you've found a potentially compelling use case for having an immutable Pin variant, but I don't understand your comments about Deref and &Pin<T> <=> &&T. Even if Pin<T> can be cast down to &T safely, that doesn't make them equivalent, because &T cannot be cast to Pin<T>. I don't see a reason to make the safe conversion unsafe (by eliminating the safe Deref impl).
Currently the map method on Pin has the signature
pub unsafe fn map<U, F>(this: &'b mut Pin<'a, T>, f: F) -> Pin<'b, U>What is the reason that it isn't the following?
pub unsafe fn map<U, F>(this: Pin<'a, T>, f: F) -> Pin<'a, U>As it stands, I can't transform a Pin of one type to a Pin of one of its fields without shortening the lifetime unnecessarily.
Another problem with the map method is that it seems to be impossible to turn a Pin of a struct into two Pins, each of a different field of the struct. What is the correct way to achieve that?
I've been using this macro for that:
macro_rules! pin_fields {
($pin:expr, ($($field:ident),+ $(,)?)) => {
unsafe {
let s = Pin::get_mut(&mut $pin);
($(Pin::new_unchecked(&mut s.$field),)+)
}
};
}In a lot of the discussion around Pin there seems to be an assumption that "projecting" a pin onto a private field should be thought of as safe. I don't think that's quite true. For instance, the doc comment on map currently reads:
You must guarantee that the data you return will not move so long as the argument value does not move (for example, because it is one of the fields of that value), and also that you do not move out of the argument you receive to the interior function.
The guarantee that "the data you return will not move so long as the argument value does not move" is the correct description of the contract that a caller of map has to uphold. The parenthetical "(for example, because it is one of the fields of that value)" seems to imply that as long as you're just returning a reference to your own private field you're guaranteed to be safe. But that's not true if you implement Drop. A Drop impl is going to see a &mut self, even when other code has already seen a Pin<Self>. It can proceed to use mem::replace or mem::swap to move out of its fields, violating the promise made by an earlier "correct" use of map.
In other words: using a "correct pin projection" call to Pin::map (the call looks like unsafe { Pin::map(&mut self, |x| &mut x.p) }), with no other invocations of unsafe, one can produce unsound/undefined behavior. Here's a playground link demonstrating this.
This doesn't imply that anything is wrong with the current API. It demonstrates is that Pin::map should be marked as unsafe, which it already is. Nor do I think there's much of a danger of people accidentally tripping over this when implementing futures or the like--you really have to go out of your way to move out of your own fields in a Drop impl, and I doubt there are many practical reasons to want to do it.
But I do think the doc comment for map might want to mention this, and I also think it invalidates some ideas for extensions that I see in the RFC and surrounding discussions:
- There should not be a macro/derive that does "pin projection" and makes it look safe. Projection really does impose a contract on the other code surrounding it, which the compiler can't fully enforce. So it ought to require the use of the
unsafekeyword whenever it's done. - The RFC mentions that if
Pinwere turned into a&'a pin Tlanguage feature, it would be "trivial to project through fields." I believe I've shown that this should still requireunsafe, even if limited to projecting private fields.
Even if Pin can be cast down to &T safely, that doesn't make them equivalent, because &T cannot be cast to Pin.
Indeed, that is not a sufficient condition. They are equal in my model because in my earlier post, we made the following definiton:
Definition 5:
PinBox<T>.shr. A pointer ptr and lifetime 'a satisfy the shared typestate ofPinBox<T>if ptr is a read-only pointer to another pointer inner_ptr such thatT.shr('a, inner_ptr)
(And, kind-of implicitly, the corresponding definition for Pin<'a, T>.shr.)
Notice how PinBox<T>.shr depends on T.shr and nothing else. This makes PinBox<T>.shr exactly the same invariant as Box<T>.shr, which implies that &Box<T> = &PinBox<T>. Similar reasoning shows that &&T = &Pin<T>.
So, this is not a consequence of the API or contract written down in the RFC. It is a consequence of the model that just has three typestates: Owned, shared, pinned. If you want to argue against &&T = &Pin<T>, you have to argue for introducing a fourth typestate, "shared pinned".
In a lot of the discussion around Pin there seems to be an assumption that "projecting" a pin onto a private field should be thought of as safe. I don't think that's quite true.
That's a very good point! Just to be clear, there is no issue with making the p field of Shenanigans public, is there? At that point, any client could write do_something_that_needs_pinning, and the intention of the RFC is to make that safe. (I don't know why the RFC specifically mentions private fields, my interpretation was always that it was supposed to work with all fields.)
It is interesting to see how this interacts with the interpretation of drop in my model. There I wrote that drop(ptr) has a precondition of T.pin(ptr). With this interpretation, the actual code at fault in your example would be the drop implementation! (Now I wonder why I did not notice this already when writing the post...) I think we do want to allow safe projections to fields eventually, and we really shouldn't provide drop with an &mut if the type does pinning. That's clearly bogus.
Is there any way we could either (a) make impl Drop unsafe if the type is !Unpin, or (b) change its signature to drop(self: Pin<Self>) if T: !Unpin? Both sound extremely far-fetched, but on the other hand both would solve @MicahChalmer's point, while preserving safety of field projections. (If only this was still pre-1.0 and we could change Drop::drop to just always take Pin<Self> ;) But of course at this point this is not a library-only solution any more. The sad part is that if we stabilize as-is, we can never have safe field projections.
@RalfJung So I am more interested in the practical questions (as you'd probably expect 😉). I think there are two:
- Should
Pin<T: !Unpin>implementDeref<Target =T>? - Should there be both a
Pin<T>and aPinMut<T>(the former being a shared pin)?
I don't see a reason to answer the first question negatively. I think you have re-opened the second question; I'm inclined to go back to two different kinds of pins (but not completely convinced). If we ever upgrade this to a language-level reference type, we'd have &pin T and &pin mut T.
It seems like no matter what we do your model probably needs a fourth type-state to accurately reflect the API invariants. I do not think converting an &&T to an &Pin<T> should be safe.
So I am more interested in the practical questions (as you'd probably expect wink).
Fair enough. ;) However, I think it is important that we have at least a basic understanding of what is and is not guaranteed around Pin once unsafe code enters the picture. Basic Rust had several years of stabilization to figure this out, now we are repeating this for Pin in a few months. While Pin is a library-only addition as far as syntax is concerned, it is a significant addition as far as the model is concerned. I think it is prudent that we document as exhaustively as possible what unsafe code is and is not allowed to assume or do around Pin.
While these concerns are theoretical right now, they will suddenly be very practical once we have the first unsoundness due to unsafe code making incompatible assumptions.
Concerning your second question:
It seems like no matter what we do your model probably needs a fourth type-state to accurately reflect the API invariants. I do not think converting an &&T to an &Pin should be safe.
That is what I expected.
If we want to be conservative, we could rename Pin to PinMut but not add PinShr (likely to be called Pin but I am trying to disambiguate here) for now, and declare that unsafe code can assume that a &PinMut<T> actually points to something pinned. Then we would have the option of adding Pin as a shared pinned reference later.
A practical reason for having PinShr has been brought up by @comex: it should be possible to provide a getter that goes from PinShr<'a, Struct> to PinShr<'a, Field>; if we use &PinMut for shared pins that doesn't work. I don't know how much such getters will be needed.
For your first question, there seem to be some strong arguments in favor: (a) the current plans for object-safe arbitrary self types, and (b) being able to safely use the vast amount of existing APIs on shared references when holding a PinShr (or PinMut).
It is unfortunate that we don't seem to have an easy way to similarly provide operations that can work both on &mut and PinMut; after all plenty of code working on &mut has no intention of using mem::swap. (This, as I see it, would be the main advantage of a !DynSized-based solution or something comparable: &mut would turn into a type for references that may or may not be pinned. We could have this as yet another library type in the Pin API, but that's rather pointless given that we already have all these &mut methods out there.)
There is one light argument against, which is types that want to do "interesting" things both for &T and PinMut<T>. That will be hard to do, not everything is possible and one has to be very careful. But I don't think that trumps the good arguments in favor.
In this case (i.e., with this impl Deref), PinShr<'a, T> should come with a safe method that turns it into a &'a T (preserving the lifetime).
And there is another concern I think we have to resolve: The rules for Pin::map and/or drop, in the light of what @MicahChalmer noticed above. We have two choices:
- Declare that using
Pin::mapwith a projection to a public field (no derefs, not even implicit) is always safe. This is my interpretation of the current RFC. This would match&mutand&. However, then we have a problem aroundDrop: We can now write safe code that causes UB given an otherwise well-formed!Unpintype. - Not do anything funny around
Drop. Then we have to add loud warnings toPin::mapthat even using it for public fields could lead to unsoundness. Even if we ever have&pin T, we will not be able to use that to access a field in safe code.
The only possible argument for the second option that I can see is that it may be the only one we can actually implement. ;) I think it is inferior in every possible way -- it makes &pin rather strange and unergonomic even if it becomes built-in one day, it is a footgun, it hinders compositionality.
There may be a way to achieve the first option, but it's not trivial and I have no idea how to make it backwards-compatibly: We could add an Unpin bound to Drop, and add a DropPinned that does not have the bound and where drop takes Pin<Self>. Maybe the Unpin bound on Drop could be enforced in a strange way where you can write the impl Drop for S, but this adds an implicit bound to S saying that it has to be Unpin. Probably not realistic. :/ (I guess this is also a point where the !DynSized-based approach works better -- it turns &mut T into "may or may not be pinned", keeping drop sound.)
@RalfJung @MicahChalmer I think its better to just document that if you move out of the field in the Drop impl, projecting to a Pin of that field elsewhere is unsound.
Indeed, its already the case today that (using unsafe code) you could move out of a field of a !Unpin type, and this is safe and well defined as long as you never project to a pin of that field elsewhere. The only difference with Drop is that the moving-out part only contains safe code. It seems to me that the notes on Pin::map need to change to note that it is not safe if you ever move out of that field, regardless of the Drop impl.
It must be safe to move out of the fields of a !Unpin type in some cases, because the generators will very likely move out of one of their fields when they return.
I think its better to just document that if you move out of the field in the Drop impl, projecting to a Pin of that field elsewhere is unsound.
This is the second option then, the one that makes &pin to a field permanently an unsafe operation.
I think this is not a small change. This fundamentally changes what pub on a field means. When using a library type, I cannot know what it does in its drop impl, so I fundamentally have no way to obtain a pinned reference to that field.
For example, I wouldn't even be allowed to go from Pin<Option<T>> to Option<Pin<T>> unless Option explicitly states that it will never have a Drop doing anything "funny". The compiler cannot understand that statement, so while Option could provide an appropriate method to do this, doing the same with a match has to remain an unsafe operation.
The only difference with Drop is that the moving-out part only contains safe code.
But that' a huge difference, isn't it? We can put arbitrary rules on what unsafe code may or may not do, but not so for safe code.
It must be safe to move out of the fields of a !Unpin type in some cases, because the generators will very likely move out of one of their fields when they return.
I suppose in this case the field will be Unpin? So we could likely have a rule saying that Pin::mut for a public field of a foreign struct is fine if that field has type Unpin. Not sure how useful this is, but it's probably better than nothing.
I want to quickly restate my confusion about &Pin<T> not providing more guarantees than &&T. &, &mut, and &pin respectively provide "shared access", "unique access", and "unique access to a value that won't be moved". Understanding &&pin as "shared access to a unique access to a type that won't be moved" tells you that the memory is shared (the uniqueness guarantee of &pin is cancelled out by the sharing of &), but you still retain the property that the type won't be moved, no?
I am not sure what you are asking or saying. Are you confused why I think "shared pinned" is a fundamental mode/typestate on its own?
The point is, "shared access" is not a thing I know how to define on its own. There are many different ways to share and coordinate the sharing, as is witnessed by the very different ways in which, e.g., Cell, RefCell, and Mutex share.
You can't just say you are sharing something ("cancelling the uniqueness guarantee" of something) you own and expect that statement to make sense. You have to say how you are sharing, and how you ensure that this cannot cause havoc. You can "share by making it read-only", or "share by only giving atomic access through synchronized loads/stores", or "share [within just one thread] by having this borrow flag coordinate which kind of access to hand out". One of the key points in RustBelt was realizing the importance of letting every type define for itself what happens when it gets shared.
I cannot think of a way to make "shared pinning" arise as the orthogonal composition of sharing and pinning. Maybe there is a way to define the notion of a "sharing mechanism" that could then be applied to either the owned or the pinned invariant to give rise to "(normal) shared" and "pinned shared", but I seriously doubt it. Also, as we have seen that falls flat for RefCell -- if RefCell does for the shared pinned invariant something similar to what it does for the just shared invariant, we certainly cannot justify that from & &pin RefCell<T> via &RefCell<T> (using Deref) via borrow_mut we can obtain a &mut reference that says that no pinning happened.
We could add an Unpin bound to Drop, and add a DropPinned that does not have the bound and where drop takes Pin.
Is the definition of Drop really the problem here? Another way to think about it is to place the blame on mem::swap and mem::replace. These are the operations that let you move something you don't own. Suppose a T: Unpin bound were added to them?
For starters, that would fix the drop hole that I pointed out - my Shenanigans would fail to compile, and I don't think I could make it to violate the pin promises without another unsafe. But it would enable more than that! If it has become safe to get a &mut reference to a previously pinned value in drop, why limit it to just drop?
Unless I'm missing something, I think that this would make it safe to borrow a &mut reference from a PinMut<T> any time you want. (I'm using PinMut to refer to the what, in current nightly, is called Pin, to avoid confusion with the discussion about shared pins.) PinMut<T> could implement DerefMut unconditionally, rather than just for T: Unpin.
It is unfortunate that we don't seem to have an easy way to similarly provide operations that can work both on &mut and PinMut; after all plenty of code working on &mut has no intention of using
mem::swap.
A DerefMut impl on PinMut would fix that, right? Code that cares about pinning, and requires PinMut, could call code that works on &mut safely and easily. The burden would be placed instead on generic functions that do want to use mem::swap--those would have to have an Unpin bound added to them, or use unsafe and take care not to violate the pin conditions.
Adding such bounds to swap and replace now would break backwards compatibility going back to the very first stable release. I don't see a realistic way to get there from here. But am I missing some other hole in thinking that it would have been the thing to do, if only this were known in the pre-1.0 days?
As it is, I don't see any better solution than what @withoutboats said - keep map unsafe, and put a message in its docs warning people not to move out of any field that was previously pinned in a drop impl.
We can put arbitrary rules on what unsafe code may or may not do, but not so for safe code.
Using unsafe always imposes rules on the surrounding safe code. The good news here is that as far as we know, if a pinnable struct has a method to project a pin to a private field, only its own drop impl can use that to violate the contract in safe code. So it's still possible to add such a projection and present users of that struct with a fully safe API.
Is the definition of Drop really the problem here?
Assigning blame is somewhat arbitrary, there are different things one could change to plug the soundness hole. But do we have agreement that changing Drop as I suggested would fix the issue?
Another way to think about it is to place the blame on mem::swap and mem::replace. These are the operations that let you move something you don't own. Suppose a T: Unpin bound were added to them?
Well, that would make &mut T generally safe to use for !Unpin types. As you observed, we wouldn't even need PinMut any more. PinMut<'a, T> in your proposal could be defined as &'a mut T, right?
This is essentially the ?Move proposal that has been discarded previously due to backwards compatibility and language complexity issues.
Using unsafe always imposes rules on the surrounding safe code.
I am not sure what you mean. Beyond the privacy boundary, this must not be the case; unsafe code cannot impose anything on its clients.
The good news here is that as far as we know, if a pinnable struct has a method to project a pin to a private field, only its own drop impl can use that to violate the contract in safe code. So it's still possible to add such a projection and present users of that struct with a fully safe API.
Yes, types can opt-in to declare the projection safe. But e.g. the borrow checker will not understand that this is a field access, so given a PinMut<Struct> you cannot use such methods to obtain PinMut to two different fields at the same time.
But do we have agreement that changing Drop as I suggested would fix the issue?
I agree, that would fix it.
We wouldn't even need PinMut any more. PinMut<'a, T> in your proposal could be defined as &'a mut T, right?
No, PinMut<'a, T> is still required to promise that the referent will never move again. With &'a mut T you could only trust it not to move for lifetime 'a. This would still be allowed, as it is today:
struct X;
impl !Unpin for X{}
fn takes_a_mut_ref(_:&mut X) { }
fn borrow_and_move_and_borrow_again() {
let mut x = X;
takes_a_mut_ref(&mut x);
let mut b = Box::new(x);
takes_a_mut_ref(&mut *b);
}It would be safe to go from PinMut<'a, T> to &'a mut T but not vice versa - PinMut::new_unchecked would still exist, and still be unsafe.
This is essentially the
?Moveproposal that has been discarded previously due to backwards compatibility and language complexity issues.
As I understand it, the ?Move proposals were trying to get all the way to not needing PinMut, by changing the fundamental language rules to prohibit the code snippet above (by making Unpin be a Move trait.) I'm not proposing anything like that - my proposal is to start at exactly the way it is in nightly now, plus:
- Add an
Unpinbound to thestd::mem::swapandstd::mem::replacefunctions - Remove the
Unpinbound from theDerefMutimpl ofPin(PinMutif that rename happens)
That's all - no fundamental language changes to how moves work, or anything like that. My claim is: yes, this would be a breaking change, but it would have less breaking impact than changing Drop (which in turn is still less drastic than the ?Move proposals), while retaining many of the benefits. In particular, it would allow safe pin projection (at least for private fields, and I think even for public? not quite sure) and prevent situations where parallel code has to be written to work with PinMut and &mut.
No, PinMut<'a, T> is still required to promise that the referent will never move again. With &'a mut T you could only trust it not to move for lifetime 'a.
I see. Makes sense.
As I understand it, the ?Move proposals were trying to get all the way to not needing PinMut, by changing the fundamental language rules to prohibit the code snippet above (by making Unpin be a Move trait.)
Understood.
My claim is: yes, this would be a breaking change, but it would have less breaking impact than changing Drop
Which change to drop do you mean? Adding an Unpin bound? You may be right, but I have no idea how widely used mem::swap and mem::replace are.
In particular, it would allow safe pin projection (at least for private fields, and I think even for public? not quite sure)
I don't see how private vs public can even make a difference here. How would a public field allow less than a private field?
But yeah, this seems to hold together overall. Future would still take a PinMut because it has to rely on things never moving, but it'd have a wider range of methods available for use.
However, the compatibility aspect is a big one. I don't think this is realistic, it'd break all generic code that calls mem::swap/mem::replace. Plus, right now unsafe code is free to implement these methods itself using ptr::read/ptr::write; this could lead to silent breakage of existing unsafe code. That's a no-go, I think.
While we're on the topic of introducing Unpin bound on mem::swap and mem::replace (and not being concerned about breakage). If we assume the "compiler built in" route is taken. Would it be possible to also introduce the same bound on mem::forget to guarantee destructors being run for stack pinned variables making thread::scoped sound and avoid "pre-pooping your pants" in certain cases?
Notice that mem::forget on a PinBox is stlll allowed. The new proposed guarantee related to drop is NOT saying "things don't leak". It says "things don't get deallocated without drop being called first". That's a very different statement. This guarantee does not help thread::scoped.
To add context, moving data out of a struct that implements Future is something that I do commonly. It comes up quite often to need to perform cleanup work if a future was never polled to completion (dropped before polling completed).
So, I most certainly would have hit this land mine when porting existing code to futures 0.3 even with documentation added to map.
@carllerche the map function says quite clearly that you must not use this to move anything. We cannot and do not want to protect against people deliberately moving out of a Pin<T>, but you have to get out of your way (using unsafe code) to do this. I would not call that a landmine.
So, which landmine are you referring to?
@RalfJung I have been trying to figure out the bounds on mapping pinned references myself and I do think this is going to be a huge footgun if it is not resolved soon. I think the first solution is preferable, despite is complexity; not being able to safely project to pinned fields makes it virtually impossible for consumers to actually use APIs that rely on pinning without writing unsafe code.
If this can't be done, I think in practice most usable APIs that use pinning will have to use PinShare. This might not be such a big handicap, I guess, but I'm still not clear on the relationship with Unpin in that case. Specifically: let's say I grab a pinshared reference and get a reference to a field on the type (for a certain lifetime). Can I really rely on it not moving once that lifetime is over? I probably can if the field is !Unpin so maybe it's fine, as long as Pin can't project out--I'm mostly worried about enums. Unless you are saying that even shared pinning cannot be safe without fixing drop--in that case, I think fixing drop to work with pinning essentially has to happen; otherwise it becomes a niche library feature that can't really be used safely, and doesn't (IMO) deserve any pride of place in the core language, even if it does happen to be very useful for Futures.
I should also mention that the only practical API I have for intrusive collections so far (I still need to work out the kinks) needs even stronger guarantees than that; it needs to be able to guarantee that drop isn't called as long as there is any borrow into the collection. I can do this using a GhostCell-style technique, but it feels very awkward and requires the user to do manual memory management (since we have to leak if the backing memory for something in the collection is dropped without being provided the token). So I'm a bit worried that automatic drop itself seems hard to make work with types that use pinning in interesting ways.
Out of curiosity: what are the arguments against adding an Unpin bound to Drop? You cited backwards compatibility, with the alternative being that you'd need to somehow automatically bound the thing that was being Dropped, but Drop already weird type system level restrictions that don't exist for other traits--why is this one so different? It's certainly not as elegant as just making drop take a Pin<T> but we can't actually make that change at this point. Is the issue that we don't actually know what to do if you do call drop on a type that only has a Unpin implementation, when the type itself is !Unpin? I'd guess throwing an exception in the drop implementation in that case might be the correct approach, since anyone who relies on drop running for generic types already needs to handle the panic case. That would mean that it would be very hard to use !Unpin types in practice without a bunch of people updating their code to use the new Drop trait (so the ecosystem would be forced to move everything to thew new version), but I think that I would be okay with that since it would still preserve soundness and not break code that didn't use !Unpin at all. Plus, the "your code panics if the library doesn't upgrade" thing would really incentivize people to move on!
In fact, here's my proposed design:
Extend the Drop trait with a second method that takes Pin, as you proposed. Make the default implementation panic. Make a specialized default implementation where T: Unpin call drop (I assume this would pass the current specialization rules? But even if it wouldn't, Drop can always be special-cased; plus, Drop functions are usually autogenerated). Then, you could magically switch the drop function to call this new drop_pinned (so the drop function as currently written can't be called directly). Now you have exactly the same behavior I proposed above, with no backwards compatibility issues and no attempt to awkwardly auto-derive a bound. It does make the Drop trait excessively strange (since you have a function named drop that you can't call directly and a function not named drop that you call whenever you write drop), but Drop is already so hardcoded and strange that I don't see this as a very big deal compared to the alternative (!Unpin types being essentially impossible to use [not just implement] without writing unsafe code).
It does have the problem that third party libraries will have to upgrade for them to be practically useful with !Unpin types, but like I said this is arguably a good thing. And for probably close to 100% of existing Drop implementations the change would just be a change to the function signature--well, I guess maybe not at the moment because you can't safely project a pinned field, but maybe a macro could be provided for that purpose? In any case, I have no idea how many drop implementations actually mutate their fields in ways that require &mut--most of the ones I can think of that do anything interesting either use unsafe or use interior mutability, but I'm probably not thinking of some common usecases.
The main thing about this approach is that were it to be taken, it'd have to be taken before Pin was stabilized. If you waited until after it was stable, it wouldn't be backwards compatible anymore to make drop implementations start throwing exceptions for generators. This is one more reason I'm really hoping Pin is not rushed in. I don't think we've explored the design consequences sufficiently.
(I do see one more potential issue: ManuallyDrop and unions in general mean that someone could probably have written a destructor over a generic type that did assume that the drop implementation within it couldn't panic, simply because it was never able to run (it also wouldn't be allowed to call any other &mut functions on the generic T, except for ones implemented on unsafe traits that promised not to panic). Since Pining a type already must guarantee that its drop implementation runs before its memory is destroyed [according to the new semantics], I believe the !Unpin type within a ManuallyDrop used for that purpose in existing code could not be considered pinned in the first place, so the existing code should still be correct if it's making that assumption; certainly, you shouldn't be able to safely project a pin into a ManuallyDrop, since it can only be safe if you guarantee its destructor is called prior to the drop. I just don't know how one would communicate this case to the compiler [since whether the !Unpin panic can be propagated without violating semantic expectations depends on whether you decided to call &mut functions other than drop or those on the aforementioned unsafe traits on the interior T, and how is the compiler supposed to know that? Can this take advantage of the "eyepatch" stuff at all, as it seems like it's intended for a similar purpose?]. Maybe you can just say all unions are Unpin by default since it's not generally safe to project a pin into their contents, so the Pin state can't enforce different guarantees from the &mut one; I'm not really sure that flies semantically, but it probably works for the purposes of the Drop implementation for existing code. For new code, hopefully a ManuallyDrop that you could project a Pin into would need to use unsafe code and [semantically] thus need to be marked !Unpin, so you'd still be okay there.
On the subject of eyepatch, though... I'm still not certain of the exact formal definition it has, but if the general idea is that no interesting generic functions are called on T, maybe one could exploit that further? Eyepatch-respecting generics could be known [through the aforementioned requirement that actually pinning a type means drop has to be called before the memory is deallocated] to always call the drop implementation of any !Unpin type that they own, and nothing else. That means that if the drop_pinned implementation for the !Unpin type was implemented, the eyepatch-respecting container would behave correctly. So this would obviate the need for !Unpin to be propagated on generic bounds in these cases. It seems possible to me that one could then compile time failure for !Unpin types that implement Drop, don't implement drop_pinned, and don't eyepatch their generic parameters, in the same way we do when you use non-eyepatch containers with self-referential lifetimes.
Existentials pose a serious backwards-compatibility risk with any compile time strategy though; with self-referential lifetimes, I believe we have a "strictly outlives" requirement that [combined with the fact that existentials carry lifetimes] can enforce drop acyclicity even if you don't know anything about the underlying type, but that's definitely not the case for Unpin, and not allowing drop implementations to compile for Box<Trait> would be a huge breaking change since it would have a lot of false positives. That's why I think a solution that fails at runtime is more realistic. Another alternative would be to require a ?Unpin bound on trait objects that might not implement Unpin; this wouldn't have runtime failure and wouldn't have false positives. However, I get the sense that the Rust team is pretty opposed to new traits of this sort).
Edit: Actually, scratch all the above: we only really need to worry about accessible fields in destructors, not &mut access generally, correct? Because we're only worried about implicit field projections from &mut self.
I may just be reinventing the !DynSized proposal, but essentially: generic containers must already themselves be !Unpin if they expose any methods that care about the pin typestate (I realize this sounds suspiciously like the incorrect parametricity argument, but hear me out!). Existentials like Box<Trait> and &mut Trait don't necessarily reflect their Unpin status, but (at least from staring at the current APIs?) I don't think Pin<Box<T>> and Pin<&mut T> necessarily have to be coercible to Pin<T> where T: !Unpin; not implementing that would mean pinned references to these types won't safely provide pinned access to their interior (note that there is some precedent for this with the relationship of &mut and &: &mut &T is not coercible to &mut T, only &T, and Box<&mut T> is not coercible to Box<T>, only &mut T; in general, when different typestates collide they don't have to be automatically propagated). I do recognize that usually &mut T, Box<T> and T are considered totally interchangeable from a typestate perspective, and this argument doesn't extend to hypothetical inline existentials, but maybe this is the substance of the DynSize proposal (don't allow safe swaps or mem::replaces for trait object values, I guess? Actually, it's already disallowed for them... but I'm assuming there's some reason why this might change in the future). That makes a compile time solution very straightforward: for any structure which (transitively) directly owns (no &mut, &, Box, or raw pointers--none of which would safely transitively propagate Pin access, except & for shared pin, but & can't be moved out of anyway--or if you decided to go with the "can't move out of trait objects" solution, you could also check fields transitively along &mut and Box I think) and has access (in a visibility sense) to a known !Unpin type (including itself), it would have to implement the second kind of drop. That seems to me to be totally fine and not be a backwards compatibility footgun at all, since no stable types are !Unpin--people might have to reimplement destructors after updating a library, but that's all, right? Moreover, internal implementation details would stay internal; only if a !Unpin field were exposed would there be any issues. Finally, all generic container types (not just Vec and standard library stuff, but essentially the entire crates.io ecosystem) would continue to work fine. What is the catastrophic thing I am missing about this solution?
(In fact, it seems to me that even if you couldn't enforce this at drop definition time, you should at least be able to do so at type instantiation time, like dropck does, since you only need to worry about fully instantiated types).
Rereading the Dynsized proposal: I observe that an argument against it was requiring immovable types to always be DynSized even before they are pinned. I argue above that we really only need to worry about this in the case of trait objects; it might be possible (although hard to justify) to enforce that coercing a !Unpin type to a trait required explicitly bounding it with + ?DynSized (or whatever; it could be done automatically, I suppose). While there may well be many cases where the size must be known for !Unpin types (indeed, I have such a use case!), or they must be able to be swapped before they are pinned, I hope there are very few such cases for the interiors of trait objects made from immovable types (the only cases we have now are for stuff like the Box -> Rc conversion that we want to explicitly forbid, right? It's actually not even clear to me that size_of_val being exposed is really an issue here or not, since I still don't know whether you are expected to be able to turn Pin<'a, Box<Trait> into Pin<'a, Trait>, but if you can't we can just rely on Sized of course). One really wants to be able to bound them with !Unpin in any case but as I said, I think people want to avoid adding more negative traits than we already need (personally, I hope that !Unpin trait objects are going to be sufficiently rare and specialized that bounding trait objects with ?Unpin rather than Unpin would be totally reasonable and not infect the type system too much; most of the uses for !Unpin I can think of wouldn't have useful implementations for most existing traits, since they would generally want to perform actions on Pin<self>. You'd also usually want to use them with PinBox or PinTypedArena or whatever at which point ?Unpin bounds look pretty natural).
I have a new design, which I think isn't nearly as awful as the old one: https://github.com/pythonesque/pintrusive/blob/master/src/main.rs. Instead of trying to make pin projections work everywhere, this design asks how we can interoperate with "legacy" Rust code that doens't know anything about pinning, from "modern" Rust code that always wants to support pinning. The obvious answer seems to be using the PinDrop trait @RalfJung proposed, but only for types that both have a custom drop, and want to project fields.
Types explicitly opt into projecting fields (corresponding to deriving the PinFields trait), which is analogous to code written in "modern" Rust; however, it makes no additional requirements on code in "legacy" Rust, instead opting to only support projecting to depth 1 for any given derivation of PinFields. It also does not attempt to move Pin through references, which I believe is probably a good idea not to do anyway. It supports structures and enums, including any disjointness analysis Rust should be able to provide, by generating a structure with identical fields and variants, but with the types replaced with Pin'd references to the types (it should be trivial to extend this to Pin and PinMut when that change is made). Obviously this is not ideal (though hopefully the optimizer can get rid of most of it) but it has the advantage that it works with borrowck and NLL and easily works with enums (as opposed to generated accessors).
The safety argument works by ensuring that either Drop is not implemented for the structure at all, or ensuring that if Drop is implemented for it, it is a trivial implementation that only calls PinDrop (a version of Drop that takes Pin). I believe this rules out all the soundness issues with projecting pinned fields, with one question mark: my main concern is finding a good argument for why disjoint fields on the exact same container (that is, fields at depth 1) that could invalidate each other's pins in their destructors, would already be invalid without pin projections. I think I can justify this if we can show that you could also do the same thing if they were held in separate PinBoxes, which implies that where they live is part of their safety contract; that means their destructors aren't safe in isolation and constructing them outside the module would require unsafe code. In other words, their correctness hinges upon the implementation of the container type, which means that it should be okay to ask more of its destructor than we would for arbitrary safe code.
@pythonesque I didn't really follow what you wrote above about DynSize, but I take it that's all outdated now anyway? So, I am only going to comment on your latest post.
To summarize, you are saying that projecting to a field of a struct/enum (including pub fields) is unsafe in general, but a type can opt-in to safe field projections by not implementing Drop. If the type wants a destructor, it has to PinDrop instead of Drop:
trait PinDrop {
fn pin_drop(self: PinMut<Self>);
}We already have typecheck look for Drop to reject moving out of a field, so it doesn't seem unrealistic to also check for Drop to reject projecting through a &pin. Of course, the "moving out of field" check would still reject if there is a PinDrop, whereas the projection would be allowed in that case.
The compiler would apply the same restrictions to PinDrop that it applies to Drop, plus it would ensure a type does not implement both Drop and PinDrop. When generating drop glue, it calls whatever kind of Drop that the type has implemented.
Does that summarize the proposal? The part I don't understand is your last paragraph, could you give an example demonstrating what you are worried about?
Now, of course I am wondering what the proof obligations are here. I think the easiest way to see this is to say that PinDrop is actually the main and only kind of destructor that formally exists, and impl Drop for T is actually syntactic sugar for impl PinDrop which calls the unsafe method PinMut::get_mut and then calls Drop::drop. This is auto-generated unsafe code, however, because pinning is a "local extension" (i.e., it is backwards compatible with existing unsafe code), that unsafe code is always safe if the given type does not care about pinning.
Slightly more formally speaking, there is a "default pinning invariant" that types have if their author does not care about pinning. Types using that default pinning invariant are automatically Unpin. Writing impl Drop for a type that has a custom invariant asserts that the type uses the "default pinning invariant". This is slightly fishy as it feels a bit like there was a proof obligation here, but there is no unsafe to warn about this -- and indeed this is not perfect, but backwards-compatibility is important so what can you do. It's not a catastrophe either, because one can argue what this really means is that it changes the proof obligation that is incurred on unsafe code elsewhere, to the extend that this unsafe code must work with the "default pinning invariant". If there is no unsafe code elsewhere in this module, everything is fine.
I could even imagine that we could add a lint against impl Drop for T unless T: Unpin, maybe restricted to the case where the module defining the type has unsafe code in it. That would be a place to educate people about the issue an encourage them to either unsafe impl Unpin (formally asserting that they use the default pinning invariant), or else impl PinDrop.
Does that summarize the proposal?
Yes, more or less (I think your proposal is actually significantly more ambitious than mine since it seems to propose automatically performing projections if Drop isn't implemented, which I think is probably a forwards compatibility issue for libraries; but maybe there's some way around that).
The part I don't understand is your last paragraph, could you give an example demonstrating what you are worried about?
Broadly speaking: I'm worried about two fields that live directly on the the same structure, one of which mutates the other when its Drop is called (possibly relying on things like drop order for safety) in a way that violates pinning invariants of the other field, but preserves its structural integrity (thus letting you witness violations of the pinning invariant). This obviously cannot use completely safe code (or else it would get rejected by dropck, among other things), so my concern is only about some hypothetical case where the destructors on the fields' types were safe to run when the fields are pinned in separate structures, but not safe when they are pinned in the same structure. My hope is that there are no such cases unless there is a shared invariant that includes the structure the fields are contained in; if there is such an invariant, we know that it must be aware that its components are not respecting the custom invariant properly and we can therefore blame code somewhere in the module.
I think the easiest way to see this is to say that PinDrop is actually the main and only kind of destructor that formally exists, and impl Drop for T is actually syntactic sugar for impl PinDrop which calls the unsafe method PinMut::get_mut and then calls Drop::drop.
Agreed. Unfortunately this avenue wasn't open to the current solution since it tries to do things in a library.
Slightly more formally speaking, there is a "default pinning invariant" that types have if their author does not care about pinning. Types using that default pinning invariant are automatically Unpin. Writing impl Drop for a type that has a custom invariant asserts that the type uses the "default pinning invariant". This is slightly fishy as it feels a bit like there was a proof obligation here, but there is no unsafe to warn about this -- and indeed this is not perfect, but backwards-compatibility is important so what can you do.
Yes, this is roughly what I have in mind... I'm just worried that I don't have a good intuition for what it would mean to actually formalize this "unsafe code in the module" guarantee. I do like your idea of a lint (I like anything that gets more people using PinDrop, in fact!) but I think unsafe impl Unpin would probably be wrong far too often for it to be a good thing to suggest, at least for types with generic public fields (but then again, for structures that don't have such fields, it would actually be true pretty often... it's largely true of the standard library, for example).
I wrote an example of how #[derive(PinnedFields)] could work: https://github.com/withoutboats/derive_pinned
I've seen people assert that derives like this are "not sound" but afaik that is not true. You would need to use unsafe code to do something that would conflict with the code generated by this derive - that is, this makes other unsafe code unsound (and I think that code - moving around ?Unpin data - is something that you can/should always avoid).
EDIT: Okay, actually read the last few posts about destructors. Will process.
@withoutboats Yeah, I think you've already seen this, but the issue was that unsafe code in a correct !Unpin type could be invalidated by a safe destructor on a type that derived PinFields (so there was no unsafe code in the module for the type that derived PinFields except for the autogenerated stuff). That's the problematic part. Take a look at the design I linked, though (ignoring the stylistic choice to create a separate structure rather than derive individual accessors--that was just me trying to make it support as many use cases as possible with the borrow checker). I was worried for a while, but I am now pretty sure #[derive(PinFields)] can still work, it just requires taking care in making sure that Drop isn't directly implemented.
I also want to bring up another point I've been thinking about (that I haven't yet seen directly resolved anywhere?): I think something that would make Pin much more usable and integrate better into existing code, would be to firmly come down on the side of essentially all of the pointer types being Unpin. That is, making &mut T, &T, *const T, *mut T, Box<T>, and so on all be considered Unpin for any T. While it may seems fishy to allow something like, say, Box<T> to be Unpin, it makes sense when you consider that you can't get a Pin to the interior of the Box out of a Pin<Box<T>>. I think this only allowing !Unpin to infect things that are "inline" is a very reasonable approach--I don't have a single use case for allowing pinning to become viral across references, and it makes the semantics of any eventual &pin type very pleasing (I worked out a table for how it would interact with the other pointer types in that scenario, and essentially if you ignore moves it makes &mut pin act the same as &mut, &pin act the same as &, and box pin act the same as box with respect to the relationship with other pointers). It's also not operationally important, as far as I can tell: in general moving a value of type A containing a pointer to a value of type B doesn't move the pointed-to value of type B, unless the value of type B is inline in the value of type A--but if that's the case, then A is automatically !Unpin since it contains B without a pointer indirection. Perhaps most importantly, it would mean that a large percentage of the types that would currently need a manual unsafe implementation of !Unpin would not need one, since most collections only hold a T behind a pointer indirection. That would both allow the current Drop to continue working, and allow these types to implement PinDrop without changing their semantics (since if a type is Unpin it can treat a Pin'd argument as &mut anyway).
I may be missing some reason why this sort of transitive pinning would be a good idea, but thus far I haven't found any. The only operational thing I might be worried about is whether it is actually possible to implement this behavior with auto traits--I think it probably would, but maybe in some cases where people are using PhantomData<T> but actually have an owned pointer to T, it would be good for them to change it to PhantomData<Box<T>>. Typically we think of those as being exactly the same semantically, but with pinning that's not completely true.
@pythonesque The terminology of "a new language" and such is kind of hazing the situation to me. My impression of what you do is:
- By default,
#[derive(PinFields)]generates a no-opDropimpl. This guarantees that you do not ever access the pinned field in the destructor. - An optional attribute changes this
Dropimpl to callPinDrop::pin_drop, and you're supposed to implementPinDrop.
Is this right?
I also believe that this whole thing only mattered if we extended the guarantees of Pin to support intrusive collections. Is this consistent with your understanding @RalfJung and @pythonesque?
All of this is rather frustrating, because what seems clear is that Drop just should take self by Pin! Making a more radical (possibly epochal) change seems appealing, but I don't see a way that isn't hugely disruptive.
Here's an example of the sort of unsoundness access to pinned fields + drop can cause: https://play.rust-lang.org/?gist=8e17d664a5285e941fe1565ce0eca1ea&version=nightly&mode=debug
The type Foo is passing an internal buffer to some sort of external API that requires the buffer to stay where it is until it is explicitly unlinked. As far as I am aware this is sound under the constraints that @cramertj proposed, after a Pin<Foo> is created you are guaranteed that it is not moved until after Drop has been called on it (with the proviso that it could instead be leaked and Drop is never called, but in that case you are guaranteed it will never be moved).
The type Bar then breaks this by moving the Foo during its Drop implementation.
I am using a very similar structure to Foo to support a radio peripheral that communicates via DMA, I can have a StableStream with an internal buffer that is written to by the radio.
Is this right?
Yes, except that it doesn't actually generate a no-op Drop impl (because types that don't implement Drop in Rust work better in general). Instead it tries to assert that Drop hasn't been implemented using some fishy library features (it works on stable but breaks under specialization--I think there's a variant that should work under specialization but doesn't right now because of issues with associated constants). If it were made a language feature it would be pretty easy to enforce this.
I also believe that this whole thing only mattered if we extended the guarantees of Pin to support intrusive collections. Is this consistent with your understanding @ralfj and @pythonesque?
No, that's not the case, unfortunately. The counterexample linked above has nothing to do with the extra guarantees for intrusive collections. A Pin'd type already has to be able to assume that it won't move before it's used again even without that guarantee, since if a method is called twice on a value from behind a pinned reference the value has no way of knowing whether it moved between the two calls. The extra guarantees that are needed to make it useful for intrusive collections add an additional thing about having to call drop before the memory is freed, but even without that guarantee drop can still be called on something that's currently pinned (behind a PinBox, for instance). If the thing that's dropped happens to include inline fields, and we allow projecting the pin from the thing that's dropped to those fields, then the destructor of the outer type can still move the inner field, pin it again (by putting the moved-out field value in a PinBox, for instance), and then call methods on it that are expecting references from when it was pinned before to still be valid. I don't really see any way around it; as long as you can implement drop, you can have this problem for any inline !Unpin field. That's why I think the least bad solution is to not let people implement drop if they want to pinning to work correctly.
All of this is rather frustrating, because what seems clear is that Drop just should take self by Pin! Making a more radical (possibly epochal) change seems appealing, but I don't see a way that isn't hugely disruptive.
Yeah, it really is annoying... I was very sulky about it for about a week. But as Ralf pointed out, we would've had to wait three more years for 1.0 before anyone figured this out... and there will always be something more.
If the thing that's dropped happens to include inline fields, and we allow projecting the pin from the thing that's dropped to those fields, then the destructor of the outer type can still move the inner field and then call methods on it that are expecting references from when it was pinned to still be valid.
The emphasized portion seems like a very big deal to me; in fact, it seems like the crux of the issue.
Initially I imagined this code, which uses our only method we have that actually cares about internal addresses:
struct TwoFutures<F>(F, F);
impl Drop for TwoFutures {
fn drop(&mut self) {
mem::swap(&mut self.0, &mut self.1);
unsafe { Pin::new_unchecked(&mut self.0).poll() }
}
}But it involves unsafe code to get back to the Pin! That code should just be considered unsound.
Can we rule out the ability for methods that take &self and &mut self to rely on the validity of internal addresses for soundness? What does that look like?
@withoutboats Even if only methods that take Pin<Self> rely on validity of internal addresses for soundness, you can still have the issue. The destructor of the outer type can mem::replace the field of the inner type (using its own &mut self reference), then PinBox::new the value, and then call a pinned method on it. No unsafe required. The only reason it's not an issue without being able to project pinned fields is that it's considered acceptable for a type that actually implements weird !Unpin methods using unsafe (or shares an invariant with a type that does) to have proof obligations on its drop implementation even if it only uses safe code there. But you can't ask that of types that just happen to contain a type that is !Unpin and have no unsafe code of their own.
I think your proposal is actually significantly more ambitious than mine since it seems to propose automatically performing projections if Drop isn't implemented, which I think is probably a forwards compatibility issue for libraries; but maybe there's some way around that
Well I think we want to do that eventually. How is that a compatibility issue?
I'm worried about two fields that live directly on the the same structure, one of which mutates the other when its Drop is called (possibly relying on things like drop order for safety) in a way that violates pinning invariants of the other field, but preserves its structural integrity (thus letting you witness violations of the pinning invariant).
But that would currently be illegal already. You must not violate other's invariants.
but I think unsafe impl Unpin would probably be wrong far too often for it to be a good thing to suggest, at least for types with generic public fields
I think it will actually be correct most of the time -- I expect most people will not provide any accessors for Pin<Self>, and in that case they are likely using the default pinning invariant and hence it is okay to unsafe impl Unpin for them.
I think something that would make Pin much more usable and integrate better into existing code, would be to firmly come down on the side of essentially all of the pointer types being Unpin. That is, making &mut T, &T, *const T, *mut T, Box, and so on all be considered Unpin for any T. While it may seems fishy to allow something like, say, Box to be Unpin, it makes sense when you consider that you can't get a Pin to the interior of the Box out of a Pin<Box>.
I agree that this will likely happen (also see my comment at #49621 (comment)). Currently I feel we should wait a bit until we feel more confident in the entire pinning thing, but indeed I see very little point in enforcing pinning beyond pointer indirections.
I am not sure how I feel about doing this to raw pointers though, we are usually extremely conservative about auto traits for them as they are used in all sorts of crazy ways.
I wrote an example of how #[derive(PinnedFields)] could work: https://github.com/withoutboats/derive_pinned
@pythonesque already said this, this but just to be clear: That library is unsound. Using it with @MicahChalmer's drop issue I can break any pinned type that actually relies on pinning. This is independent of the additional guarantee about drop being called. Let me know if an example is still needed. ;)
That library is unsound.
To clarify, the derive is just unsafe, and can't be used in combination with a manual Drop impl. By using the derive, you promise not to do any of the bad things listed in an implementation of Drop.
#50497 changes the pinning API, most importantly it renames Pin to PinMut to make room for adding a shared Pin in the future.
To clarify, the derive is just unsafe, and can't be used in combination with a manual Drop impl.
Agreed, making it unsafe that way would work. Though the test makes it look like currently it's not actually marked unsafe?
@RalfJung Right, I don't think it is at the moment. Another option i just thought of would be to have the "safe" version create an implementation of Drop for the type, blocking other manual impls of Drop. There could be an unsafe flag to turn this off. WDYT?
@cramertj yes that should also work. It would however have side-effects like more restrictive dropck and not being able to move out of fields.
@pythonesque and I had a call on Monday to discuss this. Here's a summary.
We concluded that probably the "correct" behavior would have been for Drop to take self by pin. However, transitioning to that is daunting. Though it is possible with an edition change in some form I belief, this would be extremely disruptive.
A backwards compatible change is to just make it somehow incoherent for types that can be "pin projected" to implement Drop. In his repository, @pythonesque has implemented this by generating impls from a convoluted set of traits.
One could imagine a built-in form that is a bit simpler:
- A marker trait, let's call it
PinProjection, controls whether or not a type can be pin projected. It is incoherent (through compiler built-in magic) to implement bothPinProjectionandDrop. - Another trait,
PinDrop, extendsPinProjectionto provide an alternative destructor toDrop.
Derives for generating pin projection methods like the ones @pythonesque and I have shown will also generate impls of PinProjection for the type.
We concluded that probably the "correct" behavior would have been for Drop to take self by pin. However, transitioning to that is daunting. Though it is possible with an edition change in some form I belief, this would be extremely disruptive.
Just wondering, IF we wanted to do such a thing one day, is what you're proposing future-compatible?
For example, let's say we decided to...
- make
Dropmagically accept both Pinned and non-Pinned forms, OR - rewrite everyone's
Dropsignatures for them as part of the automatic upgrade to Rust 2021 :)
(I'm not proposing either of these, but it seems hard to answer the question without concrete examples.)
@tmandry That's essentially the idea.
The big problem is a generic Drop implementation which moves the type parameter:
struct MyType<T>(Option<T>);
impl<T> Drop for MyType<T> {
fn drop(&mut self) {
let moved = self.0.take();
}
}The sort of automatic upgrade you're talking about would just break this Drop impl, which is only valid if we add a bound that T: Unpin.
I don't know of any valid use cases for actually moving ?Unpin data around in a destructor, so its just this case, in which it technically could but isn't meant to, that is really a problem.
Another trait, PinDrop, extends PinProjection to provide an alternative destructor to Drop.
Why does PinDrop extend PinProjection? That seems unnecessary.
Also this could maybe be made slightly smarter by saying that PinProjection and Drop are incompatible unless the type is Unpin (or finding some other way of removing all these new distinctions/restrictions for Unpin types).
@RalfJung we'd want PinDrop and Drop to be mutually exclusive somehow. Ultimately there are a few ways to implement this with different trade offs; I think we need an RFC for it regardless because its not a small change.
@withoutboats Agreed. I figured the exclusivity could arise from a special treatment of PinDrop, but there are obviously several ways to do that. I think it'd be very useful if Unpin types do not have to care; together with making all pointer types Unpin unconditionally this will probably help reduce the number of cases where people even have to know about this.
@withoutboats Also worth noting that the main instances I can think of where people want to, say, move generic data around in a Vec in a destructor (I chose Vec because IIRC people would like to implement methods on Vec that actually care about Pin, which means it couldn't unconditionally implement Unpin), it's actually a &mut Vec<T> or Vec<&mut T> or something. I mostly bring that up because these are cases that would probably be tripped up by the lint @RalfJung suggested, and hopefully people will therefore be able to move to PinDrop easily in those cases rather than unsafe impl Unpin (theoretically they always can do the former, of course, by bounding T with Unpin in the type, but that will be a breaking change to clients of the library).
Also, it is worth noting that while this does add a few more traits than we'd like, the traits are ones that will pretty much, never, ever show up in anyone's type signature. In particular PinProjection is a completely pointless bound unless you're writing a #[derive(PinFields)] macro because the compiler will always (I believe) be able to determine whether PinProjection holds if it can figure out what the fields on the type are, and the only thing it's useful for is projecting fields. Similarly, PinDrop should basically never need to be a bound for anything, for the same reason that Drop is almost never used as a bound. Even trait objects should be largely unaffected (unless someday we get "associated fields" I guess, but with a new feature like that we could mandate that traits with associated fields were only implementable on PinProjection types, which would neatly solve that problem).
Well I think we want to do that eventually. How is that a compatibility issue?
I suppose it's not more of one than adding a type that unimplements Send or Sync, the difference being that neither of those affect the core language to quite the same extent that this would. Doing it completely automatically seems analogous to how Rust used to treat Copy (types were always Copy if they only contained Copy types and didn't implement Drop) which was eventually changed to make it explicit because, presumably, people didn't like the fact that adding a trait (Drop) could break generic code without them realizing it (since the whole point of coherence is that additional trait implementations shouldn't break downstream crates). This seems almost identical, just with PinProjection instead of Copy. I actually liked the old behavior, I just think it would be hard to justify PinProjection working this way when Copy doesn't.
But that would currently be illegal already. You must not violate other's invariants.
Yeah, the more I think about this, the less plausible it seems that it could be a problem.
I think it will actually be correct most of the time
Well, yes, but only because most types don't implement any methods that require Pin or expose their generic fields as public. While the latter is unlikely to change, the former is not--I expect at least some of the stdlib types that are currently !Unpin to add pin projecting methods, at which point the unsafe implementation wouldn't be valid anymore. So it seems like a pretty fragile thing to me. Moreover, I am worried about increasing the amount of "boilerplate" unsafe code that people have to write; I think Send and Sync bounds are unsafe impl'd correctly about as often as they're implemented incorrectly, and Unpin would be even more insidious because the usually correct version doesn't have any bounds on T. So it seems vastly preferable to steer people towards PinDrop (I do understand why you're wary of doing it for raw pointer types, though. I'm just worried that already-unsafe code will be even more likely to do an unsafe impl without thinking, but the more I think about it the more it seems like the default for raw pointers is probably correct and flagging it with your lint would be useful).
I think it'd be very useful if Unpin types do not have to care.
I sort of agree, but since like I said people won't be using PinProjection as an actual bound, I'm not sure how much it would matter in practice. There's already a DerefMut implementation for PinMut where T: Unpin so you wouldn't get any benefit out of it today. A rule implemented in the compiler (for projections) would presumably turn PinMut::new into a kind of &pin reborrow for Unpin types, but that has nothing to do with field projections really. And since deriving PinProjection doesn't require PinProjection for its fields, you wouldn't need it just to satisfy bounds on a derive on another type. So really, what is the gain? The only thing it would really let you do is implement Drop and derive PinProjections at the same time, but we would always want people to implement PinDrop if possible anyway so that would be a net negative IMO.
I chose Vec because IIRC people would like to implement methods on Vec that actually care about Pin, which means it couldn't unconditionally implement Unpin
Hm, I don't think I like that. If Box is unconditionionally Unpin, I feel Vec should be the same. The two types are usually pretty much equivalent in their ownership.
Also we have to be careful about the drop guarantee; Vec::drain for example can lead to things being leaked in case of a panic.
This seems almost identical, just with PinProjection instead of Copy
Oh, now I understand your question. I was not actually talking about auto-deriving PinProjections as my proposal did not have a trait like that -- but indeed one consequence of my proposal would be that adding Drop is a breaking change if you have public fields.
The only thing it would really let you do is implement Drop and derive PinProjections at the same time, but we would always want people to implement PinDrop if possible anyway so that would be a net negative IMO.
That was actually the point. The less people have to care about all this pinning stuff, the better.
Clarification question: In your and @withoutboats's proposal, is PinDrop a lang item and treated by the compiler as a replacement for Drop, or is it just the trait used by derive(PinProjections) to implement Drop?
Also we have to be careful about the drop guarantee; Vec::drain for example can lead to things being leaked in case of a panic.
Well, that's true, but it doesn't actually deallocate any memory, correct? To be clear, I'd actually prefer if Vec did unconditionally implement Unpin since it would make it easier for people to just switch to PinDrop, but there was definitely talk in some of the pinning discussion about letting you pin to backing elements. Once you start talking about fields being pinned, it becomes clear that you can't always just turn the Vec into a Box<[T]> in place and then pin it, so it might actually have some value at that point (though obviously you could also add a PinVec type instead of a Vec type, as was done for Box).
That was actually the point. The less people have to care about all this pinning stuff, the better.
Hm, from my perspective that would be true initially, but in the long run we'd want to migrate as many people as possible over to a default of PinDrop, especially if the type was actually bothering to #[derive(PinProjections)] (which, again, wouldn't even be needed for any purpose if the type was Unpin--it would probably only happen in stuff like generated code). Then (maybe after &pin had been in the language for a while) you could have an epoch change that switched Drop over to DeprecatedDrop or something. For Unpin types just changing the type signature from &mut to &mut pin would pretty much solve everything though, so maybe this isn't really needed.
Clarification question: In your and @withoutboats's proposal, is PinDrop a lang item and treated by the compiler as a replacement for Drop, or is it just the trait used by derive(PinProjections) to implement Drop?
The former.
This whole Drop discussion seems like a pretty drastic oversight on behalf of the original RFC. Would it be valuable to open a new RFC to discuss this? It's somewhat hard to follow what the concerns are, which concerns are more pernicious than others, exactly how much more machinery we're going to be needing to add to the language than we originally thought, how much breakage we ought to expect (and whether it can be mitigated by the edition) in the worst and best case scenarios, how we might transition to anything that succeeds Drop, and whether we could punt on all this and still meet our goals for 2018 via some other means (like how rust-lang/rfcs#2418 suggests hiding Pin from all public APIs in order to not block on stabilization).
@bstrie I think there's only one major concern, but it's a pretty substantial one. The RFC was approved with the understanding that an eventual &pin mut type could at some point make this sort of code safe:
struct Foo<T> { foo : T }
fn bar<T>(x: &pin mut Foo<T>) -> &pin mut T {
&pin mut x.foo
}It's important for this kind of code to be safe because in practice, pinned references aren't really composable otherwise in safe code, so for example implementing any of the futures combinators would require the use of unsafe. At the time, it was thought that the use of unsafe could be lifted in most cases and therefore it wouldn't be a big deal.
Unfortunately whether this is safe depends on the Drop implementation for Foo. So if we want to support safe field projections (be it via a language feature, a custom #[derive], or any other mechanism) we have to be able to guarantee that such bad Drop implementations can't happen.
The alternative that seems to work best both enables the above code to work without the use of unsafe (with only the addition of a #[derive(PinProjections)] on top of the structure) and does not break any existing code. It can be added backwards-compatibly even if Pin is already stabilized and can even be added as a pure library (with a severe ergonomic hit). It is compatible both with #[derive] macros to generate accessors and an eventual &pin native reference type. While it requires adding at least one new trait (and possibly two, depending on how the pinned version of Drop is implemented), they never need to appear anywhere in where clauses or trait objects, and the new version of drop only needs to be implemented for types that currently have a Drop implementation and want to opt into pin projections.
Just because a solution seems like it doesn't have many downsides doesn't mean it is not a substantial change, so I think that we will almost certainly create an RFC for this proposal (@withoutboats and I discussed doing this on the call). It would be fine to punt on it in isolation, since it's fully backwards-compatible. Personally, though, I think it makes more sense to push through this change rather than get rid of pinning elsewhere.
Most people's concern with PinMut in public APIs is precisely that it will require either unsafe or threading through Unpin bounds everywhere, and this proposal solves that issue. The alternatives discussed in rust-lang/rfcs#2418 seem much more controversial, both in the actual mechanics of the way it wants to avoid dealing with pinning (which involves the proliferation of various other traits that will appear in public APIs and documentation) and in the overall complexity of the solution. Indeed, even if pinning were completely resolved, I think there are a number of questions that people don't feel have been adequately resolved with that RFC, so I think there's at least a decent chance that an RFC adding safe pin projections could end up being accepted before it.
It's true that pinning is in its early days (and I know I complained about it seeming like it was being stabilized much too quickly), but I believe the lack of safe field projection is the last major thing that discourages people from using it at all. For the sake of completeness, here are all the issues I've seen people raise with pinning, their proposed solutions, and whether the solution is backwards compatible with the existing Pin type (in very rough, biased order of how controversial I perceive the issue to be at the moment):
-
This one (can we make projecting fields be done in safe code?). To be resolved by the upcoming RFC (where possible implementation strategies will be spelled out, as well as other alternatives we considered and why they had to be discarded). All variations of the proposed resolution are backwards compatible.
-
Should pinning a value of
!Unpintype with a manual destructor imply an additional guarantee (that the value's backing memory is not invalidated until after the destructor is called), aka "changes for intrusive collections"?Still unresolved, mostly because it could break the proposed stack pinning API; if a stack pinning API that preserves this guarantee can be made to work with async / await most stakeholders seem willing to accept this (IIRC someone already tried to solve this using generators but rustc ICE'd).
Not backwards compatible with the old guarantees; requiring unsafe code to enforce the guarantee for now is compatible with both possible outcomes, but only if the unsafe code isn't allowed to rely on it being enforced for correctness. Thus, this certainly requires resolution one way or the other before pinning is stabilized.
Fortunately, users of pinning for
Futures can ignore this besides the shape of the stack pinning API. The closure-based stack pinning API is compatible with either resolution, soFutureusers not using async / await can use the closure-based API today without waiting for this to be decided. The decision most impacts people who want to use pinning for other purposes (like intrusive collections). -
Should raw pointers be unconditionally
Unpin? Still unresolved (I think I'm the only one who proposed it and I am still pretty much 50-50 on it). Would not be backwards compatible; I am marking this as controversial primarily for that reason. -
Should standard library types like
Vecbe made unconditionallyUnpin, or should they havePinfield accessors added? Still unresolved, and may need resolution on a case by case basis. For any given safe wrapper type, either adding the accessors or making the type unconditionallyUnpinis backwards compatible. -
Should
PinMut::derefbe removed? The answer basically seems to be "no" since the advantages of keeping it seem to far outweigh the disadvantages, and there seem to be workarounds for the cases that originally made people want it (specifically,Pin<RefCell<T>>). Changing it would be backwards incompatible. -
How should we provide field accessors in the short term (ignoring the drop issues)? Still unresolved: two options presented so far are https://github.com/withoutboats/derive_pinned and https://github.com/pythonesque/pintrusive. Resolution is fully backwards compatible, since after the drop changes this can be done soundly in a macro purely in library code.
-
How should we provide field accessors in the long term (i.e. should there be custom
&mut pinand&pinRust types? How should reborrowing work?). Still unresolved, but backwards compatible with all other proposed changes (to the best of my knowledge) and can obviously be punted indefinitely. -
Should safe reference-like types should be unconditionally
Unpin? Seems to be resolved (yes, they should). Resolution is fully backwards compatible. -
Should we have a shared
Pintype in addition to a uniquePinone, in order to make field accessors feasible (it doesn't work with&Pinbecause that's a reference to a reference)? Resolution was changingPintoPinMutand adding aPintype for the shared case. This was not backwards compatible, but the breaking change (PintoPinMut) was already made, and I'm pretty sure this has effectively been accepted already.
I think that pretty much covers it. As you can see all of them (besides the raw pointers and deref decision, the latter of which seems to be largely resolved at this point) have some backwards-compatible path forwards, even if pinning were stabilized today; more importantly, the fact that we can resolve field projections means that using a pinned type in your API isn't destined to be a decision you later come to regret.
In addition to the above questions, there have been other proposals that aim to rethink pinning in a much more fundamental way. The two that I think I understand the best are the @comex proposal to make !Unpin types !DynSized, and the steven099 (on internals; sorry, I don't know the Github name) proposal to have a new unsized Pinned wrapper type that makes the internals immovable (sort of like a ZST wrapper).
The !DynSized option is a fairly conservative feature (in the sense that Rust already has a similar trait available) that has the advantage that it may already be needed for dealing with opaque types. In this sense, it may be even less invasive than the proposed changes to Drop. It has a high upside as well: it automatically solves the issue with Drop because !Unpin types would be !DynSized and therefore one would not be able to move out of them. This would make &mut T and &T automatically function as PinMut and Pin wherever T was !DynSized, so you wouldn't need a proliferation of pinned versions of types and methods working on &mut T could often be made to work normally (if they were amended to not require a DynSized bound when they didn't need one).
The major downside (besides the usual concerns around ?Trait) seems to be that a !Unpin type could never be moved, which is quite different from the situation with pinned types currently. This means that composing two pinned types without using a reference would not really be possible (as far as .I can tell, anyway) and I'm not sure how or whether there is any proposed resolution to this; IIRC this was intended to work alongside a &move proposal, but I am not sure what the intended semantics of that are. I also don't understand (from the proposal) how you could have safe field projections with it, since it relies on opaque types; it seems like you'd have to use a lot of unsafe code in general to use it.
The unsized Pinned<T> type is somewhat similar in spirit, but wants to get around the above problem by allowing you to wrap a type in a ZST that makes it immovable (effectively acting unsized). Rust doesn't have anything quite comparable at the moment: PhantomData doesn't actually include an instance of the type, and the other dynamically sized types generate fat pointers and still allow moves (using APIs relying on size_of_val; this is what ?DynSized was intended to fix, so this proposal probably piggybacks on that trait again). It doesn't seem to me like this proposal actually fixes the Drop issue if you allow safe projections, and it also seems incompatible with Deref, so to me the advantages over Pin are not so clear.
if a stack pinning API that preserves this guarantee can be made to work with async / await most stakeholders seem willing to accept this (IIRC someone already tried to solve this using generators but rustc ICE'd)
For reference, this is probably refererring to #49537 which is from this attempt at a nested generator based stack pinning API. I'm not certain whether the lifetimes there would work even if the ICE is resolved though.
Still unresolved, mostly because it could break the proposed stack pinning API; if a stack pinning API that preserves this guarantee can be made to work with async / await most stakeholders seem willing to accept this (IIRC someone already tried to solve this using generators but rustc ICE'd).
I see the closure-based stack pinning API as a solution to this, with a future avenue (once &pin is a language primitive) for something more ergonomic and checked by the compiler. There is also this macro-based solution proposed by @cramertj.
Should raw pointers be unconditionally Unpin? [...] Would not be backwards compatible; I am marking this as controversial primarily for that reason.
Why do you think that adding an impl Unpin for Vec<T> is backwards compatible but doing the same for raw pointers is not?
The major downside (besides the usual concerns around ?Trait) seems to be that a !Unpin type could never be moved, which is quite different from the situation with pinned types currently. This means that composing two pinned types without using a reference would not really be possible (as far as .I can tell, anyway) and I'm not sure how or whether there is any proposed resolution to this; IIRC this was intended to work alongside a &move proposal, but I am not sure what the intended semantics of that are.
Well, under @Steven099's variant proposal (which I preferred), most users would (for now) use a Pinned<T>, which contains T by value but is !Sized (and maybe !DynSized; exact design for the trait hierarchy is open to bikeshedding). This actually ends up looking very similar to the existing proposal, except with &'a mut Pinned<T> standing in for Pin<'a, T>. But it's more composable with current and future code that's generic on &mut T (for T: ?Sized), and it's more backwards-compatible with a future design for true, native immovable types that wouldn't have to use Pinned.
To go into more detail, Pinned could look like this:
extern { type MakeMeUnsized; }
#[fundamental]
#[repr(C)]
struct Pinned<T> {
val: T,
_make_me_unsized: MakeMeUnsized,
}You wouldn't generally construct a Pinned<T> directly. Instead, you'd be able to cast from Foo<T> to Foo<Pinned<T>>, where Foo is any smart pointer that guarantees it won't move its contents:
// This would actually be a method on Box:
fn pin_box<T>(b: Box<T>) -> Box<Pinned<T>> {
unsafe { transmute(b) }
}(There could also be a stack pinning API, probably based on a macro, but that's a bit more complicated to implement.)
In this example, FakeGenerator stands in for a compiler-generated generator type:
enum FakeGenerator {
Initial,
SelfBorrowing { val: i32, reference_to_val: *const i32 },
Finished,
}Once a FakeGenerator value is pinned, user code must no longer be able to access it directly by value (foo: FakeGenerator) or even by reference (foo: &mut FakeGenerator), since the latter would allow using swap or replace. Instead, user code works directly with, e.g., &mut Pinned<FakeGenerator>. Again, this is very similar to the rules for the existing proposal's PinMut<'a, FakeGenerator>. But as an example of better composability, the compiler-generated impl can use the existing Iterator trait, rather than needing a new one that takes Pin<Self>:
impl Iterator for Pinned<FakeGenerator> {
type Item = i32;
fn next(&mut self) -> Option<Self::Item> {
/* elided */
}
}On the other hand, for initial construction, we can hand out FakeGenerator values directly and allow them to be moved around, as long as we guarantee that only non-self-borrowing states are accessible before being pinned:
impl FakeGenerator {
fn new() -> Self { FakeGenerator::Initial }
}So, if we want to compose two FakeGenerators by value, construction is straightforward:
struct TwoGenerators {
a: FakeGenerator,
b: FakeGenerator,
}
impl TwoGenerators {
fn new() -> Self {
TwoGenerators {
a: FakeGenerator::new(),
b: FakeGenerator::new(),
}
}
}Then we can pin the TwoGenerators object as a whole:
let generator = pin_box(Box::new(TwoGenerators::new()));But, as you mentioned, we need field projections: a way to go from &mut Pinned<TwoGenerators> to &mut Pinned<FakeGenerator> (accessing either a or b). Here too, this ends up looking very similar to the existing Pin design. For now, we would use a macro to generate accessors:
// Some helper methods:
impl<T> Pinned<T> {
// Only call this if you can promise not to call swap/replace/etc.:
unsafe fn unpin_mut(&mut self) -> &mut T {
&mut self.val
}
// Only call this if you promise not to call swap/replace/etc. on the
// *input*, after the borrow is over.
unsafe fn with_mut(ptr: &mut T) -> &mut Pinned<T> {
&mut *(ptr as *mut T as *mut Pinned<T>)
}
}
// These accessors would be macro-generated:
impl Pinned<TwoGenerators> {
fn a(&mut self) -> &mut Pinned<FakeGenerator> {
unsafe { Pinned::with_mut(&mut self.unpin_mut().a) }
}
fn b(&mut self) -> &mut Pinned<FakeGenerator> {
unsafe { Pinned::with_mut(&mut self.unpin_mut().b) }
}
}However, just like the existing design, the macro would need to prevent the user from implementing Drop for TwoGenerators. On the other hand, ideally the compiler would allow us to impl Drop for Pinned<TwoGenerators>. It currently rejects this with an error that "Implementations of Drop cannot be specialized", but that could be changed. IMO this would be a bit nicer than PinDrop, since we wouldn't need a new trait.
Now, as a future extension, in principle the compiler could support using native field syntax to go from Pinned<Struct> to Pinned<Field>, akin to the suggestion under the existing design that the compiler could add a native &pin T. This might be a good idea, as it wouldn't require much implementation work, relatively speaking.
However, my ideal "endgame" is not that, but something more dramatic (and more complex to implement) where the compiler would someday support "native" immovable types, including existential lifetimes. Something like:
struct SelfReferential {
foo: i32,
ref_to_foo: &'foo i32,
}These types would behave like Pinned<T>, in being !Sized etc. [1] However, unlike with Pinned, they wouldn't require an initial movable state. Instead, they would work based on "guaranteed copy elision", where the compiler would allow using immovable types in function return values, struct literals, and various other places, but guarantee that under the hood, they'd be constructed in place. So not only could we write:
let sr = SelfReferential { foo: 5, ref_to_foo: &sr.foo };(which you can already sorta kinda do)... we could also write things like:
fn make_self_referential() -> SelfReferential {
let sr = SelfReferential { foo: 5, ref_to_foo: &sr.foo };
sr
}or
let sr: Box<SelfReferential> = box SelfReferential { foo: 5, ref_to_foo: &sr.foo };Of course, the above is only a very rough sketch of what the feature would look like; the syntax I used has issues, and syntax would be the least of the many complexities involved in designing the feature. (That said, I've thought about it enough that I'm pretty confident the complexities can be worked out – that it's not just an incoherent idea that can't work.)
I'm just mentioning it as part of the motivation, back in the present, for using a !DynSized-ish design instead of the existing Pin design. I think &'a Pinned<T> is already better than Pin<'a, T> because it avoids a combinatorial explosion of pointer types. But it does inherit some of the same problems:
- It fundamentally doesn't support types that lack an initial movable state, and
- It's noisy, confusing (the difference between pinned and non-pinned references to the same type is hard to explain), and it makes immovable types feel second-class. I want immovable, self-referential types to feel first-class – both because they're inherently useful, and to make Rust look better to people coming from other languages, where making self-referential values is easy and common.
In the future I'm envisioning, native immovable types would solve both problems, and most code would never need to use the word "pin". (Though Pinned might still have some use cases, for cases where copy elision isn't good enough and you really do need an initial movable state.) In contrast, Pin bakes the concept of a separate "not-yet-pinned" state into the design of every trait that uses it. And a built-in &pin would bake it into the language.
Anyway, here is a playground link for the Pinned design above.
[1] ...though we may want a new trait ReallySized (with a less silly name), for types that are statically sized but may or may not be movable. Thing is, there's going to be some churn here anyway, because with support for unsized rvalues, many functions that currently take Sized arguments could work just as well with unsized-but-movable ones. We'll be changing both the bounds of existing functions, and recommendations for what bounds to use for future functions. I claim it might even be worth having a future edition change the default (implied) bound for function generics, although this would have some downsides.
[edit: fixed code example]
I see the closure-based stack pinning API as a solution to this, with a future avenue (once &pin is a language primitive) for something more ergonomic and checked by the compiler. There is also this macro-based solution proposed by @cramertj.
The closure-based one doesn't work properly with async/await, I believe, because you can't yield from within a closure. The macro-based one is interesting, though; if that's really safe, it's pretty ingenious. I didn't think it could work at first because a panic during one of the drops in scope could lead to the others leaking, but apparently that's been fixed with MIR?
I was also not sure about the interaction between the "destructors run before memory is deallocated" guarantee and the ability to project pinned fields; if the top level drop panicked, I had thought that projected pinned fields in the value would not have their drops run. However, on the Rust playground it actually does seem that the fields of the type have their drops run anyway even after the top level type panics, which is quite exciting! Is that guarantee actually documented anywhere? It seems necessary if stack pinning is to work with pin projections (that, or something more heavyweight, like PinDrop always aborting on panic, which seems undesirable since it would introduce a functionality difference between Drop and PinDrop).
Why do you think that adding an impl Unpin for Vec is backwards compatible but doing the same for raw pointers is not?
I see your point: someone could be relying on the auto !Unpin implementation from an owned Vec<T> field and implementing their own accessors that assumed pinning was transitive for some particular !Unpin T. While it's true that PinMut<Vec<T>> doesn't actually provide you with any safe way to get a PinMut<T>, unsafe code could still exploit PinMut::deref to get raw pointers out and make assumptions about the pointers being stable. So I guess this is another situation where it's backwards compatible only if unsafe code doesn't rely on !Unpin being transitive through Vec (or whatever). However, that kind of heavy reliance on negative reasoning seems fishy to me anyway; if you want to make sure you're !Unpin where T isn't you can always add a PhantomData<T> (I guess this argument also applies to raw pointers). A blanket statement like "it's UB to assume types in the standard library are !Unpin in unsafe code, regardless of their type parameters, unless the type explicitly opts out or its documentation declares that this can be relied upon" would probably suffice.
The macro-based one is interesting, though; if that's really safe, it's pretty ingenious. I didn't think it could work at first because a panic during one of the drops in scope could lead to the others leaking, but apparently that's been fixed with MIR?
It would be a bug in MIR if panic-during-drop would lead to skipping the drop of the remaining local variables. That should just switch from "normal drop" to "unwinding drop". Another panic will then abort the program.
It would be a bug in MIR if panic-during-drop would lead to skipping the drop of the remaining local variables. That should just switch from "normal drop" to "unwinding drop". Another panic will then abort the program.
Are other fields in the structure being dropped considered local variables in this context? That's really not clear to me from any user facing documentation (actually, this is true of the entire guarantee you are talking about--I only found out it was actually considered a bug that needed to be fixed from the issue tracker).
The thing about Pinned (what you're proposing) is that I think if we wanted to implement it (once Rust had all the features in place) we wouldn't have to do much more than this to make it backwards compatible with existing code:
type PinMut<'a, T> = &'a mut Pinned<T>;
type Pin<'a, T> = &'a Pinned<T>;(I think having a deref implementation from Pin to Pinned was also proposed). It's instructive to look at the places where it seems this wouldn't work. For example:
impl Drop for Pinned<TwoGenerators>does not work (at least, not straightforwardly) with PinMut. Even if we assume that this replaces the Drop for TwoGenerators itself when the Pinned type is constructed (I'm not sure how this would work?), Rust would still not know to call the Pinned version of the constructor for any fields that were projected, since the fields would just be held by value. If the pinned version of a destructor was just always used if it existed, this is effectively identical to PinDrop, just with weird syntax, and I don't see how it's better.
However, one is tempted to consider choosing one's destructor by recursively analyzing whether a value was rooted at a Pinned. Observe that if we ever want to allow by-value trait objects, we cannot necessarily rely on being able to decide whether to use the Pinned<T> drop or the T drop at compile time; I suppose you are thinking there could be a separate vtable entry for the Pinned version in such cases? This idea is sort of intriguing to me. It definitely requires substantial compiler support (much moreso than the proposed PinDrop), but it might be overall nicer in some ways.
However, rereading the thread, I recall there are other problems: the deref implementation for pinned types really does not work well for Pinned<T> (I suspect this is because the deref implementation on PinMut is logically wrong in some way, which is why it keeps causing problems, but it is really hard to justify losing it given its convenience--unless you make a whole bunch of types unconditionally Unpin anyway). Specifically, I find the RefCell example pretty troubling since in the presence of Pinned::deref it means that we wouldn't even be able to enforce the pinning dynamically with the existing type (I don't know if specialization would be enough). This further suggests that if we keep the deref implementation, we'll have to end up duplicating API surface almost as much with Pinned as we do with Pin; and if we don't keep it, Pinned<T> becomes amazingly hard to use. It also doesn't seem like Box<Pinned<T>> would work unless we added ?Move separately from ?DynSized (as pointed out in the thread).
All of this may be a good idea, yet in the context of current Rust the whole thing seems kind of unappealing to me, especially when you consider that basically no methods in current Rust would have ?Move bounds (meaning the lack of a deref would really hurt, unless the type was Unpin in which case Pinned offers no advantages). I suspect it is modeling what's really going on more closely by making pinning an owning property, which in turn makes it harder to get away with ad hoc decisions like PinMut::deref and makes for a much more pleasing interface (subjectively anyway), but it adds a lot of language machinery in order to do so and it doesn't sound like you think any of it is particularly useful (compared to the native immovable types you are proposing). I also don't know if what we get that we couldn't get completely backwards-compatibly (mostly, calling a different drop implementation depending on pin status) is actually that useful, even if it can be done (maybe you could save a branch in the destructor sometimes if you knew the type was pinned?). So I'm not sure if it's worth changing the PinMut proposal at this point. But maybe I am missing some really compelling concrete use case.
The thing about
Pinned(what you're proposing) is that I think if we wanted to implement it (once Rust had all the features in place) we wouldn't have to do much more than this to make it backwards compatible with existing code:type PinMut<'a, T> = &'a mut Pinned<T>; type Pin<'a, T> = &'a Pinned<T>;
First of all, Pinned itself would require minimal-to-no compiler support, if that's what you mean by "features". If you mean library design, like DynSized and related traits, then that's valid, but…
What you suggested wouldn't really be backwards compatible, since e.g. you might try to implement the same trait for both Pin<'a, T> and &'a T, which would suddenly become conflicting.
More generally, there's a significant difference in API designs. With Pin, traits expected to be impl'ed by immovable types must have their methods take PinMut<Self>, generic functions that want to take references to immovable types must look like fn foo<T>(p: PinMut<T>), etc. On the other hand, the Pinned design avoids the need for new traits in most cases, since you can impl traits for Pinned<MyStruct>. Thus:
-
Immovable types would be incompatible with existing traits whose methods take
&selfor&mut self: for example, generators could not implIterator. So we would need a bunch of new traits that are equivalent to existing ones but takePinMut<Self>instead. If we changed course and madePinMut<T>an alias for&mut Pinned<T>, we could then go back and deprecate all those duplicate traits, but that'd be pretty silly. Better to not need the duplicates in the first place. -
On the other hand, newly designed or generator-specific traits would probably take
PinMut<Self>as the only option, at the cost of adding noise for types which want to implement the traits but aren't immovable and don't need to be pinned. (Specifically, callers would have to callPinMut::newto go from&mut selftoPinMut<Self>, assumingSelf: Unpin.) Even ifPin<T>became an alias for&mut Pinned<T>, there would be no way to get rid of that noise. And the future native immovable types I'm envisioning would be in the same situation as movable types, unnecessarily needing to be wrapped inPinnedwhen they're always considered pinned.
I'll respond to the rest of your post in a second post.
Regarding Drop
I'm a bit confused by what you're saying about Drop, but to the extent you're trying to make it backwards compatible with PinMut, I'm not going to think about that because I don't think it's a good approach.
I think the best approach is that if you have a struct like TwoGenerators, you have two options:
- No manual
Dropimpl for eitherTwoGeneratorsorPinned<TwoGenerators>; - You impl
DropforPinned<TwoGenerators>; meanwhile, the same macro that gives you accessors will generate aDropimpl forTwoGeneratorsitself that simply casts self to&mut Pinned<TwoGenerators>and drops that. (This is safe: the invariant required to cast&mut Tto&mut Pinned<T>is that you won't moveTafter the borrow ends, and in the case ofDrop, you have the last borrow that will ever be created for that value.)
The only reason to have two options is that, as mentioned previously, you might not want your struct to impl Drop because structs that don't impl it treated more loosely by the borrow checker.
I don't see why you'd want to have an actual separate destructor for the pinned versus not-pinned state, so there's no need to play tricks with vtables to differentiate them.
Regarding RefCell
I don't think Pinned::deref should exist. The safe macro-generated field accessors should be enough; I don't see how that's "amazingly hard to use". It's slightly less nice than being able to use native field syntax, but someday that'll be fixed by native immovable structs. Anyway, if it is hard to use, the same problem applies to Pin.
This avoids the issue with RefCell.
especially when you consider that basically no methods in current Rust would have
?Movebounds (meaning the lack of a deref would really hurt[..])
On the contrary, everything that has a ?Sized bound is implicitly ?Move.
This makes sense because in general, it's impossible for code with a ?Sized bound to assume movability. The sole exception is unsafe code that calls size_of_val and then memcpy's that many bytes, which is why we need the hack where size_of_val would panic for immovable types (and be deprecated in favor of a new function with a proper bound).
I'm a bit confused by what you're saying about Drop, but to the extent you're trying to make it backwards compatible with PinMut, I'm not going to think about that because I don't think that's a good approach.
I was saying that if something isn't backwards compatible with PinMut, there should be a good reason for it. However, the thing you're proposing is functionally identical to PinDrop in every particular except that you want to implement it on Pinned<T> (which doesn't work in current Rust). Personally, I think specializing Drop sets a really dubious precedent and is almost certainly undesirable for reasons that have nothing to do with pinning, so I don't consider this any sort of intrinsic advantage. In any case I think PinDrop can be mostly separated from the rest of your proposal.
Anyway, if it is hard to use, the same problem applies to Pin.
Sure, and if we were willing to get rid of PinMut::deref, it would also compose nicely with types like RefCell; the difference is that we can still cobble together a solution with PinMut while supporting deref, which seems not to work with Pinned. If we were to get rid of the deref implementation, I think I would be much more likely to agree that Pinned provides a meaningful advantage.
But I'm really not sure I agree that not having deref is just a small problem in practice: for instance, it means that in its current state you cannot do anything with &Pinned<Vec<T>> where T: !Unpin, and the same would apply to basically every existing library type. This is a problem stemming from the way Unpin works, not the particular kind of reference you have. The ecosystem would have to collectively decide to do things along the lines of impl Deref for Pinned<Vec<T>> { type Target = Pinned<[T]>; } or something, which I think I agree would be preferable to a impl PinDeref<Vec<T>> if it can be made to work, but that's in a world without deref. In the world with deref, almost all libraries can get away without any pin-related accessors at all, and still have half-decent support for !Unpin types.
On the contrary, everything that has a ?Sized bound is implicitly ?Move.
Ah yes, that's a good point. Unfortunately, a whole bunch of Rust code does not work with types with a !Sized bound, because it's not the default, but at least some of it does. I don't think it's that compelling an advantage because most of the things I can do on unsized values are call & or &mut methods on them (e.g. for slices or trait objects), neither of which I would be able to do under your proposal (except for Unpin types) since you don't want Pinned::deref. Maybe the common cases could be dealt with by making #[derive] implementations also generate distinct instances for Pinned<T> or something?
Drop
Personally, I think specializing Drop sets a really dubious precedent and is almost certainly undesirable for reasons that have nothing to do with pinning, so I don't consider this any sort of intrinsic advantage. In any case I think PinDrop can be mostly separated from the rest of your proposal.
I agree this is separable, but I don't think it's dubious. At least… what you said is right, it's a form of specialization. It's not literally specializing some parent blanket impl of Drop, but the compiler-generated drop glue is doing the equivalent of specialization by calling Drop only if it is implemented. A 'userland' implementation would look like this (ignoring the fact that you can't manually call drop):
trait DropIfImplemented {
fn maybe_drop(&mut self);
}
impl<T: ?Sized> DropIfImplemented for T {
default fn maybe_drop(&mut self) {}
}
impl<T: ?Sized + Drop> DropIfImplemented for T {
fn maybe_drop(&mut self) { self.drop() }
}So, I imagine that the reason you can't currently write 'specialized' Drop impls is the same reason that specialization itself is currently unsound: incoherence between trans (which erases lifetime parameters) and typeck (which does not). In other words, if we could write, say, impl Drop for Foo<'static>, it would actually be called for any Foo<'a>, not just Foo<'static>, because codegen assumes the two types are identical.
The good news is, as you probably know, there have been attempts to find a way to limit specialized impls so that they can't create that type of incoherence. And it's expected that specialization will eventually ship with some such limit. Once that happens, I see no reason we couldn't apply the same rules to Drop impls – and to make the language as consistent as possible, we should.
Now, we don't want to block pinning on specialization. However, I claim that allowing impl Drop for Pinned<MyStruct> – or more generally, allowing impl<params> Drop for Pinned<MyStruct<params>> under the same conditions that the compiler currently allows impl<params> Drop for MyStruct<params> – is guaranteed to be a subset of what specialization will allow, so if we made a special case for it today, it will eventually disappear into a more general rule.
But again, this is separable; if people don't like this, we could have a separate trait instead.
Unpin
But I'm really not sure I agree that not having
derefis just a small problem in practice: for instance, it means that in its current state you cannot do anything with&Pinned<Vec<T>>whereT: !Unpin, and the same would apply to basically every existing library type. This is a problem stemming from the wayUnpinworks, not the particular kind of reference you have.
Er… okay, let me correct my statement. Pinned::deref should exist, but it should be bounded on Unpin – albeit I'm going to call it .Move
The only reason the Deref impl for PinMut causes problems with RefCell is that (unlike the DerefMut impl) it's not bounded on Unpin. And the reason not to have the bound is the desire to allow users to obtain &MyImmovableType, allowing immovable types to impl traits with methods that take &self, to be passed to generic functions that take &T, etc. This is fundamentally impossible for &mut self, but it mostly works with &self because you can't move out of it with mem::swap or mem::replace – that is, except when you use RefCell. But, the reasoning goes, compatibility with existing references is valuable enough that it should be supported, even if the limitation to immutable references feels arbitrary, even if it causes kludges.
With Pinned, we can support both immutable and mutable references: you just impl your traits on Pinned<MyStruct> rather than directly on MyStruct. The downside is that it's not compatible with traits or functions that take &T but separately have a Self: Sized bound; but those are relatively rare, and often unintentional.
Interestingly, Pinned itself doesn't really require Unpin to exist at all. After all, why would anyone actually create an &Pinned<Vec<T>>? With PinMut, various traits would take PinMut<Self>, so even impls of those traits for movable types would have to receive a PinMut. With Pinned, as I've said, traits would continue to take &self or &mut self, and you'd impl them for Pinned<MyStruct>. If you want to impl the same trait for Vec<T>, Pinned doesn't need to come into the picture.
However, one potential source would be the field accessor macro. If you have
struct SomePinnable {
gen: FakeGenerator,
also_a_vec: Vec<Foo>,
}then the simplest design would always generate accessors using Pinned:
impl Pinned<SomePinnable> {
fn gen(&self) -> &Pinned<FakeGenerator> { … }
fn gen_mut(&mut self) -> &mut Pinned<FakeGenerator> { … }
fn also_a_vec(&self) -> &Pinned<Vec<Foo>> { … }
fn also_a_vec_mut(&mut self) -> &mut Pinned<Vec<Foo>> { … }
}…and it's up to you to deal with the Pinned if you didn't want it. In fact, I think this is fine, because Unpin/Move should indeed exist, see below. But if it didn't exist, an alternative would be having a way to opt-in on a per-field basis to receive a direct reference rather than a Pinned one. That is, you'd have
fn also_a_vec(&self) -> &Vec<Foo> { … }
fn also_a_vec_mut(&mut self) -> &mut Vec<Foo> { … }It would be unsound to have both Pinned and non-Pinned accessors, but either by itself should be fine.
…But yeah, we do need to have a Move trait, just not for Pinned. For example, it would be part of the bound for the new version of (correction: it wouldn't be, but unsafe code would be expected to check for it before trying to memcpy arbitrary types based on the result of size_of_valsize_of_val); in the future with unsized rvalues, it'll be the bound (relaxed from Sized) for ptr::read and mem::swap and mem::replace; if we ever get &move, it would be the bound for letting you, well, move out of one; and something similar applies to guaranteed copy elision.
So, as long as we have the trait, there's no reason not to have Pinned::deref (and deref_mut) with a T: Move bound.
[edit: as pythonesque reminded me, this has different semantics from Unpin, so never mind.]
(And then types like Vec and Box will want to manually impl Move so that it applies regardless of whether the element type is Move.)
Er… okay, let me correct my statement. Pinned::deref should exist, but it should be bounded on Unpin – albeit I'm going to call it Move.
Okay, hold up. Is this ?Move or Move? The former would mean !Unpin types can't even be constructed in many cases; the latter makes me wonder how, exactly, we are referring to types like Pinned<T> (since ?DynSized isn't really the right bound for them). I certainly hope they are not one and the same--otherwise, treating Move as Unpin once again does the exact thing we are trying to avoid and makes the type immovable the moment it is constructed.
The downside is that it's not compatible with traits or functions that take &T but separately have a Self: Sized bound; but those are relatively rare, and often unintentional.
There is a much more significant practical downside, which is that few of those traits or functions actually work with &Pinned today. They could be made to work with it but it would require a huge number of extra trait implementations and (like I said) probably a significant overhaul of existing #[derive] implementations. This is also a cost that would have to be paid for new stuff as well--you'd have to implement everything for &Pinned<Self> as well if you wanted it to work on !Unpin types. This is a (much) better situation for traits that take &mut self than with PinMut, but worse for &self, which is (I suspect) much more common. Hence why I say I think this is the more correct solution (in the sense that if we didn't have many existing Rust libraries the Pinned version would be better) but possibly not the more usable one.
With Pinned, as I've said, traits would continue to take &self or &mut self, and you'd impl them for Pinned
Reimplementing every trait in Vec's API surface, just with Pinned this time, doesn't sound that great to me (especially since some of the traits don't even work with it). I'm pretty sure either of selectively implementing Deref on a case by case basis (letting &Pinned<Vec<T>> go to &[Pinned<T>], for instance), or just letting the whole of Vec be Unpin (and not allowing pin projection), is way more sane. In any case, both are more work than doing absolutely nothing, and it'd have to be replicated across a whole bunch of existing types and traits in order for immovable stuff to work with them.
I could be convinced otherwise--I do actually like the Pinned solution more the more I think about it--but I just don't know where all these new trait implementations on Pinned<T> are actually going to come from; it seems more likely to me that people just won't bother to implement them.
Interestingly, Pinned itself doesn't really require Unpin to exist at all. After all, why would anyone actually create an &Pinned<Vec>?
There are pretty good reasons for wanting to do this (such as: your pinned type has a Vec in it). Intrusive collections will run into this kind of scenario very frequently. I think any proposal predicated on the idea that people are never going to want Pinned references to existing containers, or that you have to opt in to make Unpin work, is not likely to work well. Not being able to basically opt into Rust's regular ecosystem by adding a Unpin bound would be incredibly disruptive (in fact, almost every use case I have for immovable types would become significantly harder).
With PinMut, various traits would take PinMut, so even impls of those traits for movable types would have to receive a PinMut.
Sure! The big advantage of the Pinned version is that you wouldn't need distinct traits for mutable pinned references. However, it is worse or neutral than PinMut with deref for almost every other scenario.
It would be unsound to have both Pinned and non-Pinned accessors, but either by itself should be fine.
Manual accessors requiring unsafe code to implement sound like a bad idea to me; I don't see how any such a proposal would allow accessor generation to be safe (how do you stop someone from providing non pinned accessors without making them write unsafe to assert that they won't do it?). However, as you note, using Move (assuming it actually means Unpin) will work fine.
So, as long as we have the trait, there's no reason not to have Pinned::deref (and deref_mut) with a T: Move bound.
Sure. I am specifically talking about !Unpin types here. Unpin types do not have any composition problem with PinMut either, so they are not that relevant from my perspective. However, Unpin (or Move) bounds on generics are not pleasant and ideally you should be able to avoid them wherever possible. Again, like I said earlier: Unpin and whatever is being implied by !Sized are not the same and I don't think you can treat them as being the same.
…But yeah, we do need to have a Move trait, just not for Pinned. For example, it would be part of the bound for the new version of size_of_val; in the future with unsized rvalues, it'll be the bound (relaxed from Sized) for ptr::read and mem::swap and mem::replace; if we ever get &move, it would be the bound for letting you, well, move out of one; and something similar applies to guaranteed copy elision.
I think this is once again conflating !Unpin (which says a type can have a non-default pinning invariant) and the !DynSized-like !Move; they can't really be the same without causing the unwanted freezing behavior.
Oops, you're completely right. Unpin cannot be the same as Move.
So then I think I'm back to believing Unpin and hence Pinned::deref should not exist at all, and instead we should avoid any situations (like with the accessor-generating macro) where you'd get a type like &Pinned<MovableType>. But maybe there's an argument that it should exist as a separate trait.
Reimplementing every trait in
Vec's API surface, just withPinnedthis time, doesn't sound that great to me (especially since some of the traits don't even work with it). I'm pretty sure either of selectively implementingDerefon a case by case basis (letting&Pinned<Vec<T>>go to&[Pinned<T>], for instance), or just letting the whole ofVecbeUnpin(and not allowing pin projection), is way more sane.
Yeah, I definitely did not mean to propose reimplementing the whole of Vec's API surface or anything like that.
I agree it would be a good idea to not allow "pin projection" on Vec, since &Pinned<Vec<T>> seems like an extraneous invariant – you should be allowed to move the Vec without invalidating the pin of the contents.
As an alternative, how about allowing transmuting from Vec<T> to Vec<Pinned<T>>, which would have most of the Vec API but omit the methods that can cause reallocation? That is, change the definition of Vec from the current struct Vec<T> to struct Vec<T: ?Sized + ActuallySized>, for some less silly name, where basically Sized becomes an alias for ActuallySized + Move; then add a Sized bound to the methods that can cause reallocation, and a method to do the transmute.
It would also be necessary to change the bound on the builtin slice type's element type from Sized to ActuallySized. Thinking about this does remind me of the awkwardness of changing Sized to mean something other than Sized, but on the other hand, it's still true that the vast majority of Sized bounds in existing code inherently require Move. Needing to know size_of::<T>() in order to index into a slice is a bit of an exception…
Sure! The big advantage of the
Pinnedversion is that you wouldn't need distinct traits for mutable pinned references. However, it is worse or neutral thanPinMutwithdereffor almost every other scenario.
It also has the advantage of avoiding conflict with RefCell, admittedly at the cost of conflicting with other things (Sized bounds).
Manual accessors requiring unsafe code to implement sound like a bad idea to me; I don't see how any such a proposal would allow accessor generation to be safe (how do you stop someone from providing non pinned accessors without making them write unsafe to assert that they won't do it?).
Because the accessors are impl'ed on Pinned<MyStruct>, not MyStruct directly. If you have &mut MyStruct, you can always manually access a field to obtain &mut MyField, but you're still in a movable state. If you have &mut Pinned<MyStruct>, you can't get &mut MyStruct (assuming either MyStruct is !Unpin or Unpin doesn't exist), so you have to use an accessor to get at the fields. The accessor takes &mut Pinned<MyStruct> (i.e. it takes &mut self and is impl'ed on Pinned<MyStruct>) and gives you either &mut Pinned<MyField> or &mut MyField, depending on which option you chose. But you can only have one type of accessor or the other, as the critical invariant is that you mustn't be able to get a &mut Pinned<MyField>, write to it, release the borrow, and then get a &mut MyField (and move it).
There are pretty good reasons for wanting to do this (such as: your pinned type has a
Vecin it). Intrusive collections will run into this kind of scenario very frequently. I think any proposal predicated on the idea that people are never going to wantPinnedreferences to existing containers, or that you have to opt in to makeUnpinwork, is not likely to work well. Not being able to basically opt into Rust's regular ecosystem by adding aUnpinbound would be incredibly disruptive (in fact, almost every use case I have for immovable types would become significantly harder).
I don't fully understand what you mean here, but that may be because you were reacting to my own mistake w.r.t. Unpin versus Move :)
Now that I'm corrected... If Unpin exists, then Vec should impl it. But supposing it does not exist, what exactly are the scenarios you're referring to?
For a struct that has a Vec as one of its fields, I explained above how you would get a non-pinned reference to the field (at the cost of not being able to get a pinned reference to it, which is fine).
I guess this would be problematic if you want a generic struct with a field that might contain a Vec, or might contain an immovable type, depending on the type parameter. However, there might be a different way to solve this without needing an Unpin trait which everything has to think about whether to implement.
As an alternative, how about allowing transmuting from Vec to Vec<Pinned>, which would have most of the Vec API but omit the methods that can cause reallocation?
Because...
That is, change the definition of Vec from the current struct Vec to struct Vec<T: ?Sized + ActuallySized>, for some less silly name, where basically Sized becomes an alias for ActuallySized + Move; then add a Sized bound to the methods that can cause reallocation, and a method to do the transmute.
...that sounds really, really complicated and it doesn't actually do the thing you want (which is get a regular Vec<T> out of &mut Pinned<Vec<T>> or whatever). It is sort of cool that it lets you pin a vector after the fact, so you get a nice analogue to Box<Pinned<T>>, but that's an orthogonal concern; it's just another illustration of the fact that pinning being a property of the owned type is probably correct. I think everything about Unpin is almost completely unrelated to the question of how the reference type is constructed.
Now that I'm corrected... If Unpin exists, then Vec should impl it. But supposing it does not exist, what exactly are the scenarios you're referring to?
I'll just respond to this because I think it will illustrate my point: I can't even mutate an i32 field through a PinMut without Unpin. At some point if you want to do anything with your structure you often have to move something inside it (unless it's completely immutable). Needing people to explicitly implement field accessors on Pinned<MyType> seems really annoying, especially if the field in question is always safe to move. This approach also seems like it would be really confusing to use with a builtin Pinned type since legal projections would somehow vary by field in a way that doesn't depend on the field type, something that was rejected already in Rust when mut fields were removed (and IMO, if we are going to add such an annotation unsafe is a better choice, since unsafe fields are a huge footgun in practice). Since builtin pinned types are just about the only way to make pinned enums nice to use, I do care about them being able to be somewhat consistent with the rest of the language.
But more importantly...
I guess this would be problematic if you want a generic struct with a field that might contain a Vec, or might contain an immovable type, depending on the type parameter
That's pretty much the killer use case for Unpin, and (to me) the fact that it actually seems to work is pretty fantastic and validates the entire pinning model (to the point that I think even if we were starting from before Rust 1.0 we would probably want to keep Unpin as is). Generic parameters living inline in the structure are also pretty much the only time you should need to bound by Unpin if the current plans (to make almost all the safe reference-like types implement Unpin unconditionally) go through.
But what I am really not getting is: why do you want to remove Unpin? Eliminating it buys you essentially nothing; all the nice things you get out of Pinned over PinMut (or vice versa) are pretty much unrelated to its presence. Adding it gets you easy compatibility with the rest of the Rust ecosystem. FWIW, Unpin and the unconditional implementation of deref on Pin are pretty much unrelated to each other as well, except that without Unpin all types feel the same pain that !Unpin types do (meaning it would probably make the unconditional deref implementation more useful). I can't help but feel like I am missing something.
Are other fields in the structure being dropped considered local variables in this context? That's really not clear to me from any user facing documentation
Fair enough, I opened #50765 to track this.
Specifically, I find the RefCell example pretty troubling since in the presence of Pinned::deref it means that we wouldn't even be able to enforce the pinning dynamically with the existing type (I don't know if specialization would be enough). This further suggests that if we keep the deref implementation, we'll have to end up duplicating API surface almost as much with Pinned as we do with Pin; and if we don't keep it, Pinned becomes amazingly hard to use.
The solution for RefCell is to provide extra methods borrow_pin and borrow_pin_mut (taking Pin<RefCell<T>>), and keep track of the pinned state of the interior of the RefCell at run-time. This should work for both PinMut and Pinned. So, is your argument here that Pinned does not help? It shouldn't make things any worse for RefCell either.
But then you write
the difference is that we can still cobble together a solution with PinMut while supporting deref, which seems not to work with Pinned
and I don't know what you are referring to, why should this not work with Pinned?
I don't think Pinned::deref should exist. The safe macro-generated field accessors should be enough; I don't see how that's "amazingly hard to use".
I don't see the connection between the deref and the field accessors. But I also don't see how the deref becomes more problematic with Pinned<T>, so I guess I'll wait for an answer to my above question first.
Just like @pythonesque I think that tracking the pinned state on the type behind the reference ("on the owned type") is fundamentally more correct. However, I am doubtful it can actually be turned into a more ergonomic overall API, in particular given the constraint of working with the existing Rust ecosystem.
If we are going to deliberately go with the approach we think is less "fundamentally correct", we should at least leave ourselves a whole lot of time for experimentation before stabilizing it, so we can be as confident as reasonably possible that we aren't going to wind up regretting it.
@pythonesque , wow, thanks so much for the extensive summary! Happy to hear that there's an RFC in the works. :)
It is save to call a function that takes a mutable reference to a pinned value as long as that function does not use something like mem::swap or mem::replace. Because of that it feels more natural to have these functions use the Unpin bound than to have every mutable deref of a Pin to an Unpin value be unsafe.
If a function would later be updated to use swap it would no longer be safe to call it on a mutable reference to a pinned value. When swap and replace have this bound the updated function had to do as well with makes it way more obvious that this is not a backwards compatible change.
So some thoughts I had:
- Fundamentally,
Dropprovides the same privilege asUnpin- you can get an&mutto something that was previously in anPinMut. AndDropis safe, which meansUnpinshould be safe (this ismem::forgetand leakpocalypse all over again). - That's nice, because it means that things like the current futures based APIs, that don't handle unpin generators, are all 100% safe to implement even if they take
self: PinMut<Self>(nounsafe impl Unpin). - Is the API sound if
Unpinis safe? The answer is yes: as long as generators don't implementUnpin, and it isn't safe to pin project to a!Unpintype, everything is safe. - But this means that pin projections are unsafe! Not ideal.
- Pin projections are safe if Self does not implement
UnpinorDrop[edit: true or false?] Can we automate that check?
I have some ideas for a more language-supported alternative to this library API, which involve dropping Unpin entirely and instead flipping the polarity - a Pin trait that you opt into getting these guarantees, instead of opting out. But this would require significant language support, whereas the current implementation is entirely library-based. I'll make another post after I've thought it through more.
another note because i keep forgetting:
pin projection safety depends only on the Self type, not the field type, because the field type must guarantee the safety of its public API, which is unconditional. So its not a recursive check - if Self never moves anything out of its Pin, pin-projecting to a field any type is safe.
@withoutboats FWIW this exactly matches the conclusions that @cramertj and I reached in our previous round of discussions. And I believe we can automate the mutual exclusion check, initially through some purpose-built attributes emitted by the derive.
Fundamentally, Drop provides the same privilege as Unpin - you can get an &mut to something that was previously in an PinMut. And Drop is safe, which means Unpin should be safe (this is mem::forget and leakpocalypse all over again).
I don't see the connection to leakpocalypse but agree otherwise. The only reason I am (was?) a bit hesitant is that as long as it's just Drop, this felt more like a corner-case to me that not many people have to care about. Not sure whether that's an advantage or not though. And anyway, a safe Unpin not only increases consistency here and "solves" the Drop problem by no longer making it a special case (instead we can think of every impl Drop as coming with an implicit impl Unpin); from what you say it also makes it easier to use on the Future side of things. So, it seems to be an overall win.
@pythonesque unless I am missing something, safe Unpin doesn't cause any new issues for intrusive collections either, does? If they worked despite safe Drop, they should still work.
Pin projections are safe if Self does not implement Unpin or Drop [edit: true or false?] Can we automate that check?
You initially also mentioned the field type here and I was about to ask why you think that is relevant. Now I see you edit the post. :) I agree that this is about the Self type only. In the following I pretty much repeat your argument in my terms.
Essentially, the question is: How does Self choose its pinning invariant? Per default, we assume (even if unsafe code is around!) that the pinning invariant is exactly the same as the owned invariant, i.e., the invariant is independent of the location and this type does not do pinning. As long as I cannot do anything with a PinMut<T> other than turn it into a &mut T, that's a safe assumption.
To enable field projections, the pinning invariant should instead be "all my fields are pinned at their respective type's invariant". That invariant easily justifies pinning projections, independent of the field types (i.e. they can pick whatever pinning invariant they want). Of course this invariant is incompatible with turning PinMut<T> into &mut T, so we better make sure such types are not Drop or Unpin.
I don't see the connection to leakpocalypse but agree otherwise.
just an analogy - Unpin is to Drop as mem::forget is to Rc cycles. mem::forget was originally marked unsafe, but there was no justification for it. (And the same argument that Rc cycles are an edge case was made against marking mem::forget safe.)
Copy-pasting (spiritually) from the Discord, I'd really like to see evidence that we haven't just flipped the problem: making Unpin safe to implement, by making structural pin accessors unsafe to implement (this would also be true with any unsafe trait that was introduced, right? You'd still have to write unsafe code). This annoys me because the vast majority of the time they are completely safe--basically, as long as there isn't an explicit Unpin impl for the type, just like we are always safe if there's no Drop impl for the type. With the current plan, we are requiring something much stronger--there should be an explicit !Unpin impl for the type--which will be true for many fewer types (that is just about all we can do in a library).
Unfortunately I have no idea how or whether the compiler can check whether there's a manual Unpin impl for a particular type or not, as opposed to "has any impl", and I'm not sure if it has bad interactions with specialization. If we have some definite way to perform that check, such that the creator of a type doesn't have to write any unsafe code to get structural pinning, I would be much much happier with impl Unpin being safe, I think... is that something that seems feasible?
I have a simple question that, I am trying to understand now. In generics, will unpin be an implicit bound like sized unless your API for all generic parameters?
that must be correct in order for vec to continue to be safe.
will unpin be an implicit bound like sized
No.
that must be correct in order for vec to continue to be safe.
Why do you think so?