Lokathor / bytemuck

A crate for mucking around with piles of bytes
https://docs.rs/bytemuck
Apache License 2.0
697 stars 77 forks source link

Zero-sized type (ZST) handling inconsistency between borrowed and owned slice casts. #253

Closed zachs18 closed 1 month ago

zachs18 commented 1 month ago

(For the purposes of this issue, assume Src/Dst/Zst/NonZst have the same alignment. Borrowed and owned casts intentionally have different alignment requirements, which are not the issue here.)

bytemuck::try_cast_slice<Src, Dst>[^2] is documented as returning an error (specifically Err(SizeMismatch)) when Src and Dst are a ZST and a non-ZST (in either order).

However, bytemuck::allocation::try_cast_slice_box<Src, Dst>[^1] says nothing in its documentation regarding ZSTs, currently always returns Ok(empty) when Src is a ZST and Dst is not, and currently panics with a modulo-by-zero error when Src is a non-ZST and Dst is a ZST. (playground link)

I see three possible overall resolutions to the inconsistency:

  1. Keep (and document?) the inconsistency (but fix the panic).
    • try_cast_slice_box allows [Zst] -> [NonZst], returning an empty slice.
    • try_cast_slice disallows [Zst] -> [NonZst] and [NonZst] -> [Zst], returning Err(SizeMismatch)
    • Status quo
  2. Make cast_slice behave like cast_slice_box (but fix the panic).
    • cast_slice and cast_slice_box allow [Zst] -> [NonZst] casts, returning an empty slice.
    • May break code relying on cast_slice::<Zst, NonZst> not succeeding.
  3. Make cast_slice_box behave like cast_slice.
    • Disallow any casting between non-ZST <-> ZST slices, returning Err(SizeMismatch).
    • May break code relying on cast_vec::<Zst, NonZst> succeeding.
    • (Implies option 2 below)

and two possible resolutions for the [NonZst] -> [Zst] panic:

  1. Allow empty source slices for [NonZst] -> [Zst], return an error[^3] on non-empty source slices.
    • This matches the existing documentation that the start and end content size must be the same.
  2. Disallow casting from [NonZst] to [Zst], always return an error[^3].

[^1]: Everything mentioned about cast_slice_box also applies to cast_vec, cast_slice_arc, and cast_slice_rc. [^2]: Everything mentioned about cast_slice also applies to cast_slice_mut) [^3]: On non-empty source slices, could return Err(SizeMismatch), or could return Err(OutputSliceWouldHaveSlop) ("If the output slice wouldn’t be a whole number of elements then the conversion fails.") since there is no whole number of ZSTs that would have any non-zero size, and on empty source slices since any whole number of ZSTs would have zero size.

zachs18 commented 1 month ago

Additionally, the error returned for an invalid [NonZst] -> [NonZst] cast is not consistent between try_cast_slice and try_cast_slice_box: (playground)

let x: &[i32] = &[1, 2, 3];
let y: Result<&[[i32; 2]], PodCastError> = bytemuck::try_cast_slice(x);
assert_eq!(y.unwrap_err(), PodCastError::OutputSliceWouldHaveSlop);

let x: Box<[i32]> = Box::new([1, 2, 3]);
let y: Result<Box<[[i32; 2]]>, (PodCastError, Box<[i32]>)> = bytemuck::try_cast_slice_box(x);
assert_eq!(y.unwrap_err().0, PodCastError::SizeMismatch);

(The documentation for SizeMismatch says that for slices, it is only returned when casting Zst <-> NonZst, but neither i32 nor [i32; 2] is a ZST here)

Lokathor commented 1 month ago

Long ago when starting the crate, I had the idea that a cast_slice should always be "reversible", so that you can cast S to D, then D back to S, and get what you started with.

That's neat and all, but consistency among all of this might be better, so just having ZST slices turn into 0-length non-ZST slices does seem like a compelling path.

May break code relying on cast_slice::<Zst, NonZst> not succeeding.

I wanted to respond to this separately from the rest of the option selection: This particular break seems too small to worry about. I'd say that, in general, our policy as a crate should be that any casts which do work will continue to work in future versions, but anything which doesn't cast currently might be able to cast in some future version. Basically, it's non-breaking to enable more casts to be possible, when appropriate.