Fix unsoundness in impl_for_transparent_wrapper!

src/impls.rs

@@ -441,23 +441,26 @@ safety_comment! {
 }
 
 macro_rules! impl_traits_for_atomics {
-    ($($atomics:ident [$inners:ident]),* $(,)?) => {
+    ($($atomics:ident),* $(,)?) => {
         $(
-            impl_for_transparent_wrapper!(TryFromBytes for $atomics [UnsafeCell<$inners>]);
-            impl_for_transparent_wrapper!(FromZeros for $atomics [UnsafeCell<$inners>]);
-            impl_for_transparent_wrapper!(FromBytes for $atomics [UnsafeCell<$inners>]);
-            impl_for_transparent_wrapper!(IntoBytes for $atomics [UnsafeCell<$inners>]);
+            impl_for_transparent_wrapper!(=> TryFromBytes for $atomics);
+            impl_for_transparent_wrapper!(=> FromZeros for $atomics);
+            impl_for_transparent_wrapper!(=> FromBytes for $atomics);
+            impl_for_transparent_wrapper!(=> IntoBytes for $atomics);
         )*
     };
 }
 
 #[rustfmt::skip]
 impl_traits_for_atomics!(
-    AtomicBool [bool],
-    AtomicI16 [i16], AtomicI32 [i32], AtomicI8 [i8], AtomicIsize [isize],
-    AtomicU16 [u16], AtomicU32 [u32], AtomicU8 [u8], AtomicUsize [usize],
+    AtomicI16, AtomicI32, AtomicI8, AtomicIsize,
+    AtomicU16, AtomicU32, AtomicU8, AtomicUsize,
 );
 
+impl_for_transparent_wrapper!(=> TryFromBytes for AtomicBool);
+impl_for_transparent_wrapper!(=> FromZeros for AtomicBool);
+impl_for_transparent_wrapper!(=> IntoBytes for AtomicBool);
+
 safety_comment! {
     /// SAFETY:
     /// Per [1], `AtomicBool`, `AtomicU8`, and `AtomicI8` have the same size as

src/macros.rs

@@ -208,168 +208,136 @@ macro_rules! unsafe_impl {
 macro_rules! impl_for_transparent_wrapper {
     (
         $(#[$attr:meta])*
-        $tyvar:ident $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )?
+        $($tyvar:ident $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )?)?
         => $trait:ident for $ty:ty $(; |$candidate:ident $(: MaybeAligned<$ref_repr:ty>)? $(: Maybe<$ptr_repr:ty>)?| $is_bit_valid:expr)?
     ) => {
         $(#[$attr])*
         #[allow(non_local_definitions)]
-        // SAFETY:
-        // - We explicitly add a `$tyvar: $trait` bound (regardless of what
-        //   bounds the caller has provided).
-        // - Inside of `only_derive_is_allowed_to_implement_this_trait`, `f` is
-        //   generic over the any `I: Invariants`, and is generic over the same
-        //   `$tyvar` bounds as the outer impl (with the exception of `$trait`).
-        // - `f` can only compile if its internal call to
-        //   `is_transparent_wrapper` compiles.
+
+        // This block implements `$trait` for `$ty` under the following
+        // conditions:
+        // - `$ty: TransparentWrapper`
+        // - `$ty::Inner: $trait`
+        // - For some `Xxx`, `$ty::XxxVariance = Covariant` (`Xxx` is determined
+        //   by the `@define_is_transparent_wrapper` macro arms). This bound
+        //   ensures that some layout property is the same between `$ty` and
+        //   `$ty::Inner`. Which layout property this is depends on the trait
+        //   being implemented (for example, `FromBytes` is not concerned with
+        //   alignment, but is concerned with bit validity).
         //
-        // The definition of `is_transparent_wrapper<I, T, W>` is generated by
-        // each `@is_transparent_wrapper` macro arm. Each arm is parameterized
-        // by `$trait`, and emits bounds which are sufficient to ensure that
-        // this is a sound implementation of `$trait for W` *so long as* `T:
-        // $trait`. In `f`, we call `is_transparent_wrapper<I, $tyvar, $ty>`,
-        // and so if this code compiles, that guarantees that `$ty` satisfies
-        // the same bounds for all `$tyvar: $trait`. Thus, so long as the bounds
-        // emitted by the `@is_transparent_wrapper` arm are sound, then this
-        // entire impl is sound.
+        // In other words, `$ty` is guaranteed to soundly implement `$trait`
+        // because some property of its layout is the same as `$ty::Inner`,
+        // which implements `$trait`. Most of the complexity in this macro is to
+        // ensure that the above-mentioned conditions are actually met, and that
+        // the proper variance (ie, the proper layout property) is chosen.
+
+        // SAFETY:
+        // - `is_transparent_wrapper<I, W>` requires:
+        //   - `W: TransparentWrapper<I>`
+        //   - `W::Inner: $trait`
+        // - `f` is generic over `I: Invariants`, and in its body, calls
+        //   `is_transparent_wrapper::<I, $ty>()`. Thus, this code will only
+        //   compile if, for all `I: Invariants`:
+        //   - `$ty: TransparentWrapper<I>`
+        //   - `$ty::Inner: $trait`
         //
-        // Each `@is_transparent_wrapper` arm contains its own safety comment
-        // which explains why its bounds are sound.
-        unsafe impl<$tyvar: $($(? $optbound +)* $($bound +)*)? $trait> $trait for $ty {
+        // These two facts - that `$ty: TransparentWrapper<I>` and that
+        // `$ty::Inner: $trait` - are the preconditions to the full safety
+        // proofs, which are completed below in the
+        // `@define_is_transparent_wrapper` macro arms. The safety proof is
+        // slightly different for each trait.
+        unsafe impl<$($tyvar $(: $(? $optbound +)* $($bound +)*)?)?> $trait for $ty {
             #[allow(dead_code, clippy::missing_inline_in_public_items)]
             #[cfg_attr(coverage_nightly, coverage(off))]
             fn only_derive_is_allowed_to_implement_this_trait() {
                 use crate::{pointer::invariant::Invariants, util::*};
 
-                impl_for_transparent_wrapper!(@is_transparent_wrapper $trait);
+                impl_for_transparent_wrapper!(@define_is_transparent_wrapper $trait);
 
                 #[cfg_attr(coverage_nightly, coverage(off))]
-                fn f<I: Invariants, $tyvar $(: $(? $optbound +)* $($bound +)*)?>() {
-                    is_transparent_wrapper::<I, $tyvar, $ty>();
+                fn f<I: Invariants, $($tyvar $(: $(? $optbound +)* $($bound +)*)?)?>() {
+                    is_transparent_wrapper::<I, $ty>();
                 }
             }
 
             impl_for_transparent_wrapper!(
                 @is_bit_valid
-                <$tyvar: $($(? $optbound +)* $($bound +)*)?>
+                $(<$tyvar $(: $(? $optbound +)* $($bound +)*)?>)?
                 $trait for $ty
             );
         }
     };
-    (
-        $(#[$attr:meta])*
-        for $ty:ty [$inner:ty] $(; |$candidate:ident $(: MaybeAligned<$ref_repr:ty>)? $(: Maybe<$ptr_repr:ty>)?| $is_bit_valid:expr)?
-    ) => {};
-    (
-        $(#[$attr:meta])*
-        $trait:ident $(, $traits:ident)* for $ty:ty [$inner:ty] $(; |$candidate:ident $(: MaybeAligned<$ref_repr:ty>)? $(: Maybe<$ptr_repr:ty>)?| $is_bit_valid:expr)?
-    ) => {
-        impl_for_transparent_wrapper!(
-            $(#[$attr])*
-            $($traits),* for $ty [$inner] $(; |$candidate $(: MaybeAligned<$ref_repr>)? $(: Maybe<$ptr_repr>)?| $is_bit_valid:expr)?
-        );
-
-        $(#[$attr])*
-        #[allow(non_local_definitions)]
-        // SAFETY:
-        // - We explicitly add a `$tyvar: $trait` bound (regardless of what
-        //   bounds the caller has provided).
-        // - Inside of `only_derive_is_allowed_to_implement_this_trait`, `f` is
-        //   generic over the any `I: Invariants`.
-        // - `f` can only compile if its internal call to
-        //   `is_transparent_wrapper` compiles.
-        //
-        // The definition of `is_transparent_wrapper<I, T, W>` is generated by
-        // each `@is_transparent_wrapper` macro arm. Each arm is parameterized
-        // by `$trait`, and emits bounds which are sufficient to ensure that
-        // this is a sound implementation of `$trait for W` *so long as* `T:
-        // $trait`. In `f`, we call `is_transparent_wrapper<I, $inner, $ty>`,
-        // and so if this code compiles, that guarantees that `$ty` satisfies
-        // the same bounds so long as `$inner: $trait`. Thus, so long as the
-        // bounds emitted by the `@is_transparent_wrapper` arm are sound, then
-        // this entire impl is sound.
-        //
-        // Each `@is_transparent_wrapper` arm contains its own safety comment
-        // which explains why its bounds are sound.
-        unsafe impl $trait for $ty {
-            #[allow(dead_code, clippy::missing_inline_in_public_items)]
-            #[cfg_attr(coverage_nightly, coverage(off))]
-            fn only_derive_is_allowed_to_implement_this_trait() {
-                use crate::{pointer::invariant::Invariants, util::*};
-
-                impl_for_transparent_wrapper!(@is_transparent_wrapper $trait);
-
-                #[cfg_attr(coverage_nightly, coverage(off))]
-                fn f<I: Invariants>() {
-                    is_transparent_wrapper::<I, $inner, $ty>();
-                }
-            }
-
-            impl_for_transparent_wrapper!(@is_bit_valid $trait for $ty);
-        }
-    };
-    (@is_transparent_wrapper Immutable) => {
-        // SAFETY: `W: TransparentWrapper<UnsafeCellVariance=Covariant>`
+    (@define_is_transparent_wrapper Immutable) => {
+        // SAFETY: `W: TransparentWrapper<UnsafeCellVariance = Covariant>`
         // requires that `W` has `UnsafeCell`s at the same byte offsets as
-        // `W::Inner = T`. `T: Immutable` implies that `T` does not contain any
-        // `UnsafeCell`s, and so `W` does not contain any `UnsafeCell`s. Thus,
-        // `W` can soundly implement `Immutable`.
-        #[cfg_attr(coverage_nightly, coverage(off))]
-        fn is_transparent_wrapper<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, UnsafeCellVariance=Covariant> + ?Sized>() {}
-    };
-    (@is_transparent_wrapper FromZeros) => {
-        // SAFETY: `W: TransparentWrapper<ValidityVariance=Covariant>` requires
-        // that `W` has the same bit validity as `W::Inner = T`. `T: FromZeros`
-        // implies that the all-zeros bit pattern is a bit-valid instance of
-        // `T`, and so the all-zeros bit pattern is a bit-valid instance of `W`.
-        // Thus, `W` can soundly implement `FromZeros`.
-        #[cfg_attr(coverage_nightly, coverage(off))]
-        fn is_transparent_wrapper<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, ValidityVariance=Covariant> + ?Sized>() {}
-    };
-    (@is_transparent_wrapper FromBytes) => {
-        // SAFETY: `W: TransparentWrapper<ValidityVariance=Covariant>` requires
-        // that `W` has the same bit validity as `W::Inner = T`. `T: FromBytes`
-        // implies that any initialized bit pattern is a bit-valid instance of
-        // `T`, and so any initialized bit pattern is a bit-valid instance of
-        // `W`. Thus, `W` can soundly implement `FromBytes`.
-        #[cfg_attr(coverage_nightly, coverage(off))]
-        fn is_transparent_wrapper<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, ValidityVariance=Covariant> + ?Sized>() {}
-    };
-    (@is_transparent_wrapper IntoBytes) => {
-        // SAFETY: `W: TransparentWrapper<ValidityVariance=Covariant>` requires
-        // that `W` has the same bit validity as `W::Inner = T`. `T: IntoBytes`
-        // implies that no bit-valid instance of `T` contains uninitialized
-        // bytes, and so no bit-valid instance of `W` contains uninitialized
-        // bytes. Thus, `W` can soundly implement `IntoBytes`.
-        #[cfg_attr(coverage_nightly, coverage(off))]
-        fn is_transparent_wrapper<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, ValidityVariance=Covariant> + ?Sized>() {}
-    };
-    (@is_transparent_wrapper Unaligned) => {
-        // SAFETY: `W: TransparentWrapper<AlignmentVariance=Covariant>` requires
-        // that `W` has the same alignment as `W::Inner = T`. `T: Unaligned`
-        // implies `T`'s alignment is 1, and so `W`'s alignment is 1. Thus, `W`
-        // can soundly implement `Unaligned`.
-        #[cfg_attr(coverage_nightly, coverage(off))]
-        fn is_transparent_wrapper<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, AlignmentVariance=Covariant> + ?Sized>() {}
-    };
-    (@is_transparent_wrapper TryFromBytes) => {
-        // SAFETY: `W: TransparentWrapper<ValidityVariance=Covariant>` requires
-        // that `W` has the same bit validity as `W::Inner = T`. `T:
-        // TryFromBytes` implies that `<T as TryFromBytes>::is_bit_valid(c)`
-        // only returns `true` if `c` references a bit-valid instance of `T`.
-        // Thus, `<T as TryFromBytes>::is_bit_valid(c)` only returns `true` if
-        // `c` references a bit-valid instance of `W`. Below, we implement `<W
-        // as TryFromBytes>::is_bit_valid` by deferring to `<T as
-        // TryFromBytes>::is_bit_valid`. Thus, it is sound for `W` to implement
-        // `TryFromBytes` with this implementation of `is_bit_valid`.
+        // `W::Inner`. `W::Inner: Immutable` implies that `W::Inner` does not
+        // contain any `UnsafeCell`s, and so `W` does not contain any
+        // `UnsafeCell`s. Since `W = $ty`, `$ty` can soundly implement
+        // `Immutable`.
+        impl_for_transparent_wrapper!(@define_is_transparent_wrapper Immutable, UnsafeCellVariance)
+    };
+    (@define_is_transparent_wrapper FromZeros) => {
+        // SAFETY: `W: TransparentWrapper<ValidityVariance = Covariant>`
+        // requires that `W` has the same bit validity as `W::Inner`. `W::Inner:
+        // FromZeros` implies that the all-zeros bit pattern is a bit-valid
+        // instance of `W::Inner`, and so the all-zeros bit pattern is a
+        // bit-valid instance of `W`. Since `W = $ty`, `$ty` can soundly
+        // implement `FromZeros`.
+        impl_for_transparent_wrapper!(@define_is_transparent_wrapper FromZeros, ValidityVariance)
+    };
+    (@define_is_transparent_wrapper FromBytes) => {
+        // SAFETY: `W: TransparentWrapper<ValidityVariance = Covariant>`
+        // requires that `W` has the same bit validity as `W::Inner`. `W::Inner:
+        // FromBytes` implies that any initialized bit pattern is a bit-valid
+        // instance of `W::Inner`, and so any initialized bit pattern is a
+        // bit-valid instance of `W`. Since `W = $ty`, `$ty` can soundly
+        // implement `FromBytes`.
+        impl_for_transparent_wrapper!(@define_is_transparent_wrapper FromBytes, ValidityVariance)
+    };
+    (@define_is_transparent_wrapper IntoBytes) => {
+        // SAFETY: `W: TransparentWrapper<ValidityVariance = Covariant>`
+        // requires that `W` has the same bit validity as `W::Inner`. `W::Inner:
+        // IntoBytes` implies that no bit-valid instance of `W::Inner` contains
+        // uninitialized bytes, and so no bit-valid instance of `W` contains
+        // uninitialized bytes. Since `W = $ty`, `$ty` can soundly implement
+        // `IntoBytes`.
+        impl_for_transparent_wrapper!(@define_is_transparent_wrapper IntoBytes, ValidityVariance)
+    };
+    (@define_is_transparent_wrapper Unaligned) => {
+        // SAFETY: `W: TransparentWrapper<AlignmentVariance = Covariant>`
+        // requires that `W` has the same alignment as `W::Inner`. `W::Inner:
+        // Unaligned` implies `W::Inner`'s alignment is 1, and so `W`'s
+        // alignment is 1. Since `W = $ty`, `W` can soundly implement
+        // `Unaligned`.
+        impl_for_transparent_wrapper!(@define_is_transparent_wrapper Unaligned, AlignmentVariance)
+    };
+    (@define_is_transparent_wrapper TryFromBytes) => {
+        // SAFETY: `W: TransparentWrapper<ValidityVariance = Covariant>`
+        // requires that `W` has the same bit validity as `W::Inner`. `W::Inner:
+        // TryFromBytes` implies that `<W::Inner as
+        // TryFromBytes>::is_bit_valid(c)` only returns `true` if `c` references
+        // a bit-valid instance of `W::Inner`. Thus, `<W::Inner as
+        // TryFromBytes>::is_bit_valid(c)` only returns `true` if `c` references
+        // a bit-valid instance of `W`. Below, we implement `<W as
+        // TryFromBytes>::is_bit_valid` by deferring to `<W::Inner as
+        // TryFromBytes>::is_bit_valid`. Since `W = $ty`, it is sound for `$ty`
+        // to implement `TryFromBytes` with this implementation of
+        // `is_bit_valid`.
+        impl_for_transparent_wrapper!(@define_is_transparent_wrapper TryFromBytes, ValidityVariance)
+    };
+    (@define_is_transparent_wrapper $trait:ident, $variance:ident) => {
         #[cfg_attr(coverage_nightly, coverage(off))]
-        fn is_transparent_wrapper<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, ValidityVariance=Covariant> + ?Sized>() {}
+        fn is_transparent_wrapper<I: Invariants, W: TransparentWrapper<I, $variance = Covariant> + ?Sized>()
+        where
+            W::Inner: $trait,
+        {}
     };
     (
         @is_bit_valid
         $(<$tyvar:ident $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )?>)?
         TryFromBytes for $ty:ty
     ) => {
-        // SAFETY: See safety comment in `(@is_transparent_wrapper
+        // SAFETY: See safety comment in `(@define_is_transparent_wrapper
         // TryFromBytes)` macro arm for an explanation of why this is a sound
         // implementation of `is_bit_valid`.
         #[inline]

tests/ui-nightly/include_value_not_from_bytes.stderr

@@ -10,12 +10,12 @@ error[E0277]: the trait bound `NotZerocopy<u32>: zerocopy::FromBytes` is not sat
    = help: the following other types implement trait `zerocopy::FromBytes`:
              ()
              AU16
-             AtomicBool
              AtomicI16
              AtomicI32
              AtomicI8
              AtomicIsize
              AtomicU16
+             AtomicU32
            and $N others
 note: required by a bound in `AssertIsFromBytes`
   --> tests/ui-nightly/include_value_not_from_bytes.rs:15:42

tests/ui-nightly/transmute-dst-not-frombytes.stderr

@@ -10,12 +10,12 @@ error[E0277]: the trait bound `NotZerocopy: zerocopy::FromBytes` is not satisfie
    = help: the following other types implement trait `zerocopy::FromBytes`:
              ()
              AU16
-             AtomicBool
              AtomicI16
              AtomicI32
              AtomicI8
              AtomicIsize
              AtomicU16
+             AtomicU32
            and $N others
 note: required by a bound in `AssertIsFromBytes`
   --> tests/ui-nightly/transmute-dst-not-frombytes.rs:19:41

tests/ui-nightly/transmute-mut-dst-not-frombytes.stderr

@@ -9,13 +9,13 @@ error[E0277]: the trait bound `Dst: FromBytes` is not satisfied
    |
    = help: the following other types implement trait `FromBytes`:
              ()
-             AtomicBool
              AtomicI16
              AtomicI32
              AtomicI8
              AtomicIsize
              AtomicU16
              AtomicU32
+             AtomicU8
            and $N others
 note: required by a bound in `AssertDstIsFromBytes`
   --> tests/ui-nightly/transmute-mut-dst-not-frombytes.rs:24:38

tests/ui-nightly/transmute-mut-src-not-frombytes.stderr

@@ -9,13 +9,13 @@ error[E0277]: the trait bound `Src: FromBytes` is not satisfied
    |
    = help: the following other types implement trait `FromBytes`:
              ()
-             AtomicBool
              AtomicI16
              AtomicI32
              AtomicI8
              AtomicIsize
              AtomicU16
              AtomicU32
+             AtomicU8
            and $N others
 note: required by a bound in `AssertSrcIsFromBytes`
   --> tests/ui-nightly/transmute-mut-src-not-frombytes.rs:24:38
@@ -32,13 +32,13 @@ error[E0277]: the trait bound `Src: FromBytes` is not satisfied
    |
    = help: the following other types implement trait `FromBytes`:
              ()
-             AtomicBool
              AtomicI16
              AtomicI32
              AtomicI8
              AtomicIsize
              AtomicU16
              AtomicU32
+             AtomicU8
            and $N others
 note: required by a bound in `AssertSrcIsFromBytes`
   --> tests/ui-nightly/transmute-mut-src-not-frombytes.rs:24:38

tests/ui-nightly/transmute-ref-dst-not-frombytes.stderr

@@ -10,12 +10,12 @@ error[E0277]: the trait bound `Dst: zerocopy::FromBytes` is not satisfied
    = help: the following other types implement trait `zerocopy::FromBytes`:
              ()
              AU16
-             AtomicBool
              AtomicI16
              AtomicI32
              AtomicI8
              AtomicIsize
              AtomicU16
+             AtomicU32
            and $N others
 note: required by a bound in `AssertDstIsFromBytes`
   --> tests/ui-nightly/transmute-ref-dst-not-frombytes.rs:23:34