Rewrite `pin` module documentation to clarify usage and invariants #116129

fu5ha · 2023-09-25T00:20:59Z

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 #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.

rustbot · 2023-09-25T00:21:07Z

r? @scottmcm

(rustbot has picked a reviewer for you, use r? to override)

Manishearth · 2023-09-25T19:31:26Z

Just going to say, thanks for doing this! I've was very disappointed when the old PR didn't end up getting through because it got bogged down, hoping this will have better luck. The Pin docs need a lot of work and this ought to significantly move the needle on that!

fu5ha · 2023-09-26T16:25:44Z

(Let me know if anyone starts a review of this and I will stop force-pushing to the branch so your comments don't break!)

library/core/src/pin.rs

kpreid · 2023-09-27T20:52:14Z

library/core/src/pin.rs

+
+/// A pointer which pins its pointee in place.
+///
+/// [`Pin`] is a wrapper around some kind of pointer `Ptr` which makes that pointer "pin" its


Shouldn't the documentation use the same type-variable name as the struct itself does, for clarity? That is, both Ptr or both P.

(I would go with P.)

Perhaps, yeah. I do think that Ptr is much more clear to someone unfamiliar with pinning. P sounds like it could be the thing being pinned! But it is unfortunate to have a split....

Not sure what the best solution is.

FWIW, I am very strongly in favor of Ptr; seeing the docs using it (is a sight for sore eyes 😍 and) does improve a lot the otherwise uncommon notion that this wrapper acts through pointer indirection rather than wrapping the "pinned thing" itself.

I do agree that consistency with what the struct actually uses would be desirable, which is why it may be sensible to rename that parameter in the actual stdlib code

FWIW I would also be in favor of renaming it in the actual code as well, though that may be a bit more of a controversial change... it has my +1 though. I guess I'll leave this in discussion for now and wait for further feedback and then we can decide where to go.

I've changed the code and all references in the docs from Pin<P> to Pin<Ptr> in cd13f5c ... :)

It's more common for std to use single-letter generics but I believe this is a case where using the "full" version is justified.

Manishearth · 2023-10-03T14:56:16Z

cc @rust-lang/libs-api

Would really like to see this go through review this time: would it be possible to be more careful about blocking and no blocking concerns so that something can land; better than the status quo but potentially with space for further improvement?

scottmcm · 2023-10-03T19:39:03Z

I'm essentially pin-ignorant, so am not a good person to review this.
r? libs

workingjubilee · 2023-10-05T22:43:44Z

library/core/src/marker.rs

+/// Types that do not need to follow the rules of pinning.
 ///
-/// Rust itself has no notion of immovable types, and considers moves (e.g.,
-/// through assignment or [`mem::replace`]) to always be safe.
+/// For information on what "pinning" is, see the [`pin` module] documentation.
 ///
-/// The [`Pin`][Pin] type is used instead to prevent moves through the type
-/// system. Pointers `P<T>` wrapped in the [`Pin<P<T>>`][Pin] wrapper can't be
-/// moved out of. See the [`pin` module] documentation for more information on
-/// pinning.
+/// Implementing the `Unpin` trait for `T` lifts the restrictions of pinning off that type.
+/// This means that, if `T: Unpin`, it cannot be assumed that a value of type `T` will be bound
+/// by the invariants that pinning infers, *even* when "pinned" by a [`Pin<Ptr>`] pointing at it.
+/// When a value of type `T` is pointed at by a [`Pin<Ptr>`], [`Pin`] will not restrict access
+/// to the pointee value like it normally would, thus allowing the user to do anything that they
+/// normally could with a non-[`Pin`]-wrapped `Ptr` to that value.
 ///
-/// Implementing the `Unpin` trait for `T` lifts the restrictions of pinning off
-/// the type, which then allows moving `T` out of [`Pin<P<T>>`][Pin] with
-/// functions such as [`mem::replace`].
+/// The idea of this trait is to alleviate the reduced ergonomics of APIs that require the use
+/// of [`Pin`] for soundness for some types, but which also want to be used by other types that
+/// don't care about pinning. The prime example of such an API is [`Future::poll`]. There are many


I'm strongly persuaded that we should accept this PR, or at least some of it, based on the strength of this improvement in the docs alone. If we do find any reason to block part of this, or even quibble for any serious length of time, I would prefer that we ask for it to be split up. Pin and Unpin is a rare case where trying to describe it in terms of "positive license", instead of negative, is mostly confounding, and this fixes that.

I disagree. I find the explanation in terms of "negative license" that this PR attempts confusing. Traits should always be explained in terms of what they do, not in terms of what their absence lets you not do.

While that is true of non-unsafe Rust, I do think that the moment unsafe Rust is involved (which may very well happen when working with the pinning invariants), then knowing what cannot happen with a given (non-Unpin) type is what allows the unsafe code to assume certain things:

e.g., unsafe code can rely on absence of Send implying absence of unique access crossing-thread boundaries, and likewise for Sync and shared access. Similarly, unsafe code can rely on absence of Unpin to know that witnessing a Pin-wrapped pointer to such a type means that the pointee's (shallow) memory shall not be invalidated (deällocated or relocated) before it is dropped.

(this kind of relates to, conceptually, Unpin almost having deserved to be an unsafe trait)

Such negative reasoning is particularly fragile when using unsafe. Everything unsafe does should be positively justified from the type's invariants, not negatively justified from what Pin won't do. Pin is just a library type, it won't do certain things because the invariants won't let it, but it's the invariants that really matter.

It is fragile, but it is also necessary, and this is one of the key sources of confusions behind this type. The mere existence of Pin has made reasoning about unsafe code more confusing even though Pin itself doesn't have language level magic. The current docs make people incorrectly question their understanding of the overall safety model of Rust. That's not good.

We can and should do both: explain in both positive and negative terms. The docs need to be able to get on the train of having a vaguely correct mental model before it tries to give them a precise one. Currently the docs seem to leave people stranded at the station.

I'm very aware that pinning is a language feature

No, I know you know this. What I'm saying is that this is one of the key sources of confusion for people. I think negative license is actually relevant for Pin.

For Unpin, yes, I think that's trickier. I still think this PR is an improvement and we can revisit Unpin specifically after this lands.

I think negative license is actually relevant for Pin.

We remain in disagreement here. I find negative license basically never a good way to define things, certainly not in this case.

I'm not at all opposed to explaining this as a consequence arising from the underlying mechanism. I'm just not happy with presenting this as what Pin is.

@danielhenrymantilla

e.g., unsafe code can rely on absence of Send implying absence of unique access crossing-thread boundaries,

This is just contraposition. Whenever "P implies Q" is true, then also "not Q implies not P" is true. But almost always, "P implies Q" is still the thing you want to explain, and view "not Q implies not P" just as a consequence. A consequence worth mentioning, but not the defining property.

I find it basically always more clear to state a concept without negations rather than with negations, when possible.

I'm not at all opposed to explaining this as a consequence arising from the underlying mechanism. I'm just not happy with presenting this as what Pin is.

I think that would be fine too but I think the current text does not achieve this well at all. I think turning this text into something that talks about what pin "is" and then immediately goes into this as a consequence is not a major edit for these docs. I think that's a tweak of the first few paragraphs of Pin.

I do think the Unpin docs could do with more rearranging.

Fundamentally, I have found the existing positive license to be extremely confusing to people trying to learn this. I think negative license is a bad way to define things from a spec point of view, but I think from a docs/explainer point of view it can work, and I think what we have here is miles better than what existed in the past.

(Personally my favorite explanation of pin is by talking about it like a type of typestate but I'm not sure that's the right tack for the docs here either)

Incidentally, I think I just encountered a case where negative reasoning easily leads to wrong conclusions, and focusing on the positive properties that we actually have and need to prove provides clarity.

I am worried we'll draw ourselves into a corner with a negative wording in the documentation, because people will take that as the spec and they might draw wrong conclusions and then we get "interesting" soundness issues down the road, when their wrong interpretation of the spec, based on misleading docs, collides with the actual spec.

Yeah; I think you're coming at this from your position of dealing with opsem stuff and while I am sympathetic to the danger of these docs being misinterpretable, that is the case for most of Rust's opsem already: the docs are not sufficient for this. I'm coming at this from the position of "the first thing these docs should achieve is teaching people what these types are about. Laser precision about soundness can be secondary".

We should absolutely tighten up the language here to include that laser precision as well, and thanks for filing the followup for that.

We don't need to go into that much depth at this stage

Co-authored-by: Ralf Jung <post@ralfj.de> Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

Co-authored-by: Ralf Jung <post@ralfj.de>

Manishearth · 2024-01-07T17:02:49Z

@Amanieu this seems ready!

Amanieu · 2024-01-07T18:59:05Z

@bors r+ rollup

I'm sure there's a lot more space for further improvements to the documentation, but the new text is already much better than the old one so I'm going to go ahead and merge this. Further improvements should be in a separate PR.

bors · 2024-01-07T18:59:08Z

📌 Commit 7fd841c has been approved by Amanieu

It is now in the queue for this repository.

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.

RalfJung · 2024-01-07T21:15:31Z

I have opened #119714 to track the remaining concerns; feel free to extend that list if there are more open discussions here that I missed.

RalfJung · 2024-01-07T21:17:34Z

The PR history should IMO have been squashed a bit before landing this, but I guess it's too late now, the PR has already been rolled up.

…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#116129 (Rewrite `pin` module documentation to clarify usage and invariants) - rust-lang#119702 (Miri subtree update) - rust-lang#119703 (Impl trait tweaks) - rust-lang#119705 (Support `~const` in associated functions in trait impls) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#116129 (Rewrite `pin` module documentation to clarify usage and invariants) - rust-lang#119703 (Impl trait diagnostic tweaks) - rust-lang#119705 (Support `~const` in associated functions in trait impls) - rust-lang#119708 (Unions are not `PointerLike`) - rust-lang#119711 (Delete unused makefile in tests/ui) r? `@ghost` `@rustbot` modify labels: rollup

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.

rustbot assigned scottmcm Sep 25, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 25, 2023