Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Statically prevent ZST slices rather than panicking at runtime? #325

Closed
joshlf opened this issue Sep 4, 2023 · 4 comments
Closed

Statically prevent ZST slices rather than panicking at runtime? #325

joshlf opened this issue Sep 4, 2023 · 4 comments
Labels
compatibility-breaking Changes that are (likely to be) breaking

Comments

@joshlf
Copy link
Member

joshlf commented Sep 4, 2023

zerocopy/src/lib.rs

Lines 1731 to 1735 in 07854bc

pub fn new_slice(bytes: B) -> Option<Ref<B, [T]>> {
let remainder = bytes
.len()
.checked_rem(mem::size_of::<T>())
.expect("Ref::new_slice called on a zero-sized type");

Alternatively, maybe we could just give these types well-defined semantics (e.g., always synthesize a pointer with 0 slice elements or isize::MAX slice elements). Need to be very careful that none of our operations can be round-tripped losslessly; this might break that.

There are also reasons to be skeptical of wanting to prevent this at all:

In theory I like the idea of being able to have code like this fail at compile time. However, what do you do about a user who wants to write code like this?

pub fn do_thing<T: ?Sized + KnownLayout>() {
    if T::LAYOUT.trailing_slice_elem_type == Some(0) {
        // Do workaround thing
    } else {
        // Do other thing
    }
}

As in, you can't say that it's a compile-time error to have code that could trigger this panic because the user might have their own logic that ensures it'll never be encountered at runtime.

As of this writing, per #1149, we intend to use post-monomorphization errors for ZST slice conversions. We can always make our restrictions looser in the future. If we do decide to loosen restrictions, note the following caveats:

  • We cannot support "exact" conversions since those promise to produce a reference to the entire input byte sequence
  • We can support prefix and suffix conversions, but we will need to relax their semantics. Currently, prefix and suffix conversions promise to produce the "largest possible" value that will fit in the given byte sequence. While we could in principle argue that a slice with the maximum number of elements satisfies this promise, it clearly violates the spirit. It would likely make more sense to explicitly carve out ZST slices in the documentation.

See also #202, #284, #349 (comment), #1125, and #1149

@jswrenn
Copy link
Collaborator

jswrenn commented Apr 30, 2024

I'd quite like to see this code work:

fn main() {
    use zerocopy::*;
    use zerocopy_derive::*;
    
    #[derive(FromBytes, Immutable, KnownLayout)]
    #[repr(C)]
    struct Zesty {
        leading_sized: u8,
        trailing_dst: [()],
    }

    let bytes = [0u8; 100];

    for count in 0..256 {
        let (zesty, _) = Zesty::from_prefix_with_trailing_elements(bytes.as_slice(), count).unwrap();
        assert_eq!(zesty.trailing_dst.len(), count);
    }
}

...but I don't think that will be achievable until we have Ref wrap a Ptr. Right now, the count provided to from_prefix_with_trailing_elements is only used to length-validate and appropriately slit the byteslice. The count is then discarded. Later, Ref::into_ref attempts to reverse-engineer what the count must have been, based on the length of the byte slice. In the case of DSTs with trailing ZSTs, it cannot possible know the user-intended count.

jswrenn added a commit that referenced this issue May 1, 2024
Presently, we deny ZSTy DSTs in our APIs via panicking at runtime. However, the
ZSTiness of a DST is statically detectable and can be denied instead at compile
time. This PR replaces our ZSTy DST panics with compile-time assertions. Doing
gives us the freedom later provide meaningful runtime semantics in such cases.

Partially addresses #325
Closes #1149
jswrenn added a commit that referenced this issue May 1, 2024
Presently, we deny ZSTy DSTs in our APIs via panicking at runtime. However, the
ZSTiness of a DST is statically detectable and can be denied instead at compile
time. This PR replaces our ZSTy DST panics with compile-time assertions. Doing
gives us the freedom later provide meaningful runtime semantics in such cases.

Partially addresses #325
Closes #1149
jswrenn added a commit that referenced this issue May 1, 2024
Presently, we deny ZSTy DSTs in our APIs via panicking at runtime. However, the
ZSTiness of a DST is statically detectable and can be denied instead at compile
time. This PR replaces our ZSTy DST panics with compile-time assertions. Doing
gives us the freedom later provide meaningful runtime semantics in such cases.

Partially addresses #325
Closes #1149
joshlf pushed a commit that referenced this issue May 1, 2024
Presently, we deny ZSTy DSTs in our APIs via panicking at runtime. However, the
ZSTiness of a DST is statically detectable and can be denied instead at compile
time. This PR replaces our ZSTy DST panics with compile-time assertions. Doing
gives us the freedom later provide meaningful runtime semantics in such cases.

Partially addresses #325
Closes #1149
@joshlf
Copy link
Member Author

joshlf commented May 1, 2024

@jswrenn and I discussed this offline.

Currently, supporting ZSTy types even when an explicit count is given is not possible. In particular, all of our APIs that take a slice element count bottom out in one of Ref's methods that use Ptr::try_cast_into in order to convert the inner byte slice back to a reference (& or &mut) to the target type, T. try_cast_into does not currently support ZSTy types, and so these APIs cannot either.

Eventually, if we can use Ptr directly rather than Ref in the FromBytes APIs, we may be able to lift this restriction for the FromBytes APIs in particular. Lifting that restriction for Ref will likely require having Ref store a Ptr (#368).

github-merge-queue bot pushed a commit that referenced this issue May 1, 2024
Presently, we deny ZSTy DSTs in our APIs via panicking at runtime. However, the
ZSTiness of a DST is statically detectable and can be denied instead at compile
time. This PR replaces our ZSTy DST panics with compile-time assertions. Doing
gives us the freedom later provide meaningful runtime semantics in such cases.

Partially addresses #325
Closes #1149
@joshlf
Copy link
Member Author

joshlf commented Oct 6, 2024

@jswrenn Do we think that this should be closed given our PME strategy?

@jswrenn
Copy link
Collaborator

jswrenn commented Oct 7, 2024

Yes, we've thoroughly addressed this.

@jswrenn jswrenn closed this as completed Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility-breaking Changes that are (likely to be) breaking
Projects
None yet
Development

No branches or pull requests

2 participants