Support testing TryFromBytes for !NoCell types (#924)

Previously, our test harness for `TryFromBytes` types assumed that those
types also implemented `NoCell`. This commit introduces machinery that
allows us to conditionally execute certain steps of our tests depending
on whether or not the type under test implements `NoCell`.

This commit also lifts the `T: NoCell` bound in `TryFromBytes for
MaybeUninit<T>` in order to test this machinery.

Makes progress on #5

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

src/lib.rs

@@ -3577,7 +3577,7 @@ safety_comment! {
  /// [1] TODO(https://github.com/rust-lang/rust/pull/121215)
  /// [2] https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#layout-1
  unsafe_impl!(T: NoCell => NoCell for MaybeUninit<T>);
- unsafe_impl!(T: NoCell => TryFromBytes for MaybeUninit<T>);
+ unsafe_impl!(T => TryFromBytes for MaybeUninit<T>);
  unsafe_impl!(T => FromZeros for MaybeUninit<T>);
  unsafe_impl!(T => FromBytes for MaybeUninit<T>);
  unsafe_impl!(T: Unaligned => Unaligned for MaybeUninit<T>);
@@ -8167,48 +8167,245 @@ mod tests {
  @failure [0x01; mem::size_of::<*mut NotZerocopy>()];
  );
 
+ // Use the trick described in [1] to allow us to call methods
+ // conditional on certain trait bounds.
+ //
+ // In all of these cases, methods return `Option<R>`, where `R` is the
+ // return type of the method we're conditionally calling. The "real"
+ // implementations (the ones defined in traits using `&self`) return
+ // `Some`, and the default implementations (the ones defined as inherent
+ // methods using `&mut self`) return `None`.
+ //
+ // [1] https://github.com/dtolnay/case-studies/blob/master/autoref-specialization/README.md
+ mod autoref_trick {
+ use super::*;
+
+ pub(super) struct AutorefWrapper<T: ?Sized>(pub(super) PhantomData<T>);
+
+ pub(super) trait TestTryFromRef<T: ?Sized> {
+ fn test_try_from_ref<'bytes>(
+ &self,
+ bytes: &'bytes [u8],
+ ) -> Option<Option<&'bytes T>>;
+
+ fn test_try_from_mut<'bytes>(
+ &self,
+ bytes: &'bytes mut [u8],
+ ) -> Option<Option<&'bytes mut T>>;
+ }
+
+ impl<T: TryFromBytes + NoCell + KnownLayout + ?Sized> TestTryFromRef<T> for AutorefWrapper<T> {
+ fn test_try_from_ref<'bytes>(
+ &self,
+ bytes: &'bytes [u8],
+ ) -> Option<Option<&'bytes T>> {
+ Some(T::try_from_ref(bytes))
+ }
+
+ fn test_try_from_mut<'bytes>(
+ &self,
+ bytes: &'bytes mut [u8],
+ ) -> Option<Option<&'bytes mut T>> {
+ Some(T::try_from_mut(bytes))
+ }
+ }
+
+ pub(super) trait TestTryReadFrom<T> {
+ fn test_try_read_from(&self, bytes: &[u8]) -> Option<Option<T>>;
+ }
+
+ // TODO(#5): Remove the `NoCell` bound once `try_read_from` no
+ // longer requires that bound.
+ impl<T: TryFromBytes + NoCell> TestTryReadFrom<T> for AutorefWrapper<T> {
+ fn test_try_read_from(&self, bytes: &[u8]) -> Option<Option<T>> {
+ Some(T::try_read_from(bytes))
+ }
+ }
+
+ pub(super) trait TestAsBytes<T: ?Sized> {
+ fn test_as_bytes<'slf, 't>(&'slf self, t: &'t T) -> Option<&'t [u8]>;
+ }
+
+ impl<T: IntoBytes + NoCell + ?Sized> TestAsBytes<T> for AutorefWrapper<T> {
+ fn test_as_bytes<'slf, 't>(&'slf self, t: &'t T) -> Option<&'t [u8]> {
+ Some(t.as_bytes())
+ }
+ }
+ }
+
+ use autoref_trick::*;
+
+ // Asserts that `$ty` is one of a list of types which are allowed to not
+ // provide a "real" implementation for `$fn_name`. Since the
+ // `autoref_trick` machinery fails silently, this allows us to ensure
+ // that the "default" impls are only being used for types which we
+ // expect.
+ //
+ // Note that, since this is a runtime test, it is possible to have an
+ // allowlist which is too restrictive if the function in question is
+ // never called for a particular type. For example, if `as_bytes` is not
+ // supported for a particular type, and so `test_as_bytes` returns
+ // `None`, methods such as `test_try_from_ref` may never be called for
+ // that type. As a result, it's possible that, for example, adding
+ // `as_bytes` support for a type would cause other allowlist assertions
+ // to fail. This means that allowlist assertion failures should not
+ // automatically be taken as a sign of a bug.
+ macro_rules! assert_on_allowlist {
+ ($fn_name:ident($ty:ty) $(: $($tys:ty),*)?) => {{
+ use core::any::TypeId;
+
+ let allowlist: &[TypeId] = &[ $($(TypeId::of::<$tys>()),*)? ];
+ let allowlist_names: &[&str] = &[ $($(stringify!($tys)),*)? ];
+
+ let id = TypeId::of::<$ty>();
+ assert!(allowlist.contains(&id), "{} is not on allowlist for {}: {:?}", stringify!($ty), stringify!($fn_name), allowlist_names);
+ }};
+ }
+
  // Asserts that `$ty` implements any `$trait` and doesn't implement any
  // `!$trait`. Note that all `$trait`s must come before any `!$trait`s.
  //
  // For `T: TryFromBytes`, uses `TryFromBytesTestable` to test success
- // and failure cases for `TryFromBytes::is_bit_valid`.
+ // and failure cases.
  macro_rules! assert_impls {
  ($ty:ty: TryFromBytes) => {
+ // "Default" implementations that match the "real"
+ // implementations defined in the `autoref_trick` module above.
+ #[allow(unused)]
+ impl AutorefWrapper<$ty> {
+ fn test_try_from_ref<'bytes>(&mut self, _bytes: &'bytes [u8]) -> Option<Option<&'bytes $ty>> {
+ assert_on_allowlist!(test_try_from_ref($ty));
+
+ None
+ }
+
+ fn test_try_from_mut<'bytes>(&mut self, _bytes: &'bytes mut [u8]) -> Option<Option<&'bytes mut $ty>> {
+ assert_on_allowlist!(test_try_from_mut($ty));
+
+ None
+ }
+
+ fn test_try_read_from(&mut self, _bytes: &[u8]) -> Option<Option<&$ty>> {
+ assert_on_allowlist!(
+ test_try_read_from($ty):
+ str,
+ ManuallyDrop<[u8]>,
+ ManuallyDrop<[bool]>,
+ [u8],
+ [bool]
+ );
+
+ None
+ }
+
+ fn test_as_bytes(&mut self, _t: &$ty) -> Option<&[u8]> {
+ assert_on_allowlist!(
+ test_as_bytes($ty):
+ Option<&'static UnsafeCell<NotZerocopy>>,
+ Option<&'static mut UnsafeCell<NotZerocopy>>,
+ Option<NonNull<UnsafeCell<NotZerocopy>>>,
+ Option<Box<UnsafeCell<NotZerocopy>>>,
+ Option<fn()>,
+ Option<FnManyArgs>,
+ Option<extern "C" fn()>,
+ Option<ECFnManyArgs>,
+ MaybeUninit<u8>,
+ MaybeUninit<NotZerocopy>,
+ MaybeUninit<UnsafeCell<()>>,
+ *const NotZerocopy,
+ *mut NotZerocopy
+ );
+
+ None
+ }
+ }
+
  <$ty as TryFromBytesTestable>::with_passing_test_cases(|val| {
  let c = Ptr::from_ref(val);
  let c = c.forget_aligned();
 
- // SAFETY:
- // TODO(#899): This is unsound. `$ty` is not necessarily
- // `IntoBytes`, but that's the corner we've backed ourselves
- // into by using `Ptr::from_ref`.
+ // Test `is_bit_valid` directly. NOTE: It's very important
+ // that we test this directly in addition to testing other
+ // methods below. Those methods rely on the `autoref_trick`
+ // module, which is very brittle, and fails silently
+ // (namely, bugs manifest as code simply not running and
+ // thus tests succeeding). While we have
+ // `assert_on_allowlist!` to help prevent regressions, we
+ // shouldn't rely on it. `is_bit_valid` is the most
+ // important thing to test, and so it's crucial that we test
+ // this directly in a way that will reliably be tested even
+ // if we have an undiscovered bug in our `autoref_trick`
+ // machinery.
+ //
+ // SAFETY: TODO(#899): This is unsound. `$ty` is not
+ // necessarily `IntoBytes`, but that's the corner we've
+ // backed ourselves into by using `Ptr::from_ref`.
  let c = unsafe { c.assume_initialized() };
  let res = <$ty as TryFromBytes>::is_bit_valid(c);
  assert!(res, "{}::is_bit_valid({:?}): got false, expected true", stringify!($ty), val);
 
- // TODO(#5): In addition to testing `is_bit_valid`, test the
- // methods built on top of it. This would both allow us to
- // test their implementations and actually convert the bytes
- // to `$ty`, giving Miri a chance to catch if this is
- // unsound (ie, if our `is_bit_valid` impl is buggy).
- //
- // The following code was tried, but it doesn't work because
- // a) some types are not `IntoBytes` and, b) some types are
- // not `Sized`.
+ // TODO(#494): These tests only get exercised for types
+ // which are `IntoBytes`. Once we implement #494, we should
+ // be able to support non-`IntoBytes` types by zeroing
+ // padding.
+
+ // We define `w` and `ww` since, in the case of the inherent
+ // methods, Rust thinks they're both borrowed mutably at the
+ // same time (given how we use them below). If we just
+ // defined a single `w` and used it for multiple operations,
+ // this would conflict.
  //
- // let r = <$ty as TryFromBytes>::try_from_ref(val.as_bytes()).unwrap();
- // assert_eq!(r, &val);
- // let r = <$ty as TryFromBytes>::try_from_mut(val.as_mut_bytes()).unwrap();
- // assert_eq!(r, &mut val);
- // let v = <$ty as TryFromBytes>::try_read_from(val.as_bytes()).unwrap();
- // assert_eq!(v, val);
+ // We `#[allow(unused_mut]` for the cases where the "real"
+ // impls are used, which take `&self`.
+ #[allow(unused_mut)]
+ let (mut w, mut ww) = (AutorefWrapper::<$ty>(PhantomData), AutorefWrapper::<$ty>(PhantomData));
+
+ // `bytes` is `Some(val.as_bytes())` if `$ty: IntoBytes +
+ // NoCell` and `None` otherwise.
+ let bytes = w.test_as_bytes(val);
+
+ // The inner closure returns
+ // `Some($ty::try_from_ref(bytes))` if `$ty: NoCell` and
+ // `None` otherwise.
+ let res = bytes.and_then(|bytes| ww.test_try_from_ref(bytes));
+ if let Some(res) = res {
+ assert!(res.is_some(), "{}::try_from_ref({:?}): got `None`, expected `Some`", stringify!($ty), val);
+ }
+
+ if let Some(bytes) = bytes {
+ let mut bytes = bytes.to_vec();
+ let res = ww.test_try_from_mut(bytes.as_mut_slice());
+ if let Some(res) = res {
+ assert!(res.is_some(), "{}::try_from_mut({:?}): got `None`, expected `Some`", stringify!($ty), val);
+ }
+ }
+
+ let res = bytes.and_then(|bytes| ww.test_try_read_from(bytes));
+ if let Some(res) = res {
+ assert!(res.is_some(), "{}::try_read_from({:?}): got `None`, expected `Some`", stringify!($ty), val);
+ }
  });
  #[allow(clippy::as_conversions)]
  <$ty as TryFromBytesTestable>::with_failing_test_cases(|c| {
- let res = <$ty as TryFromBytes>::try_from_ref(c);
- assert!(res.is_none(), "{}::try_from_ref({:?}): got Some, expected None", stringify!($ty), c);
- let res = <$ty as TryFromBytes>::try_from_mut(c);
- assert!(res.is_none(), "{}::try_from_mut({:?}): got Some, expected None", stringify!($ty), c);
+ #[allow(unused_mut)] // For cases where the "real" impls are used, which take `&self`.
+ let mut w = AutorefWrapper::<$ty>(PhantomData);
+
+ // This is `Some($ty::try_from_ref(c))` if `$ty: NoCell` and
+ // `None` otherwise.
+ let res = w.test_try_from_ref(c);
+ if let Some(res) = res {
+ assert!(res.is_none(), "{}::try_from_ref({:?}): got Some, expected None", stringify!($ty), c);
+ }
+
+ let res = w.test_try_from_mut(c);
+ if let Some(res) = res {
+ assert!(res.is_none(), "{}::try_from_mut({:?}): got Some, expected None", stringify!($ty), c);
+ }
+
+ let res = w.test_try_read_from(c);
+ if let Some(res) = res {
+ assert!(res.is_none(), "{}::try_read_from({:?}): got Some, expected None", stringify!($ty), c);
+ }
  });
 
  #[allow(dead_code)]
@@ -8562,8 +8759,8 @@ mod tests {
  assert_impls!(ManuallyDrop<[UnsafeCell<()>]>: KnownLayout, FromZeros, FromBytes, IntoBytes, Unaligned, !NoCell, !TryFromBytes);
 
  assert_impls!(MaybeUninit<u8>: KnownLayout, NoCell, TryFromBytes, FromZeros, FromBytes, Unaligned, !IntoBytes);
- assert_impls!(MaybeUninit<NotZerocopy>: KnownLayout, FromZeros, FromBytes, !NoCell, !TryFromBytes, !IntoBytes, !Unaligned);
- assert_impls!(MaybeUninit<UnsafeCell<()>>: KnownLayout, FromZeros, FromBytes, Unaligned, !TryFromBytes, !NoCell, !IntoBytes);
+ assert_impls!(MaybeUninit<NotZerocopy>: KnownLayout, TryFromBytes, FromZeros, FromBytes, !NoCell, !IntoBytes, !Unaligned);
+ assert_impls!(MaybeUninit<UnsafeCell<()>>: KnownLayout, TryFromBytes, FromZeros, FromBytes, Unaligned, !NoCell, !IntoBytes);
 
  assert_impls!(Wrapping<u8>: KnownLayout, NoCell, TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned);
  // This test is important because it allows us to test our hand-rolled
@@ -8572,10 +8769,10 @@ mod tests {
  assert_impls!(Wrapping<NotZerocopy>: KnownLayout, !NoCell, !TryFromBytes, !FromZeros, !FromBytes, !IntoBytes, !Unaligned);
  assert_impls!(Wrapping<UnsafeCell<()>>: KnownLayout, FromZeros, FromBytes, IntoBytes, Unaligned, !NoCell, !TryFromBytes);
 
- assert_impls!(Unalign<u8>: KnownLayout, NoCell, TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned, TryFromBytes);
+ assert_impls!(Unalign<u8>: KnownLayout, NoCell, TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned);
  // This test is important because it allows us to test our hand-rolled
  // implementation of `<Unalign<T> as TryFromBytes>::is_bit_valid`.
- assert_impls!(Unalign<bool>: KnownLayout, NoCell, TryFromBytes, FromZeros, IntoBytes, Unaligned, TryFromBytes, !FromBytes);
+ assert_impls!(Unalign<bool>: KnownLayout, NoCell, TryFromBytes, FromZeros, IntoBytes, Unaligned, !FromBytes);
  assert_impls!(Unalign<NotZerocopy>: Unaligned, !NoCell, !KnownLayout, !TryFromBytes, !FromZeros, !FromBytes, !IntoBytes);
 
  assert_impls!(