Support generic NonZero<T>

[carlsverre]

What is the name of your project?
An unreleased but in-development storage engine that will power SQLSync v2.

Please provide a link to your project (GitHub repository, crates.io page, etc).
https://github.com/orbitinghail/sqlsync
https://github.com/orbitinghail

What features would you like from zerocopy?
I'd like zerocopy to handle the new generic NonZero<T> type in the same way as it handles other NonZero types. I'm happy to take a crack at this if it's straightforward and doesn't have any soundness errors.

This feature is already listed here #1120 but I'd like to raise it as a more current issue in order to get it resolved (assuming it doesn't have any soundness issues).

My current workaround is definining a type like so:

// this is my base type
#[derive(...zerocopy derives...)]
#[repr(transparent)]
pub struct LSN(NonZero<u64>);

// this is my workaround type
#[derive(...zerocopy derives...)]
#[repr(transparent)]
pub struct MaybeLSN(Option<NonZero<u64>>);

impl From<MaybeLSN> for Option<LSN> {
    fn from(value: MaybeLSN) -> Self {
        value.0.map(LSN)
    }
}

impl From<Option<LSN>> for MaybeLSN {
    fn from(value: Option<LSN>) -> Self {
        Self(value.map(|lsn| lsn.0))
    }
}

The downside of the above is an additional wrapping/unwrapping layer and a lot of generated code. It probably gets compiled out of the actual binary, but it adds unneeded complexity.

Once this issue is resolved, I should be able to replace usages of MaybeLSN with Option<LSN>.

I'm happy to fix this myself if that would be acceptable and someone is available to help check the soundness of the implementation.

Thanks for an AMAZING project btw. I love zerocopy.

[jswrenn]

Sure, I'd be happy to provide mentorship. If you'd like to take this on, the first step is to adapt this documentation on NonZero* to NonZero; e.g.:

/// # Layout
///
/// `NonZero<T>` is guaranteed to have the same layout and bit validity as `T`
/// with the exception that the all-zero bit pattern is not a valid instance.
/// 
/// `Option<NonZero<T>>` is is guaranteed to be compatible with `T`, including
/// in FFI.
///
/// Thanks to the [null pointer optimization], `NonZero<T>` and
/// `Option<NonZero<T>>` are guaranteed to have the same size and alignment.
/// 
/// [null pointer optimization]: crate::option#representation

Then, once that lands, submit a PR simplifying and genericizing these impls:

zerocopy/src/impls.rs

Lines 245 to 365 in f495088

	safety_comment! {
	// `NonZeroXxx` is `IntoBytes`, but not `FromZeros` or `FromBytes`.
	//
	/// SAFETY:
	/// - `IntoBytes`: `NonZeroXxx` has the same layout as its associated
	/// primitive. Since it is the same size, this guarantees it has no
	/// padding - integers have no padding, and there's no room for padding
	/// if it can represent all of the same values except 0.
	/// - `Unaligned`: `NonZeroU8` and `NonZeroI8` document that
	/// `Option<NonZeroU8>` and `Option<NonZeroI8>` both have size 1. [1] [2]
	/// This is worded in a way that makes it unclear whether it's meant as a
	/// guarantee, but given the purpose of those types, it's virtually
	/// unthinkable that that would ever change. `Option` cannot be smaller
	/// than its contained type, which implies that, and `NonZeroX8` are of
	/// size 1 or 0. `NonZeroX8` can represent multiple states, so they cannot
	/// be 0 bytes, which means that they must be 1 byte. The only valid
	/// alignment for a 1-byte type is 1.
	///
	/// TODO(#429):
	/// - Add quotes from documentation.
	/// - Add safety comment for `Immutable`. How can we prove that `NonZeroXxx`
	/// doesn't contain any `UnsafeCell`s? It's obviously true, but it's not
	/// clear how we'd prove it short of adding text to the stdlib docs that
	/// says so explicitly, which likely wouldn't be accepted.
	///
	/// [1] https://doc.rust-lang.org/1.81.0/std/num/type.NonZeroU8.html
	///
	/// `NonZeroU8` is guaranteed to have the same layout and bit validity as `u8` with
	/// the exception that 0 is not a valid instance
	///
	/// [2] https://doc.rust-lang.org/1.81.0/std/num/type.NonZeroI8.html
	/// TODO(https://github.com/rust-lang/rust/pull/104082): Cite documentation
	/// that layout is the same as primitive layout.
	unsafe_impl!(NonZeroU8: Immutable, IntoBytes, Unaligned);
	unsafe_impl!(NonZeroI8: Immutable, IntoBytes, Unaligned);
	assert_unaligned!(NonZeroU8, NonZeroI8);
	unsafe_impl!(NonZeroU16: Immutable, IntoBytes);
	unsafe_impl!(NonZeroI16: Immutable, IntoBytes);
	unsafe_impl!(NonZeroU32: Immutable, IntoBytes);
	unsafe_impl!(NonZeroI32: Immutable, IntoBytes);
	unsafe_impl!(NonZeroU64: Immutable, IntoBytes);
	unsafe_impl!(NonZeroI64: Immutable, IntoBytes);
	unsafe_impl!(NonZeroU128: Immutable, IntoBytes);
	unsafe_impl!(NonZeroI128: Immutable, IntoBytes);
	unsafe_impl!(NonZeroUsize: Immutable, IntoBytes);
	unsafe_impl!(NonZeroIsize: Immutable, IntoBytes);
	/// SAFETY:
	/// - The safety requirements for `unsafe_impl!` with an `is_bit_valid`
	/// closure:
	/// - Given `t: *mut NonZeroXxx` and `let r = *mut xxx`, `r` refers to an
	/// object of the same size as that referred to by `t`. This is true
	/// because `NonZeroXxx` and `xxx` have the same size. [1] Neither `r`
	/// nor `t` refer to any `UnsafeCell`s because neither `NonZeroXxx` [2]
	/// nor `xxx` do.
	/// - Since the closure takes a `&xxx` argument, given a `Maybe<'a,
	/// NonZeroXxx>` which satisfies the preconditions of
	/// `TryFromBytes::<NonZeroXxx>::is_bit_valid`, it must be guaranteed
	/// that the memory referenced by that `MabyeValid` always contains a
	/// valid `xxx`. Since `NonZeroXxx`'s bytes are always initialized [1],
	/// `is_bit_valid`'s precondition requires that the same is true of its
	/// argument. Since `xxx`'s only bit validity invariant is that its
	/// bytes must be initialized, this memory is guaranteed to contain a
	/// valid `xxx`.
	/// - The impl must only return `true` for its argument if the original
	/// `Maybe<NonZeroXxx>` refers to a valid `NonZeroXxx`. The only
	/// `xxx` which is not also a valid `NonZeroXxx` is 0. [1]
	///
	/// [1] Per https://doc.rust-lang.org/1.81.0/core/num/type.NonZeroU16.html:
	///
	/// `NonZeroU16` is guaranteed to have the same layout and bit validity as
	/// `u16` with the exception that `0` is not a valid instance.
	///
	/// [2] `NonZeroXxx` self-evidently does not contain `UnsafeCell`s. This is
	/// not a proof, but we are accepting this as a known risk per #1358.
	unsafe_impl!(NonZeroU8: TryFromBytes; |n: MaybeAligned<u8>| NonZeroU8::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroI8: TryFromBytes; |n: MaybeAligned<i8>| NonZeroI8::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroU16: TryFromBytes; |n: MaybeAligned<u16>| NonZeroU16::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroI16: TryFromBytes; |n: MaybeAligned<i16>| NonZeroI16::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroU32: TryFromBytes; |n: MaybeAligned<u32>| NonZeroU32::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroI32: TryFromBytes; |n: MaybeAligned<i32>| NonZeroI32::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroU64: TryFromBytes; |n: MaybeAligned<u64>| NonZeroU64::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroI64: TryFromBytes; |n: MaybeAligned<i64>| NonZeroI64::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroU128: TryFromBytes; |n: MaybeAligned<u128>| NonZeroU128::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroI128: TryFromBytes; |n: MaybeAligned<i128>| NonZeroI128::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroUsize: TryFromBytes; |n: MaybeAligned<usize>| NonZeroUsize::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	unsafe_impl!(NonZeroIsize: TryFromBytes; |n: MaybeAligned<isize>| NonZeroIsize::new(n.read_unaligned::<BecauseImmutable>()).is_some());
	}
	safety_comment! {
	/// SAFETY:
	/// - `TryFromBytes` (with no validator), `FromZeros`, `FromBytes`,
	/// `IntoBytes`: The Rust compiler reuses `0` value to represent `None`,
	/// so `size_of::<Option<NonZeroXxx>>() == size_of::<xxx>()`; see
	/// `NonZeroXxx` documentation.
	/// - `Unaligned`: `NonZeroU8` and `NonZeroI8` document that
	/// `Option<NonZeroU8>` and `Option<NonZeroI8>` both have size 1. [1] [2]
	/// This is worded in a way that makes it unclear whether it's meant as a
	/// guarantee, but given the purpose of those types, it's virtually
	/// unthinkable that that would ever change. The only valid alignment for
	/// a 1-byte type is 1.
	///
	/// TODO(#429): Add quotes from documentation.
	///
	/// [1] https://doc.rust-lang.org/stable/std/num/struct.NonZeroU8.html
	/// [2] https://doc.rust-lang.org/stable/std/num/struct.NonZeroI8.html
	///
	/// TODO(https://github.com/rust-lang/rust/pull/104082): Cite documentation
	/// for layout guarantees.
	unsafe_impl!(Option<NonZeroU8>: TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned);
	unsafe_impl!(Option<NonZeroI8>: TryFromBytes, FromZeros, FromBytes, IntoBytes, Unaligned);
	assert_unaligned!(Option<NonZeroU8>, Option<NonZeroI8>);
	unsafe_impl!(Option<NonZeroU16>: TryFromBytes, FromZeros, FromBytes, IntoBytes);
	unsafe_impl!(Option<NonZeroI16>: TryFromBytes, FromZeros, FromBytes, IntoBytes);
	unsafe_impl!(Option<NonZeroU32>: TryFromBytes, FromZeros, FromBytes, IntoBytes);
	unsafe_impl!(Option<NonZeroI32>: TryFromBytes, FromZeros, FromBytes, IntoBytes);
	unsafe_impl!(Option<NonZeroU64>: TryFromBytes, FromZeros, FromBytes, IntoBytes);
	unsafe_impl!(Option<NonZeroI64>: TryFromBytes, FromZeros, FromBytes, IntoBytes);
	unsafe_impl!(Option<NonZeroU128>: TryFromBytes, FromZeros, FromBytes, IntoBytes);
	unsafe_impl!(Option<NonZeroI128>: TryFromBytes, FromZeros, FromBytes, IntoBytes);
	unsafe_impl!(Option<NonZeroUsize>: TryFromBytes, FromZeros, FromBytes, IntoBytes);
	unsafe_impl!(Option<NonZeroIsize>: TryFromBytes, FromZeros, FromBytes, IntoBytes);
	}

[carlsverre]

Thanks @jswrenn for the guidance! I've submitted the doc change and will work on the zerocopy change once that lands.

Seeing as NonZero<T> was added in Rust 1.79 but zerocopy has a MSRV of 1.65, it seems that I should retain all of the existing NonZero* impls when the generic_nonzero feature is not detected, switching over to a single generic impl when it is. I'll put together a PR to start figuring out how that will look soon.

[jswrenn]

Sounds good! Base your zerocopy PR off the v0.8.x branch, and follow the model used for zerocopy-diagnostic-on-unimplemented-1-78-0/zerocopy_diagnostic_on_unimplemented_1_78_0 for version selection.

[carlsverre]

Ok, I started the implementation and quickly realized that I had completely misunderstood the root issue. The problem is not that zerocopy is missing a generic NonZero<T> impl, the problem is that Option<T> only derives Immutable.

To repeat my original use case for clarity, my goal is that zerocopy is able to realize that Option<LSN> derives Immutable, IntoBytes, KnownLayout, TryFromBytes.

#[derive(TryFromBytes, KnownLayout, IntoBytes, Immutable)]
#[repr(transparent)]
pub struct LSN(NonZeroU64);

// this currently fails
#[derive(TryFromBytes, KnownLayout, IntoBytes, Immutable)]
#[repr(C)]
pub struct Foo {
    lsn: Option<LSN>
}

I should have realized this sooner, sorry about that. I still think that adding Layout docs to NonZero<T> is a good change though so I'll drive that to completion.

Unfortunately, I'm not aware of a way to detect a repr(transparent) type in order to safely derive a more generic Option<T>. It sounds like to do this well would require compiler assistance. If you know of any way to do this I'd love to learn more.

In addition, NonZero<T> currently requires that T derives ZeroablePrimitive which is a Sealed trait that only is implemented for the same set of numeric types that NonZero* is defined for.

In conclusion, adding NonZero<T> support to zerocopy will increase the size of the codebase and widen the test scope without adding any additional functionality beyond what is already supported. If the restriction on ZeroablePrimitive is ever lifted from NonZero<T> (or widened beyond the NonZero* types) then it may be worth revisiting this issue. However for now, we should close this.

[joshlf]

I looked into this a bit more, and IMO I don't think it's possible for us to support this. In particular, the current definition of NonZero looks like this:

pub struct NonZero<T>(/* private fields */)
where
    T: ZeroablePrimitive

In order to add a generic implementation for NonZero<T>, we'd need to write impl<T: ZeroablePrimitive> ... for NonZero<T>. Unfortunately, ZeroablePrimitive is currently an unstable trait. Until that becomes stable, we're out of luck.

---

I've closed this, but feel free to keep commenting here.