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

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

Merged
merged 8 commits into from
Feb 15, 2022

Conversation

danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Jan 21, 2022

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 (archive link), contrary to popular belief, that it is actually possible to implement and feature such a macro:

let foo: Pin<&mut PhantomPinned> = pin!(PhantomPinned);
stuff(foo);

or, directly:

stuff(pin!(PhantomPinned));
  • For context, historically, this used to require one of the two following syntaxes:

    • let foo = PhantomPinned;
      pin!(foo);
      stuff(foo);
    • pin! {
          let foo = PhantomPinned;
      }
      stuff(foo);

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

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(),
        }
    }
}

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

video1167605346.mp4

Implementation

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

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

and voilà!

Comments and context

This is Pin::new_unchecked(&mut { $value }), so, for starters, let's
review such a hypothetical macro (that any user-code could define):

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:

    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.

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

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,

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

becomes:

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,

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

becomes:

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_macros being fully hygienic (def_site hygiene).


TODO

  • 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:

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2022
@danielhenrymantilla
Copy link
Contributor Author

@rustbot modify labels: +T-libs-api +A-async-await +F-decl_macro +A-pin

@rustbot rustbot added A-async-await Area: Async & Await A-pin Area: Pin F-decl_macro `#![feature(decl_macro)]` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 21, 2022
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@ibraheemdev ibraheemdev left a comment

Choose a reason for hiding this comment

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

👍, but I think this deserves a beginner-friendly explanation on top of why this is needed/useful, e.g: https://docs.rs/tokio/latest/tokio/macro.pin.html. Also, this needs a tracking issue.

@rust-log-analyzer

This comment has been minimized.

@yoshuawuyts
Copy link
Member

cc @rust-lang/wg-async-foundations

@cramertj
Copy link
Member

What led to the decision to call this pin! rather than pin_mut!? I'm aware of inconsistency here between futures and tokio, and it would be good to resolve.

Personally, I think that since there are known use-cases for Pin<&T>, it seems like it would be valuable to offer both pin_ref and pin_mut, as is standard in other library APIs.

@cramertj
Copy link
Member

(That said, it also requires move-access to a value in order to use this macro even in the &-case, and the resulting Pin<&mut T> can be converted into a Pin<&T>, so I'd understand only offering this one.)

@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented Jan 21, 2022

@cramertj Right now I just followed the more pervasive name out there, but I have no opinions on this matter whatsoever.
In a way, I even prefer pin_mut!, but I have to admit I like the look of that s/Box::pin/pin!/diff 😄. Ideally we'd have had something like <&mut _>::pin! or mut_ref::pin!, so as to be consistent with Box::pin naming, but lacking that, I can see the appeal of pin_mut!.

FWIW, now that the macro is so ergonomic, there is an argument to be made for pin_ref! (besides sheer symmetry):
let x = pin_mut!(…).as_ref(); will yield a "dangling"/unusable x, because the .as_ref() then breaks the temporary promotion.

So with all that being said, I personally lean ever so slightly towards pin_mut!, but let's see what others think on the matter. It's something easy to change before the stabilization anyways. Lemme add the concern to the tracking issue.

@rust-log-analyzer

This comment has been minimized.

@danielhenrymantilla danielhenrymantilla marked this pull request as ready for review January 21, 2022 23:16
@eddyb
Copy link
Member

eddyb commented Jan 22, 2022

cc @nikomatsakis This could maybe use a more direct reference to the let RHS temporaries "scope extension" rules, but I forget where this is described, the best I can find is a section in the reference.

@rust-log-analyzer

This comment has been minimized.

@danielhenrymantilla
Copy link
Contributor Author

@eddyb agreed, I searched among the RFCs and surprisingly there was no normative stuff. But that reference link you provided is great! I'll add a permalink to it to the comments tomorrow

@danielhenrymantilla
Copy link
Contributor Author

The exact rules for temporary lifetime extension are subject to change. This is describing the current behavior only.

Hmm, isn't it super-hard for these rules to change? Both reducing and enlarging the scope of temporaries can break code out there, both at compile-time (if borrows are involved) or at runtime (if lock guards are involved).

@eholk
Copy link
Contributor

eholk commented Jan 22, 2022

This looks awesome!

I suppose I'd have a slight preference towards pin_mut! as well, for symmetry with similar APIs, but I don't feel strongly. Having a short name like pin! is also nice.

@taiki-e
Copy link
Member

taiki-e commented Jan 22, 2022

I would prefer pin! over pin_mut!. (I've considered its renaming in the past in pin-utils and futures: rust-lang/pin-utils#32)

Considering the stack pinning utilities for shared references are unneeded, we probably don't need mut of pin_mut! macro.

@Darksonn
Copy link
Contributor

I know that people sometimes claim that Pin<&T> is useful in some cases, but I have yet to see an actual use-case. I support the name pin!.

library/core/src/pin.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

danielhenrymantilla and others added 3 commits February 14, 2022 17:27
Co-Authored-By: Mara Bos <m-ou.se@m-ou.se>
Since `decl_macro`s and/or `Span::def_site()` is deemed quite unstable,
no public-facing macro that relies on it can hope to be, itself, stabilized.

We circumvent the issue by no longer relying on field privacy for safety and,
instead, relying on an unstable feature-gate to act as the gate keeper for
non users of the macro (thanks to `allow_internal_unstable`).

This is technically not correct (since a `nightly` user could technically enable
the feature and cause unsoundness with it); or, in other words, this makes the
feature-gate used to gate the access to the field be (technically unsound, and
in practice) `unsafe`. Hence it having `unsafe` in its name.

Back to the macro, we go back to `macro_rules!` / `mixed_site()`-span rules thanks
to declaring the `decl_macro` as `semitransparent`, which is a hack to basically have
`pub macro_rules!`

Co-Authored-By: Mara Bos <m-ou.se@m-ou.se>
This thus still makes it technically possible to enable the feature, and thus
to trigger UB without `unsafe`, but this is fine since incomplete features are
known to be potentially unsound (labelled "may not be safe").

This follows from the discussion at rust-lang#93176 (comment)
@m-ou-se
Copy link
Member

m-ou-se commented Feb 14, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 14, 2022

📌 Commit bf2a9dc has been approved by m-ou-se

@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 Feb 14, 2022
@bors
Copy link
Contributor

bors commented Feb 15, 2022

⌛ Testing commit bf2a9dc with merge 6421a49...

@bors
Copy link
Contributor

bors commented Feb 15, 2022

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 6421a49 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 15, 2022
@bors bors merged commit 6421a49 into rust-lang:master Feb 15, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 15, 2022
@m-ou-se m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 15, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6421a49): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

yvt added a commit to r3-os/r3 that referenced this pull request Jun 18, 2022
`core::pin::pin!` was unstably added by [rust-lang/rust#93176][1]. This
replaces `pin_utils::pin_mut!`.

[1]: rust-lang/rust#93176
@est31 est31 mentioned this pull request Mar 12, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 8, 2024
…, r=Amanieu

Rename `pointer` field on `Pin`

A few days ago, I was helping another user create a self-referential type using `PhantomPinned`. However, I noticed an odd behavior when I tried to access one of the type's fields via `Pin`'s `Deref` impl:

```rust
use std::{marker::PhantomPinned, ptr};

struct Pinned {
    data: i32,
    pointer: *const i32,
    _pin: PhantomPinned,
}

fn main() {
    let mut b = Box::pin(Pinned {
        data: 42,
        pointer: ptr::null(),
        _pin: PhantomPinned,
    });
    {
        let pinned = unsafe { b.as_mut().get_unchecked_mut() };
        pinned.pointer = &pinned.data;
    }
    println!("{}", unsafe { *b.pointer });
}
```

```rust
error[E0658]: use of unstable library feature 'unsafe_pin_internals'
  --> <source>:19:30
   |
19 |     println!("{}", unsafe { *b.pointer });
   |                              ^^^^^^^^^

error[E0277]: `Pinned` doesn't implement `std::fmt::Display`
  --> <source>:19:20
   |
19 |     println!("{}", unsafe { *b.pointer });
   |                    ^^^^^^^^^^^^^^^^^^^^^ `Pinned` cannot be formatted with the default formatter
   |
   = help: the trait `std::fmt::Display` is not implemented for `Pinned`
   = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
```

Since the user named their field `pointer`, it conflicts with the `pointer` field on `Pin`, which is public but unstable since Rust 1.60.0 with rust-lang#93176. On versions from 1.33.0 to 1.59.0, where the field on `Pin` is private, this program compiles and prints `42` as expected.

To avoid this confusing behavior, this PR renames `pointer` to `__pointer`, so that it's less likely to conflict with a `pointer` field on the underlying type, as accessed through the `Deref` impl. This is technically a breaking change for anyone who names their field `__pointer` on the inner type; if this is undesirable, it could be renamed to something more longwinded. It's also a nightly breaking change for any external users of `unsafe_pin_internals`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 26, 2024
…, r=Amanieu,dtolnay

Rename `pointer` field on `Pin`

A few days ago, I was helping another user create a self-referential type using `PhantomPinned`. However, I noticed an odd behavior when I tried to access one of the type's fields via `Pin`'s `Deref` impl:

```rust
use std::{marker::PhantomPinned, ptr};

struct Pinned {
    data: i32,
    pointer: *const i32,
    _pin: PhantomPinned,
}

fn main() {
    let mut b = Box::pin(Pinned {
        data: 42,
        pointer: ptr::null(),
        _pin: PhantomPinned,
    });
    {
        let pinned = unsafe { b.as_mut().get_unchecked_mut() };
        pinned.pointer = &pinned.data;
    }
    println!("{}", unsafe { *b.pointer });
}
```

```rust
error[E0658]: use of unstable library feature 'unsafe_pin_internals'
  --> <source>:19:30
   |
19 |     println!("{}", unsafe { *b.pointer });
   |                              ^^^^^^^^^

error[E0277]: `Pinned` doesn't implement `std::fmt::Display`
  --> <source>:19:20
   |
19 |     println!("{}", unsafe { *b.pointer });
   |                    ^^^^^^^^^^^^^^^^^^^^^ `Pinned` cannot be formatted with the default formatter
   |
   = help: the trait `std::fmt::Display` is not implemented for `Pinned`
   = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
```

Since the user named their field `pointer`, it conflicts with the `pointer` field on `Pin`, which is public but unstable since Rust 1.60.0 with rust-lang#93176. On versions from 1.33.0 to 1.59.0, where the field on `Pin` is private, this program compiles and prints `42` as expected.

To avoid this confusing behavior, this PR renames `pointer` to `__pointer`, so that it's less likely to conflict with a `pointer` field on the underlying type, as accessed through the `Deref` impl. This is technically a breaking change for anyone who names their field `__pointer` on the inner type; if this is undesirable, it could be renamed to something more longwinded. It's also a nightly breaking change for any external users of `unsafe_pin_internals`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 26, 2024
…, r=Amanieu,dtolnay

Rename `pointer` field on `Pin`

A few days ago, I was helping another user create a self-referential type using `PhantomPinned`. However, I noticed an odd behavior when I tried to access one of the type's fields via `Pin`'s `Deref` impl:

```rust
use std::{marker::PhantomPinned, ptr};

struct Pinned {
    data: i32,
    pointer: *const i32,
    _pin: PhantomPinned,
}

fn main() {
    let mut b = Box::pin(Pinned {
        data: 42,
        pointer: ptr::null(),
        _pin: PhantomPinned,
    });
    {
        let pinned = unsafe { b.as_mut().get_unchecked_mut() };
        pinned.pointer = &pinned.data;
    }
    println!("{}", unsafe { *b.pointer });
}
```

```rust
error[E0658]: use of unstable library feature 'unsafe_pin_internals'
  --> <source>:19:30
   |
19 |     println!("{}", unsafe { *b.pointer });
   |                              ^^^^^^^^^

error[E0277]: `Pinned` doesn't implement `std::fmt::Display`
  --> <source>:19:20
   |
19 |     println!("{}", unsafe { *b.pointer });
   |                    ^^^^^^^^^^^^^^^^^^^^^ `Pinned` cannot be formatted with the default formatter
   |
   = help: the trait `std::fmt::Display` is not implemented for `Pinned`
   = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
```

Since the user named their field `pointer`, it conflicts with the `pointer` field on `Pin`, which is public but unstable since Rust 1.60.0 with rust-lang#93176. On versions from 1.33.0 to 1.59.0, where the field on `Pin` is private, this program compiles and prints `42` as expected.

To avoid this confusing behavior, this PR renames `pointer` to `__pointer`, so that it's less likely to conflict with a `pointer` field on the underlying type, as accessed through the `Deref` impl. This is technically a breaking change for anyone who names their field `__pointer` on the inner type; if this is undesirable, it could be renamed to something more longwinded. It's also a nightly breaking change for any external users of `unsafe_pin_internals`.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 27, 2024
Rollup merge of rust-lang#119562 - LegionMammal978:rename-pin-pointer, r=Amanieu,dtolnay

Rename `pointer` field on `Pin`

A few days ago, I was helping another user create a self-referential type using `PhantomPinned`. However, I noticed an odd behavior when I tried to access one of the type's fields via `Pin`'s `Deref` impl:

```rust
use std::{marker::PhantomPinned, ptr};

struct Pinned {
    data: i32,
    pointer: *const i32,
    _pin: PhantomPinned,
}

fn main() {
    let mut b = Box::pin(Pinned {
        data: 42,
        pointer: ptr::null(),
        _pin: PhantomPinned,
    });
    {
        let pinned = unsafe { b.as_mut().get_unchecked_mut() };
        pinned.pointer = &pinned.data;
    }
    println!("{}", unsafe { *b.pointer });
}
```

```rust
error[E0658]: use of unstable library feature 'unsafe_pin_internals'
  --> <source>:19:30
   |
19 |     println!("{}", unsafe { *b.pointer });
   |                              ^^^^^^^^^

error[E0277]: `Pinned` doesn't implement `std::fmt::Display`
  --> <source>:19:20
   |
19 |     println!("{}", unsafe { *b.pointer });
   |                    ^^^^^^^^^^^^^^^^^^^^^ `Pinned` cannot be formatted with the default formatter
   |
   = help: the trait `std::fmt::Display` is not implemented for `Pinned`
   = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
```

Since the user named their field `pointer`, it conflicts with the `pointer` field on `Pin`, which is public but unstable since Rust 1.60.0 with rust-lang#93176. On versions from 1.33.0 to 1.59.0, where the field on `Pin` is private, this program compiles and prints `42` as expected.

To avoid this confusing behavior, this PR renames `pointer` to `__pointer`, so that it's less likely to conflict with a `pointer` field on the underlying type, as accessed through the `Deref` impl. This is technically a breaking change for anyone who names their field `__pointer` on the inner type; if this is undesirable, it could be renamed to something more longwinded. It's also a nightly breaking change for any external users of `unsafe_pin_internals`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-pin Area: Pin merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.