expand the documentation on the `Unpin` trait #53104

nivkner · 2018-08-06T08:18:52Z

provides an overview of the Pin API which the trait is for,
and show how it can be used in making self referencial structs

part of #49150

provides an overview of the Pin API which the trait is for, and show how it can be used in making self referencial structs part of rust-lang#49150

rust-highfive · 2018-08-06T08:19:02Z

r? @Mark-Simulacrum

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

rust-highfive · 2018-08-06T10:14:53Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:43:26] ....................................................................................................
[00:43:28] ....................................................................................................
[00:43:31] ....................................................................................................
[00:43:33] ....................................................................................................
[00:43:36] iiiiiiiii...........................................................................................
[00:43:42] ....................................................................................................
[00:43:45] .....i..............................................................................................
[00:43:48] ..............i.....................................................................................
[00:43:51] ....................................................................................................
---
[01:09:29] travis_fold:end:stage0-linkchecker

[01:09:29] travis_time:end:stage0-linkchecker:start=1533550472595478625,finish=1533550474910704309,duration=2315225684

[01:09:30] std/marker/trait.Unpin.html:22: broken link - alloc/boxed/struct.PinMut.html
[01:09:38] core/marker/trait.Unpin.html:22: broken link - alloc/boxed/struct.PinMut.html
2920804 ./obj
2811796 ./obj/build
2205056 ./obj/build/x86_64-unknown-linux-gnu
792660 ./src
---
147700 ./obj/build/bootstrap/debug/incremental
145140 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu
145136 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
133276 ./obj/build/bootstrap/debug/incremental/bootstrap-jdsuci5s9dha
133272 ./obj/build/bootstrap/debug/incremental/bootstrap-jdsuci5s9dha/s-f3lhifvg0m-1jpurfj-1zjq8g928qpl7
132792 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Mark-Simulacrum · 2018-08-06T12:28:35Z

r? @RalfJung

steveklabnik · 2018-08-06T15:26:11Z

src/libcore/marker.rs

@@ -603,15 +603,99 @@ unsafe impl<T: ?Sized> Freeze for *mut T {}
 unsafe impl<'a, T: ?Sized> Freeze for &'a T {}
 unsafe impl<'a, T: ?Sized> Freeze for &'a mut T {}

-/// Types which can be moved out of a `PinMut`.
+/// A trait that indicates that it is safe to move an object of a type implementing it.


Rustdoc docs really like

/// A single sentence summary /// /// Real docs start here...

The old code was in this style, this patch throws it out.

steveklabnik

formatting/capitalization/punctuation nits

Thanks for this!

steveklabnik · 2018-08-06T15:26:27Z

src/libcore/marker.rs

+/// [`Deref`]: ../ops/trait.Deref.html
+/// [`swap`]: ../mem/fn.swap.html
+///
+/// # example


This should be /// # Examples

steveklabnik · 2018-08-06T15:26:36Z

src/libcore/marker.rs

+/// use std::marker::Pinned;
+/// use std::ptr::NonNull;
+///
+/// // this is a self referencial struct since the slice field points to the data field.


steveklabnik · 2018-08-06T15:26:41Z

src/libcore/marker.rs

+/// use std::ptr::NonNull;
+///
+/// // this is a self referencial struct since the slice field points to the data field.
+/// // we cannot inform the compiler about that with a normal reference,


steveklabnik · 2018-08-06T15:26:48Z

src/libcore/marker.rs

+/// // this is a self referencial struct since the slice field points to the data field.
+/// // we cannot inform the compiler about that with a normal reference,
+/// // since this pattern cannot be described with the usual borrowing rules.
+/// // instead we use a raw pointer, though one which is known to not be null,


steveklabnik · 2018-08-06T15:27:04Z

src/libcore/marker.rs

+/// // we cannot inform the compiler about that with a normal reference,
+/// // since this pattern cannot be described with the usual borrowing rules.
+/// // instead we use a raw pointer, though one which is known to not be null,
+/// // since we know its pointing at the string.


steveklabnik · 2018-08-06T15:27:16Z

src/libcore/marker.rs

+/// }
+///
+/// impl Unmovable {
+///     // to ensure the data doesn't move when the function returns,


steveklabnik · 2018-08-06T15:27:23Z

src/libcore/marker.rs

+/// impl Unmovable {
+///     // to ensure the data doesn't move when the function returns,
+///     // we place it in the heap where it will stay for the lifetime of the object,
+///     // and the only way to access it would be through a pointer to it


needs a period at the end

steveklabnik · 2018-08-06T15:27:31Z

src/libcore/marker.rs

+/// }
+///
+/// let unmoved = Unmovable::new("hello".to_string());
+/// // the pointer should point to the correct location,


steveklabnik · 2018-08-06T15:27:39Z

src/libcore/marker.rs

+/// let unmoved = Unmovable::new("hello".to_string());
+/// // the pointer should point to the correct location,
+/// // so long as the struct hasn't moved.
+/// // meanwhile, we are free to move the pointer around


Meanwhile, and a period at the end

steveklabnik · 2018-08-06T15:27:51Z

src/libcore/marker.rs

+/// let mut still_unmoved = unmoved;
+/// assert_eq!(still_unmoved.slice, NonNull::from(&still_unmoved.data));
+///
+/// // now the only way to access to data (safely) is immutably,


RalfJung · 2018-08-07T11:55:47Z

I disagree with the changed introduction: Unpin is not about moving (in general). That's why it is called Unpin, not Move. Rust still does not have truly "immovable types": if you have x: T, you can still do let y = Box::new(x) to move x around, for any T. In this sense (which is the only sense the compiler knows about), all types are safe to move, always.
Unpin is about moving after being pinned. It cannot be explained without explaining pinning. The proposed documentation first creates the wrong frame of mind and then later has to correct for this, saying "[Unpin does not] change the behavior of the compiler around moves" and giving an example. I think it would be much better to start with the right terminology from the beginning, and then later explain how this is different from truly immovable types.

However, I think the examples are very useful. :) And we certainly need that explanation about Unpin not being Move. Just the high-level structure needs some changes. And probably Unpin is not the best place to start. The plan in #49150 was to have a std::pin module and explain the gist of pinning there; Unpin can then build on top of that. Your example would probably fit better with that module-level documentation.

If you want to help push pinning over the stabilization line, I think that a good place to start: Having a central location (the std::pin module) where pinning is described, and then connect that with Unpin, PinMut and PinBox, respectively. However, I am not sure if @withoutboats or someone else already plans to do that, so I suggest you coordinate in #49150.

nivkner · 2018-08-07T18:24:37Z

removed the things that should go into module-level documentation.
in total this PR clarifies what it means by "move out of a pointer",
how this trait stops moves, added an Unpin != Move explanation,
and generalized the description to account for PinBox.

will try to help with the std::pin module in another PR.

RalfJung · 2018-08-13T19:08:21Z

src/libcore/marker.rs

+/// this trait cannot prevent types from moving by itself.
+///
+/// Instead it can be used to prevent moves through the type system,
+/// by controlling the behavior of special pointers types like [`PinMut`],


"pointer types" (spurious "s")

RalfJung · 2018-08-13T19:09:55Z

src/libcore/marker.rs

+///
+/// Instead it can be used to prevent moves through the type system,
+/// by controlling the behavior of special pointers types like [`PinMut`],
+/// which "pin" the type in place by wrapping it in a type which can only be dereferenced immutably.


Does the "by ..." part help? It doesn't really explain anything, does it?

Maybe just link to the PinMut docs. Or maybe say something along the lines of "by not allowing it to be moved out e.g. through mutable references (unless T: Unpin)".

RalfJung · 2018-08-13T19:10:34Z

src/libcore/marker.rs

+/// which "pin" the type in place by wrapping it in a type which can only be dereferenced immutably.
+///
+/// Implementing this trait lifts the restrictions of pinning off a type,
+/// which then allows it to move out of said pointers with functions such as [`swap`].


The remark to swap is a bit mysterious. Maybe add an example?

RalfJung · 2018-08-13T19:10:47Z

LGTM! Just some minor comments.

RalfJung · 2018-08-17T16:29:55Z

src/libcore/marker.rs

-/// by controlling the behavior of special pointers types like [`PinMut`],
-/// which "pin" the type in place by wrapping it in a type which can only be dereferenced immutably.
+/// by controlling the behavior of special pointer types like [`PinMut`],
+/// which "pin" the type in place by not allowing it to be moved out via mutable references.


I would remove the "via" part or make it a "for example" -- after all, PinMut prevents all ways to move something out. It doesn't just prevent moving out via &mut.

RalfJung · 2018-08-17T16:30:42Z

src/libcore/marker.rs

+///
+/// let mut string = "this".to_string();
+/// let mut pinned_string = PinMut::new(&mut string);
+/// replace(&mut *pinned_string, "other".to_string());


I'd add a comment in the code here saying that the &mut * is where we make use of the Unpin

RalfJung · 2018-08-21T09:36:53Z

Awesome, thanks!

@bors r+

bors · 2018-08-21T09:36:54Z

📌 Commit 6ae915b has been approved by RalfJung

RalfJung · 2018-08-21T09:37:14Z

@bors p=rollup

RalfJung · 2018-08-21T09:38:31Z

@bors r+ rollup

bors · 2018-08-21T09:38:32Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Replace usages of ptr::offset with ptr::{add,sub}. #53329

bors · 2018-08-21T09:38:33Z

📌 Commit 6ae915b has been approved by RalfJung

expand the documentation on the `Unpin` trait provides an overview of the Pin API which the trait is for, and show how it can be used in making self referencial structs part of rust-lang#49150

@ghost

Rollup of 17 pull requests Successful merges: - #53030 (Updated RELEASES.md for 1.29.0) - #53104 (expand the documentation on the `Unpin` trait) - #53213 (Stabilize IP associated constants) - #53296 (When closure with no arguments was expected, suggest wrapping) - #53329 (Replace usages of ptr::offset with ptr::{add,sub}.) - #53363 (add individual docs to `core::num::NonZero*`) - #53370 (Stabilize macro_vis_matcher) - #53393 (Mark libserialize functions as inline) - #53405 (restore the page title after escaping out of a search) - #53452 (Change target triple used to check for lldb in build-manifest) - #53462 (Document Box::into_raw returns non-null ptr) - #53465 (Remove LinkMeta struct) - #53492 (update lld submodule to include RISCV patch) - #53496 (Fix typos found by codespell.) - #53521 (syntax: Optimize some literal parsing) - #53540 (Moved issue-53157.rs into src/test/ui/consts/const-eval/) - #53551 (Avoid some Place clones.) Failed merges: r? @ghost

@withoutboats

move the Pin API into its own module for centralized documentation This implements the change proposed by @withoutboats in #49150, as suggested by @RalfJung in the review of #53104, along with the documentation that was originally in it, that was deemed more appropriate in module-level documentation. r? @RalfJung

expand the documentation on the Unpin trait

038ce65

provides an overview of the Pin API which the trait is for, and show how it can be used in making self referencial structs part of rust-lang#49150

rust-highfive assigned Mark-Simulacrum Aug 6, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2018

correct explenation on the usage of NonNull

6845dc4

nivkner force-pushed the unpin_doc branch 2 times, most recently from 6374346 to 6845dc4 Compare August 6, 2018 10:06

fix link to PinBox

9b7d710

rust-highfive assigned RalfJung and unassigned Mark-Simulacrum Aug 6, 2018

steveklabnik reviewed Aug 6, 2018

View reviewed changes

steveklabnik suggested changes Aug 6, 2018

View reviewed changes

fix style issues in doc comment

87bbd2e

nivkner force-pushed the unpin_doc branch from 69a8994 to 87bbd2e Compare August 6, 2018 21:01

remove general pinning information from Unpin trait

68e766a

This was referenced Aug 7, 2018

Tracking issue for Pin APIs (RFC 2349) #49150

Closed

move the Pin API into its own module for centralized documentation #53227

Merged

RalfJung reviewed Aug 13, 2018

View reviewed changes

add example for moving out of pointer

03530fa

RalfJung reviewed Aug 17, 2018

View reviewed changes

clarify use of Unpin and pinning types

6ae915b

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2018

kennytm mentioned this pull request Aug 21, 2018

Rollup of 17 pull requests #53530

Merged

bors merged commit 6ae915b into rust-lang:master Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expand the documentation on the `Unpin` trait #53104

expand the documentation on the `Unpin` trait #53104

nivkner commented Aug 6, 2018

rust-highfive commented Aug 6, 2018

rust-highfive commented Aug 6, 2018

Mark-Simulacrum commented Aug 6, 2018

steveklabnik Aug 6, 2018

steveklabnik left a comment

steveklabnik Aug 6, 2018

steveklabnik Aug 6, 2018

steveklabnik Aug 6, 2018

steveklabnik Aug 6, 2018

steveklabnik Aug 6, 2018

steveklabnik Aug 6, 2018

steveklabnik Aug 6, 2018

steveklabnik Aug 6, 2018

steveklabnik Aug 6, 2018

steveklabnik Aug 6, 2018

RalfJung commented Aug 7, 2018 •

edited

Loading

nivkner commented Aug 7, 2018 •

edited

Loading

RalfJung Aug 13, 2018

RalfJung Aug 13, 2018

RalfJung Aug 13, 2018

RalfJung commented Aug 13, 2018

RalfJung Aug 17, 2018 •

edited

Loading

RalfJung Aug 17, 2018

RalfJung commented Aug 21, 2018

bors commented Aug 21, 2018

RalfJung commented Aug 21, 2018

RalfJung commented Aug 21, 2018

bors commented Aug 21, 2018

bors commented Aug 21, 2018

expand the documentation on the Unpin trait #53104

expand the documentation on the Unpin trait #53104

Conversation

nivkner commented Aug 6, 2018

rust-highfive commented Aug 6, 2018

rust-highfive commented Aug 6, 2018

Mark-Simulacrum commented Aug 6, 2018

Choose a reason for hiding this comment

steveklabnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Aug 7, 2018 • edited Loading

nivkner commented Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Aug 13, 2018

RalfJung Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Aug 21, 2018

bors commented Aug 21, 2018

RalfJung commented Aug 21, 2018

RalfJung commented Aug 21, 2018

bors commented Aug 21, 2018

bors commented Aug 21, 2018

expand the documentation on the `Unpin` trait #53104

expand the documentation on the `Unpin` trait #53104

RalfJung commented Aug 7, 2018 •

edited

Loading

nivkner commented Aug 7, 2018 •

edited

Loading

RalfJung Aug 17, 2018 •

edited

Loading