Implement sound `Drop` support in our little pin_project alternative.
Unlike `pin_project`, the function is just marked unsafe to make it sound (as it's not safe to drop a struct). This is maybe a slight ergonomic hit, but for the most part we don't expect people to implement this manually -- in the near term, it's only going to be autogenerated as part of bindings generation. Also, it's not _that_ bad. Worst case, we could define a macro much like `pin_project` does, to make it look like you're defining a safe function.
Otherwise, this follows relatively closely with the approach described in https://docs.rs/pin-project/latest/pin_project/attr.pin_project.html#safety
After this CL, the pin projection implemented here should be sound, without safety holes that would allow UB in safe code. Followup CLs will actually start using this in interop, so that we can use the `ctor!()` macro.
---
Error scenarios and their error messages, tested by hand:
defined PinnedDrop without PinnedDrop enabled:
```
error[E0119]: conflicting implementations of trait `ctor::PinnedDrop` for type `test_pinned_drop::DropStruct`
--> third_party/crubit/rs_bindings_from_cc/support/ctor_proc_macros_test.rs:201:5
|
201 | #[::ctor::recursively_pinned]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `test_pinned_drop::DropStruct`
202 | struct DropStruct(Rc<Cell<bool>>);
203 | impl ::ctor::PinnedDrop for DropStruct {
| -------------------------------------- first implementation here
|
= note: this error originates in the attribute macro `::ctor::recursively_pinned` (in Nightly builds, run with -Z macro-backtrace for more info)
```
forgot to define PinnedDrop, with PinnedDrop enabled:
```
error[E0277]: the trait bound `DropStruct: PinnedDrop` is not satisfied
--> third_party/crubit/rs_bindings_from_cc/support/ctor_proc_macros_test.rs:201:5
|
201 | #[::ctor::recursively_pinned(PinnedDrop)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `PinnedDrop` is not implemented for `DropStruct`
|
= help: the following other types implement trait `PinnedDrop`:
...
= note: this error originates in the attribute macro `::ctor::recursively_pinned` (in Nightly builds, run with -Z macro-backtrace for more info)
```
defined Drop (without PinnedDrop enabled)
```
error[E0119]: conflicting implementations of trait `ctor::macro_internal::DoNotImplDrop` for type `test_pinned_drop::DropStruct`
--> third_party/crubit/rs_bindings_from_cc/support/ctor_proc_macros_test.rs:201:5
|
201 | #[::ctor::recursively_pinned]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: conflicting implementation in crate `ctor`:
- impl<T> DoNotImplDrop for T
where T: Drop;
= note: this error originates in the attribute macro `::ctor::recursively_pinned` (in Nightly builds, run with -Z macro-backtrace for more info)
```
defined Drop (*with* PinnedDrop enabled), instead of PinnedDrop:
```
error[E0119]: conflicting implementations of trait `std::ops::Drop` for type `test_pinned_drop::DropStruct`
--> third_party/crubit/rs_bindings_from_cc/support/ctor_proc_macros_test.rs:201:5
|
201 | #[::ctor::recursively_pinned(PinnedDrop)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `test_pinned_drop::DropStruct`
...
208 | impl Drop for DropStruct {
| ------------------------ first implementation here
|
= note: this error originates in the attribute macro `::ctor::recursively_pinned` (in Nightly builds, run with -Z macro-backtrace for more info)
```
PiperOrigin-RevId: 447860359
diff --git a/rs_bindings_from_cc/support/ctor.rs b/rs_bindings_from_cc/support/ctor.rs
index 33f95c9..7fcb259 100644
--- a/rs_bindings_from_cc/support/ctor.rs
+++ b/rs_bindings_from_cc/support/ctor.rs
@@ -601,6 +601,14 @@
pub use std::mem::MaybeUninit;
pub use std::pin::Pin;
+ /// Trait which causes compilation error if a `#[recursively_pinned]` struct
+ /// impls `Drop`.
+ ///
+ /// Idea from https://docs.rs/pin-project/latest/pin_project/attr.pin_project.html#safety
+ pub trait DoNotImplDrop {}
+ #[allow(drop_bounds)]
+ impl<T: Drop> DoNotImplDrop for T {}
+
/// Drops a pointer when dropped.
///
/// Safety: this will drop the pointer, so this is only safe if no other
@@ -634,11 +642,11 @@
pub fn require_recursively_pinned<_T: RecursivelyPinned>() {}
}
-// =====
-// ctor!
-// =====
+// =====================
+// #[recursively_pinned]
+// =====================
-/// The RecursivelyPinned trait asserts that when the struct is pinned, every
+/// The `RecursivelyPinned` trait asserts that when the struct is pinned, every
/// field is also pinned.
///
/// Safety: Only use if you never directly access fields of a pinned object. For
@@ -648,6 +656,33 @@
/// struct.
pub unsafe trait RecursivelyPinned {}
+/// The drop trait for `#[recursively_pinned(PinnedDrop)]` types.
+///
+/// It is never valid to implement `Drop` for a recursively-pinned type, as this
+/// would be unsound: the `&mut self` in `drop` would allow the pin guarantee to
+/// be violated.
+///
+/// Instead, if such a struct is to implement drop, it must pass `PinnedDrop` to
+/// `recursively_pinned`, and implement the `PinnedDrop` trait.
+///
+/// See also the [analogous `pin_project` feature](https://docs.rs/pin-project/latest/pin_project/attr.pinned_drop.html)
+pub trait PinnedDrop {
+ /// Run the drop logic for self.
+ ///
+ /// ## Safety
+ ///
+ /// If called from anywhere other than the automatically-generated
+ /// `Drop::drop`, the behavior is undefined.
+ ///
+ /// To manually drop the value, use `ManuallyDrop` or use
+ /// `std::ptr::drop_in_place` (etc.) instead.
+ unsafe fn pinned_drop(self: Pin<&mut Self>);
+}
+
+// =====
+// ctor!
+// =====
+
/// The `ctor!` macro evaluates to a `Ctor` for a Rust struct, with
/// user-specified fields.
///
diff --git a/rs_bindings_from_cc/support/ctor_proc_macros.rs b/rs_bindings_from_cc/support/ctor_proc_macros.rs
index 5b4e709..d7cafdc 100644
--- a/rs_bindings_from_cc/support/ctor_proc_macros.rs
+++ b/rs_bindings_from_cc/support/ctor_proc_macros.rs
@@ -5,7 +5,9 @@
use proc_macro::TokenStream;
use proc_macro2::{Ident, Span};
use quote::{quote, quote_spanned, ToTokens as _};
+use syn::parse::Parse;
use syn::spanned::Spanned as _;
+use syn::Token;
// TODO(jeanpierreda): derive constructors and assignment for copy and move.
@@ -217,7 +219,37 @@
Ok((projected, impl_block))
}
-/// #[recursively_pinned] pins every field, similar to #[pin_project], and marks the struct `!Unpin`.
+#[derive(Default)]
+struct RecursivelyPinnedArgs {
+ is_pinned_drop: bool,
+}
+
+impl Parse for RecursivelyPinnedArgs {
+ fn parse(input: syn::parse::ParseStream) -> syn::Result<Self> {
+ let args = <syn::punctuated::Punctuated<Ident, Token![,]>>::parse_terminated(input)?;
+ if args.len() > 1 {
+ return Err(syn::Error::new(
+ input.span(), // not args.span(), as that is only for the first argument.
+ &format!("expected at most 1 argument, got: {}", args.len()),
+ ));
+ }
+ let is_pinned_drop = if let Some(arg) = args.first() {
+ if arg != "PinnedDrop" {
+ return Err(syn::Error::new(
+ arg.span(),
+ "unexpected argument (wasn't `PinnedDrop`)",
+ ));
+ }
+ true
+ } else {
+ false
+ };
+ Ok(RecursivelyPinnedArgs { is_pinned_drop })
+ }
+}
+
+/// `#[recursively_pinned]` pins every field, similar to `#[pin_project]`, and
+/// marks the struct `!Unpin`.
///
/// Example:
///
@@ -237,9 +269,35 @@
/// field: i32,
/// }
/// ```
+///
+/// ## Arguments
+///
+/// ### `PinnedDrop`
+///
+/// To define a destructor for a recursively-pinned struct, pass `PinnedDrop`
+/// and implement the `PinnedDrop` trait.
+///
+/// `#[recursively_pinned]` prohibits implementing `Drop`, as that would make it
+/// easy to violate the `Pin` guarantee. Instead, to define a destructor, one
+/// must define a `PinnedDrop` impl, as so:
+///
+/// ```
+/// #[recursively_pinned(PinnedDrop)]
+/// struct S {
+/// field: i32,
+/// }
+///
+/// impl PinnedDrop for S {
+/// unsafe fn pinned_drop(self: Pin<&mut Self>) {
+/// println!("I am being destroyed!");
+/// }
+/// }
+/// ```
+///
+/// (This is analogous to `#[pin_project(PinnedDrop)]`.)
#[proc_macro_attribute]
pub fn recursively_pinned(args: TokenStream, item: TokenStream) -> TokenStream {
- assert!(args.is_empty(), "recursively_pinned does not take any arguments.");
+ let args = syn::parse_macro_input!(args as RecursivelyPinnedArgs);
let input = syn::parse_macro_input!(item as syn::DeriveInput);
let (projected, projected_impl) = match projected_struct(input.clone()) {
@@ -250,6 +308,25 @@
let name = input.ident.clone();
+ let drop_impl = if args.is_pinned_drop {
+ quote! {
+ impl Drop for #name {
+ fn drop(&mut self) {
+ unsafe {::ctor::PinnedDrop::pinned_drop(::std::pin::Pin::new_unchecked(self))}
+ }
+ }
+ }
+ } else {
+ quote! {
+ impl ::ctor::macro_internal::DoNotImplDrop for #name {}
+ /// A no-op PinnedDrop that will cause an error if the user also defines PinnedDrop,
+ /// due to forgetting to pass `PinnedDrop` to #[recursively_pinned(PinnedDrop)]`.
+ impl ::ctor::PinnedDrop for #name {
+ unsafe fn pinned_drop(self: ::std::pin::Pin<&mut Self>) {}
+ }
+ }
+ };
+
let expanded = quote! {
#input
#projected
@@ -263,6 +340,8 @@
}
}
+ #drop_impl
+
unsafe impl ::ctor::RecursivelyPinned for #name {}
impl !Unpin for #name {}
};
diff --git a/rs_bindings_from_cc/support/ctor_proc_macros_test.rs b/rs_bindings_from_cc/support/ctor_proc_macros_test.rs
index 9fd70f9..8bce3be 100644
--- a/rs_bindings_from_cc/support/ctor_proc_macros_test.rs
+++ b/rs_bindings_from_cc/support/ctor_proc_macros_test.rs
@@ -192,3 +192,33 @@
assert_eq!(p.y, 0.0);
// TODO(jeanpierreda): negative compilation test for e.g. `p.x = 1;`
}
+
+// TODO(jeanpierreda): negative compilation tests for invalid parameters to
+// #[recursively_pinned]:
+// * unknown parameter
+// * passing the same parameter twice
+// * extra parameters after parameter parsing is complete
+
+// TODO(jeanpierreda): negative compilation tests for Drop / PinnedDrop failures:
+// * implemented Drop
+// * forgot to implement PinnedDrop
+// * implemented PinnedDrop, but forgot to pass in PinnedDrop to
+// `::ctor::recursively_pinned`.
+
+#[test]
+fn test_pinned_drop() {
+ use ::std::cell::Cell;
+ use ::std::rc::Rc;
+
+ #[::ctor::recursively_pinned(PinnedDrop)]
+ struct DropStruct(Rc<Cell<bool>>);
+ impl ::ctor::PinnedDrop for DropStruct {
+ unsafe fn pinned_drop(self: ::std::pin::Pin<&mut Self>) {
+ (&*self.project().0).set(true);
+ }
+ }
+
+ let called_drop = Rc::new(Cell::new(false));
+ let _ = DropStruct(called_drop.clone());
+ assert!(called_drop.get(), "PinnedDrop::pinned_drop was not called");
+}
