Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite Pin<P> docs to clarify guarantees and uses #88500

Closed
wants to merge 1 commit into from

Conversation

mcy
Copy link
Contributor

@mcy mcy commented Aug 30, 2021

(With apologies to @RalfJung.)

The documentation today does not give a complete treatment of pinning
from first principles, which appropriately describes how to design types
that use it, nor does it provide formal statements of the guarantees
users need to be aware of.

This rewrite attempts to address these in a way that makes the concept
more approachable while also making the documentation more normative.

This is primarily motivated by my experience designing the moveit crate.
I'd like to thank @Manishearth and @anp for reviewing early drafts of this
new documentation.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2021
@m-ou-se m-ou-se added A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools A-pin Area: Pin T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 30, 2021
@rust-log-analyzer

This comment has been minimized.

Comment on lines +113 to +116
//! Pinning does not require any compiler "magic", only a specific contract between the library API
//! and its users. This differs from e.g. [`UnsafeCell`] which changes the semantics of a program's
//! compiled output. A [`Pin<P>`] is a handle to a value which does not allow moving the value out,
//! but Rust still considers all values themselves to be moveable with e.g. [`mem::swap`].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To the best of my knowledge, this is not 100% accurate: Pin<&mut T> being a newtype of &mut T is unsound because &mut T implies more (wrt aliasing) than we want Pin<&mut T> to.

Either "compiler magic" or a new bound (which we can't add now AFAIK) would be necessary.

For more history of this issue, see #63818.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I intentionally glossed over the fussiness around noalias, since I get the impression that we don't actually know what we want to do here. It's not clear to me that we want Pin<&mut T> to alias less or if we're going to come up with some newtype such as &mut A<T> to capture that aliasing contract.

@mcy
Copy link
Contributor Author

mcy commented Aug 30, 2021

Hmm, it looks like the link checker CI pass is very confused by some of the things I did in the documentation. I'd appreciate some help here; it's entirely possible the tool rejects some otherwise-correct links.

@Manishearth
Copy link
Member

Oh i wonder if it's because those are module level docs and they may be resolved relative to the parent module? I was under the impression we handled that but I could be wrong

@Manishearth
Copy link
Member

cc @GuillaumeGomez @jyn514 about the linkchecker

@eddyb
Copy link
Member

eddyb commented Aug 30, 2021

I'm not exactly qualified to review this, but there's one thing I noticed, and I'm wondering how much it matters.

A lot of the examples around address sensitivity talk about what I would call "internal address sensitivity", i.e. something akin to a self-borrow, where the data type contains its own address (whether as a pointer or not - even a hash would count) - we call most of these "self-referential data types", I suppose.

However, it's not the only kind of "phantom borrow" (i.e. not visible to borrowck) that can take advatange of Pin, and the other category I would call "external address sensitivity". This usually involves some kind of global state, e.g.:

  • the OS holding the address, and either accessing the data, or associating an OS resource with it
    • the Linux "Fast Userspace muTEX" (futex) API works a bit like this, but AFAIK only while locked, so Pin shouldn't be needed, the lock guard borrow should be enough
  • process/thread-wide data structures, temporarily pointing to shorter-lived data
    • this could be "roots" for some garbage collector, informing it of pointers into its GC heap, that it cannot itself see (e.g. on the native stack)
    • arguably something like glibc's FILE* could be modeled using Pin<Box<FILE>> - crucially, there is a global linked list through all live FILEs

One thing a lot of these usecases would have in common is relying on the Drop guarantee (assuming it's even strong enough) offered by Pin, to "deregister themselves" from the global state (wherever it may exist).

Like I said at the start, I'm not sure how much it matters, and even if there's agreement this kind of usecase should be documented/exemplified, I doubt it would need to be handled by this PR, but I wanted to note it nevertheless.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed a bunch of issues with the markdown syntax - given that I doubt there's a rustdoc bug here, but happy to take another look after the issues are fixed.

library/core/src/pin.rs Outdated Show resolved Hide resolved
library/core/src/pin.rs Outdated Show resolved Hide resolved
library/core/src/pin.rs Outdated Show resolved Hide resolved
library/core/src/pin.rs Show resolved Hide resolved
library/core/src/pin.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2021
library/core/src/pin.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't read all the way through, but I've got some comments.

Comment on lines +43 to +46
//! let mut tracker = AddrTracker::default();
//! tracker.check_for_move();
//! // May panic!
//! // tracker.check_for_move();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced about this particular example. As far as I understand it, you do in fact need an operation somewhere that moves it for the compiler to be allowed to change its address. In fact, this is why Pin doesn't require any magic as you write further down.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler makes no such guarantee. It is permitted to perform arbitrary moves of stack variables at any time as convenient.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that it would follow that the unsafe block in this snippet is incorrect and that it could panic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

library/core/src/pin.rs Outdated Show resolved Hide resolved
library/core/src/pin.rs Outdated Show resolved Hide resolved
library/core/src/pin.rs Outdated Show resolved Hide resolved
library/core/src/pin.rs Outdated Show resolved Hide resolved
Comment on lines +357 to +363
//! 2. *Notice of Destruction.* If `x: T` was ever reachable through any [`Pin<P>`] type, its
//! destructor must be run (until it either returns or panics) before `x`'s storage can be
//! overwritten. The "until further notice" in (1) includes this mandatory destruction. This is
//! often called the "[`Drop`] guarantee".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth mentioning that you don't have to drop it if you leak the memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is noted in the larger section on the drop guarantee. I think it's a bit of a footnote more than anything else.

Comment on lines 362 to 380
//! ## Address Stability
//! [address-stability]: #address-stability
//!
//! Crucially, we have to be able to rely on [`drop`] being called. If an element
//! could be deallocated or otherwise invalidated without calling [`drop`], the pointers into it
//! from its neighboring elements would become invalid, which would break the data structure.
//! The precise meaning of "address stability" is subtle, because "the same object" is not well-defined.
//! It is easiest to reason about it in terms of *visibility of mutations*. If [`unsafe`] code mutates
//! through a [`Pin<P>`], all code that stashed a raw pointer into it will see the mutation. In other
//! words, [`unsafe`] code can rely on the same value in memory being updated by all uses of a particular
//! [`Pin<P>`], not to mention that those stashed raw pointers remain valid in the first place.
//!
//! Therefore, pinning also comes with a [`drop`]-related guarantee.
//! When a [`List`] stores a [`Node`], it needs to assume that appending a second node will mutate the
//! first node, so that later, when the first node is removed, it knows that its predecessor is the
//! second node.
//!
//! # `Drop` guarantee
//! When writing generic code, it's not possible to know what [`unsafe`] code has recorded about the
//! pointee's address, so it must be very careful to observe this invariant. Thankfully, most of this
//! is already enforced by the [`Pin<P>`] API, so only [`unsafe`] code needs to worry about this.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better way to phrase this is that what pinning actually does is simply prevent safe code from performing any mutations whatsoever to the memory location without running that memory location's destructor first. The mutations that are prevented are typically thought of as types of moves, which is why we use "prevents moves" as the terminology for this, but it is not inherently incorrect for unsafe code to allow you to swap two pinned values, assuming that the unsafe block doesn't violate the invariants of some other unsafe blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I agree. The intent of this section is to be extremely pedantic about what guarantees are made to unsafe code, rather than the emergent properties of the behavior you describe. This topic is complex enough as it is, and I'd rather not have people have to deduce the "I can record the address and no one will mess with it" lemma from that property.

library/core/src/pin.rs Outdated Show resolved Hide resolved
@mcy mcy requested a review from jyn514 September 3, 2021 15:28
@rust-log-analyzer

This comment has been minimized.

@@ -387,6 +725,13 @@ use crate::hash::{Hash, Hasher};
use crate::marker::{Sized, Unpin};
use crate::ops::{CoerceUnsized, Deref, DerefMut, DispatchFromDyn, Receiver};

use crate::{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume these are for intra-doc links. You'll need to allow(unused_imports), rustc doesn't know that rustdoc uses these.

Suggested change
use crate::{
#[allow(unused_imports)]
use crate::{

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I had used #[cfg(doc)] but that made one of the CI passes really confused.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, cfg(doc) breaks the links when they're exported, it's not set for dependencies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment to clarify that these are used (only) by the documentation, and that that's why unused_imports is disabled here?

The documentation today does not give a complete treatment of pinning
from first principles, which appropriately describes how to design types
that use it, nor does it provide formal statements of the guarantees
users need to be aware of.

This rewrite attempts to address these in a way that makes the concept
more approachable while also making the documentation more normative.
@mcy mcy requested a review from jyn514 September 3, 2021 15:34
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The links look good to me, I can take another look if it fails again. I don't know enough about Pin to have opinions on the actual changes.

//! [`drop`]: Drop::drop "ops::Drop::drop"
//! [`poll`]: Future::poll "future::Future::poll"
//!
//! <!-- These require `alloc`, but we're in `core`. -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify this:

Suggested change
//! <!-- These require `alloc`, but we're in `core`. -->
// These require `alloc`, but we're in `core`.

@@ -387,6 +725,13 @@ use crate::hash::{Hash, Hasher};
use crate::marker::{Sized, Unpin};
use crate::ops::{CoerceUnsized, Deref, DerefMut, DispatchFromDyn, Receiver};

use crate::{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, cfg(doc) breaks the links when they're exported, it's not set for dependencies.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-10 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Set({"src/doc/edition-guide"}) not skipped for "bootstrap::doc::EditionGuide" -- not in ["src/tools/tidy"]
Rustbook (x86_64-unknown-linux-gnu) - edition-guide
Building stage0 tool linkchecker (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.15s
core/pin/index.html:16: broken link fragment `#a-self-referential-struct` pointing to `core/pin/index.html`
core/pin/index.html:17: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:306: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:308: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:324: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:324: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:362: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:389: broken link fragment `#a-self-referential-struct` pointing to `core/pin/index.html`
core/pin/index.html:416: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:464: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:465: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:466: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:479: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:480: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:481: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:481: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:482: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
core/pin/index.html:483: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `core/pin/index.html`
std/pin/index.html:16: broken link fragment `#a-self-referential-struct` pointing to `std/pin/index.html`
std/pin/index.html:17: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:306: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:308: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:324: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:324: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:362: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:389: broken link fragment `#a-self-referential-struct` pointing to `std/pin/index.html`
std/pin/index.html:416: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:464: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:465: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:466: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:479: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:480: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:481: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:481: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:482: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
std/pin/index.html:483: broken link fragment `#an-intrusive-doubly-linked-list` pointing to `std/pin/index.html`
number of HTML files scanned: 28355
number of HTML redirects found: 8861
number of links checked: 1640578
number of links ignored due to external: 64691

//! * [Subtle Details][subtle-details]
//!
//! # What is "*moving*"?
//! [what-is-moving]: #what-is-moving
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try adding self in front so it doesn't break when re-exported.

Suggested change
//! [what-is-moving]: #what-is-moving
//! [what-is-moving]: self#what-is-moving

(and the same for all the other fragment links)

@bors
Copy link
Contributor

bors commented Sep 25, 2021

☔ The latest upstream changes (presumably #88343) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

//! > this precise definition of moving a value.
//!
//! Common smart-pointer types such as [`Box<T>`] and [`&mut T`] allow removing and replacing the
//! values they contain: you can move out of a [`Box<T>`], or you can use [`mem::swap`]. Putting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It might be better to mention mem::replace here instead of mem::swap. mem::replace is usually the right function for moving out of a &mut. (Same below.)

//! [what-is-pinning]: #what-is-pinning
//!
//! `Pin` wraps a pointer type `P`, but removes the ability to actually obtain a `P` from it in safe
//! code. By disallowing access to the type behind the pointer, `Pin` prevents us from using
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disallowing access to the type

This makes it sound like it erases the type. (Like you won't be able to know what a Pin<..> points at.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! code. By disallowing access to the type behind the pointer, `Pin` prevents us from using
//! code. By restricting the access to the type behind the pointer, `Pin` prevents us from using

//! * e.g. calling [`poll`] for the first time on the produced [`Future`]
//! 3. Further [`unsafe`] operations assume that its address is stable.
//! * e.g. subsequent calls to [`poll`]
//! 4. The value is destroyed, undoing its address-sensitivity.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undoing its address-sensitivity

What does this mean, when the object is destroyed? It sounds a bit like 'you can move it afterwards', but there's no 'afterwards' for 'it', which makes this sentence a bit confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! 4. The value is destroyed, undoing its address-sensitivity.
//! 4. Before the value is invalidated (_e.g._, deallocated), it is _dropped_, giving it a chance to "unregister" / clear outstanding pointers to it.

Comment on lines +109 to +111
//! pinned data when they are called on a pinned pointer and do not *move* out of their `self`
//! parameter. It is unsound for [`unsafe`] code to wrap such "evil" pointers; see
//! [`Pin<P>::new_unchecked`] for details.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the new documentation here uses 'pinned' as the adjective for the data, the pointee of Pin<..>. But here (and on the docs on struct Pin) it's also used for the pointer itself. I'm worried that using both 'pinned pointer' and 'pinned data' can get confusing, since 'pinned' has a different meaning in those two cases. I think it might be best to keep 'pinned data' and use a different phrase for the Pin<..> pointer.

(Otherwise Pin<&mut *const i32> will be a pinned¹ pointer to a pinned² non-pinned¹ pointer to a non-pinned² integer. ^^')

Comment on lines +153 to +155
//! [`Pin<P>`]. When [`T: Unpin`][Unpin], <code>[Pin]<[`Box<T>`]></code> and
//! [`Box<T>`] function identically, as do <code>[Pin]<[`&mut T`]></code> and
//! [`&mut T`].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! [`Pin<P>`]. When [`T: Unpin`][Unpin], <code>[Pin]<[`Box<T>`]></code> and
//! [`Box<T>`] function identically, as do <code>[Pin]<[`&mut T`]></code> and
//! [`&mut T`].
//! [`Pin<P>`]. When [`T: Unpin`][Unpin], <code>[Pin]<[Box\<T>]></code> and
//! [`Box<T>`] function identically, as do <code>[Pin]<[&mut T]></code> and
//! [`&mut T`], and so on.

Comment on lines +598 to +602
//! For example, the `prev` and `succ` fields of a [`Node`]
//! are always either null or valid, so [`Node`] could provide a projection with
//! type <code>fn([Pin]<[`&mut Node`]>) -> [Pin]<[`&mut Node`]></code> for each
//! of them. These fields need to be structurally-pinned, since the outer [`List`]
//! assumes every [`Node`] in it is pinned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! For example, the `prev` and `succ` fields of a [`Node`]
//! are always either null or valid, so [`Node`] could provide a projection with
//! type <code>fn([Pin]<[`&mut Node`]>) -> [Pin]<[`&mut Node`]></code> for each
//! of them. These fields need to be structurally-pinned, since the outer [`List`]
//! assumes every [`Node`] in it is pinned.
//! Going back to our earlier linked list example, the `prev` and `succ` fields of a [`Node`]
//! are always either null or valid, so [`Node`] could provide a projection with
//! type <code>fn([Pin]<[`&mut Node`]>) -> [Pin]<[`&mut Node`]></code> for each
//! of them. These fields need to be structurally-pinned, since the outer [`List`]
//! assumes every [`Node`] in it is pinned.

@@ -387,6 +725,13 @@ use crate::hash::{Hash, Hasher};
use crate::marker::{Sized, Unpin};
use crate::ops::{CoerceUnsized, Deref, DerefMut, DispatchFromDyn, Receiver};

use crate::{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment to clarify that these are used (only) by the documentation, and that that's why unused_imports is disabled here?

//!
//! Pinning and [`Unpin`] only affect the pointee type [`P::Target`][Target], not the pointer type
//! `P` itself. For example, whether or not [`Box<T>`] is [`Unpin`] has no effect on the behavior of
//! <code>[Pin]<[`Box<T>`]></code> because `T` is the pointee type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using backtics inside <code> tags will result in nested <code> tags. (Same many times below too.)

Suggested change
//! <code>[Pin]<[`Box<T>`]></code> because `T` is the pointee type.
//! <code>[Pin]<[Box\<T>]></code> because `T` is the pointee type.

//!
//! For example, you could implement [`Drop`][Drop] as follows:
//! For this to work, a [`drop`-related guarantee][drop-guarantee] is required. If a node could
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! For this to work, a [`drop`-related guarantee][drop-guarantee] is required. If a node could
//! For this to work, we rely on [the `Drop` guarantee][drop-guarantee]. If a node could

Comment on lines +426 to +430
//! The [`drop`] function takes [`&mut self`], but this is called *even if your
//! type was previously pinned*! Implementing [`Drop`] requires some care, since it is as if
//! the compiler automatically called [`Pin::get_unchecked_mut`].
//! This can never cause a problem in safe code, because implementing an address-sensitive type
//! requires unsafe code (such as the [linked list above][linked-list]).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about anything that's Unpin that safely implements Deref and Drop, such as a Box? Dropping a Pin<Box<T>> drops the Box<T> which will drop the T, but what prevents the Box::drop from not moving that T before dropping it?

I'm not sure where this warning should go, but it'd be good to call out that any pointer type that will be used as the P in Pin<P> should also be careful with its Drop implementation, even though that type itself might not use any unsafety.

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice initiative, Pin is such a subtle API that improving its docs is always welcome! I've written my own set of nits, which can be grouped within the following categories:

  • Pin<P> ought to be renamed to Pin<Ptr>: if we are striving for clarity, that name change will make it crystal clear that it is the pointer and not the pointee which is Pin-wrapped, all throughout each and every explanation.

  • An intrusive doubly-linked list that is not lifetime-infected is incredibly tricky hard to get right: while Pin gives us guarantees about pointers being safe to dereference right when witnessed non-null, there can easily be concurrency / aliasing issues (including parallelism / thread-safety ones which are luckily dodged by the usage of raw pointers, but which ought to be mentioned) and APIs such as fn next(… '_ Node) -> … '_ Node cannot be featured.

  • adding some details / clarifications to some sentences or paragraphs, feel free to further tweak my own suggestions.

  • I believe that before talking of any guarantee that Pin makes, a short disclaimer ought to be added saying that none of the guarantees hold when the pointee is Unpin (linking to the full paragraph about it). Indeed, we should consider the case of a truncated read of the docs, especially when snippets are involved: there should be a clear "opt out of Unpin" part of the snippet whenever it makes assumptions about Pin-induced properties.

//! [what-is-moving]: #what-is-moving
//!
//! All values in Rust are trivially *moveable*. This means that the address at which
//! a value is located is not stable, even if no explicit semantic moves occur and its `Drop`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add this:

Suggested change
//! a value is located is not stable, even if no explicit semantic moves occur and its `Drop`
//! a value is located is not stable in between borrows, even if no explicit semantic moves occur and its `Drop`

at the very least, since while a value is borrowed, the borrowee can obviously not be moved, not even by the compiler.

But much like @Darksonn, I'm skeptical of this whole thing: I agree with their snippets regarding "interrupted pin references" having to be well-behaved. And let's hope that isn't guaranteed by Pin magic, since, as you mention later down the line, Pin ought to just be a simple API-restriction construction, not something built into the language (but for the definition of the Future trait, of course, and its auto-implementation for async blocks).

//! associated with <code>[Pin]\<P></code>, we discuss some examples for how it might be used.
//! Feel free to [skip to where the theoretical discussion continues](#drop-guarantee).
//! Rust does not guarantee that `check_for_move()` will never panic, because the
//! compiler is permitted to *move* `tracker` to enable optimizations like pass-by-value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe such optimizations would only kick in if the address itself were never to be witnessed; but let's ask @RalfJung about this: maybe a new issue on the UCG repo ought to be added?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we allow the compiler to insert moves.

The fact that all types are movable is part of the contract between a library and its environment, it is not part of what the compiler deals with.

//! [what-is-pinning]: #what-is-pinning
//!
//! `Pin` wraps a pointer type `P`, but removes the ability to actually obtain a `P` from it in safe
//! code. By disallowing access to the type behind the pointer, `Pin` prevents us from using
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! code. By disallowing access to the type behind the pointer, `Pin` prevents us from using
//! code. By restricting the access to the type behind the pointer, `Pin` prevents us from using

//! code. By disallowing access to the type behind the pointer, `Pin` prevents us from using
//! operations like [`mem::swap`] to *move* out of the pointer.
//!
//! ## Address-sensitive types
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! ## Address-sensitive types
//! ## Own-address-sensitive types

//! [`Future`] in between calls to [`poll`] would invalidate these pointers, leaving the next call
//! to [`poll`] with dangling references!
//!
//! Such types are *address-sensitive*: they incorporate the address of `self` into an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! Such types are *address-sensitive*: they incorporate the address of `self` into an
//! Such types are *own-address-sensitive*: they incorporate the address of `self` into an

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or self-address, etc.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it is good to speak of address-sensitive types here. It remains true that all types are movable, i.e., fundamentally not address-sensitive. This is true also for futures -- a freshly created future (before poll is called the first time) is movable. But certain values can be put into an "address-sensitive state", and that is what Pin tracks.

//! to be structurally pinned, because neither [`List`] nor
//! [`Node`] assume anything about it.
//!
//! ### When pinning *is* structural for `field`...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! ### When pinning *is* structural for `field`...
//! ### Choosing pinning *to be* structural for `field`

//! For example, the `prev` and `succ` fields of a [`Node`]
//! are always either null or valid, so [`Node`] could provide a projection with
//! type <code>fn([Pin]<[`&mut Node`]>) -> [Pin]<[`&mut Node`]></code> for each
//! of them. These fields need to be structurally-pinned, since the outer [`List`]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not true, because the lifetimes are disconnected / untangled: the prev or succ nodes may be dropped after the function call but while the borrow is being held.

//! requires unsafe code, so the fact that [`Unpin`] is a safe trait does not break
//! 1. *Structural [`Unpin`].* A struct can be [`Unpin`] if, and only if, all of its
//! structurally-pinned fields are, too. This is [`Unpin`]'s behavior by default.
//! However, as author, it is your responsibility to not write something like
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! However, as author, it is your responsibility to not write something like
//! However, as a library author, it is your responsibility not to write something like

//!
//! 3. *Structural Notice of Destruction.* You must uphold the the [`Drop` guarantee][drop-guarantee]:
//! once your struct is pinned, the struct's storage cannot be re-used without calling the
//! structurally-pinned fields' destructors, too.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! structurally-pinned fields' destructors, too.
//! structurally-pinned fields' destructors as well.

//! methods to get pinned references to elements. However, it could *not* allow calling
//! [`pop`][Vec::pop] on a pinned <code>[Vec]\<T></code> because that would move the (structurally
//! [`pop`][Vec::pop] on a pinned [`Vec<T>`] because that would move the (structurally
//! pinned) contents! Nor could it allow [`push`][Vec::push], which might reallocate and thus also
//! move the contents.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.drain() could also be an interesting example of a method that must be forbidden, because of mem::forget(vec.drain(..)).

More generally, the interesting case that could actually be added to the standard library would be that of arrays and slices, which, in the case of a heap-buffer, would lead to Box<[T]> and &mut [T] offering structural projections.

@JohnCSimon
Copy link
Member

Ping from triage:
@mcy can you please post your status on this PR?

//!
//! ## [`Unpin`]
//!
//! The vast majority of Rust types are not address-sensitive; these types
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, it's not types that are address-sensitive, it's values. So I think a more accurate way to put this would be to say that the majority of types do not have an address-sensitive state.

//! [address-stability]: #address-stability
//!
//! The precise meaning of "address stability" is subtle, because "the same object" is not well-defined.
//! It is easiest to reason about it in terms of *visibility of mutations*. If [`unsafe`] code mutates
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I think this is best described in terms of which invariants / properties must be satisfied by the data. That's how a mathematical formal treatment of pinning would work (or at least I don't see how else it would work, and I know this way works). The key difference between the invariant of a 'regular' Rust value and the invariant of a value in its address-sensitive state is that the latter invariant can depend on the address. The regular invariant satisfies the property that it keeps holding when the same sequence of bytes is put at a different address in memory; the address-sensitive invariant does not have that property.

I don't know what 'visibility' is supposed to mean.

@JohnCSimon
Copy link
Member

Ping from triage: I'm closing this one due to inactivity over the last few months, please feel to reopen when you are ready to continue with this.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Dec 12, 2021
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Dec 12, 2021
@JohnCSimon
Copy link
Member

This is for @mcy

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 7, 2024
Rewrite `pin` module documentation to clarify usage and invariants

The documentation of `pin` today does not give a complete treatment of pinning from first principles, nor does it adequately help build intuition and understanding for how the different elements of the pinning story fit together.

This rewrite attempts to address these in a way that makes the concept more approachable while also making the documentation more normative.

This PR picks up where `@mcy` left off in rust-lang#88500 (thanks to him for the original work and `@Manishearth` for mentioning it such that I originally found it). I've directly incorporated much of the feedback left on the original PR and have rewritten and changed some of the main conceits of the prose to better adhere to the feedback from the reviewers on that PR or just explain something in (hopefully) a better way.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 7, 2024
Rewrite `pin` module documentation to clarify usage and invariants

The documentation of `pin` today does not give a complete treatment of pinning from first principles, nor does it adequately help build intuition and understanding for how the different elements of the pinning story fit together.

This rewrite attempts to address these in a way that makes the concept more approachable while also making the documentation more normative.

This PR picks up where `@mcy` left off in rust-lang#88500 (thanks to him for the original work and `@Manishearth` for mentioning it such that I originally found it). I've directly incorporated much of the feedback left on the original PR and have rewritten and changed some of the main conceits of the prose to better adhere to the feedback from the reviewers on that PR or just explain something in (hopefully) a better way.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2024
Rollup merge of rust-lang#116129 - fu5ha:better-pin-docs-2, r=Amanieu

Rewrite `pin` module documentation to clarify usage and invariants

The documentation of `pin` today does not give a complete treatment of pinning from first principles, nor does it adequately help build intuition and understanding for how the different elements of the pinning story fit together.

This rewrite attempts to address these in a way that makes the concept more approachable while also making the documentation more normative.

This PR picks up where `@mcy` left off in rust-lang#88500 (thanks to him for the original work and `@Manishearth` for mentioning it such that I originally found it). I've directly incorporated much of the feedback left on the original PR and have rewritten and changed some of the main conceits of the prose to better adhere to the feedback from the reviewers on that PR or just explain something in (hopefully) a better way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools A-pin Area: Pin S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.