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

Replace unsafe_project with safe pin_projectable attribute #18

Merged
merged 14 commits into from
Aug 5, 2019

Conversation

Aaron1011
Copy link
Collaborator

This PR replaces the unsafe_project attribute with a new, completely safe pin_projectable attribute. This attribtute enforces all of the guaranteees of pin projection:

  • #[repr(packed)] types are disallowed completely, via a compile-time error.
  • A Drop impl is unconditionally provided. A custom drop function can be provided by annotating a function with #[pinned_drop]. This function takes a Pin<&mut MyType>, which ensures that the user cannot move out of pinned fields in safe code.
  • An Unpin impl is unconditionally provided. By default, this generates the same bounds as the (now removed) Unpin argument - all pin-projected fields are required to implement Unpin. A manual impl can be provided via the unsafe_Unpin attribute, and a impl of the new unsafe trait UnsafeUnpin.

This is a significant, non-backwards-compatible refactor of this crate. However, I believe it provides several significant benefits:

  • Pin projection is now a safe operation. In the vast majority of cases, consumers of this crate can create pin projections without a single use of unsafe. This reduces the number of unsafe blocks that must be audited, and increases confidence that a crate does not trigger undefined behavior.

  • The expressive power of the #[unsafe_project] and #[pin_projectable] is the same. Any code written using #[unsafe_project] can be adapted to #[pin_projectable], even if relied in the unsafe nature of #[unsafe_project]:

    • UnsafeUnpin can be used to obtain complete control over the generated Unpin impl.
    • Pin::get_unchecked_mut can be used within a #[pinned_drop] function to obtain a &mut MyStruct, effectively turing #[pinned_drop] back into a regular Drop impl.
    • For #[repr(packed)] structs, there are two possible cases:
      • Pin projection is never used - no fields have the #[pin] attribute, or project() is never called on the base struct. In this case, using this crate for the struct is completely pointless, and the #[unsafe_project] attribute can be removed.
      • Pin projection is used. This is immediate undefined behavior - the new #[pin_projectable]` attribute is simply not allowing you to write broken code.
  • Anything with the potential for undefined behavior now requires usage of the unsafe keyword, whearas the previous #[unsafe_project] attribute only required typing the word 'unsafe' in an argument. Using the actual unsafe keyword allows for proper integration with the unsafe_code lint, and tools cargo geiger. Note that the unsafe_Unpin argument still obeys this rule - the UnsafeUnpin trait it enables is unsafe, and failing to provide an impl of UnsafeUnpin is completely safe.

Unfortunately, this PR requires pin-project to be split into two crates - pin-project and pin-project-internal. This is due to the fact that proc-macro crates cannot currently export anything other than proc macros. The crates are split as follows:

  • A new pin-project-internal crates provides almost all of the functionality of this crate, with the exception of the UnsafeUnpin trait.
  • The pin-project crate now re-exports everything from pin-project-internal, with added doc comments. It also provides the UnsafeUnpin trait.

Because the pin-project-internal crate must reference the UnsafeUnpin trait from pin-project, pin-project-internal implicitly depends on pin-project. To ensure that users can rename their dependency on pin-project, the crate proc_macro_crate is used to dynamically determine the name of the pin-project crate at macro invocation time.

Due to several issues with Rustdoc's handling of procedural macros, the documentation for the pin-project crate will not currently render properly - however, all doctests will still run correctly. Once rust-lang/rust#62855 and rust-lang/rust#63048 are merged, the documentation will build correctly on nightly Rust.

@taiki-e: I'm happy to work with you to make any adjustments necessary to get this merged (once the referenced rustc PRs are merged).

Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I like this to be safer and to have the equivalent expressive power as the old version.

I added thoughts inline. It seems to me that the guarantee added in this PR works well. but I felt pin_projectable/pin_project! were a little hard to use, so I commented on alternatives. If that looks good, I'm happy to send PR to your branch (or I can merge this PR and write follow up PR).

pin-project/src/lib.rs Show resolved Hide resolved
pin-project-internal/src/lib.rs Outdated Show resolved Hide resolved
pin-project-internal/Cargo.toml Outdated Show resolved Hide resolved

match &*args.to_string() {
"" => Ok(Self { generics: generics.clone(), auto: true }),
"unsafe_Unpin" => Ok(Self { generics: generics.clone(), auto: false }),
Copy link
Owner

@taiki-e taiki-e Jul 28, 2019

Choose a reason for hiding this comment

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

I feel it is good to change this to UpperCamelCase (UnsafeUnpin) or snake_case (unsafe_unpin).

(This may depend on #18 (comment))

Copy link

@Nemo157 Nemo157 Jul 29, 2019

Choose a reason for hiding this comment

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

Another idea for this would be to require it be an unsafe impl Unpin inside a pin_project! block, something like

//! WARNING: Unsound code

pin_project! {
    #[pin_projectable]
    pub struct Blah<T> {
        #[pin]
        field: T,
    }

    unsafe impl<T> Unpin for Blah<T> {}
}

I don't know how this interacts with tools like cargo-geiger, since the unsafe keyword won't make it into the expanded code, but it at least gets it there much more obviously when checking via rg.

@taiki-e
Copy link
Owner

taiki-e commented Jul 28, 2019

cc @Nemo157 @cramertj @RalfJung @shepmaster
Any opinion on this? (especially on safety and ergonomics)

@taiki-e taiki-e added this to the v0.4 milestone Jul 28, 2019
@shepmaster
Copy link

Of note is that I'm just a user of the crate, so my opinion of the safety is pretty scarce.

I'll pull this down and test with SNAFU, but one thing I don't immediately see in the PR is an upgrading guide, which means any users have to struggle a bit to figure out how to map old ideas to new ideas.

non-backwards-compatible refactor

I'd be curious to know what is not backwards compatible. This is part of the upgrading guide — what do I need to watch out for when upgrading?

pin-project-internal

Completely subjective, but I prefer the -derive suffix.

@Aaron1011
Copy link
Collaborator Author

@shepmaster: I can add additional docs on how to upgrade.

I'd be curious to know what is not backwards compatible. This is part of the upgrading guide — what do I need to watch out for when upgrading?

The main things are:

  • #[unsafe_project] has been replaced with #[pin_projectable]
  • The Unpin argument has been removed - an Unpin impl is now generated by default.
  • Drop impls must be specified with #[pinned_drop] instead of via a normal Drop impl.
  • Unpin impls must be specified with an impl of UnsafeUnpin, instead of implementing the normal Unpin trait.

@Aaron1011
Copy link
Collaborator Author

Completely subjective, but I prefer the -derive suffix.

I chose the -internal suffix to emphasize the fact that users of this crate shouldn't have to care about it at all. The only crate you should ever have to deal with (unless you're working on pin-project itself) is pin-project.

@shepmaster
Copy link

Thanks!

My upgrade was smooth and was mostly done via find-and-replace:

diff --git a/src/futures/try_future.rs b/src/futures/try_future.rs
index 883c144..4293eaa 100644
--- a/src/futures/try_future.rs
+++ b/src/futures/try_future.rs
@@ -3,7 +3,7 @@
 //! [`TryFuture`]: futures_core::future::TryFuture
 
 use futures_core::future::TryFuture;
-use pin_project::unsafe_project;
+use pin_project::pin_projectable;
 use std::{
     marker::PhantomData,
     pin::Pin,
@@ -124,7 +124,7 @@ where
 /// Future for the [`context`](TryFutureExt::context) combinator.
 ///
 /// See the [`TryFutureExt::context`] method for more details.
-#[unsafe_project(Unpin)]
+#[pin_projectable]
 #[derive(Debug)]
 #[must_use = "futures do nothing unless polled"]
 pub struct Context<Fut, C, E> {
@@ -163,7 +163,7 @@ where
 /// Future for the [`with_context`](TryFutureExt::with_context) combinator.
 ///
 /// See the [`TryFutureExt::with_context`] method for more details.
-#[unsafe_project(Unpin)]
+#[pin_projectable]
 #[derive(Debug)]
 #[must_use = "futures do nothing unless polled"]
 pub struct WithContext<Fut, F, E> {
diff --git a/src/futures/try_stream.rs b/src/futures/try_stream.rs
index 869f453..81a27ed 100644
--- a/src/futures/try_stream.rs
+++ b/src/futures/try_stream.rs
@@ -3,7 +3,7 @@
 //! [`TryStream`]: futures_core::TryStream
 
 use futures_core::stream::TryStream;
-use pin_project::unsafe_project;
+use pin_project::pin_projectable;
 use std::{
     marker::PhantomData,
     pin::Pin,
@@ -126,7 +126,7 @@ where
 /// Stream for the [`context`](TryStreamExt::context) combinator.
 ///
 /// See the [`TryStreamExt::context`] method for more details.
-#[unsafe_project(Unpin)]
+#[pin_projectable]
 #[derive(Debug)]
 #[must_use = "streams do nothing unless polled"]
 pub struct Context<St, C, E> {
@@ -168,7 +168,7 @@ where
 /// Stream for the [`with_context`](TryStreamExt::with_context) combinator.
 ///
 /// See the [`TryStreamExt::with_context`] method for more details.
-#[unsafe_project(Unpin)]
+#[pin_projectable]
 #[derive(Debug)]
 #[must_use = "streams do nothing unless polled"]
 pub struct WithContext<St, F, E> {

README.md Outdated Show resolved Hide resolved
@RalfJung
Copy link
Contributor

This is pretty cool! I am not deeply familiar with proc macros, so I will not attempt to review the implementation, but I can help with the conceptual part of this -- in particular with the safety analysis (based on @Aaron1011's excellent PR summary, not the actual code).

The pin module docs list four requirements for pin projects. Let's go over them. One is not to offer any "moving" operations, but those operations would need unsafe code themselves or they would be offered by the field type, so it would be that type's responsibility to get them right. The other three are mentioned in the OP.

#[repr(packed)] types are disallowed completely, via a compile-time error.

Check -- assuming this works with how proc macros are implemented (I left a question in the code).

A Drop impl is unconditionally provided. A custom drop function can be provided by annotating a function with #[pinned_drop]. This function takes a Pin<&mut MyType>.

Check.

An Unpin impl is unconditionally provided.

This is the sublte part. There is an accepted and unstably implemented RFC to allow overlapping instances for marker traits. However, @cramertj already asked about this exact case there a year ago and it seems consensus is that traits will not permit overlapping impls without oping-in top that. Assuming that's how things work out, I think we are safe here as well.

I still feel now it might have been a mistake to make Unpin safe, but at least we now have good arguments for how permitting overlapping impls!


match &*args.to_string() {
"" => Ok(Self { generics: generics.clone(), auto: true }),
"unsafe_Unpin" => Ok(Self { generics: generics.clone(), auto: false }),
Copy link

@Nemo157 Nemo157 Jul 29, 2019

Choose a reason for hiding this comment

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

Another idea for this would be to require it be an unsafe impl Unpin inside a pin_project! block, something like

//! WARNING: Unsound code

pin_project! {
    #[pin_projectable]
    pub struct Blah<T> {
        #[pin]
        field: T,
    }

    unsafe impl<T> Unpin for Blah<T> {}
}

I don't know how this interacts with tools like cargo-geiger, since the unsafe keyword won't make it into the expanded code, but it at least gets it there much more obviously when checking via rg.

@RalfJung
Copy link
Contributor

Another idea for this would be to require it be an unsafe impl Unpin inside a pin_project! block, something like

Or maybe it should be an attribute, a bit like what this does for Drop?

#[pin_projectable]
unsafe impl<T> Unpin ...

@Nemo157
Copy link

Nemo157 commented Jul 29, 2019

Ah, I see @taiki-e had an alternative suggestion to the current pin_project! wrapper for Drop. If it's possible to do something similar for forcing unsafe impl to be used with Unpin then that sounds good as well.

@Aaron1011
Copy link
Collaborator Author

@RalfJung: I don't really like the idea of rewriting impl blocks. While the end result is the same, you are not directly implementing the Unpin trait.

I think we should minimize the amount of rewriting done by the macro, and should instead prefer generating additional code.

@RalfJung
Copy link
Contributor

@Aaron1011 Fair. However, isn't the latest proposal for Drop doing the same?

#[pinned_drop]
impl<T: Debug, U: Debug> Drop for Foo<T, U> {
    fn drop(self: Pin<&mut Self>) {
        let foo = self.project();
        // ...
    }
}

This looks like you are implementing Drop, but that's not what happens.

@RalfJung
Copy link
Contributor

RalfJung commented Aug 4, 2019

@Aaron1011 sounds reasonable! I was mostly trying to make sure the same approach is used for both Unpin and Drop -- the situation is similar, so the solution should also be similar.

@Aaron1011
Copy link
Collaborator Author

@taiki-e: My main concern with #[pinned_drop] was making it impossible for users to directly call it. If we make the call go through a PinnedDrop trait, the method will need to be unsafe (since calling it twice on the same object would be U.B.). My approach avoids the need to modify the user's function to add unsafe.

@taiki-e
Copy link
Owner

taiki-e commented Aug 5, 2019

@Aaron1011
I think the way @RalfJung mentions is making trait a hidden API to be used only by macros. But this way certainly has no effect for users trying to break into the private API...

@taiki-e
Copy link
Owner

taiki-e commented Aug 5, 2019

bors try

bors bot added a commit that referenced this pull request Aug 5, 2019
@taiki-e taiki-e mentioned this pull request Aug 5, 2019
11 tasks
Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

I'm going to merge this PR and release 0.4-alpha. Rustdoc issues are tracked by #21.

Thank you again @Aaron1011!

@taiki-e
Copy link
Owner

taiki-e commented Aug 5, 2019

bors r+

bors bot added a commit that referenced this pull request Aug 5, 2019
18: Replace `unsafe_project` with safe `pin_projectable` attribute r=taiki-e a=Aaron1011

This PR replaces the `unsafe_project` attribute with a new, completely safe `pin_projectable` attribute. This attribtute enforces all of the guaranteees of pin projection:

* `#[repr(packed)]` types are disallowed completely, via a compile-time error.
* A `Drop` impl is unconditionally provided. A custom drop function can be provided by annotating a function with `#[pinned_drop]`. This function takes a `Pin<&mut MyType>`, which ensures that the user cannot move out of pinned fields in safe code.
* An `Unpin` impl is unconditionally provided. By default, this generates the same bounds as the (now removed) `Unpin` argument - all pin-projected fields are required to implement `Unpin`. A manual impl can be provided via the `unsafe_Unpin` attribute, and a impl of the new `unsafe` trait `UnsafeUnpin`.

This is a significant, non-backwards-compatible refactor of this crate. However, I believe it provides several significant benefits:

* Pin projection is now *a safe operation*. In the vast majority of cases, consumers of this crate can create pin projections without a single use of `unsafe`. This reduces the number of `unsafe` blocks that must be audited, and increases confidence that a crate does not trigger undefined behavior.
* The expressive power of the `#[unsafe_project]` and `#[pin_projectable]` is the same. Any code written using `#[unsafe_project]` can be adapted to `#[pin_projectable]`, even if relied in the `unsafe` nature of `#[unsafe_project]`:

  * `UnsafeUnpin` can be used to obtain complete control over the generated `Unpin` impl.
  * `Pin::get_unchecked_mut` can be used within a `#[pinned_drop]` function to obtain a `&mut MyStruct`, effectively turing `#[pinned_drop]` back into a regular `Drop` impl.
  * For `#[repr(packed)]` structs, there are two possible cases:
    * Pin projection is never used - no fields have the `#[pin]` attribute, or `project()` is never called on the base struct. In this case, using this crate for the struct is completely pointless, and the `#[unsafe_project]` attribute can be removed.
    * Pin projection *is* used. This is immediate undefined behavior - the new `#[pin_projectable`]` attribute is simply not allowing you to write broken code.
* Anything with the potential for undefined behavior now requires usage of the `unsafe` keyword, whearas the previous `#[unsafe_project]` attribute only required typing the word 'unsafe' in an argument. Using the actual `unsafe` keyword allows for proper integration with the `unsafe_code` lint, and tools [cargo geiger](https://github.com/anderejd/cargo-geiger). Note that the `unsafe_Unpin` argument still obeys this rule - the `UnsafeUnpin` trait it enables is unsafe, and failing to provide an impl of `UnsafeUnpin` is completely safe.

Unfortunately, this PR requires `pin-project` to be split into two crates - `pin-project` and `pin-project-internal`. This is due to the fact that `proc-macro` crates cannot currently export anything other than proc macros.  The crates are split as follows:
* A new `pin-project-internal` crates provides almost all of the functionality of this crate, with the exception of the `UnsafeUnpin` trait.
* The `pin-project` crate now re-exports everything from `pin-project-internal`, with added doc comments. It also provides the `UnsafeUnpin` trait.

Because the `pin-project-internal` crate must reference the `UnsafeUnpin` trait from `pin-project`, `pin-project-internal` implicitly depends on `pin-project`. To ensure that users can rename their dependency on `pin-project`, the crate [proc_macro_crate](https://crates.io/crates/proc_macro_crate) is used to dynamically determine the name of the `pin-project` crate at macro invocation time.

Due to several issues with Rustdoc's handling of procedural macros, the documentation for the `pin-project` crate will not currently render properly - however, all doctests will still run correctly. Once rust-lang/rust#62855 and rust-lang/rust#63048 are merged, the documentation will build correctly on nightly Rust.

@taiki-e: I'm happy to work with you to make any adjustments necessary to get this merged (once the referenced rustc PRs are merged).

Co-authored-by: Aaron Hill <aa1ronham@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 5, 2019

Build succeeded

  • taiki-e.pin-project

@bors bors bot merged commit 60e4242 into taiki-e:master Aug 5, 2019
@bors

This comment has been minimized.

@taiki-e
Copy link
Owner

taiki-e commented Aug 5, 2019

Oops, I seem to have missed some reviews from @Nemo157. Fixed in #22.

bors bot added a commit that referenced this pull request Aug 10, 2019
33: Remove pin_project! macro and add #[pinned_drop] attribute r=taiki-e a=taiki-e

This removes `pin_project!` block and adds `#[pinned_drop]` attribute as proc-macro-attribute.
If you want a custom `Drop` implementation, you need to pass the `PinnedDrop` argument to `pin_project` attribute instead of defining items in `pin_project!` block.

In the previous implementation, `#[pinned_drop]` implemented `Drop` directly, but this PR changes it to implement this via a private unsafe trait method.

Also, this renames `pin_projectable` to `pin_project`.

cc #21, #26
Related: #18 (comment)

### Examples
Before:
```rust
use std::fmt::Debug;
use pin_project::{pin_project, pin_projectable};
use std::pin::Pin;

pin_project! {
    #[pin_projectable]
    pub struct Foo<T: Debug, U: Debug> {
        #[pin] pinned_field: T,
        unpin_field: U
    }

    #[pinned_drop]
    fn my_drop_fn<T: Debug, U: Debug>(foo: Pin<&mut Foo<T, U>>) {
        let foo = foo.project();
        println!("Dropping pinned field: {:?}", foo.pinned_field);
        println!("Dropping unpin field: {:?}", foo.unpin_field);
    }
}
```

After:
```rust
use std::fmt::Debug;
use pin_project::{pin_project, pinned_drop};
use std::pin::Pin;

#[pin_project(PinnedDrop)]
pub struct Foo<T: Debug, U: Debug> {
    #[pin] pinned_field: T,
    unpin_field: U
}

#[pinned_drop]
fn my_drop_fn<T: Debug, U: Debug>(foo: Pin<&mut Foo<T, U>>) {
    let foo = foo.project();
    println!("Dropping pinned field: {:?}", foo.pinned_field);
    println!("Dropping unpin field: {:?}", foo.unpin_field);
}
```

### TODO
- [x] Update docs



Co-authored-by: Taiki Endo <te316e89@gmail.com>
bors bot added a commit that referenced this pull request Aug 11, 2019
40: Change 'unsafe_Unpin' to UpperCamelCase (UnsafeUnpin) r=taiki-e a=taiki-e

Unifies naming convention for arguments.

Resolves #18 (comment) 

cc @Aaron1011

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@taiki-e taiki-e mentioned this pull request Sep 21, 2019
bors bot added a commit that referenced this pull request Sep 21, 2019
100: Remove "renamed" feature r=taiki-e a=taiki-e

Unlike using `$crate` with declarative macros, re-export of procedural macros are not supported even if `proc-macro-crate` is used.
Also, crate name matches the macro name, so I don't think there is a need to rename it.

Related: #18 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@taiki-e taiki-e added the breaking-change This proposes a breaking change label Sep 24, 2019
@taiki-e taiki-e mentioned this pull request Sep 25, 2019
bors bot added a commit that referenced this pull request Sep 25, 2019
109: Release 0.4.0 r=taiki-e a=taiki-e

cc #21

### Changes since the latest 0.3 release:

* **Pin projection has become a safe operation.** In the absence of other unsafe code that you write, it is impossible to cause undefined behavior. (#18)

* `#[unsafe_project]` attribute has been replaced with `#[pin_project]` attribute. (#18, #33)

* The `Unpin` argument has been removed - an `Unpin` impl is now generated by default. (#18)

* Drop impls must be specified with `#[pinned_drop]` instead of via a normal `Drop` impl. (#18, #33, #86)

* `Unpin` impls must be specified with an impl of `UnsafeUnpin`, instead of implementing the normal `Unpin` trait. (#18)

* `#[pin_project]` attribute now determines the visibility of the projection type/method is based on the original type. (#96)

* `#[pin_project]` can now be used for public type with private field types. (#53)

* `#[pin_project]` can now interoperate with `#[cfg()]`. (#77)

* Added `project_ref` method to `#[pin_project]` types. (#93)

* Added `#[project_ref]` attribute. (#93)

* Removed "project_attr" feature and always enable `#[project]` attribute. (#94)

* `#[project]` attribute can now be used for `impl` blocks. (#46)

* `#[project]` attribute can now be used for `use` statements. (#85)

* `#[project]` attribute now supports `match` expressions at the position of the initializer expression of `let` expressions. (#51)

### Changes since the 0.4.0-beta.1 release:

* Fixed an issue that caused an error when using `#[pin_project(UnsafeUnpin)]` and not providing a manual `UnsafeUnpin` implementation on a type with no generics or lifetime. (#107)


Co-authored-by: Taiki Endo <te316e89@gmail.com>
vsrinivas pushed a commit to vsrinivas/fuchsia that referenced this pull request Jan 13, 2020
The rust crate pin-project provides a safe mechanism for projecting
pinned values of a struct. This is safer than using pin-utils, because
it is impossible to cause undefined behavior (unless you use `unsafe`):

taiki-e/pin-project#18

Additionally this switches the package resolver to using pin-project.

This project was approved by OSRB fxb/43118.

TEST: this is not expected to change behaivor, and should be covered by
the package resolver tests.

Fixed: 43878

Change-Id: Ib1bdd08710318e0bdaa983dc0e676b93b1139cd4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This proposes a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants