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

Change #[pinned_drop] to trait implementation #86

Merged
merged 1 commit into from
Sep 11, 2019
Merged

Change #[pinned_drop] to trait implementation #86

merged 1 commit into from
Sep 11, 2019

Conversation

taiki-e
Copy link
Owner

@taiki-e taiki-e commented Sep 10, 2019

proc-macro just rewrites the trait path and adds unsafe.
It needs to write the documentation more.

Examples

Before:

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> {}

After:

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 Self>) {
        // ..
    }
}

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

Closes #26

@taiki-e
Copy link
Owner Author

taiki-e commented Sep 10, 2019

We also need to decide the method name: drop vs pinned_drop

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

@taiki-e taiki-e force-pushed the pinned-drop branch 2 times, most recently from a4837ec to c579bb5 Compare September 11, 2019 00:20
@taiki-e taiki-e mentioned this pull request Sep 11, 2019
11 tasks
@taiki-e taiki-e added this to the v0.4 milestone Sep 11, 2019
@taiki-e taiki-e self-assigned this Sep 11, 2019
@taiki-e taiki-e marked this pull request as ready for review September 11, 2019 16:20
@taiki-e
Copy link
Owner Author

taiki-e commented Sep 11, 2019

bors r+

bors bot added a commit that referenced this pull request Sep 11, 2019
86: Change #[pinned_drop] to trait implementation r=taiki-e a=taiki-e

proc-macro just rewrites the trait path and adds `unsafe`.
It needs to write the documentation more.

### Examples
Before:
```rust
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> {}
```

After:

```rust
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 Self>) {
        // ..
    }
}

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

Closes #26

Co-authored-by: Taiki Endo <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 11, 2019

Build succeeded

  • taiki-e.pin-project

@bors bors bot merged commit ad9e8a6 into master Sep 11, 2019
@RalfJung
Copy link
Contributor

RalfJung commented Sep 11, 2019

proc-macro just rewrites the trait path and adds unsafe.

Why does it have to add unsafe? And why is it safe to do so?
I am somewhat surprised that the pinned_drop macro is needed at all.

@taiki-e
Copy link
Owner Author

taiki-e commented Sep 11, 2019

@RalfJung

Why does it have to add unsafe? And why is it safe to do so?
I am somewhat surprised that the pinned_drop macro is needed at all.

It is safe to implement PinnedDrop::drop, but it is not safe to call it. This is because destructors can be called multiple times (double dropping is unsound: rust-lang/rust#62360).

Ideally, it would be desirable to be able to prohibit manual calls in the same way as Drop::drop, but the library cannot. So, by using macros and replacing them with private unsafe traits, we prevent users from calling PinnedDrop::drop.

@taiki-e taiki-e deleted the pinned-drop branch September 11, 2019 17:15
@RalfJung
Copy link
Contributor

Thanks, that explanation should be in the code somewhere.

One thing though: if it is safe to implement, why is the trait unsafe? It should not be. Only its method should be unsafe, reflecting that it is unsafe to call.

@taiki-e
Copy link
Owner Author

taiki-e commented Sep 11, 2019

that explanation should be in the code somewhere.

Although it is in a place that was not changed by this PR, I think this should be explained in the document, so I filed #88.

One thing though: if it is safe to implement, why is the trait unsafe? It should not be. Only its method should be unsafe, reflecting that it is unsafe to call.

Indeed. I forgot to update it.

bors bot added a commit that referenced this pull request Sep 15, 2019
91: Make PinnedDrop safe trait r=taiki-e a=taiki-e

#86 (comment)

Co-authored-by: Taiki Endo <[email protected]>
@taiki-e taiki-e added the A-drop Area: #[pinned_drop] and Drop 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 <[email protected]>
@taiki-e taiki-e removed their assignment Oct 14, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weirdly different approach to Unpin vs Drop
2 participants