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

HeaderSection: Change slices to fixed-length arrays #329

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tertsdiepraam
Copy link
Contributor

@tertsdiepraam tertsdiepraam commented Jun 6, 2024

HeaderSection, Header and HeaderCounts all have for_message_slice which takes a slice and panics if the slice is too short. This is annoying because the compiler cannot help us enforce that constraint at compile-time and users of domain would need to read the docs carefully to use those functions correctly.

This PR changes those methods to instead take a &[u8; N] with the length of the internal representation of these types. The advantage of this can be seen by the tests this PR removes: instead of panicking they now no longer compile.

Additionally, I changed the as_slice methods for these types to as_array. Since a reference to an array derefs to a slice, these are mostly compatible.

There's another notable change: HeaderCounts does not take a slice of the entire message anymore, but only of the counts part (so just 8 bytes). This makes that function simpler, which I think is better because that makes it easier to guarantee its correctness. The burden of finding the right offset to get it from is now on the caller. This means that users probably do not want to use HeaderCounts::for_message_chunk but instead HeaderSection::for_message_chunk followed by .counts(). This makes it clearer what information should be passed in. There's even a case to be made that {Header, HeaderCounts}::for_message_chunk should not be public.

We can now do the conversions with mem::transmute, but there's a pattern that makes the transmutes even safer. mem::transmute statically guarantees that the types it converts have the same size. That's nice, but not enough in our case since we want to guarantee that the size of the referenced types are the same. Therefore, I add a second unused mem::transmute call like this:

let _ = || {
    unsafe { mem::transmute::<[u8; N], Self>(*s) };
};

This does not compile if [u8; N] and Self have different sizes, therefore guaranteeing safety. It's a bit strange, but I think it works.

This is a draft PR because:

  1. It's a breaking change and we might not want this or deprecate some of the functions I changed or removed first.
  2. We might want to remove the non-mut conversions and returned owned (copied) values instead.
  3. This currently requires Rust 1.77, so if we downgrade to 1.76, I'd have to change some things before we can merge this.

This is a breaking change.

@tertsdiepraam tertsdiepraam force-pushed the slices-to-arrays branch 2 times, most recently from 5c51d8b to 79677b2 Compare June 6, 2024 15:17
Copy link
Member

@partim partim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

I think we can do this in a non-breaking fashion by leaving the old functions in place for now and only remove them on the next breaking change. For the new functions, perhaps for_array and for_array_mut are better names. Similarly, keep as_slice (we can probably just keep it forever) and add the new as_array methods.

If you can do the chunks in a way compatible with 1.76, then I think I would prefer that.

pub fn for_message_chunk(message: &[u8; 8]) -> &Self {
// SAFETY: The transmute is sound because
// - HeaderCounts has #[repr(transparent)] and
// - the array has the correct size as evidenced by this (unused)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would like this comment to more clearly state that this transmute is only for length checking and will compile away. Even with the comment it is a bit “What?“ right now.

src/base/header.rs Outdated Show resolved Hide resolved
@tertsdiepraam tertsdiepraam added the breaking A PR that includes a breaking change. label Jun 12, 2024
@tertsdiepraam tertsdiepraam marked this pull request as ready for review June 12, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A PR that includes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants