Practice good Self hygiene (#2127) (#2313)

This commit revises the `KnownLayout` derive on `repr(C)` target structs to
preserve the correct resolution of `Self` when constructing
`__ZerocopyKnownLayoutMaybeUninit`. This type contains a `MaybeUninit` version
of each of the target struct's field types. Previously, it achieved this by
simply copying the tokens corresponding to field types from the definition of
the target struct into the definition of `__ZerocopyKnownLayoutMaybeUninit`

However, on types like this:

    #[repr(C)]
    struct Struct([u8; Self::N]);

…this approach is insufficient. Pasted into `__ZerocopyKnownLayoutMaybeUninit`,
`Self` ceases to refer to the target struct and instead refers to
`__ZerocopyKnownLayoutMaybeUninit`.

To preserve `Self` hygiene, this commit defines a struct for projecting the
field types of the target struct based on their index:

    pub unsafe trait Field<Index> {
        type Type: ?Sized;
    }

…then implements it for each of the field types of the target struct; e.g.:

    struct Index<const N: usize>;

    impl Field<Index<0>> for Struct {
        type Type = [u8; Self::N];
    }

With this, the fields of `__ZerocopyKnownLayoutMaybeUninit` can be defined
hygienically; e.g., as `<Struct as Field<0>>::Type`.

Fixes #2116

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>

Author: google-pr-creation-bot

src/util/macro_util.rs

@@ -29,6 +29,20 @@ use crate::{
     Immutable, IntoBytes, Ptr, TryFromBytes, Unalign, ValidityError,
 };
 
+/// Projects the type of the field at `Index` in `Self`.
+///
+/// The `Index` parameter is any sort of handle that identifies the field; its
+/// definition is the obligation of the implementer.
+///
+/// # Safety
+///
+/// Unsafe code may assume that this accurately reflects the definition of
+/// `Self`.
+pub unsafe trait Field<Index> {
+    /// The type of the field at `Index`.
+    type Type: ?Sized;
+}
+
 #[cfg_attr(
     zerocopy_diagnostic_on_unimplemented,
     diagnostic::on_unimplemented(

zerocopy-derive/src/ext.rs

@@ -8,7 +8,7 @@
 
 use proc_macro2::{Span, TokenStream};
 use quote::ToTokens;
-use syn::{Data, DataEnum, DataStruct, DataUnion, Field, Ident, Index, Type};
+use syn::{Data, DataEnum, DataStruct, DataUnion, Field, Ident, Index, Type, Visibility};
 
 pub(crate) trait DataExt {
     /// Extracts the names and types of all fields. For enums, extracts the names
@@ -19,23 +19,23 @@ pub(crate) trait DataExt {
     /// makes sense because we don't care about where they live - we just care
     /// about transitive ownership. But for field names, we'd only use them when
     /// generating is_bit_valid, which cares about where they live.
-    fn fields(&self) -> Vec<(TokenStream, &Type)>;
+    fn fields(&self) -> Vec<(&Visibility, TokenStream, &Type)>;
 
-    fn variants(&self) -> Vec<Vec<(TokenStream, &Type)>>;
+    fn variants(&self) -> Vec<Vec<(&Visibility, TokenStream, &Type)>>;
 
     fn tag(&self) -> Option<Ident>;
 }
 
 impl DataExt for Data {
-    fn fields(&self) -> Vec<(TokenStream, &Type)> {
+    fn fields(&self) -> Vec<(&Visibility, TokenStream, &Type)> {
         match self {
             Data::Struct(strc) => strc.fields(),
             Data::Enum(enm) => enm.fields(),
             Data::Union(un) => un.fields(),
         }
     }
 
-    fn variants(&self) -> Vec<Vec<(TokenStream, &Type)>> {
+    fn variants(&self) -> Vec<Vec<(&Visibility, TokenStream, &Type)>> {
         match self {
             Data::Struct(strc) => strc.variants(),
             Data::Enum(enm) => enm.variants(),
@@ -53,11 +53,11 @@ impl DataExt for Data {
 }
 
 impl DataExt for DataStruct {
-    fn fields(&self) -> Vec<(TokenStream, &Type)> {
+    fn fields(&self) -> Vec<(&Visibility, TokenStream, &Type)> {
         map_fields(&self.fields)
     }
 
-    fn variants(&self) -> Vec<Vec<(TokenStream, &Type)>> {
+    fn variants(&self) -> Vec<Vec<(&Visibility, TokenStream, &Type)>> {
         vec![self.fields()]
     }
 
@@ -67,11 +67,11 @@ impl DataExt for DataStruct {
 }
 
 impl DataExt for DataEnum {
-    fn fields(&self) -> Vec<(TokenStream, &Type)> {
+    fn fields(&self) -> Vec<(&Visibility, TokenStream, &Type)> {
         map_fields(self.variants.iter().flat_map(|var| &var.fields))
     }
 
-    fn variants(&self) -> Vec<Vec<(TokenStream, &Type)>> {
+    fn variants(&self) -> Vec<Vec<(&Visibility, TokenStream, &Type)>> {
         self.variants.iter().map(|var| map_fields(&var.fields)).collect()
     }
 
@@ -81,11 +81,11 @@ impl DataExt for DataEnum {
 }
 
 impl DataExt for DataUnion {
-    fn fields(&self) -> Vec<(TokenStream, &Type)> {
+    fn fields(&self) -> Vec<(&Visibility, TokenStream, &Type)> {
         map_fields(&self.fields.named)
     }
 
-    fn variants(&self) -> Vec<Vec<(TokenStream, &Type)>> {
+    fn variants(&self) -> Vec<Vec<(&Visibility, TokenStream, &Type)>> {
         vec![self.fields()]
     }
 
@@ -96,12 +96,13 @@ impl DataExt for DataUnion {
 
 fn map_fields<'a>(
     fields: impl 'a + IntoIterator<Item = &'a Field>,
-) -> Vec<(TokenStream, &'a Type)> {
+) -> Vec<(&'a Visibility, TokenStream, &'a Type)> {
     fields
         .into_iter()
         .enumerate()
         .map(|(idx, f)| {
             (
+                &f.vis,
                 f.ident
                     .as_ref()
                     .map(ToTokens::to_token_stream)

zerocopy-derive/src/lib.rs

@@ -144,8 +144,8 @@ fn derive_known_layout_inner(ast: &DeriveInput, _top_level: Trait) -> Result<Tok
         Some((trailing_field, leading_fields)),
     ) = (is_repr_c_struct, fields.split_last())
     {
-        let (_name, trailing_field_ty) = trailing_field;
-        let leading_fields_tys = leading_fields.iter().map(|(_name, ty)| ty);
+        let (_vis, trailing_field_name, trailing_field_ty) = trailing_field;
+        let leading_fields_tys = leading_fields.iter().map(|(_vis, _name, ty)| ty);
 
         let core_path = quote!(::zerocopy::util::macro_util::core_reexport);
         let repr_align = repr
@@ -266,20 +266,66 @@ fn derive_known_layout_inner(ast: &DeriveInput, _top_level: Trait) -> Result<Tok
                 Default::default()
             };
 
+            // Generate a valid ident for a type-level handle to a field of a
+            // given `name`.
+            let field_index =
+                |name| Ident::new(&format!("__Zerocopy_Field_{}", name), ident.span());
+
+            let field_indices: Vec<_> =
+                fields.iter().map(|(_vis, name, _ty)| field_index(name)).collect();
+
+            // Define the collection of type-level field handles.
+            let field_defs = field_indices.iter().zip(&fields).map(|(idx, (vis, _, _))| {
+                quote! {
+                    #[allow(non_camel_case_types)]
+                    #vis struct #idx;
+                }
+            });
+
+            let field_impls = field_indices.iter().zip(&fields).map(|(idx, (_, _, ty))| quote! {
+                // SAFETY: `#ty` is the type of `#ident`'s field at `#idx`.
+                unsafe impl #impl_generics ::zerocopy::util::macro_util::Field<#idx> for #ident #ty_generics
+                where
+                    #predicates
+                {
+                    type Type = #ty;
+                }
+            });
+
+            let trailing_field_index = field_index(trailing_field_name);
+            let leading_field_indices =
+                leading_fields.iter().map(|(_vis, name, _ty)| field_index(name));
+
+            let trailing_field_ty = quote! {
+                <#ident #ty_generics as
+                    ::zerocopy::util::macro_util::Field<#trailing_field_index>
+                >::Type
+            };
+
             let methods = make_methods(&parse_quote! {
                 <#trailing_field_ty as ::zerocopy::KnownLayout>::MaybeUninit
             });
 
             quote! {
+                #(#field_defs)*
+
+                #(#field_impls)*
+
                 // SAFETY: This has the same layout as the derive target type,
-                // except that it admits uninit bytes. This is ensured by using the
-                // same repr as the target type, and by using field types which have
-                // the same layout as the target type's fields, except that they
-                // admit uninit bytes.
+                // except that it admits uninit bytes. This is ensured by using
+                // the same repr as the target type, and by using field types
+                // which have the same layout as the target type's fields,
+                // except that they admit uninit bytes. We indirect through
+                // `Field` to ensure that occurrences of `Self` resolve to
+                // `#ty`, not `__ZerocopyKnownLayoutMaybeUninit` (see #2116).
                 #repr
                 #[doc(hidden)]
                 #vis struct __ZerocopyKnownLayoutMaybeUninit<#params> (
-                    #(::zerocopy::util::macro_util::core_reexport::mem::MaybeUninit<#leading_fields_tys>,)*
+                    #(::zerocopy::util::macro_util::core_reexport::mem::MaybeUninit<
+                        <#ident #ty_generics as
+                            ::zerocopy::util::macro_util::Field<#leading_field_indices>
+                        >::Type
+                    >,)*
                     <#trailing_field_ty as ::zerocopy::KnownLayout>::MaybeUninit
                 )
                 where
@@ -295,9 +341,6 @@ fn derive_known_layout_inner(ast: &DeriveInput, _top_level: Trait) -> Result<Tok
                 unsafe impl #impl_generics ::zerocopy::KnownLayout for __ZerocopyKnownLayoutMaybeUninit #ty_generics
                 where
                     #trailing_field_ty: ::zerocopy::KnownLayout,
-                    // This bound may appear to be superfluous, but is required
-                    // on our MSRV (1.55) to avoid an ICE.
-                    <#trailing_field_ty as ::zerocopy::KnownLayout>::MaybeUninit: ::zerocopy::KnownLayout,
                     #predicates
                 {
                     #[allow(clippy::missing_inline_in_public_items)]
@@ -494,8 +537,8 @@ fn derive_try_from_bytes_struct(
 ) -> Result<TokenStream, Error> {
     let extras = try_gen_trivial_is_bit_valid(ast, top_level).unwrap_or_else(|| {
         let fields = strct.fields();
-        let field_names = fields.iter().map(|(name, _ty)| name);
-        let field_tys = fields.iter().map(|(_name, ty)| ty);
+        let field_names = fields.iter().map(|(_vis, name, _ty)| name);
+        let field_tys = fields.iter().map(|(_vis, _name, ty)| ty);
         quote!(
             // SAFETY: We use `is_bit_valid` to validate that each field is
             // bit-valid, and only return `true` if all of them are. The bit
@@ -552,8 +595,8 @@ fn derive_try_from_bytes_union(
         FieldBounds::All(&[TraitBound::Slf, TraitBound::Other(Trait::Immutable)]);
     let extras = try_gen_trivial_is_bit_valid(ast, top_level).unwrap_or_else(|| {
         let fields = unn.fields();
-        let field_names = fields.iter().map(|(name, _ty)| name);
-        let field_tys = fields.iter().map(|(_name, ty)| ty);
+        let field_names = fields.iter().map(|(_vis, name, _ty)| name);
+        let field_tys = fields.iter().map(|(_vis, _name, ty)| ty);
         quote!(
             // SAFETY: We use `is_bit_valid` to validate that any field is
             // bit-valid; we only return `true` if at least one of them is. The
@@ -1419,12 +1462,13 @@ fn impl_block<D: DataExt>(
         parse_quote!(#ty: #(#traits)+*)
     }
     let field_type_bounds: Vec<_> = match (field_type_trait_bounds, &fields[..]) {
-        (FieldBounds::All(traits), _) => {
-            fields.iter().map(|(_name, ty)| bound_tt(ty, normalize_bounds(trt, traits))).collect()
-        }
+        (FieldBounds::All(traits), _) => fields
+            .iter()
+            .map(|(_vis, _name, ty)| bound_tt(ty, normalize_bounds(trt, traits)))
+            .collect(),
         (FieldBounds::None, _) | (FieldBounds::Trailing(..), []) => vec![],
         (FieldBounds::Trailing(traits), [.., last]) => {
-            vec![bound_tt(last.1, normalize_bounds(trt, traits))]
+            vec![bound_tt(last.2, normalize_bounds(trt, traits))]
         }
         (FieldBounds::Explicit(bounds), _) => bounds,
     };
@@ -1436,7 +1480,7 @@ fn impl_block<D: DataExt>(
     let padding_check_bound =
         padding_check.and_then(|check| (!fields.is_empty()).then_some(check)).map(|check| {
             let variant_types = variants.iter().map(|var| {
-                let types = var.iter().map(|(_name, ty)| ty);
+                let types = var.iter().map(|(_vis, _name, ty)| ty);
                 quote!([#(#types),*])
             });
             let validator_context = check.validator_macro_context();

zerocopy-derive/src/output_tests.rs

@@ -176,20 +176,47 @@ fn test_known_layout() {
                         <U>::pointer_to_metadata(ptr as *mut _)
                     }
                 }
+                #[allow(non_camel_case_types)]
+                struct __Zerocopy_Field_0;
+                #[allow(non_camel_case_types)]
+                struct __Zerocopy_Field_1;
+                unsafe impl<T, U> ::zerocopy::util::macro_util::Field<__Zerocopy_Field_0>
+                for Foo<T, U> {
+                    type Type = T;
+                }
+                unsafe impl<T, U> ::zerocopy::util::macro_util::Field<__Zerocopy_Field_1>
+                for Foo<T, U> {
+                    type Type = U;
+                }
                 #[repr(C)]
                 #[repr(align(2))]
                 #[doc(hidden)]
                 struct __ZerocopyKnownLayoutMaybeUninit<T, U>(
-                    ::zerocopy::util::macro_util::core_reexport::mem::MaybeUninit<T>,
-                    <U as ::zerocopy::KnownLayout>::MaybeUninit,
+                    ::zerocopy::util::macro_util::core_reexport::mem::MaybeUninit<
+                        <Foo<T, U> as ::zerocopy::util::macro_util::Field<__Zerocopy_Field_0>>::Type,
+                    >,
+                    <<Foo<
+                        T,
+                        U,
+                    > as ::zerocopy::util::macro_util::Field<
+                        __Zerocopy_Field_1,
+                    >>::Type as ::zerocopy::KnownLayout>::MaybeUninit,
                 )
                 where
-                    U: ::zerocopy::KnownLayout;
-                unsafe impl<T, U> ::zerocopy::KnownLayout
-                for __ZerocopyKnownLayoutMaybeUninit<T, U>
+                    <Foo<
+                        T,
+                        U,
+                    > as ::zerocopy::util::macro_util::Field<
+                        __Zerocopy_Field_1,
+                    >>::Type: ::zerocopy::KnownLayout;
+                unsafe impl<T, U> ::zerocopy::KnownLayout for __ZerocopyKnownLayoutMaybeUninit<T, U>
                 where
-                    U: ::zerocopy::KnownLayout,
-                    <U as ::zerocopy::KnownLayout>::MaybeUninit: ::zerocopy::KnownLayout,
+                    <Foo<
+                        T,
+                        U,
+                    > as ::zerocopy::util::macro_util::Field<
+                        __Zerocopy_Field_1,
+                    >>::Type: ::zerocopy::KnownLayout,
                 {
                     #[allow(clippy::missing_inline_in_public_items)]
                     fn only_derive_is_allowed_to_implement_this_trait() {}
@@ -205,7 +232,12 @@ fn test_known_layout() {
                         meta: Self::PointerMetadata,
                     ) -> ::zerocopy::util::macro_util::core_reexport::ptr::NonNull<Self> {
                         use ::zerocopy::KnownLayout;
-                        let trailing = <<U as ::zerocopy::KnownLayout>::MaybeUninit as KnownLayout>::raw_from_ptr_len(
+                        let trailing = <<<Foo<
+                            T,
+                            U,
+                        > as ::zerocopy::util::macro_util::Field<
+                            __Zerocopy_Field_1,
+                        >>::Type as ::zerocopy::KnownLayout>::MaybeUninit as KnownLayout>::raw_from_ptr_len(
                             bytes,
                             meta,
                         );
@@ -218,7 +250,12 @@ fn test_known_layout() {
                     }
                     #[inline(always)]
                     fn pointer_to_metadata(ptr: *mut Self) -> Self::PointerMetadata {
-                        <<U as ::zerocopy::KnownLayout>::MaybeUninit>::pointer_to_metadata(
+                        <<<Foo<
+                            T,
+                            U,
+                        > as ::zerocopy::util::macro_util::Field<
+                            __Zerocopy_Field_1,
+                        >>::Type as ::zerocopy::KnownLayout>::MaybeUninit>::pointer_to_metadata(
                             ptr as *mut _,
                         )
                     }