[derive] TryFromBytes on unions w/o Immutable

TODO:
- Add tests in zerocopy-derive/tests for unions with UnsafeCells
- Add tests (location TBD) for calling &mut methods on TryFromBytes on
  unions with UnsafeCells

gherrit-pr-id: If86f182f8e3fda5d12d680c400c7a0a7f7095ce4

Author: joshlf

src/impls.rs

@@ -808,6 +808,8 @@ unsafe impl<T: TryFromBytes + ?Sized> TryFromBytes for UnsafeCell<T> {
     {
     }
 
+    const IS_IMMUTABLE: bool = false;
+
     #[inline]
     fn is_bit_valid<A: invariant::Reference>(candidate: Maybe<'_, Self, A>) -> bool {
         // The only way to implement this function is using an exclusive-aliased

src/lib.rs

@@ -1448,6 +1448,9 @@ pub unsafe trait TryFromBytes {
     where
         Self: Sized;
 
+    #[doc(hidden)]
+    const IS_IMMUTABLE: bool;
+
     /// Does a given memory range contain a valid instance of `Self`?
     ///
     /// # Safety

src/macros.rs

@@ -780,6 +780,10 @@ macro_rules! cryptocorrosion_derive_traits {
                 $($field_ty: $crate::FromBytes,)*
             )?
         {
+            const IS_IMMUTABLE: bool = true $(
+                && <$field_ty as $crate::TryFromBytes>::IS_IMMUTABLE
+            )*;
+
             fn is_bit_valid<A>(_c: $crate::Maybe<'_, Self, A>) -> bool
             where
                 A: $crate::pointer::invariant::Reference
@@ -923,6 +927,10 @@ macro_rules! cryptocorrosion_derive_traits {
                 $field_ty: $crate::FromBytes,
             )*
         {
+            const IS_IMMUTABLE: bool = true $(
+                && <$field_ty as $crate::TryFromBytes>::IS_IMMUTABLE
+            )*;
+
             fn is_bit_valid<A>(_c: $crate::Maybe<'_, Self, A>) -> bool
             where
                 A: $crate::pointer::invariant::Reference

src/util/macros.rs

@@ -134,6 +134,9 @@ macro_rules! unsafe_impl {
         #[cfg_attr(all(coverage_nightly, __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS), coverage(off))]
         fn only_derive_is_allowed_to_implement_this_trait() {}
 
+        // TODO: THIS IS UNSOUND. We are just using it to unblock ourselves.
+        const IS_IMMUTABLE: bool = true;
+
         #[inline]
         fn is_bit_valid<AA: crate::pointer::invariant::Reference>($candidate: Maybe<'_, Self, AA>) -> bool {
             $is_bit_valid
@@ -143,6 +146,10 @@ macro_rules! unsafe_impl {
         #[allow(clippy::missing_inline_in_public_items)]
         #[cfg_attr(all(coverage_nightly, __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS), coverage(off))]
         fn only_derive_is_allowed_to_implement_this_trait() {}
+
+        // TODO: THIS IS UNSOUND. We are just using it to unblock ourselves.
+        const IS_IMMUTABLE: bool = true;
+
         #[inline(always)] fn is_bit_valid<AA: crate::pointer::invariant::Reference>(_: Maybe<'_, Self, AA>) -> bool { true }
     };
     (@method $trait:ident) => {
@@ -217,6 +224,10 @@ macro_rules! impl_for_transmute_from {
         $(<$tyvar:ident $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )?>)?
         TryFromBytes for $ty:ty [UnsafeCell<$repr:ty>]
     ) => {
+        // TODO: THIS IS UNSOUND because we don't know that `$ty` and `$repr`
+        // have the same interior mutability.
+        const IS_IMMUTABLE: bool = <$repr as TryFromBytes>::IS_IMMUTABLE;
+
         #[inline]
         fn is_bit_valid<A: crate::pointer::invariant::Reference>(candidate: Maybe<'_, Self, A>) -> bool {
             let c: Maybe<'_, Self, crate::pointer::invariant::Exclusive> = candidate.into_exclusive_or_pme();
@@ -232,6 +243,10 @@ macro_rules! impl_for_transmute_from {
         $(<$tyvar:ident $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )?>)?
         TryFromBytes for $ty:ty [<$repr:ty>]
     ) => {
+        // TODO: THIS IS UNSOUND because we don't know that `$ty` and `$repr`
+        // have the same interior mutability.
+        const IS_IMMUTABLE: bool = <$repr as TryFromBytes>::IS_IMMUTABLE;
+
         #[inline]
         fn is_bit_valid<A: crate::pointer::invariant::Reference>(candidate: Maybe<'_, Self, A>) -> bool {
             // SAFETY: This macro ensures that `$repr` and `Self` have the same

zerocopy-derive/src/enum.rs

@@ -8,7 +8,7 @@
 
 use proc_macro2::{Span, TokenStream};
 use quote::quote;
-use syn::{parse_quote, DataEnum, Error, Fields, Generics, Ident, Path};
+use syn::{parse_quote, DataEnum, DeriveInput, Error, Fields, Generics, Ident, Path};
 
 use crate::{derive_try_from_bytes_inner, repr::EnumRepr, Trait};
 
@@ -210,6 +210,7 @@ fn generate_variants_union(generics: &Generics, data: &DataEnum) -> TokenStream
 /// - `repr(int)`: <https://doc.rust-lang.org/reference/type-layout.html#primitive-representation-of-enums-with-fields>
 /// - `repr(C, int)`: <https://doc.rust-lang.org/reference/type-layout.html#combining-primitive-representations-of-enums-with-fields-and-reprc>
 pub(crate) fn derive_is_bit_valid(
+    ast: &DeriveInput,
     enum_ident: &Ident,
     repr: &EnumRepr,
     generics: &Generics,
@@ -280,7 +281,10 @@ pub(crate) fn derive_is_bit_valid(
         }
     });
 
+    let is_immutable = crate::gen_is_immutable(ast, zerocopy_crate);
     Ok(quote! {
+        #is_immutable
+
         // SAFETY: We use `is_bit_valid` to validate that the bit pattern of the
         // enum's tag corresponds to one of the enum's discriminants. Then, we
         // check the bit validity of each field of the corresponding variant.

zerocopy-derive/src/lib.rs

@@ -745,7 +745,11 @@ fn derive_try_from_bytes_struct(
             let fields = strct.fields();
             let field_names = fields.iter().map(|(_vis, name, _ty)| name);
             let field_tys = fields.iter().map(|(_vis, _name, ty)| ty);
+
+            let is_immutable = gen_is_immutable(ast, zerocopy_crate);
             quote!(
+                #is_immutable
+
                 // 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
                 // validity of a struct is just the composition of the bit
@@ -808,15 +812,15 @@ fn derive_try_from_bytes_union(
     top_level: Trait,
     zerocopy_crate: &Path,
 ) -> TokenStream {
-    // FIXME(#5): Remove the `Immutable` bound.
-    let field_type_trait_bounds =
-        FieldBounds::All(&[TraitBound::Slf, TraitBound::Other(Trait::Immutable)]);
     let extras =
         try_gen_trivial_is_bit_valid(ast, top_level, zerocopy_crate).unwrap_or_else(|| {
             let fields = unn.fields();
             let field_names = fields.iter().map(|(_vis, name, _ty)| name);
             let field_tys = fields.iter().map(|(_vis, _name, ty)| ty);
+            let is_immutable = gen_is_immutable(ast, zerocopy_crate);
             quote!(
+                #is_immutable
+
                 // 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
                 // bit validity of a union is not yet well defined in Rust, but it
@@ -828,16 +832,35 @@ fn derive_try_from_bytes_union(
                 where
                     ___ZerocopyAliasing: #zerocopy_crate::pointer::invariant::Reference,
                 {
-                    use #zerocopy_crate::util::macro_util::core_reexport;
+                    use #zerocopy_crate::{
+                        util::macro_util::core_reexport,
+                        pointer::invariant::Aliasing
+                    };
+
+                    trait ConstAssert {
+                        const IS_READ: bool;
+                    }
+
+                    // TODO: What do we name `TryFromBytes`?
+                    impl<T: #zerocopy_crate::TryFromBytes, A: #zerocopy_crate::pointer::invariant::Aliasing> ConstAssert for (T, A) {
+                        const IS_READ: bool = {
+                            assert!(T::IS_IMMUTABLE || A::IS_EXCLUSIVE);
+                            true
+                        };
+                    }
+
+                    assert!(<(Self, ___ZerocopyAliasing) as ConstAssert>::IS_READ);
 
                     false #(|| {
                         // SAFETY:
                         // - `project` is a field projection, and so it addresses a
                         //   subset of the bytes addressed by `slf`
                         // - ..., and so it preserves provenance
-                        // - Since `Self: Immutable` is enforced by
-                        //   `self_type_trait_bounds`, neither `*slf` nor the
-                        //   returned pointer's referent contain any `UnsafeCell`s
+                        // - By the preceding assert, it is either the case that
+                        //   `Self: Immutable` or that `___ZerocopyAliasing` is
+                        //   `Exclusive`. In the former case, `Self: Immutable`
+                        //   ensures that `*slf` nor the returned pointer's
+                        //   referent contain any `UnsafeCell`s.
                         let field_candidate = unsafe {
                             let project = |slf: core_reexport::ptr::NonNull<Self>| {
                                 let slf = slf.as_ptr();
@@ -860,7 +883,7 @@ fn derive_try_from_bytes_union(
                 }
             )
         });
-    ImplBlockBuilder::new(ast, unn, Trait::TryFromBytes, field_type_trait_bounds, zerocopy_crate)
+    ImplBlockBuilder::new(ast, unn, Trait::TryFromBytes, FieldBounds::ALL_SELF, zerocopy_crate)
         .inner_extras(extras)
         .build()
 }
@@ -887,9 +910,9 @@ fn derive_try_from_bytes_enum(
         (Some(is_bit_valid), _) => is_bit_valid,
         // SAFETY: It would be sound for the enum to implement `FomBytes`, as
         // required by `gen_trivial_is_bit_valid_unchecked`.
-        (None, true) => unsafe { gen_trivial_is_bit_valid_unchecked(zerocopy_crate) },
+        (None, true) => unsafe { gen_trivial_is_bit_valid_unchecked(ast, zerocopy_crate) },
         (None, false) => {
-            r#enum::derive_is_bit_valid(&ast.ident, &repr, &ast.generics, enm, zerocopy_crate)?
+            r#enum::derive_is_bit_valid(ast, &ast.ident, &repr, &ast.generics, enm, zerocopy_crate)?
         }
     };
 
@@ -932,8 +955,12 @@ fn try_gen_trivial_is_bit_valid(
     // make this no longer true. To hedge against these, we include an explicit
     // `Self: FromBytes` check in the generated `is_bit_valid`, which is
     // bulletproof.
+
+    let is_immutable = gen_is_immutable(ast, zerocopy_crate);
     if top_level == Trait::FromBytes && ast.generics.params.is_empty() {
         Some(quote!(
+            #is_immutable
+
             // SAFETY: See inline.
             fn is_bit_valid<___ZerocopyAliasing>(
                 _candidate: #zerocopy_crate::Maybe<Self, ___ZerocopyAliasing>,
@@ -963,6 +990,21 @@ fn try_gen_trivial_is_bit_valid(
     }
 }
 
+fn gen_is_immutable(ast: &DeriveInput, zerocopy_crate: &Path) -> proc_macro2::TokenStream {
+    let fields = match &ast.data {
+        Data::Struct(strct) => strct.fields(),
+        Data::Enum(enm) => enm.fields(),
+        Data::Union(unn) => unn.fields(),
+    };
+    let field_tys = fields.iter().map(|(_vis, _name, ty)| ty);
+
+    quote!(
+        const IS_IMMUTABLE: bool = true #(
+            && <#field_tys as #zerocopy_crate::TryFromBytes>::IS_IMMUTABLE
+        )*;
+    )
+}
+
 /// Generates a `TryFromBytes::is_bit_valid` instance that unconditionally
 /// returns true.
 ///
@@ -974,8 +1016,14 @@ fn try_gen_trivial_is_bit_valid(
 ///
 /// The caller must ensure that all initialized bit patterns are valid for
 /// `Self`.
-unsafe fn gen_trivial_is_bit_valid_unchecked(zerocopy_crate: &Path) -> proc_macro2::TokenStream {
+unsafe fn gen_trivial_is_bit_valid_unchecked(
+    ast: &DeriveInput,
+    zerocopy_crate: &Path,
+) -> proc_macro2::TokenStream {
+    let is_immutable = gen_is_immutable(ast, zerocopy_crate);
     quote!(
+        #is_immutable
+
         // SAFETY: The caller of `gen_trivial_is_bit_valid_unchecked` has
         // promised that all initialized bit patterns are valid for `Self`.
         fn is_bit_valid<___ZerocopyAliasing>(
@@ -1146,12 +1194,7 @@ fn derive_from_zeros_union(
     unn: &DataUnion,
     zerocopy_crate: &Path,
 ) -> TokenStream {
-    // FIXME(#5): Remove the `Immutable` bound. It's only necessary for
-    // compatibility with `derive(TryFromBytes)` on unions; not for soundness.
-    let field_type_trait_bounds =
-        FieldBounds::All(&[TraitBound::Slf, TraitBound::Other(Trait::Immutable)]);
-    ImplBlockBuilder::new(ast, unn, Trait::FromZeros, field_type_trait_bounds, zerocopy_crate)
-        .build()
+    ImplBlockBuilder::new(ast, unn, Trait::FromZeros, FieldBounds::ALL_SELF, zerocopy_crate).build()
 }
 
 /// A struct is `FromBytes` if:
@@ -1223,12 +1266,7 @@ fn derive_from_bytes_union(
     unn: &DataUnion,
     zerocopy_crate: &Path,
 ) -> TokenStream {
-    // FIXME(#5): Remove the `Immutable` bound. It's only necessary for
-    // compatibility with `derive(TryFromBytes)` on unions; not for soundness.
-    let field_type_trait_bounds =
-        FieldBounds::All(&[TraitBound::Slf, TraitBound::Other(Trait::Immutable)]);
-    ImplBlockBuilder::new(ast, unn, Trait::FromBytes, field_type_trait_bounds, zerocopy_crate)
-        .build()
+    ImplBlockBuilder::new(ast, unn, Trait::FromBytes, FieldBounds::ALL_SELF, zerocopy_crate).build()
 }
 
 fn derive_into_bytes_struct(

zerocopy-derive/tests/union_from_bytes.rs

@@ -15,23 +15,23 @@ include!("include.rs");
 // A union is `imp::FromBytes` if:
 // - all fields are `imp::FromBytes`
 
-#[derive(Clone, Copy, imp::Immutable, imp::FromBytes)]
+#[derive(Clone, Copy, imp::FromBytes)]
 union Zst {
     a: (),
 }
 
 util_assert_impl_all!(Zst: imp::FromBytes);
 test_trivial_is_bit_valid!(Zst => test_zst_trivial_is_bit_valid);
 
-#[derive(imp::Immutable, imp::FromBytes)]
+#[derive(imp::FromBytes)]
 union One {
     a: u8,
 }
 
 util_assert_impl_all!(One: imp::FromBytes);
 test_trivial_is_bit_valid!(One => test_one_trivial_is_bit_valid);
 
-#[derive(imp::Immutable, imp::FromBytes)]
+#[derive(imp::FromBytes)]
 union Two {
     a: u8,
     b: Zst,
@@ -40,7 +40,7 @@ union Two {
 util_assert_impl_all!(Two: imp::FromBytes);
 test_trivial_is_bit_valid!(Two => test_two_trivial_is_bit_valid);
 
-#[derive(imp::Immutable, imp::FromBytes)]
+#[derive(imp::FromBytes)]
 union TypeParams<'a, T: imp::Copy, I: imp::Iterator>
 where
     I::Item: imp::Copy,
@@ -58,7 +58,7 @@ test_trivial_is_bit_valid!(TypeParams<'static, (), imp::IntoIter<()>> => test_ty
 
 // Deriving `imp::FromBytes` should work if the union has bounded parameters.
 
-#[derive(imp::Immutable, imp::FromBytes)]
+#[derive(imp::FromBytes)]
 #[repr(C)]
 union WithParams<'a: 'b, 'b: 'a, T: 'a + 'b + imp::FromBytes, const N: usize>
 where

zerocopy-derive/tests/union_from_zeros.rs

@@ -15,29 +15,29 @@ include!("include.rs");
 // A union is `imp::FromZeros` if:
 // - all fields are `imp::FromZeros`
 
-#[derive(Clone, Copy, imp::Immutable, imp::FromZeros)]
+#[derive(Clone, Copy, imp::FromZeros)]
 union Zst {
     a: (),
 }
 
 util_assert_impl_all!(Zst: imp::FromZeros);
 
-#[derive(imp::Immutable, imp::FromZeros)]
+#[derive(imp::FromZeros)]
 union One {
     a: bool,
 }
 
 util_assert_impl_all!(One: imp::FromZeros);
 
-#[derive(imp::Immutable, imp::FromZeros)]
+#[derive(imp::FromZeros)]
 union Two {
     a: bool,
     b: Zst,
 }
 
 util_assert_impl_all!(Two: imp::FromZeros);
 
-#[derive(imp::Immutable, imp::FromZeros)]
+#[derive(imp::FromZeros)]
 union TypeParams<'a, T: imp::Copy, I: imp::Iterator>
 where
     I::Item: imp::Copy,
@@ -54,7 +54,7 @@ util_assert_impl_all!(TypeParams<'static, (), imp::IntoIter<()>>: imp::FromZeros
 
 // Deriving `imp::FromZeros` should work if the union has bounded parameters.
 
-#[derive(imp::Immutable, imp::FromZeros)]
+#[derive(imp::FromZeros)]
 #[repr(C)]
 union WithParams<'a: 'b, 'b: 'a, T: 'a + 'b + imp::FromZeros, const N: usize>
 where