Make zerocopy-derive an optional dependency (#176)

This requires a few changes to accomodate: - We still want to be able to derive zerocopy traits on our own types so that we can rely on it to validate the soundness of those impls for us. That means that, when the `derive` feature is enabled, we still use custom derives, but when it's disabled, we implement traits manually. However, we also need to ensure that our manual trait impls have the correct bounds required in order for those impls to be sound. In order to ensure this, we introduce an `impl_or_verify!` macro which, when building with `derive` enabled, verifies that the manual impl matches the one emitted by our custom derive. - Since some doc examples use custom derives, we can no longer run doc tests unconditionally in CI - when the `derive` feature is disabled, doc tests fail. Instead, we disable doc tests by default (in order to run them, `cargo test` must be run with the `--doc` flag). We introduce a separate "Run doc tests" step in CI - this step only runs when the `derive` feature is enabled. Closes #169
google · Aug 16, 2023 · 11a92e4 · 11a92e4
1 parent 4565750
commit 11a92e4
Show file tree

Hide file tree

Showing 13 changed files with 556 additions and 114 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -33,7 +33,7 @@ jobs:
         # versions.
         toolchain: [ "msrv", "stable", "nightly" ]
         target: [ "i686-unknown-linux-gnu", "x86_64-unknown-linux-gnu", "arm-unknown-linux-gnueabi", "aarch64-unknown-linux-gnu", "powerpc-unknown-linux-gnu", "powerpc64-unknown-linux-gnu", "wasm32-wasi" ]
-        features: [ "--no-default-features", "" , "--features __internal_use_only_features_that_work_on_stable", "--all-features" ]
+        features: [ "--no-default-features", "", "--features __internal_use_only_features_that_work_on_stable", "--all-features" ]
         crate: [ "zerocopy", "zerocopy-derive" ]
         exclude:
           # Exclude any combination which uses a non-nightly toolchain but
@@ -168,6 +168,20 @@ jobs:
       # TODO(#22): Re-enable testing on wasm32-wasi once it works.
       if: matrix.toolchain == 'nightly' && matrix.target != 'wasm32-wasi'
 
+    - name: Run doc tests
+      # We explicitly pass `--doc` here because doc tests are disabled by
+      # default in zerocopy's `Cargo.toml`. This is because some doc examples
+      # make use of derives, and so would fail without the `derive` feature
+      # enabled. We skip this step for `zerocopy` when the `derive` feature is
+      # omitted for that reason.
+      run: cargo +${{ env.ZC_TOOLCHAIN }} test --doc --package ${{ matrix.crate }} --target ${{ matrix.target }} ${{ matrix.features }} --verbose
+      # Only run tests when targetting x86 (32- or 64-bit) - we're executing on
+      # x86_64, so we can't run tests for any non-x86 target.
+      #
+      # TODO(https://github.com/dtolnay/trybuild/issues/184#issuecomment-1269097742):
+      # Run compile tests when building for other targets.
+      if: ${{ (contains(matrix.target, 'x86_64') || contains(matrix.target, 'i686')) && !(matrix.crate == 'zerocopy' && !contains(matrix.features, 'derive')) }}
+
     - name: Clippy check
       run: cargo +${{ env.ZC_TOOLCHAIN }} clippy --package ${{ matrix.crate }} --target ${{ matrix.target }} ${{ matrix.features }} --tests --verbose
 
@@ -257,8 +271,14 @@ jobs:
           ver_zerocopy=$(version zerocopy)
           ver_zerocopy_derive=$(version zerocopy-derive)
 
+          # The non-dev dependency version (`.kind == null` filters out the dev
+          # dependency).
           zerocopy_derive_dep_ver=$(cargo metadata --format-version 1 \
-            | jq -r ".packages[] | select(.name == \"zerocopy\").dependencies[] | select(.name == \"zerocopy-derive\").req")
+            | jq -r ".packages[] | select(.name == \"zerocopy\").dependencies[] | select((.name == \"zerocopy-derive\") and .kind == null).req")
+          # The dev dependency version (`.kind == \"dev\"` selects only the dev
+          # dependency).
+          zerocopy_derive_dev_dep_ver=$(cargo metadata --format-version 1 \
+            | jq -r ".packages[] | select(.name == \"zerocopy\").dependencies[] | select((.name == \"zerocopy-derive\") and .kind == \"dev\").req")
 
           if [[ "$ver_zerocopy" == "$ver_zerocopy_derive" ]]; then
             echo "Same crate version ($ver_zerocopy) found for zerocopy and zerocopy-derive." | tee -a $GITHUB_STEP_SUMMARY
@@ -276,3 +296,11 @@ jobs:
               | tee -a $GITHUB_STEP_SUMMARY >&2
             exit 1
           fi
+
+          if [[ "=$ver_zerocopy_derive" == "$zerocopy_derive_dev_dep_ver" ]]; then
+            echo "In dev mode, zerocopy depends upon same version of zerocopy-derive in-tree ($zerocopy_derive_dev_dep_ver)." | tee -a $GITHUB_STEP_SUMMARY
+          else
+            echo "In dev mode, zerocopy depends upon different version of zerocopy-derive ($zerocopy_derive_dev_dep_ver) than the one in-tree ($ver_zerocopy_derive)." \
+              | tee -a $GITHUB_STEP_SUMMARY >&2
+            exit 1
+          fi
diff --git a/Cargo.toml b/Cargo.toml
@@ -28,9 +28,24 @@ all-features = true
 pinned-stable = "1.69.0"
 pinned-nightly = "nightly-2023-05-25"
 
+# Don't run doctests during `cargo test` without the `--doc` flag (the `--doc`
+# flag will still cause doctests to run despite this directive). Running
+# doctests without the `derive` feature fails because our doc examples make use
+# of zerocopy-derive, which is an optional dependency and is disabled by
+# default. Unfortunately, there's no way to automatically include
+# zerocopy-derive as a dependency during doctests, so we have to do it manually
+# by passing the appropriate flags in CI.
+[lib]
+doctest = false
+# Not technically necessary - this is the default value for `path` - but if a
+# `lib` section is present, then `cargo readme` expects it to have a `path`
+# field, and will fail if it's missing.
+path = "src/lib.rs"
+
 [features]
 default = ["byteorder"]
 
+derive = ["zerocopy-derive"]
 alloc = []
 simd = []
 simd-nightly = ["simd"]
@@ -40,7 +55,7 @@ simd-nightly = ["simd"]
 __internal_use_only_features_that_work_on_stable = ["alloc", "simd"]
 
 [dependencies]
-zerocopy-derive = { version = "=0.7.0-alpha.5", path = "zerocopy-derive" }
+zerocopy-derive = { version = "=0.7.0-alpha.5", path = "zerocopy-derive", optional = true }
 
 [dependencies.byteorder]
 version = "1.3"
@@ -56,3 +71,5 @@ static_assertions = "1.1"
 # sometimes change the output format slightly, so a version mismatch can cause
 # CI test failures.
 trybuild = "=1.0.80"
+# In tests, unlike in production, zerocopy-derive is not optional
+zerocopy-derive = { version = "=0.7.0-alpha.5", path = "zerocopy-derive" }
diff --git a/README.md b/README.md
@@ -45,6 +45,10 @@ handling file formats, network packet layouts, etc which don't provide
 alignment guarantees and which may use a byte order different from that of
 the execution platform.
 
+`derive`: Provides derives for the core marker traits via the
+`zerocopy-derive` crate. These derives are re-exported from `zerocopy`, so
+it is not necessary to depend on `zerocopy-derive` directly.
+
 `simd`: When the `simd` feature is enabled, `FromZeroes`, `FromBytes`, and
 `AsBytes` impls are emitted for all stable SIMD types which exist on the
 target platform. Note that the layout of SIMD types is not yet stabilized,

diff --git a/src/byteorder.rs b/src/byteorder.rs
@@ -68,19 +68,12 @@ use core::{
     num::TryFromIntError,
 };
 
-use zerocopy_derive::*;
-
-use crate::{
-    // This allows the custom derives to work. See the comment on the `zerocopy`
-    // module for an explanation.
-    zerocopy,
-    AsBytes,
-};
-
 // We don't reexport `WriteBytesExt` or `ReadBytesExt` because those are only
 // available with the `std` feature enabled, and zerocopy is `no_std` by
 // default.
-pub use byteorder::{BigEndian, ByteOrder, LittleEndian, NativeEndian, NetworkEndian, BE, LE};
+pub use ::byteorder::{BigEndian, ByteOrder, LittleEndian, NativeEndian, NetworkEndian, BE, LE};
+
+use super::*;
 
 macro_rules! impl_fmt_trait {
     ($name:ident, $native:ident, $trait:ident) => {
@@ -179,11 +172,23 @@ example of how it can be used for parsing UDP packets.
 [`FromBytes`]: crate::FromBytes
 [`AsBytes`]: crate::AsBytes
 [`Unaligned`]: crate::Unaligned"),
-            #[derive(FromZeroes, FromBytes, AsBytes, Unaligned, Copy, Clone, Eq, PartialEq, Hash)]
+            #[derive(Copy, Clone, Eq, PartialEq, Hash)]
+            #[cfg_attr(any(feature = "derive", test), derive(FromZeroes, FromBytes, AsBytes, Unaligned))]
             #[repr(transparent)]
             pub struct $name<O>([u8; $bytes], PhantomData<O>);
         }
 
+        safety_comment! {
+            /// SAFETY:
+            /// `$name<O>` is `repr(transparent)`, and so it has the same layout
+            /// as its only non-zero field, which is a `u8` array. `u8` arrays
+            /// are `FromZeroes`, `FromBytes`, `AsBytes`, and `Unaligned`.
+            impl_or_verify!(O => FromZeroes for $name<O>);
+            impl_or_verify!(O => FromBytes for $name<O>);
+            impl_or_verify!(O => AsBytes for $name<O>);
+            impl_or_verify!(O => Unaligned for $name<O>);
+        }
+
         impl<O> Default for $name<O> {
             fn default() -> $name<O> {
                 $name::ZERO
@@ -471,7 +476,7 @@ module!(native_endian, NativeEndian, "native-endian");
 
 #[cfg(test)]
 mod tests {
-    use byteorder::NativeEndian;
+    use ::byteorder::NativeEndian;
 
     use {
         super::*,

diff --git a/src/lib.rs b/src/lib.rs
@@ -44,6 +44,10 @@
 //! alignment guarantees and which may use a byte order different from that of
 //! the execution platform.
 //!
+//! `derive`: Provides derives for the core marker traits via the
+//! `zerocopy-derive` crate. These derives are re-exported from `zerocopy`, so
+//! it is not necessary to depend on `zerocopy-derive` directly.
+//!
 //! `simd`: When the `simd` feature is enabled, `FromZeroes`, `FromBytes`, and
 //! `AsBytes` impls are emitted for all stable SIMD types which exist on the
 //! target platform. Note that the layout of SIMD types is not yet stabilized,
@@ -133,13 +137,17 @@
 #![cfg_attr(not(test), no_std)]
 #![cfg_attr(feature = "simd-nightly", feature(stdsimd))]
 
+#[macro_use]
+mod macros;
+
 #[cfg(feature = "byteorder")]
 pub mod byteorder;
 #[doc(hidden)]
 pub mod derive_util;
 
 #[cfg(feature = "byteorder")]
 pub use crate::byteorder::*;
+#[cfg(any(feature = "derive", test))]
 pub use zerocopy_derive::*;
 
 use core::{
@@ -166,10 +174,10 @@ use {
     core::{alloc::Layout, ptr::NonNull},
 };
 
-// This is a hack to allow derives of `FromZeroes`, `FromBytes`, `AsBytes`, and
-// `Unaligned` to work in this crate. They assume that zerocopy is linked as an
-// extern crate, so they access items from it as `zerocopy::Xxx`. This makes
-// that still work.
+// This is a hack to allow zerocopy-derive derives to work in this crate. They
+// assume that zerocopy is linked as an extern crate, so they access items from
+// it as `zerocopy::Xxx`. This makes that still work.
+#[cfg(any(feature = "derive", test))]
 mod zerocopy {
     pub(crate) use crate::*;
 }
@@ -178,7 +186,7 @@ mod zerocopy {
 /// instance of the type.
 ///
 /// WARNING: Do not implement this trait yourself! Instead, use
-/// `#[derive(FromZeroes)]`.
+/// `#[derive(FromZeroes)]` (requires the `derive` Cargo feature).
 ///
 /// Any memory region of the appropriate length which is guaranteed to contain
 /// only zero bytes can be viewed as any `FromZeroes` type with no runtime
@@ -401,7 +409,7 @@ pub unsafe trait FromZeroes {
 /// Types for which any byte pattern is valid.
 ///
 /// WARNING: Do not implement this trait yourself! Instead, use
-/// `#[derive(FromBytes)]`.
+/// `#[derive(FromBytes)]` (requires the `derive` Cargo feature).
 ///
 /// `FromBytes` types can safely be deserialized from an untrusted sequence of
 /// bytes because any byte sequence corresponds to a valid instance of the type.
@@ -502,7 +510,7 @@ pub unsafe trait FromBytes: FromZeroes {
 /// Types which are safe to treat as an immutable byte slice.
 ///
 /// WARNING: Do not implement this trait yourself! Instead, use
-/// `#[derive(AsBytes)]`.
+/// `#[derive(AsBytes)]` (requires the `derive` Cargo feature).
 ///
 /// `AsBytes` types can be safely viewed as a slice of bytes. In particular,
 /// this means that, in any valid instance of the type, none of the bytes of the
@@ -677,7 +685,7 @@ pub unsafe trait AsBytes {
 /// Types with no alignment requirement.
 ///
 /// WARNING: Do not implement this trait yourself! Instead, use
-/// `#[derive(Unaligned)]`.
+/// `#[derive(Unaligned)]` (requires the `derive` Cargo feature).
 ///
 /// If `T: Unaligned`, then `align_of::<T>() == 1`.
 ///
@@ -696,81 +704,6 @@ pub unsafe trait Unaligned {
         Self: Sized;
 }
 
-/// Documents multiple unsafe blocks with a single safety comment.
-///
-/// Invoked as:
-///
-/// ```rust,ignore
-/// safety_comment! {
-///     // Non-doc comments come first.
-///     /// SAFETY:
-///     /// Safety comment starts on its own line.
-///     macro_1!(args);
-///     macro_2! { args };
-/// }
-/// ```
-///
-/// The macro invocations are emitted, each decorated with the following
-/// attribute: `#[allow(clippy::undocumented_unsafe_blocks)]`.
-macro_rules! safety_comment {
-    (#[doc = r" SAFETY:"] $(#[doc = $_doc:literal])* $($macro:ident!$args:tt;)*) => {
-        #[allow(clippy::undocumented_unsafe_blocks)]
-        const _: () = { $($macro!$args;)* };
-    }
-}
-
-/// Unsafely implements trait(s) for a type.
-macro_rules! unsafe_impl {
-    // Implement `$trait` for `$ty` with no bounds.
-    ($ty:ty: $trait:ty) => {
-        unsafe impl $trait for $ty { fn only_derive_is_allowed_to_implement_this_trait() {} }
-    };
-    // Implement all `$traits` for `$ty` with no bounds.
-    ($ty:ty: $($traits:ty),*) => {
-        $( unsafe_impl!($ty: $traits); )*
-    };
-    // For all `$tyvar` with no bounds, implement `$trait` for `$ty`.
-    ($tyvar:ident => $trait:ident for $ty:ty) => {
-        unsafe impl<$tyvar> $trait for $ty { fn only_derive_is_allowed_to_implement_this_trait() {} }
-    };
-    // For all `$tyvar: ?Sized` with no bounds, implement `$trait` for `$ty`.
-    ($tyvar:ident: ?Sized => $trait:ident for $ty:ty) => {
-        unsafe impl<$tyvar: ?Sized> $trait for $ty { fn only_derive_is_allowed_to_implement_this_trait() {} }
-    };
-    // For all `$tyvar: $bound`, implement `$trait` for `$ty`.
-    ($tyvar:ident: $bound:path => $trait:ident for $ty:ty) => {
-        unsafe impl<$tyvar: $bound> $trait for $ty { fn only_derive_is_allowed_to_implement_this_trait() {} }
-    };
-    // For all `$tyvar: $bound + ?Sized`, implement `$trait` for `$ty`.
-    ($tyvar:ident: ?Sized + $bound:path => $trait:ident for $ty:ty) => {
-        unsafe impl<$tyvar: ?Sized + $bound> $trait for $ty { fn only_derive_is_allowed_to_implement_this_trait() {} }
-    };
-    // For all `$tyvar: $bound` and for all `const $constvar: $constty`,
-    // implement `$trait` for `$ty`.
-    ($tyvar:ident: $bound:path, const $constvar:ident: $constty:ty => $trait:ident for $ty:ty) => {
-        unsafe impl<$tyvar: $bound, const $constvar: $constty> $trait for $ty {
-            fn only_derive_is_allowed_to_implement_this_trait() {}
-        }
-    };
-}
-
-/// Uses `align_of` to confirm that a type or set of types have alignment 1.
-///
-/// Note that `align_of<T>` requires `T: Sized`, so this macro doesn't work for
-/// unsized types.
-macro_rules! assert_unaligned {
-    ($ty:ty) => {
-        // We only compile this assertion under `cfg(test)` to avoid taking an
-        // extra non-dev dependency (and making this crate more expensive to
-        // compile for our dependents).
-        #[cfg(test)]
-        static_assertions::const_assert_eq!(core::mem::align_of::<$ty>(), 1);
-    };
-    ($($ty:ty),*) => {
-        $(assert_unaligned!($ty);)*
-    };
-}
-
 safety_comment! {
     /// SAFETY:
     /// Per the reference [1], "the unit tuple (`()`) ... is guaranteed as a
@@ -1213,10 +1146,23 @@ mod simd {
 // [2] https://github.com/google/zerocopy/pull/126#discussion_r1018512323
 // [3] https://github.com/google/zerocopy/issues/209
 #[allow(missing_debug_implementations)]
-#[derive(FromZeroes, FromBytes, Unaligned, Default, Copy)]
+#[derive(Default, Copy)]
+#[cfg_attr(any(feature = "derive", test), derive(Unaligned, FromZeroes, FromBytes, AsBytes))]
 #[repr(C, packed)]
 pub struct Unalign<T>(T);
 
+safety_comment! {
+    /// SAFETY:
+    /// - `Unalign<T>` is `repr(packed)`, so it is unaligned regardless of the
+    ///   alignment of `T`, and so we don't require that `T: Unaligned`
+    /// - `Unalign<T>` has the same bit validity as `T`, and so it is
+    ///   `FromZeroes`, `FromBytes`, or `AsBytes` exactly when `T` is as well.
+    impl_or_verify!(T => Unaligned for Unalign<T>);
+    impl_or_verify!(T: FromZeroes => FromZeroes for Unalign<T>);
+    impl_or_verify!(T: FromBytes => FromBytes for Unalign<T>);
+    impl_or_verify!(T: AsBytes => AsBytes for Unalign<T>);
+}
+
 // Note that `Unalign: Clone` only if `T: Copy`. Since the inner `T` may not be
 // aligned, there's no way to safely call `T::clone`, and so a `T: Clone` bound
 // is not sufficient to implement `Clone` for `Unalign`.
@@ -1444,16 +1390,6 @@ impl<T: Copy> Unalign<T> {
     }
 }
 
-safety_comment! {
-    /// SAFETY:
-    /// Since `T: AsBytes`, we know that it's safe to construct a `&[u8]` from
-    /// an aligned `&T`. Since `&[u8]` itself has no alignment requirements, it
-    /// must also be safe to construct a `&[u8]` from a `&T` at any address.
-    /// Since `Unalign<T>` is `#[repr(C, packed)]`, everything about its layout
-    /// except for its alignment is the same as `T`'s layout.
-    unsafe_impl!(T: AsBytes => AsBytes for Unalign<T>);
-}
-
 impl<T: Unaligned> Deref for Unalign<T> {
     type Target = T;