Skip to content

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