Re: `Pin` from `&mut` refs

[Nemo157]

This is a reply to this specific post in `Pin` from `&mut` refs? after thinking about it a little bit more and re-reading one of the doc clarifications from the pin stabilization thread (cc @zrk).

Does the current Pin forbids us from doing something like that ? It looks to me like Foo is never moved after the pin is constructed, yet its destructor is never called.

Where “that” includes using ManuallyDrop to allow deallocating a value that has previously been pinned without calling Drop::drop on it.

{
    let mut foo = ManuallyDrop::new(Foo { ... });
    let foo = unsafe { Pin::<&mut Foo>::new_unchecked(&mut foo)};
    foo.foo()
}

@RalfJung directly addressed this in the “Drop Guarantee” section of this comment, under this guarantee that code is unsound, you cannot construct a Pin pointing into a ManuallyDrop instance without guaranteeing that you will manually drop the value before deallocation.

[zrk]

Thank you, this is a very interesting comment inside of a very interesting discussion (if only a bit on the longer side).

What’s unclear to me is whether this guarantee is actually part of the stabilized API or not? If so, where is this stated (I skimmed through the related RFC but although it states that a goal is “Never to move before being dropped” it isn’t really clear about whether this entails ManuallyDrop<T>)? I second some of the comments about improving the documentation. How would that happen now that stabilization occurred on beta? Should we submit PRs? If we do so before the next stable, will the documentation in that next stable, or will it have to go through beta first?

[Nemo157]

There is an issue open about this (which is actually what prompted me to look back at the guarantees and notice this again).

Issue: Expand Pinning documentation

I don’t really know what the plan is here, it seems pretty important to me that at least the invariants provided/required by Pin are unambiguously documented in the release it stabilizes, even if the documentation on how to actually use it remains lacking for a release or two. (Even more importantly, because it’s just a carrier of invariants it can’t weaken or strengthen those invariants without being a breaking change on one side or the other of its API).

[RalfJung]

it seems pretty important to me that at least the invariants provided/required by Pin are unambiguously documented in the release it stabilizes

I agree, but the lang team decided to move forward without waiting for better documentation. Pin is in beta and will become stable on March 1st. All that we can do now is submit documentation to keep the time where Pin is insufficiently documented as short as possible.

[zrk]

What’s unclear to me is whether the added documentation would go directly to stable or go through beta first?

As @Nemo157 put it, documentation for the Pin type is crucial in the sense that it is “just a carrier of invariants”. Basically the documentation of the “safety” of unsafe methods of Pin describes the contract of “pinned objects” that cannot be enforced by the language (since all types are movable).

What if the added documentation allows for soundness loopholes, or more generally doesn’t correctly express the conditions for safety? Wouldn’t it be more reasonable to first put it in beta, allowing existing Pin code user to check the validity of their code wrt contracts introduced by the documentation, and then only later move to stable?

On the one hand, the current “imprecise” documentation does not really express a lot of guarantees on “pinned objects”, and so cannot really be a source of unsoundness, can it? On the other hand, the current documentation seems insufficient to actually use the module (I had to ask questions on this forum and read github issues to get some “tribal knowledge” that should be official documentation)…

[RalfJung]

What’s unclear to me is whether the added documentation would go directly to stable or go through beta first?

It would ride the trains like every other patch. (We could ask for a backport, though – not sure if that would be warranted in this case.)

[dhm]

the current “imprecise” documentation does not really express a lot of guarantees on “pinned objects", and so cannot really be a source of unsoundness, can it?

We have:

In order to prevent objects from moving, they must be pinned by wrapping a pointer to the data in the Pin type. Doing this prohibits moving the value behind the pointer. For example, Pin<Box<T>> functions much like a regular Box<T> , but doesn’t allow moving T .

For me the guarantees from an API-consumer point of view are pretty well expressed (i.e., if someone somehow gets a Pin<impl Deref<Target = T>>, they can then rely on T's address never changing) ; the documentation problem is “for the other side of the API”, i.e. for the person that goes unsafe and attempts to construct such a Pin object (e.g. the wrong but dangerously intuitive Pin<&'a mut T> : From<&'a mut T>).

That is “the worst kind” of guarantees, i.e. one that will “inevitably” lead to unsoundness (take, for instance, the damage caused by an “innocent-looking” function such as ::std::mem::zeroed<T : Sized> () -> T)

[Nemo157]

What about the code snippet in the OP? From the discussions surrounding the pinning API before it was stabilized I believe there was consensus that this was undefined behaviour, according to the quote you just posted from the documentation I don’t know whether it is defined or not. Does deallocating the memory without having called Drop::drop on it count as “moving the value” (i.e. does the value still exist until it is dropped, even if it is inaccessible, and does deallocating memory count as moving)?

For me the guarantees from an API-consumer point of view are pretty well expressed […] the documentation problem is “for the other side of the API” […]

These guarantees are the same though, Pin<P> where P::Target: !Unpin just carries some invariants from someone calling Pin::new_unchecked to someone calling Pin::get_unchecked_mut, if you understand the invariants provided to you by Pin::get_unchecked_mut then you understand the invariants you must uphold when calling Pin::new_unchecked.

[dhm]

Yeah, I said that as if there were 2 different documentations ; there is, of course, one single documentation, the one at the root of ::std::pin combined with the documentation of the associated unsafe functions of ::std::pin::Pin.

The documentation states that, if T : !Unpin, then T's address cannot change if comes from PinPtr<T>. That documentation, imho, requires less thought from a consumer point of view, since they can just go: “great, I somehow have this magic invariant, I don’t need to think too much about it”; whereas a Pin creator needs to think about more complex questions, such as the life of T after death no longer being PinPtr<T>.

Which, by the way, if I have understood that correctly (which I may not), is the dangerous thing to avoid with PinPtr<T> : once PinPtr<T> has existed, no code should have access to &mut T (since it could move T and thus cause UB if T : !UnPin), which is a problem because of <T as Drop>::drop : fn(&mut T). Obviously, it should not be forbidden to have Drop + !UnPin, it’s just that it should now be seen as an unsafe combination, thus requiring special care and attention from the programmer.

Notation

PinPtr<T> = Pin<impl Deref<Target = T>> where T : !UnPin

[Nemo157]

That documentation, imho, requires less thought from a consumer point of view, since they can just go: “great, I somehow have this magic invariant, I don’t need to think too much about it”

One thing I just realised from this, as a consumer of Pin you don’t need to fully understand all the invariants. It is ok to approximate the invariants with a weaker version that covers all your use-cases, if you can reliably find a weaker version without fully understanding the invariants. Whereas as a creator of Pin you probably do need to understand all the invariants, you could obviously provide a simpler stronger invariant if one exists, but to reliably find that stronger invariant you will need to fully understand the minimum invariants required.

As a sort of extension from this are things like the pin-utils macros. These mostly give a list of simpler invariants you must satisfy for use of the macro usage to be sound. While it is possible to soundly use the macros while carefully violating some of those invariants, when you understand the internals of the macro and the pinning invariants, that should never be needed in normal usage and you can get away with just learning the simpler invariants the macro requires.

[withoutboats]

Yea a “normal user” (e.g. someone implementing a manual Future or an executor) shouldn’t need to use more than the safe constructors in std and the macros in pin-utils to create or unwrap Pins. Use cases outside of that are pretty exceptional and I think they pretty much do require a very deep understanding of the APIs. Documentation is not yet optimized for increasing the number of people who have that deep understanding, focusing more on the normal cases that are handled well by the safe APIs and the pin-utils crate.

[dhm]

Regarding the Drop + !Unpin issue, what about something along these lines ?

[Yato]

Don’t you have to move into auto_drop, which is UB if you already pinned the value? Also now you need to call auto_drop or pin_drop to run destructors, which is undesirable.

[dhm]

Don’t you have to move i no auto_drop , which is UB if you already pinned the value

the auto_drop part is just to “emulate” the compiler, I should have used mem::ManuallyDrop but it would have been too cumbersome to code just for a sketched idea.

The idea is that, if the suggested code is correct, PinDrop would be the new real Drop and the current Drop would just be there to maintain retro-compatibility thanks to the impl<T : Drop + Unpin> PinDrop for T

[Yato]

Oh, I see, thanks for the clarification.

[dhm]

Regarding the Drop + !Unpin issue, what about something along these lines ?

It turns out it would still require Unpin to behave as Sized (i.e., need for ?Unpin to opt out of the trait bound in generics) in order to prevent breakage. This, in turn, would mean that all currently generic collections would not be compatible by default with !Unpin types

[RalfJung]

I have submitted a PR to add the missing information to the Pin documentation:

improve Pin documentation

Feedback and proof-reading in that PR would be welcome!

[dhm]

Awesome job @RalfJung, we really needed this kind of sound and detailed documentation!! Good job on specifying what the Drop guarantees are and how they relate to unsafe constructions; and the counterexamples really help clarify some more sneaky unsafeties (like with RefCell and Pin projections). Thanks.

One minor subjective thing: what about using Pointer<T> instead of SmartPointer<T>? I feel like our good old references &[mut] T may not always be perceived as smart. Actually, you could let SmartPointer be, but remind at the ::core::pin (module) level that this terminology also applies to &[mut] T.

[RalfJung]

One minor subjective thing: what about using Pointer<T> instead of SmartPointer<T> ? I feel like our good old references &[mut] T may not always be perceived as smart . Actually, you could let SmartPointer be, but remind at the ::core::pin (module) level that this terminology also applies to &[mut] T .

I changed them to use Pointer, thanks for the suggestion.