Merge taiki-e#91

91: Make PinnedDrop safe trait r=taiki-e a=taiki-e

taiki-e#86 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>

Author: bors[bot]

examples/pinned_drop-expanded.rs

@@ -72,18 +72,27 @@ impl<'a, T> ::core::ops::Drop for Foo<'a, T> {
         // Safety - we're in 'drop', so we know that 'self' will
         // never move again.
         let pinned_self = unsafe { ::core::pin::Pin::new_unchecked(self) };
-        // We call `pinned_drop` only once. Since `UnsafePinnedDrop::drop`
+        // We call `pinned_drop` only once. Since `PinnedDrop::drop`
         // is an unsafe function and a private API, it is never called again in safe
         // code *unless the user uses a maliciously crafted macro*.
         unsafe {
-            ::pin_project::__private::UnsafePinnedDrop::drop(pinned_self);
+            ::pin_project::__private::PinnedDrop::drop(pinned_self);
         }
     }
 }
 
+// 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 traits, we prevent users from calling
+// PinnedDrop::drop.
+//
 // Users can implement `Drop` safely using `#[pinned_drop]`.
 // **Do not call or implement this trait directly.**
-unsafe impl<T> ::pin_project::__private::UnsafePinnedDrop for Foo<'_, T> {
+impl<T> ::pin_project::__private::PinnedDrop for Foo<'_, T> {
     // Since calling it twice on the same object would be UB,
     // this method is unsafe.
     unsafe fn drop(mut self: ::core::pin::Pin<&mut Self>) {

pin-project-internal/Cargo.toml

@@ -8,7 +8,7 @@ repository = "https://github.com/taiki-e/pin-project"
 documentation = "https://docs.rs/pin-project-internal/"
 keywords = ["pin", "macros", "attribute"]
 categories = ["rust-patterns"]
-description = "An interal crate to support pin_project - do not use directly"
+description = "An internal crate to support pin_project - do not use directly"
 
 [package.metadata.docs.rs]
 all-features = true

pin-project-internal/src/lib.rs

@@ -1,4 +1,4 @@
-//! An interal crate to support pin_project - **do not use directly**
+//! An internal crate to support pin_project - **do not use directly**
 
 #![recursion_limit = "256"]
 #![doc(html_root_url = "https://docs.rs/pin-project-internal/0.4.0-alpha.11")]

pin-project-internal/src/pin_project/attribute.rs

@@ -227,7 +227,7 @@ impl Context {
             let crate_path = &self.crate_path;
 
             let call = quote_spanned! { pinned_drop =>
-                ::#crate_path::__private::UnsafePinnedDrop::drop(pinned_self)
+                ::#crate_path::__private::PinnedDrop::drop(pinned_self)
             };
 
             quote! {
@@ -237,7 +237,7 @@ impl Context {
                         // Safety - we're in 'drop', so we know that 'self' will
                         // never move again.
                         let pinned_self = unsafe { ::core::pin::Pin::new_unchecked(self) };
-                        // We call `pinned_drop` only once. Since `UnsafePinnedDrop::drop`
+                        // We call `pinned_drop` only once. Since `PinnedDrop::drop`
                         // is an unsafe function and a private API, it is never called again in safe
                         // code *unless the user uses a maliciously crafted macro*.
                         unsafe {

pin-project-internal/src/pinned_drop.rs

@@ -11,18 +11,16 @@ pub(crate) fn attribute(mut input: ItemImpl) -> TokenStream {
         let (impl_generics, _, where_clause) = input.generics.split_for_impl();
 
         let mut tokens = e.to_compile_error();
-        // Generate a dummy `UnsafePinnedDrop` implementation.
+        // Generate a dummy `PinnedDrop` implementation.
         // In many cases, `#[pinned_drop] impl` is declared after `#[pin_project]`.
         // Therefore, if `pinned_drop` compile fails, you will also get an error
-        // about `UnsafePinnedDrop` not being implemented.
+        // about `PinnedDrop` not being implemented.
         // This can be prevented to some extent by generating a dummy
-        // `UnsafePinnedDrop` implementation.
+        // `PinnedDrop` implementation.
         // We already know that we will get a compile error, so this won't
         // accidentally compile successfully.
         tokens.extend(quote! {
-            unsafe impl #impl_generics ::#crate_path::__private::UnsafePinnedDrop
-                for #self_ty #where_clause
-            {
+            impl #impl_generics ::#crate_path::__private::PinnedDrop for #self_ty #where_clause {
                 unsafe fn drop(self: ::core::pin::Pin<&mut Self>) {}
             }
         });
@@ -101,7 +99,7 @@ fn parse(item: &mut ItemImpl) -> Result<()> {
             let crate_path = crate_path();
 
             *path = syn::parse2(quote_spanned! { path.span() =>
-                ::#crate_path::__private::UnsafePinnedDrop
+                ::#crate_path::__private::PinnedDrop
             })
             .unwrap();
         } else {
@@ -120,35 +118,33 @@ fn parse(item: &mut ItemImpl) -> Result<()> {
     if item.unsafety.is_some() {
         return Err(error!(item.unsafety, "implementing the trait `PinnedDrop` is not unsafe"));
     }
-    item.unsafety = Some(token::Unsafe::default());
-
     if item.items.is_empty() {
         return Err(error!(item, "not all trait items implemented, missing: `drop`"));
-    } else {
-        for (i, item) in item.items.iter().enumerate() {
-            match item {
-                ImplItem::Const(item) => {
-                    return Err(error!(
-                        item,
-                        "const `{}` is not a member of trait `PinnedDrop`", item.ident
-                    ));
-                }
-                ImplItem::Type(item) => {
-                    return Err(error!(
-                        item,
-                        "type `{}` is not a member of trait `PinnedDrop`", item.ident
-                    ));
-                }
-                ImplItem::Method(method) => {
-                    parse_method(method)?;
-                    if i != 0 {
-                        return Err(error!(method, "duplicate definitions with name `drop`"));
-                    }
-                }
-                _ => {
-                    let _: Nothing = syn::parse2(item.to_token_stream())?;
+    }
+
+    for (i, item) in item.items.iter().enumerate() {
+        match item {
+            ImplItem::Const(item) => {
+                return Err(error!(
+                    item,
+                    "const `{}` is not a member of trait `PinnedDrop`", item.ident
+                ));
+            }
+            ImplItem::Type(item) => {
+                return Err(error!(
+                    item,
+                    "type `{}` is not a member of trait `PinnedDrop`", item.ident
+                ));
+            }
+            ImplItem::Method(method) => {
+                parse_method(method)?;
+                if i != 0 {
+                    return Err(error!(method, "duplicate definitions with name `drop`"));
                 }
             }
+            _ => {
+                let _: Nothing = syn::parse2(item.to_token_stream())?;
+            }
         }
     }
 

src/lib.rs

@@ -127,17 +127,22 @@ pub mod __private {
     #[doc(hidden)]
     pub use pin_project_internal::__PinProjectAutoImplUnpin;
 
-    // This is an internal helper trait used by `pin-project-internal`.
-    // This allows us to force an error if the user tries to provide
-    // a regular `Drop` impl when they specify the `PinnedDrop` argument.
+    // 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 traits, we prevent users from calling
+    // PinnedDrop::drop.
     //
     // Users can implement `Drop` safely using `#[pinned_drop]`.
     // **Do not call or implement this trait directly.**
-    #[allow(unsafe_code)]
     #[doc(hidden)]
-    pub unsafe trait UnsafePinnedDrop {
+    pub trait PinnedDrop {
         // Since calling it twice on the same object would be UB,
         // this method is unsafe.
+        #[allow(unsafe_code)]
         #[doc(hidden)]
         unsafe fn drop(self: Pin<&mut Self>);
     }

tests/ui/pinned_drop/forget-pinned-drop.stderr

@@ -1,10 +1,10 @@
-error[E0277]: the trait bound `Foo: pin_project::__private::UnsafePinnedDrop` is not satisfied
+error[E0277]: the trait bound `Foo: pin_project::__private::PinnedDrop` is not satisfied
  --> $DIR/forget-pinned-drop.rs:6:15
   |
 6 | #[pin_project(PinnedDrop)] //~ ERROR E0277
-  |               ^^^^^^^^^^ the trait `pin_project::__private::UnsafePinnedDrop` is not implemented for `Foo`
+  |               ^^^^^^^^^^ the trait `pin_project::__private::PinnedDrop` is not implemented for `Foo`
   |
-  = note: required by `pin_project::__private::UnsafePinnedDrop::drop`
+  = note: required by `pin_project::__private::PinnedDrop::drop`
 
 error: aborting due to previous error
 

tests/ui/pinned_drop/invalid.stderr

@@ -70,13 +70,13 @@ error: duplicate definitions with name `drop`
 143 |     fn drop(self: Pin<&mut Self>) {} //~ ERROR duplicate definitions with name `drop`
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error[E0277]: the trait bound `A: pin_project::__private::UnsafePinnedDrop` is not satisfied
+error[E0277]: the trait bound `A: pin_project::__private::PinnedDrop` is not satisfied
  --> $DIR/invalid.rs:6:15
   |
 6 | #[pin_project(PinnedDrop)]
-  |               ^^^^^^^^^^ the trait `pin_project::__private::UnsafePinnedDrop` is not implemented for `A`
+  |               ^^^^^^^^^^ the trait `pin_project::__private::PinnedDrop` is not implemented for `A`
   |
-  = note: required by `pin_project::__private::UnsafePinnedDrop::drop`
+  = note: required by `pin_project::__private::PinnedDrop::drop`
 
 error: aborting due to 13 previous errors