lgtm

@bors: r+

(There should be plenty of time for further comments before this actually gets to the front of the queue...)

📌 Commit 15d2f96 has been approved by aturon

👍

Add Option::as_pin_mut

This fixes the pinned invariant for Option to be structural, i.e., a pinned option has its content pinned. I feel this should be written down explicitly somewhere, maybe next to the enum Option? One consequence of this is that adding impl<T> Unpin for Option<T> would lead to unsoundness.

@RalfJung Yes, this does fix that invariant-- i'm also interested in doing the same for other structures in std, such as AssertUnwindSafe, Result, and Box. There's not a good way to explicitly opt-out of Unpin in the case where you want this structure-- I think we might want a language feature for this. Perhaps it's not necessary, as the crate that performs pinning projections is the one that would have to add the impl? It seems unlikely that an impl for Option would fall through the cracks, but I could see impls like this sneaking into community libraries.

@cramertj One thing @RalfJung and I discussed was that there should still be a way to unsafely implement Unpin that would not conflict with the existence of pin projections--this would preserve our existing ability to conditionally implement Unpin with pin projections.

I don't think I have any concerns about this change otherwise, conceptually. However, I am a little wary about this implementation (just marking it safe) because unlike Drop, Unpin can (currently) be implemented with bounds that don't always exist on the type itself, so I'm not sure how easy it will be to integrate detecting it into the existing procedural macro. Obviously that would not be an issue for a compiler-enforced solution, but that hasn't actually been created yet.

i'm also interested in doing the same for other structures in std, such as AssertUnwindSafe, Result, and Box

Why Box? I expected Box to be unconditionally Unpin, like other smart pointers. Result should be fine though and other non-pointer-wrappers.

@RalfJung Why would Box and other smart pointers be unconditionally Unpin? Does it not seem useful to project in those cases? I had thought that Arc would even be unconditionally unpin and give access to Pin of the innards, although perhaps we'd want to make that into a wrapper type so that we could add ArcBox someday (which would allow you to move out of an Arc if the refcount was one).

I would usually expect that when something is pinned, that is a statement about the memory this thing is allocated in -- which doesn't propagate through pointers. Also see the discussion at #49621.

@RalfJung Yes, that intuition makes sense. I had a different intuition, though-- I was thinking of Box as a sort of single-field wrapper type like AssertUnwindSafe that just happened to store its inner field in a separate allocation. Neither interpretation is more clearly correct to me than the other, and I don't see strong utility in being able to project into a box (just use PInBox if you don't need to move out) nor making Box unconditionally Unpin (what does being able to move out of a PinMut<Box<NotUnpinType>> get you?).

I lean slightly towards my original intuition (single-field wrapper with out-of-line storage) because it allows more broad trait impls and makes more code that expects to be able to project PinMut correct.

⌛ Testing commit 15d2f96 with merge c2d4603...

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing c2d4603 to master...