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

Tracking Issue for core::pin::pin! #93178

Closed
4 tasks done
danielhenrymantilla opened this issue Jan 21, 2022 · 10 comments
Closed
4 tasks done

Tracking Issue for core::pin::pin! #93178

danielhenrymantilla opened this issue Jan 21, 2022 · 10 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Jan 21, 2022

Feature gate: #![feature(pin_macro)]

This is a tracking issue for core::pin::pin!, which allows pinning values to the stack / local scope.

Public API

// core::pin

/// API: `fn pin<T>($value: T) -> Pin<&'local mut T>`
pub macro pin($value:expr $(,)?) {}

Steps / History

(un)Resolved Questions

@danielhenrymantilla danielhenrymantilla added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 21, 2022
@danielhenrymantilla danielhenrymantilla changed the title Tracking Issue for the core::pin::pin! Tracking Issue for core::pin::pin! Jan 21, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 15, 2022
…ro, r=m-ou-se

Add a stack-`pin!`-ning macro to `core::pin`.

  - rust-lang#93178

`pin!` allows pinning a value to the stack. Thanks to being implemented in the stdlib, which gives access to `macro` macros, and to the private `.pointer` field of the `Pin` wrapper, [it was recently discovered](https://rust-lang.zulipchat.com/#narrow/stream/187312-wg-async-foundations/topic/pin!.20.E2.80.94.20the.20.22definitive.22.20edition.20.28a.20rhs-compatible.20pin-nin.2E.2E.2E/near/268731241) ([archive link](https://zulip-archive.rust-lang.org/stream/187312-wg-async-foundations/topic/A.20rhs-compatible.20pin-ning.20macro.html#268731241)), contrary to popular belief, that it is actually possible to implement and feature such a macro:

```rust
let foo: Pin<&mut PhantomPinned> = pin!(PhantomPinned);
stuff(foo);
```
or, directly:

```rust
stuff(pin!(PhantomPinned));
```

  - For context, historically, this used to require one of the two following syntaxes:

      - ```rust
        let foo = PhantomPinned;
        pin!(foo);
        stuff(foo);
        ```

      -  ```rust
         pin! {
             let foo = PhantomPinned;
         }
         stuff(foo);
         ```

This macro thus allows, for instance, doing things like:

```diff
fn block_on<T>(fut: impl Future<Output = T>) -> T {
    // Pin the future so it can be polled.
-   let mut fut = Box::pin(fut);
+   let mut fut = pin!(fut);

    // Create a new context to be passed to the future.
    let t = thread::current();
    let waker = Arc::new(ThreadWaker(t)).into();
    let mut cx = Context::from_waker(&waker);

    // Run the future to completion.
    loop {
        match fut.as_mut().poll(&mut cx) {
            Poll::Ready(res) => return res,
            Poll::Pending => thread::park(),
        }
    }
}
```

  - _c.f._, https://doc.rust-lang.org/1.58.1/alloc/task/trait.Wake.html

And so on, and so forth.

I don't think such an API can get better than that, barring full featured language support (`&pin` references or something), so I see no reason not to start experimenting with featuring this in the stdlib already 🙂

  - cc `@rust-lang/wg-async-foundations` \[EDIT: this doesn't seem to have pinged anybody 😩, thanks `@yoshuawuyts` for the real ping\]

r? `@joshtriplett`

___

# Docs preview

https://user-images.githubusercontent.com/9920355/150605731-1f45c2eb-c9b0-4ce3-b17f-2784fb75786e.mp4

___

# Implementation

The implementation ends up being dead simple (so much it's embarrassing):

```rust
pub macro pin($value:expr $(,)?) {
    Pin { pointer: &mut { $value } }
}
```

_and voilà_!

  - The key for it working lies in [the rules governing the scope of anonymous temporaries](https://doc.rust-lang.org/1.58.1/reference/destructors.html#temporary-lifetime-extension).

<details><summary>Comments and context</summary>

This is `Pin::new_unchecked(&mut { $value })`, so, for starters, let's
review such a hypothetical macro (that any user-code could define):
```rust
macro_rules! pin {( $value:expr ) => (
    match &mut { $value } { at_value => unsafe { // Do not wrap `$value` in an `unsafe` block.
        $crate::pin::Pin::<&mut _>::new_unchecked(at_value)
    }}
)}
```

Safety:
  - `type P = &mut _`. There are thus no pathological `Deref{,Mut}` impls that would break `Pin`'s invariants.
  - `{ $value }` is braced, making it a _block expression_, thus **moving** the given `$value`, and making it _become an **anonymous** temporary_.
    By virtue of being anonynomous, it can no longer be accessed, thus preventing any attemps to `mem::replace` it or `mem::forget` it, _etc._

This gives us a `pin!` definition that is sound, and which works, but only in certain scenarios:

  - If the `pin!(value)` expression is _directly_ fed to a function call:
    `let poll = pin!(fut).poll(cx);`

  - If the `pin!(value)` expression is part of a scrutinee:

    ```rust
    match pin!(fut) { pinned_fut => {
        pinned_fut.as_mut().poll(...);
        pinned_fut.as_mut().poll(...);
    }} // <- `fut` is dropped here.
    ```

Alas, it doesn't work for the more straight-forward use-case: `let` bindings.

```rust
let pinned_fut = pin!(fut); // <- temporary value is freed at the end of this statement
pinned_fut.poll(...) // error[E0716]: temporary value dropped while borrowed
                     // note: consider using a `let` binding to create a longer lived value
```

  - Issues such as this one are the ones motivating rust-lang/rfcs#66

This makes such a macro incredibly unergonomic in practice, and the reason most macros out there had to take the path of being a statement/binding macro (_e.g._, `pin!(future);`) instead of featuring the more intuitive ergonomics of an expression macro.

Luckily, there is a way to avoid the problem. Indeed, the problem stems from the fact that a temporary is dropped at the end of its enclosing statement when it is part of the parameters given to function call, which has precisely been the case with our `Pin::new_unchecked()`!

For instance,

```rust
let p = Pin::new_unchecked(&mut <temporary>);
```

becomes:

```rust
let p = { let mut anon = <temporary>; &mut anon };
```

However, when using a literal braced struct to construct the value, references to temporaries can then be taken. This makes Rust change the lifespan of such temporaries so that they are, instead, dropped _at the end of the enscoping block_.

For instance,
```rust
let p = Pin { pointer: &mut <temporary> };
```

becomes:

```rust
let mut anon = <temporary>;
let p = Pin { pointer: &mut anon };
```

which is *exactly* what we want.

Finally, we don't hit problems _w.r.t._ the privacy of the `pointer` field, or the unqualified `Pin` name, thanks to `decl_macro`s being _fully_ hygienic (`def_site` hygiene).

</details>

___

# TODO

  - [x] Add compile-fail tests with attempts to break the `Pin` invariants thanks to the macro (_e.g._, try to access the private `.pointer` field, or see what happens if such a pin is used outside its enscoping scope (borrow error));
  - [ ] Follow-up stuff:
      - [ ] Try to experiment with adding `pin!` to the prelude: this may require to be handled with some extra care, as it may lead to issues reminiscent of those of `assert_matches!`: rust-lang#82913
      - [x] Create the tracking issue.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 20, 2022
Fix the generator example for `pin!()`

The previous generator example is not actually self-referential, since the reference is created after the yield.

CC rust-lang#93178 (tracking issue)
@jsenkpiel-godaddy
Copy link

I feel like this should be called pin_mut! and have a pin_ref! counterpart, because you can't feed it T but only &mut T/&T.

@RalfJung
Copy link
Member

Not sure what you mean, you can give it a T just fine. You will get a Pin<&mut T>, but that can be easily converted into Pin<&T> if needed via as_ref.

@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented Oct 29, 2022

Ok, after all this time, there have been no issues with nightly users of this feature, and the macro seems as good as it is possible to have at the moment.

The main unresolved question being about the naming of the macro: should it remain as pin!, or should it become pin_mut! to pave the way for a pin_ref! counterpart?

While pin_mut! would be a safe / conservative choice, I think it would be overkill here.
To illustrate why, let's start by drawing some parallels with Box or other ownership-based APIs.

This is a legitimate think to do, since pin!ning is the stack/local counterpart of Box::pinning, i.e., its key aspect is ownership-based, not borrowing-based: contrary to the usual getter design of .as_{ref,mut}, .deref{,_mut}, ptr::addr_of{_mut}!, pin! consumes ownership of its input!

And in the same fashion that we have Box::leak() consuming ownership of the input, and returning a borrow in output, a borrow which happens to be an exclusive one, pin!'s design is quite similar.

And we don't have have Box::leak_mut() + Box::leak_ref(), we just just have Box::leak(). Users wanting a shared reference after leaking just have to &* (the idea being that such function returns &mut because it's maximally capable, a shared reference being a strictly less powerful API):

let p: &mut _ = Box::leak(thing); // `thing` **moved out**
let r: &_ = &*p;
let p: Pin<&mut _> = pin!(thing); // `thing` **moved out**
let r: Pin<&_> = p.as_ref();
  • (there is one difference, though: pin! uses subtle lifetime extension1, which doesn't allow for methods to be directly called on the pin!ned thing as it is being assigned to a local. This makes the two lines in the example be required for proper usage, which isn't the case for Box).

On the contrary, if we went with pin_mut!, we'd then have a symmetric pin_ref! / pin_mut! situation, which I actually find not that useful:

That is, there is little motivation in having a symmetric convenience API for a situation which is hugely asymmetric.

Worse, keeping the symmetry will actually end up being confusing, much like having Box::leak_mut() vs. Box::leak_ref() could have been: since the ref variant could be implemented in terms of the mut one, users could legitimately wonder if there was some other difference they might have missed: it would be a case of accidental complexity, as I view it.

  • That is: having both pin_mut! and pin_ref! could be confusing for Alan, should they decide to try to write block_on, select!, or something like that without resorting to Box::pin.

Conclusion

pin_mut! does not seem to pull its weight compared to pin!; we should go with the latter, and ensure a "pit of success" for people such as Alan to use the right tool from the start 🙂, and avoid some bits of accidental complexity.

  • (In the future, should pin_ref! really be deemed useful, we could always imitate the .project() / .project_ref() design of #[pin_project]: pin! and pin_ref!. This would offer the tiny extra flexibility of not requiring two statements for the Pin<&_> case, while still heavily favoring the &mut case, as it should be).

Footnotes

  1. [EDIT] as @tmandry points out, this doesn't even have to remain being an issue in the long term, as finding a way to solve RFC 66 (Better temporary lifetimes (tracking issue for RFC 66) #15023) could help fix this 🤞

  2. Granted, .project() is a potentially more frequent name for other APIs, but from randomly skimming the grep.app page results, I was seeing elements indicative of work on Futures or Pinning.

@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented Oct 29, 2022

That's why I'd vote for:

  • sticking to pin! (no pin_mut!), and should people agree with this, it means it's time for:
  • starting an FCP for its stabilization (should I make a tentative stabilization PR to trigger it, or should I wait for the FCP before making the PR?)

@tmandry
Copy link
Member

tmandry commented Oct 29, 2022

This all sounds good to me. I'm excited to see this move forward, thank you for pushing on it!

should I make a tentative stabilization PR to trigger it, or should I wait for the FCP to before make the PR?

Either way is fine but personally I'd make a PR, less chance of getting lost :)

@tmandry
Copy link
Member

tmandry commented Oct 29, 2022

  • (there is one difference, though: pin! uses subtle lifetime extension, which doesn't allow for methods to be directly called on the pin!ned thing as it is being assigned to a local. This makes the two lines in the example be required for proper usage, which isn't the case for Box).

It's worth stating that this is a deficiency of Rust's lifetime extension rules (a deficiency that the definition of pin! itself works around by being in std), and would be fixed when RFC 66 is implemented (#15023).

JohnTitor pushed a commit to JohnTitor/rust that referenced this issue Jan 8, 2023
…macro, r=dtolnay

Stabilize `::{core,std}::pin::pin!`

As discussed [over here](rust-lang#93178 (comment)), it looks like a decent time to stabilize the `pin!` macro.

### Public API

```rust
// in module `core::pin`

/// API: `fn pin<T>($value: T) -> Pin<&'local mut T>`
pub macro pin($value:expr $(,)?) {
    …
}
```

  - Tracking issue: rust-lang#93178

(now all this needs is an FCP by the proper team?)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jan 12, 2023
…macro, r=dtolnay

Stabilize `::{core,std}::pin::pin!`

As discussed [over here](rust-lang#93178 (comment)), it looks like a decent time to stabilize the `pin!` macro.

### Public API

```rust
// in module `core::pin`

/// API: `fn pin<T>($value: T) -> Pin<&'local mut T>`
pub macro pin($value:expr $(,)?) {
    …
}
```

  - Tracking issue: rust-lang#93178

(now all this needs is an FCP by the proper team?)
compiler-errors added a commit to compiler-errors/rust that referenced this issue Jan 12, 2023
…macro, r=dtolnay

Stabilize `::{core,std}::pin::pin!`

As discussed [over here](rust-lang#93178 (comment)), it looks like a decent time to stabilize the `pin!` macro.

### Public API

```rust
// in module `core::pin`

/// API: `fn pin<T>($value: T) -> Pin<&'local mut T>`
pub macro pin($value:expr $(,)?) {
    …
}
```

  - Tracking issue: rust-lang#93178

(now all this needs is an FCP by the proper team?)
RalfJung pushed a commit to RalfJung/miri that referenced this issue Jan 13, 2023
…dtolnay

Stabilize `::{core,std}::pin::pin!`

As discussed [over here](rust-lang/rust#93178 (comment)), it looks like a decent time to stabilize the `pin!` macro.

### Public API

```rust
// in module `core::pin`

/// API: `fn pin<T>($value: T) -> Pin<&'local mut T>`
pub macro pin($value:expr $(,)?) {
    …
}
```

  - Tracking issue: #93178

(now all this needs is an FCP by the proper team?)
@jkarneges
Copy link
Contributor

jkarneges commented Jan 17, 2023

For anyone wanting to start using this style of pinning without having to bump their MSRV, below is a workaround. The extra Option, Deref, and Future impls helped make it work as a drop-in substitution in our application (no need for extra .as_mut() calls). Feedback welcome.

use std::future::Future;
use std::ops::Deref;
use std::pin::Pin;
use std::task::{Context, Poll};

pub struct Pinner<'a, T> {
    pub unsafe_pointer: &'a mut T,
}

impl<'a, T> Pinner<'a, T> {
    pub fn as_mut(&mut self) -> Pin<&mut T> {
        // SAFETY: as long as Pinner is only ever constructed via the pin!()
        // macro and the unsafe_pointer field is never directly accessed,
        // then the value is safe to pin here. this is because the macro
        // ensures the input is turned into a borrowed anonymous temporary,
        // preventing any further access to the original value after Pinner
        // is constructed, and Pinner has no methods that enable moving out
        // of the reference. the word "unsafe" is used in the field name to
        // discourage direct access. this is the best we can do, since the
        // field must be public for the macro to work.
        unsafe { Pin::new_unchecked(self.unsafe_pointer) }
    }

    pub fn set(&mut self, value: T) {
        self.as_mut().set(value)
    }
}

impl<'a, T> Pinner<'a, Option<T>> {
    pub fn as_pin_mut(&mut self) -> Option<Pin<&mut T>> {
        self.as_mut().as_pin_mut()
    }
}

impl<'a, T> Deref for Pinner<'a, T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        self.unsafe_pointer
    }
}

impl<'a, T> Future for Pinner<'a, T>
where
    T: Future,
{
    type Output = T::Output;

    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        T::poll(Pin::into_inner(self).as_mut(), cx)
    }
}

#[macro_export]
macro_rules! pin {
    ($x:expr) => {
        crate::Pinner {
            unsafe_pointer: &mut { $x },
        }
    };
}

@ojeda
Copy link
Contributor

ojeda commented Apr 1, 2023

Should this one be closed?

thomcc pushed a commit to tcdi/postgrestd that referenced this issue May 31, 2023
…dtolnay

Stabilize `::{core,std}::pin::pin!`

As discussed [over here](rust-lang/rust#93178 (comment)), it looks like a decent time to stabilize the `pin!` macro.

### Public API

```rust
// in module `core::pin`

/// API: `fn pin<T>($value: T) -> Pin<&'local mut T>`
pub macro pin($value:expr $(,)?) {
    …
}
```

  - Tracking issue: #93178

(now all this needs is an FCP by the proper team?)
@carrycooldude
Copy link

Error is coming on this one please check:

error[E0658]: use of unstable library feature 'pin_macro'
--> /home/kartikey/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/wiremock-0.5.19/src/mock_server/bare_server.rs:247:32
|
247 | let mut notification = pin!(notify.notified());
| ^^^
|
= note: see issue #93178 #93178 for more information

Checking fake v2.8.0

error[E0658]: use of unstable library feature 'pin_macro'
--> /home/kartikey/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/wiremock-0.5.19/src/mock_server/bare_server.rs:6:5
|
6 | use std::pin::pin;
| ^^^^^^^^^^^^^
|
= note: see issue #93178 #93178 for more information

@danielhenrymantilla
Copy link
Contributor Author

@carrycooldude this means the version of rust (toolchain) you are using is too old for that wiremock (transitive?) dependency you have. Please use ≥1.68.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants