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

Weirdly different approach to Unpin vs Drop #26

Closed
RalfJung opened this issue Aug 5, 2019 · 4 comments · Fixed by #86
Closed

Weirdly different approach to Unpin vs Drop #26

RalfJung opened this issue Aug 5, 2019 · 4 comments · Fixed by #86
Assignees
Labels
A-drop Area: #[pinned_drop] and Drop A-unpin Area: Unpin and UnsafeUnpin C-enhancement Category: A new feature or an improvement for an existing one
Milestone

Comments

@RalfJung
Copy link
Contributor

RalfJung commented Aug 5, 2019

Looking at these two examples

use std::fmt::Debug;
use pin_project::pin_project;
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);
    }
}

fn main() {
    Foo { pinned_field: true, unpin_field: 40 };
}
use pin_project::{pin_projectable, UnsafeUnpin};
use std::pin::Pin;

#[pin_projectable(unsafe_Unpin)]
struct Foo<T, U> {
    #[pin]
    future: T,
    field: U,
}

impl<T, U> Foo<T, U> {
    fn baz(self: Pin<&mut Self>) {
        let this = self.project();
        let _: Pin<&mut T> = this.future; // Pinned reference to the field
        let _: &mut U = this.field; // Normal reference to the field
    }
}

unsafe impl<T: Unpin, U> UnsafeUnpin for Foo<T, U> {} // Conditional Unpin impl

it is odd that for a very similar situation (let the user "hook" into the auto-generated trait impl), this crate uses two totally different approaches: one time it is an attribute at the type, and one time it is a block around the entire thing plus an attribute at the impl.

To make the crate easier to use, it would be much better if the same approach could be used both times.

bors bot added a commit that referenced this issue 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>
@taiki-e
Copy link
Owner

taiki-e commented Aug 11, 2019

pin_project! block was removed in #33, so the current Drop impl is as follows:

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);
}

@RalfJung
Copy link
Contributor Author

So how does it look like when combining a manual drop impl and an UnsafeUnpin override?

@taiki-e
Copy link
Owner

taiki-e commented Aug 12, 2019

It is as follows:

use pin_project::{pin_project, pinned_drop, UnsafeUnpin};
use std::pin::Pin;

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

#[pinned_drop]
fn do_drop<T, U>(this: Pin<&mut Foo<T, U>>) {
    // ..
}

unsafe impl<T: Unpin, U> UnsafeUnpin for Foo<T, U> {}

I feel this is easier to read/write than the previous one.

@taiki-e
Copy link
Owner

taiki-e commented Aug 12, 2019

However, I think it is more consistent to use attributes to impl.

use pin_project::{pin_project, pinned_drop};
use std::pin::Pin;

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

#[pinned_drop]
impl<T, U> PinnedDrop for Foo<T, U> {
    fn drop(self: Pin<&mut Foo<T, U>>) {
        // ..
    }
}

unsafe impl<T: Unpin, U> UnsafeUnpin for Foo<T, U> {}

@taiki-e taiki-e added this to the v0.4 milestone Aug 12, 2019
@taiki-e taiki-e self-assigned this Sep 7, 2019
@bors bors bot closed this as completed in 3f365f1 Sep 11, 2019
@bors bors bot closed this as completed in #86 Sep 11, 2019
@taiki-e taiki-e added A-drop Area: #[pinned_drop] and Drop A-unpin Area: Unpin and UnsafeUnpin labels Sep 23, 2019
@taiki-e taiki-e added the C-enhancement Category: A new feature or an improvement for an existing one label Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-drop Area: #[pinned_drop] and Drop A-unpin Area: Unpin and UnsafeUnpin C-enhancement Category: A new feature or an improvement for an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants