Allow either order of `derive(CtorFrom_Default)` / `#[recursively_pinned]`. Primary error: the layout of `CtorInitializedFields` is ignored, I can just clear the attrs! Oopsie woopsie. However, even if we do that, there's other problems that make these macros compose very poorly in the original order. Apparently, ideally, either order should work, but this is definitely messy to support. Another solution would be to detect if they're misordered and swap the order in the macro. I think this is worth deferring to some later time to do "properly" -- this only matters during the process of porting a C++ class to Rust, and that's somewhat of a theoretical concern given the current state of interop. PiperOrigin-RevId: 463065300
diff --git a/rs_bindings_from_cc/support/ctor_proc_macros.rs b/rs_bindings_from_cc/support/ctor_proc_macros.rs index 35a521d..2795a77 100644 --- a/rs_bindings_from_cc/support/ctor_proc_macros.rs +++ b/rs_bindings_from_cc/support/ctor_proc_macros.rs
@@ -13,6 +13,8 @@ // TODO(jeanpierreda): derive constructors and assignment for copy and move. +const FIELD_FOR_MUST_USE_CTOR: &'static str = "__must_use_ctor_to_initialize"; + #[proc_macro_derive(CtorFrom_Default)] pub fn derive_default(item: TokenStream) -> TokenStream { let input = syn::parse_macro_input!(item as syn::DeriveInput); @@ -25,17 +27,23 @@ if let syn::Fields::Unit = data.fields { quote! {} } else { - let filled_fields = data.fields.iter().enumerate().map(|(i, field)| { + let filled_fields = data.fields.iter().enumerate().filter_map(|(i, field)| { let field_i = syn::Index::from(i); - let field_name = match &field.ident { - None => quote! {#field_i}, - Some(name) => quote! {#name}, + let field_name; + // This logic is here in case you derive default on the output of + // `#[recursively_pinned]`, but it's obviously not very flexible. For example, + // maybe we want to compute a non-colliding field name, and maybe there are + // other ordering problems. + match &field.ident { + Some(name) if name == FIELD_FOR_MUST_USE_CTOR => return None, + Some(name) => field_name = quote! {#name}, + None => field_name = quote! {#field_i}, }; let field_type = &field.ty; - quote_spanned! {field.span() => + Some(quote_spanned! {field.span() => #field_name: <#field_type as ::ctor::CtorNew<()>>::ctor_new(()) - } + }) }); quote! {{ #(#filled_fields),* }} } @@ -139,7 +147,8 @@ field.ty = syn::Type::Path(pin_ty); }; // returns the braced parts of a projection pattern and return value. - // e.g. {foo, bar, ..}, {foo: Pin::new_unchecked(foo), bar: Pin::new_unchecked(bar)} + // e.g. {foo, bar, ..}, {foo: Pin::new_unchecked(foo), bar: + // Pin::new_unchecked(bar)} let pat_project = |fields: &mut syn::Fields| { let mut pat = quote! {}; let mut project = quote! {}; @@ -313,7 +322,7 @@ attrs: vec![], vis: syn::Visibility::Inherited, // TODO(jeanpierreda): better hygiene: work even if a field has the same name. - ident: Some(Ident::new("__must_use_ctor_to_initialize", Span::call_site())), + ident: Some(Ident::new(FIELD_FOR_MUST_USE_CTOR, Span::call_site())), colon_token: Some(<syn::Token![:]>::default()), ty: syn::parse_quote!([u8; 0]), }); @@ -431,7 +440,8 @@ // This causes `ctor!(Foo {})` to work, but `Foo{}` to complain of a missing // field. let mut ctor_initialized_input = input.clone(); - // TODO(b/200067242): Remove all attributes that aren't `repr`. + // Removing repr(C) triggers dead-code detection. + ctor_initialized_input.attrs = vec![syn::parse_quote!(#[allow(dead_code)])]; // TODO(jeanpierreda): This should really check for name collisions with any types // used in the fields. Collisions with other names don't matter, because the // type is locally defined within a narrow scope. @@ -513,11 +523,11 @@ definition, quote! { const _: () = { - #[repr(C)] - struct __CrubitCtorS {x: i32} - unsafe impl ::ctor::RecursivelyPinned for S { - type CtorInitializedFields = __CrubitCtorS; - } + #[allow(dead_code)] + struct __CrubitCtorS {x: i32} + unsafe impl ::ctor::RecursivelyPinned for S { + type CtorInitializedFields = __CrubitCtorS; + } }; } ); @@ -567,7 +577,7 @@ definition, quote! { const _: () = { - #[repr(C)] + #[allow(dead_code)] enum __CrubitCtorE { A, B(i32),
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 db73464..4b2d72e 100644 --- a/rs_bindings_from_cc/support/ctor_proc_macros_test.rs +++ b/rs_bindings_from_cc/support/ctor_proc_macros_test.rs
@@ -180,24 +180,21 @@ .x; } -// TODO(b/200067242): Uncomment this test. The derive attribute should be -// cleared on the `CtorInitializedFields` copy of the struct. -// -// #[test] -// fn test_recursively_pinned_struct_derive_default() { -// #[::ctor::recursively_pinned] -// #[derive(::ctor::CtorFrom_Default)] -// struct Struct { -// x: i32, -// y: f32, -// } -// -// ::ctor::emplace! { -// let p = <Struct as ::ctor::CtorNew<()>>::ctor_new(()); -// } -// assert_eq!(p.x, 0); -// assert_eq!(p.y, 0.0); -// } +#[test] +fn test_recursively_pinned_struct_derive_default() { + #[::ctor::recursively_pinned] + #[derive(::ctor::CtorFrom_Default)] + struct Struct { + x: i32, + y: f32, + } + + ::ctor::emplace! { + let p = <Struct as ::ctor::CtorNew<()>>::ctor_new(()); + } + assert_eq!(p.x, 0); + assert_eq!(p.y, 0.0); +} /// The same as the previous test, but with the attribute order swapped. /// This only compiles with macro_attributes_in_derive_output.