Update to a new pinning API. #53877

withoutboats · 2018-09-01T04:56:23Z

~~Blocked on #53843 because of method resolution problems with new pin type.~~

cc @RalfJung @pythonesque anyone interested in #49150

rust-highfive · 2018-09-01T05:12: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:04:36] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:36] tidy error: /checkout/src/libcore/pin.rs:69: trailing whitespace
[00:04:37] some tidy checks failed
[00:04:37] 
[00:04:37] 
[00:04:37] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:37] 
[00:04:37] 
[00:04:37] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:37] Build completed unsuccessfully in 0:00:50
[00:04:37] Build completed unsuccessfully in 0:00:50
[00:04:37] Makefile:79: recipe for target 'tidy' failed
[00:04:37] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1b7ae70b
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0038a018:start=1535778313314153079,finish=1535778313322697020,duration=8543941
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1e33e658
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1383e2c2
travis_time:start:1383e2c2
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:11a2c5a7
$ dmesg | grep -i kill

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)

RalfJung · 2018-09-01T14:02:56Z

src/liballoc/boxed.rs

+#[unstable(feature = "pin", issue = "49150")]
+impl<T> From<Box<T>> for Pin<Box<T>> {
+    fn from(boxed: Box<T>) -> Self {
+        unsafe { Pin::new_unchecked(boxed) }


This could use a brief justification why the unsafe is okay.

RalfJung · 2018-09-01T14:02:59Z

src/liballoc/boxed.rs

+
+    #[unstable(feature = "pin", issue = "49150")]
+    pub fn pinned(x: T) -> Pin<Box<T>> {
+        unsafe { Pin::new_unchecked(box x) }


This could use a brief justification why the unsafe is okay.

src/libcore/pin.rs

RalfJung · 2018-09-01T14:07:52Z

src/libcore/pin.rs

    #[unstable(feature = "pin", issue = "49150")]
-    pub fn reborrow<'b>(&'b mut self) -> PinMut<'b, T> {
-        PinMut { inner: self.inner }
+    pub fn get(this: Pin<&'a T>) -> &'a T {


Why do we have set for all Pin<P> where P: DerefMut<Target = T>, but get only for Pin<&T>?

You can't use 'Deref' here since you want the lifetime to be equal to that of the original reference, not of your borrow of it.

Oh, I see... this is actually the method witnessing that we can safely have shared references to pinned data! It has such an innocent name, I did not realize that. :)

RalfJung · 2018-09-01T14:10:11Z

src/libcore/pin.rs

    fn deref(&self) -> &T {
-        &*self.inner
+        &*self.pointer


I'd prefer if you could use something like #49150 (comment). In fact, I'd prefer if Pin could be in its own little private local submodule which only exposes those 4 functions (as_ref, as_shr, unpin_mut, unpin_shr) and new_unchecked; and everything else lived outside that module. Just to be sure we do not accidentally unpin something anywhere here.

At the least, this could use get so that we only have one place witnessing the Pin<&T> -> &T conversion.

Unfortunately, you can't quite implement it all just using Deref like that, since Deref only gives you the lifetime of the borrow, not the lifetime of the original object.

Well there could be some core function that Deref and this uses.

Thomasdezeeuw · 2018-09-03T12:25:12Z

src/libcore/pin.rs

+    ///
+    /// This constructor is unsafe because we cannot guarantee that the target data
+    /// is properly pinned by this pointer. If the constructed `Pin<P>` does not guarantee
+    /// that the data is "pinned," constructing a `Pin<P>` is undefined behavior and could lead


The comma is inside the quotes and previously you used italics (*) rather then qoutes, but it's just a small thing.

cramertj · 2018-09-15T00:43:45Z

I've made some changes, including assorted cleanup, fixing the method resolution issue, making as_ref, as_mut, and set take self rather than this (to allow method-position syntax), generalizing the CoerceUnsized impl, and reexporting Unpin. PTAL.

r? @aturon

src/libcore/pin.rs

+
+    /// Get a mutable reference to the data inside of this `Pin`.
+    ///
+    /// This requires that the data inside this `Pin` is `Unpin`.


aturon · 2018-09-17T23:23:13Z

@cramertj After you address my comment, r=me.

cramertj · 2018-09-17T23:32:10Z

@bors r=aturon

bors · 2018-09-17T23:32:11Z

📌 Commit 3ec1810 has been approved by aturon

bors · 2018-09-17T23:43:25Z

⌛ Testing commit 3ec1810 with merge 8bbe4f147d0a8b407cc3a840dd3197dcb0145ed3...

RalfJung · 2018-09-18T09:45:18Z

src/libcore/pin.rs

+    pub fn new(pointer: P) -> Pin<P> {
+        // Safety: the value pointed to is `Unpin`, and so has no requirements
+        // around pinning.
+        unsafe { Pin::new_unchecked(pointer) }


Is this always okay? We have impl Deref for non-pointer types as well, and we have no safety contract at all for what you put into P::Target.

If P::Target is Unpin, I'd expect basically what my comment says: that Pin<ThingThatTargetsT> is semantically equivalent to ThingThatTargetsT (no pinning invariant exists).

I find that a rather surprising interaction. I might implement Deref like e.g. TyCtxt does, to "inherit" a whole bunch of methods from one of the members, and I will then usually not expect any interaction of that with pinning.

Or I might have a PinnedBox that does propagate pinning (that's safe, as long as Unpin is only conditionally implemented). Naturally I want Deref for PinnedBox.

When this new Pin API was introduced, it was clear that the "key point" here are the constructors that create a Pin. We discussed either making this manually for all types, or having a new unsafe trait that lets you do this. I do not remember seeing any blanket implementation like this that relies on an existing safe trait, and I am pretty worried about soundness of this. I'd feel much better if we could remove this part. What is the motivation?

IMO this new API was always just a different way to spell PinBox, PinMut, PinRef etc. that allowed for reusing some methods. This is one of the places where we get reuse: rather than providing safe new_rc, new_arc, new_ref, and new_mut functions where T: Unpin, we can have a single new method that supplies that bound.

Well, we get reuse and also expand this to every type ever that implements Deref. That's a big deal.^^ My impression was that constructors of Pin<P> would be carefully curated, because they are the only thing that's "stopping the flood" and making sure that this is all well-behaved.

That said, I cannot construct a counter-example. Pin<P> (where P is not &[mut]) won't ever do anything except for giving you a Pin<&[mut] P::Target>, and we check that that's harmless because P::Target: Unpin. So I am feeling uneasy but cannot actually think of a way in which it would break things.

src/libcore/pin.rs

RalfJung · 2018-09-18T18:23:33Z

src/libcore/pin.rs

+    P::Target: Unpin
+{
+    fn deref_mut(&mut self) -> &mut P::Target {
+        &mut *self.pointer


Actually, couldn't you implement this as Pin::get_mut(self.as_mut())?

I think so, yes.

Yay. :)

However, now the comments above as_mut/as_ref don't match reality any more, do they?

cramertj · 2018-09-18T18:48:46Z

@bors r=aturon

cramertj · 2018-09-19T00:31:39Z

@bors r=aturon

bors · 2018-09-19T00:31:40Z

📌 Commit 574bca7 has been approved by aturon

bors · 2018-09-19T06:56:28Z

⌛ Testing commit 574bca7 with merge 1e21c9a...

@r

Update to a new pinning API. ~~Blocked on #53843 because of method resolution problems with new pin type.~~ @r? @cramertj cc @RalfJung @pythonesque anyone interested in #49150

bors · 2018-09-19T09:20:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 1e21c9a to master...

rust-highfive assigned cramertj Sep 1, 2018

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

Update to a new pinning API.

974bdc8

withoutboats force-pushed the compositional-pin branch from 3f1811e to 974bdc8 Compare September 1, 2018 04:58

RalfJung reviewed Sep 1, 2018

View reviewed changes

src/libcore/pin.rs Show resolved Hide resolved

kennytm added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Sep 1, 2018

RalfJung reviewed Sep 1, 2018

View reviewed changes

Thomasdezeeuw reviewed Sep 3, 2018

View reviewed changes

TimNN removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2018

rust-highfive assigned aturon and unassigned cramertj Sep 15, 2018

cramertj changed the title ~~[DO NOT MERGE] Update to a new pinning API.~~ Update to a new pinning API. Sep 15, 2018

This comment has been minimized.

Sign in to view

cramertj force-pushed the compositional-pin branch from b2fc5e5 to 166392d Compare September 17, 2018 18:33

This comment has been minimized.

Sign in to view

cramertj force-pushed the compositional-pin branch from 166392d to b3a695b Compare September 17, 2018 21:13

aturon reviewed Sep 17, 2018

View reviewed changes

src/libcore/pin.rs

/// Get a mutable reference to the data inside of this `Pin`.

///

/// This requires that the data inside this `Pin` is `Unpin`.

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

Cleanup and fix method resolution issue

3ec1810

cramertj force-pushed the compositional-pin branch from b3a695b to 3ec1810 Compare September 17, 2018 23:31

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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Sep 17, 2018

Remove outdated rustdoc PinBox tests

403d12d

RalfJung reviewed Sep 18, 2018

View reviewed changes

src/libcore/pin.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

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 Sep 18, 2018

RalfJung reviewed Sep 18, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

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

Cleanup Deref impls and add ?Sized bound to &mut T impls

574bca7

cramertj force-pushed the compositional-pin branch from 85508e2 to 574bca7 Compare September 18, 2018 22:43

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 Sep 19, 2018

cramertj mentioned this pull request Sep 19, 2018

Remove spawning from task::Context #54339

Merged

bors merged commit 574bca7 into rust-lang:master Sep 19, 2018

bors mentioned this pull request Sep 19, 2018

Implement object-safety for arbitrary_self_types #50173

Closed

pnkfelix mentioned this pull request Sep 19, 2018

uiify run-pass #54223

Merged

Thomasdezeeuw mentioned this pull request Sep 20, 2018

Update to latest pinning API rust-lang/futures-rs#1265

Closed

Update to a new pinning API. #53877

Update to a new pinning API. #53877

Conversation

withoutboats commented Sep 1, 2018 • edited by cramertj Loading

rust-highfive commented Sep 1, 2018

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

cramertj commented Sep 15, 2018

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

aturon commented Sep 17, 2018

cramertj commented Sep 17, 2018

bors commented Sep 17, 2018

bors commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj commented Sep 18, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cramertj commented Sep 19, 2018

bors commented Sep 19, 2018

bors commented Sep 19, 2018

bors commented Sep 19, 2018

withoutboats commented Sep 1, 2018 •

edited by cramertj

Loading

RalfJung Sep 18, 2018 •

edited

Loading