Skip to content

Rewrite pin module documentation to clarify usage and invariants #116129

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

Merged
merged 34 commits into from
Jan 8, 2024
Merged
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
bfd63b2
Rewrite `Pin<P>` docs to clarify guarantees and uses
mcy Aug 30, 2021
8241ca6
mostly done
fu5ha Sep 25, 2023
ba3b934
Fix examples, finish polishing
fu5ha Sep 25, 2023
584f098
fix broken links
fu5ha Sep 25, 2023
ec8a01a
fix one more broken link
fu5ha Sep 25, 2023
46f9d77
update doubly linked list commentary and fix links
fu5ha Sep 25, 2023
db5b19e
improve intro and `Unpin`-related discussion
fu5ha Sep 26, 2023
6818d92
improve intro and discussion of pinning as library contract
fu5ha Sep 26, 2023
bebbe24
improve `Pin` struct docs and add examples
fu5ha Sep 26, 2023
82a6817
fix link in footnote
fu5ha Sep 27, 2023
e2e8746
reword unpin auto impl section
fu5ha Sep 27, 2023
7c9c712
improve structural Unpin + formatting
fu5ha Sep 27, 2023
252a83b
add section on manual owning ptr managed solution via @kpreid
fu5ha Sep 28, 2023
f2447a6
fix typos
fu5ha Sep 28, 2023
921d37d
fix imports
fu5ha Sep 28, 2023
6d5f43d
edit new section for typos and better wording
fu5ha Sep 28, 2023
de2e748
fix typos and edit prose
fu5ha Sep 28, 2023
6e88279
`Pin<P>` -> `Pin<Ptr>`
fu5ha Oct 3, 2023
469c78b
improve `Pin` and `Pin::new` docs
fu5ha Oct 3, 2023
e0210e6
justify motivation of `Unpin` better
fu5ha Oct 3, 2023
f0827b3
fix broken link
fu5ha Oct 3, 2023
d7a886a
update ui tests
fu5ha Oct 3, 2023
9997114
improve `Pin::new_unchecked` docs
fu5ha Oct 3, 2023
058fb50
trim section on managed-box model
fu5ha Oct 3, 2023
0050676
Rephrase unpin docs in terms of pinning-agnosticness
Manishearth Jan 7, 2024
936ceb2
lifetime -> lifespan where relevant. improve docs on as_ref()
Manishearth Jan 7, 2024
4c25246
Clean up guarantees wording
Manishearth Jan 7, 2024
68bdedd
Apply suggestions from code review
Manishearth Jan 7, 2024
6553d0d
punctuation in parens
Manishearth Jan 7, 2024
6a54ed7
valid
Manishearth Jan 7, 2024
a573c7c
footnote on dropping futures
Manishearth Jan 7, 2024
b1830f1
clean up structural pinning
Manishearth Jan 7, 2024
df6d449
Update library/core/src/pin.rs
Manishearth Jan 7, 2024
7fd841c
link
Manishearth Jan 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
improve structural Unpin + formatting
  • Loading branch information
fu5ha authored and Manishearth committed Jan 7, 2024
commit 7c9c71260e8fded5cb52ca9418b6d99398be2b8c
18 changes: 10 additions & 8 deletions library/core/src/pin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -753,9 +753,11 @@
//! 1. *Structural [`Unpin`].* A struct can be [`Unpin`] if, and only if, all of its
Copy link
Member

Choose a reason for hiding this comment

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

This is not quite an "only if" -- the struct itself could be "where the pinning starts", like a manual implementation of a self-referential future.

Copy link
Member

Choose a reason for hiding this comment

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

This seems footnote-worthy but I don't think I can craft an appropriately nitpicky footnote for this

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer some sort of rewording here. This is basically stating a definition of Unpin but it's a different definition from what other parts of the docs say. Really a struct can be Unpin if and only if it is "pinning-agnostic", to use the terms from the trait dos.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is good for a followup if you want to take a stab at it.

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
//! 1. *Structural [`Unpin`].* A struct can be [`Unpin`] if, and only if, all of its
//! 1. *Structural [`Unpin`].* A struct can be [`Unpin`] only if all of its

Copy link
Member

@Manishearth Manishearth Jan 7, 2024

Choose a reason for hiding this comment

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

I think we need more clarity in this section if we're removing the "if and only if" but I think this is fine for now.

//! structurally-pinned fields are, too. This is [`Unpin`]'s behavior by default.
//! However, as a libray author, it is your responsibility not to write something like
//! <code>unsafe impl\<T> [Unpin] for Struct\<T> {}</code>. (Adding *any* projection
//! operation requires unsafe code, so the fact that [`Unpin`] is a safe trait does not break
//! the principle that you only have to worry about any of this if you use [`unsafe`].)
//! <code>impl\<T> [Unpin] for Struct\<T> {}</code> and then offer a method that provides
//! structural pinning to an inner field of `T`, which may not be [`Unpin`]! (Adding *any*
//! projection operation requires unsafe code, so the fact that [`Unpin`] is a safe trait does
//! not break the principle that you only have to worry about any of this if you use
//! [`unsafe`].)
//!
//! 2. *Pinned Destruction.* As discussed [above][drop-impl], [`drop`] takes
//! [`&mut self`], but the struct (and hence its fields) might have been pinned
Expand All @@ -764,14 +766,14 @@
//!
//! As a consequence, the struct *must not* be [`#[repr(packed)]`][packed].
//!
//! 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, as well.
//! 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, as well.
//!
//! This can be tricky, as witnessed by [`VecDeque<T>`]: the destructor of [`VecDeque<T>`]
//! can fail to call [`drop`] on all elements if one of the destructors panics. This violates
//! the [`Drop` guarantee][drop-guarantee], because it can lead to elements being deallocated without
//! their destructor being called.
//! the [`Drop` guarantee][drop-guarantee], because it can lead to elements being deallocated
//! without their destructor being called.
//!
//! [`VecDeque<T>`] has no pinning projections, so its destructor is sound. If it wanted
//! to provide such structural pinning, its destructor would need to abort the process if any
Expand Down