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

impl Unpin for Pin #49621

Merged
merged 2 commits into from
Apr 5, 2018
Merged

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Apr 3, 2018

@withoutboats
Copy link
Contributor

cc @cramertj @RalfJung

Does the impl already exist for PinBox?

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 3, 2018

No, I could add that as well if you want.

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2018
@RalfJung
Copy link
Member

RalfJung commented Apr 3, 2018

I thought we'd maybe wait a bit and at least make sure we have consensus on the guarantees provided by Pin before we add Unpin instances for all the types. The extra guarantee suggested by @cramertj is still "in the air" AFAIK? It's not in the RFC.

Once we are fine with that, we probably want instances for &, &mut, Box and Vec as well---or at least, we could add these.

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 3, 2018

I thought we'd maybe wait a bit and at least make sure we have consensus on the guarantees provided by Pin before we add Unpin instances for all the types.

I definitely agree with that in general.

For the specific case of impl Unpin for Pin I believe this must be valid, no matter what invariants/guarantees having a trait built on Pin<Self> requires. I have tried a couple of times to write out why I believe this, but I'm having real difficulty putting it into words.

Maybe one way to state it is: having an instance of Pin<T> states that you have guaranteed the invariants required to call methods on some trait Foo where Foo is defined only over Pin<Self>. Having an instance of Pin<Pin<T>> states nothing more about what invariants you guarantee, no matter what those invariants are, the inner Pin<T> is already guaranteeing them so the outer Pin<T> is isomorphic to &mut Pin<T>. Unpin is specifically a marker trait to denote that for some type U: Unpin then Pin<U> is isomorphic to &mut U. As I just stated this applies to Pin<Pin<T>>, therefore Pin<T>: Unpin.

Also, deciding whether this implementation is sound or not (before deciding for other types) is going to greatly affect those of us experimenting with Pin based APIs. If it's not sound then that blocks some potential API designs.


As I briefly mentioned in the tracking issue this is important for ergonomics. Similar to most object safe traits (presuming that Pin<Self> is eventually deemed to be object safe) if you have a trait like

trait Foo {
    fn foo(self: Pin<Self>);
}

then you want to provide an implementation for references to the trait, currently that requires unsafe code to perform the re-borrow (which, if `Pin: !Unpin is presumably unsound), but as I said above I believe that re-borrow to be guaranteed sound so it should be possible to do it with just safe code:

impl<'a, T: Foo + ?Sized + 'a> Foo for Pin<'a, T> {
    fn foo(mut self: Pin<Self>) {
        <T as Foo>::foo(Pin::borrow(unsafe { Pin::get_mut(&mut self) }))
        // <T as Foo>::foo(Pin::borrow(&mut *self))
    }
}

which would then allow functions to take trait objects:

fn bar(mut foo: Pin<Foo>) {
     Pin::borrow(&mut foo).foo()
}

and waaaay in the future if re-borrowing is formalised and made something that custom types can implement (or sooner if re-borrowing of Pin specifically is special cased into the compiler):

impl<'a, T: Foo + ?Sized + 'a> Foo for Pin<'a, T> {
    fn foo(self: Pin<Self>) {
        <T as Foo>::foo(*self)
    }
}

fn bar(foo: Pin<Foo>) {
    foo.foo()
}

I realise that arguing from how Pin could ergonomically behave isn't really the best way to show that it is definitely Unpin. But, I believe that if these ergonomic goals are not possible then Pin is going to be relegated to only very niche use cases. Whereas I am currently very hopeful that it's possible to build an amazing embedded framework designed from the ground up around immovable Futures.

@RalfJung
Copy link
Member

RalfJung commented Apr 4, 2018

I do not see any fundamental reason why we should not be able to declare that Pin acts "transitively" or "structurally" in terms of the pinned typestate. In this case, Pin<Pin<T>> would state that the inner pointer itself will never move. However, even in that case, going from Pin<'a, Pin<'b, T>> to Pin<'a, T> via dereferencing would be legal (it'd just have to be a primitive operation). Is that enough for your pattern?

However, I have to agree insofar as I cannot think if any way in which making Pin "transitive" in this way is useful. Usually making a type "transitive" gives rise to additional operations on Pin<T>; for example a "transitive" Vec would provide a fn get_pin(self: Pin<Vec<T>>, idx: usize) -> Pin<T>. This would be the only way to get a Pin for something inside a Vec. However, adding such an operation is incompatible with making Vec<T>: Unpin. However, in the case of Pin itself, the additional operation is getting a Pin to the inner thing, which you also get if we just make Pin<T>: Unpin. This is also consistent with the general impression I have that "pointer-like types" will typically be Unpin.


Concerning your use-case for Pin<Pin<T>>, I do not entirely follow. Why would I want to have a nested Pin here? I would think if I have a foo: Pin<Foo>, I can just call foo.foo() even without your additional impl; after all, foo already has exactly the right type.

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 4, 2018

Concerning your use-case for Pin<Pin>, I do not entirely follow. Why would I want to have a nested Pin here? I would think if I have a foo: Pin, I can just call foo.foo() even without your additional impl; after all, foo already has exactly the right type.

You're right, that use-case does not actually need the forwarding implementation (far too many re-revisions of my examples made that disappear 🙁). There are definitely use-cases that require it though.

Reading the API guideline on taking R: io::Read by value gives some hint towards why this forwarding implementation is useful, reusing the trait from above:

fn bar<F: Foo>(foo: F) {
    stack_pinned(foo, |mut foo: Pin<F>| { // When invoked from `baz2` this is `Pin<Pin<baz::F>>`
        Pin::borrow(&mut foo).foo();
    });
}

fn baz1<F: Foo>(foo: F) {
    bar(foo);
    bar(foo); // Error: use of moved value
}

fn baz2<F: Foo>(foo: F) {
    stack_pinned(foo, |mut foo: Pin<F>| {
        bar(Pin::borrow(&mut foo));
        bar(Pin::borrow(&mut foo)); // just `bar(foo);` with re-borrowing support
    }
}

bar is a function that takes in some Foo, uses stack pinning internally to ensure it's safe to call methods on Foo and calls the method.

baz1 attempts to take a Foo by value and call bar on it twice, this obviously errors because bar also takes foo by value.

baz2 is the same as baz1, but it internally stack pins foo before passing it to bar. This then makes the stack pinning in bar a no-op and uses the impl<F: Foo> Foo for Pin<F> implementation to forward the calls directly to the concrete type of Foo (and should optimize out to just direct function calls).


Writing this out made me realise another important reason for Pin: Unpin and traits providing forwarding implementations, if a function doesn't want to internally handle pinning its arguments it can require F: Foo + Unpin. This will then be callable with some instance that is actually Unpin, or the caller should be able to stack pin prior to calling the function (I think this is analogous with somehow making Future == StableFuture + Unpin for nicer interoperability).

fn bar<F: Foo + Unpin>(mut foo: F) {
    Pin::new(&mut foo).foo(); // just `foo.foo()` with `&pin` coercion I think...
}

fn baz<F: Foo>(foo: F) {
    stack_pinned(foo, |mut foo: Pin<F>| {
        bar(Pin::borrow(&mut foo));
    }
}

EDIT: Fixed some mut and &muts after testing, these examples do definitely compile with this PR's change.

@RalfJung
Copy link
Member

RalfJung commented Apr 5, 2018

Reading the API guideline on taking R: io::Read by value gives some hint towards why this forwarding implementation is useful

Interesting, I was not aware of this particular API trick.

There seems to be a confusion between bar and bar1 in your examples... did you mean fn bar<F: Foo>(foo: F) or does this refer back to the bar from the previous post?

Writing this out made me realise another important reason for Pin: Unpin and traits providing forwarding implementations, if a function doesn't want to internally handle pinning its arguments it can require F: Foo + Unpin. This will then be callable with some instance that is actually Unpin, or the caller should be able to stack pin prior to calling the function (I think this is analogous with somehow making Future == StableFuture + Unpin for nicer interoperability).

This is neat. I somehow feel like you're cheating here because you seem to get a free generalization from a function that requires Unpin to one that does not... but I can't point out anything wrong you'd be doing, so maybe this is fine?

I guess the reason this works is that baz consumes the foo: F, so it will be briefly pinned and then destroyed. That always works. There's no way to get anything similar for &mut F. So, not sure how useful your trick is, but it is definitely neat.


If I phrase this in terms of the formalism I started to develop, the question is: What is the pinned typestate invariant for PinBox<T>? (The situation for Pin is the same, just with an additional lifetime mixing things up a little.) I think the most natural choice is

PinBox<T>.pin(ptr) := exists |inner_ptr| ptr.points_to_owned(inner_ptr) && T.pin(inner_ptr)

I somehow though that would make PinBox<T>: Unpin only of T: Unpin, but I just realized I was entirely wrong. It seems I literally cannot come up with a model for PinBox and Pin that does not make them always Unpin... after all, their content is already always pinned anyway, so what else would you do?

@withoutboats
Copy link
Contributor

I believe (based on my sloppy informal reasoning) that all the smart pointers in libstd can implement Unpin regardless of their inner type. If @RalfJung agrees (based on his less sloppy semi-formal reasoning), then I think we should provide the impls.

@RalfJung
Copy link
Member

RalfJung commented Apr 5, 2018

For now I agree for Pin and PinBox. I think I realized yesterday that my model cannot actually handle interior collections yet, so I'd like to write a blogpost about them and understand them better before I make any more general claims.

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 5, 2018

There seems to be a confusion between bar and bar1 in your examples... did you mean fn bar<F: Foo>(foo: F) or does this refer back to the bar from the previous post?

Sorry, I originally had the two examples from that post in one, missed renaming the function in the first from bar1 to bar when splitting them, fixed now.

@withoutboats
Copy link
Contributor

@Nemo157 can you add PinBox to this PR? then I'll r+ :D

@withoutboats
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 5, 2018

📌 Commit a29d4d9 has been approved by withoutboats

@bors 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 Apr 5, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 5, 2018
bors added a commit that referenced this pull request Apr 5, 2018
Rollup of 8 pull requests

Successful merges:

 - #49045 (Make queries thread safe)
 - #49350 (Expand macros in `extern {}` blocks)
 - #49497 (Chalkify - Tweak `Clause` definition and HRTBs)
 - #49597 (proc_macro: Reorganize public API)
 - #49686 (typos)
- #49621
- #49697
- #49705

Failed merges:
@alexcrichton alexcrichton merged commit a29d4d9 into rust-lang:master Apr 5, 2018
@RalfJung RalfJung mentioned this pull request May 24, 2018
@Nemo157 Nemo157 deleted the impl-unpin-for-pin branch January 16, 2019 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants