Skip to content

impl AnyBitPattern for [MaybeUninit<T: AnyBitPattern>; N]#317

Merged
fu5ha merged 1 commit intoLokathor:mainfrom
zdivelbiss:main
Sep 5, 2025
Merged

impl AnyBitPattern for [MaybeUninit<T: AnyBitPattern>; N]#317
fu5ha merged 1 commit intoLokathor:mainfrom
zdivelbiss:main

Conversation

@zdivelbiss
Copy link
Copy Markdown
Contributor

@zdivelbiss zdivelbiss commented Jul 16, 2025 •
edited
Loading

Provides the following implementation:

#[cfg(all(feature = "zeroable_maybe_uninit", feature = "min_const_generics"))]
#[cfg_attr(
  feature = "nightly_docs",
  doc(cfg(all(
    feature = "zeroable_maybe_uninit",
    feature = "min_const_generics"
  )))
)]
unsafe impl<T, const N: usize> AnyBitPattern for [core::mem::MaybeUninit<T>; N] where
  T: AnyBitPattern
{
}

Which allows arrays of MaybeUninit<T: AnyBitPattern> to be treated as AnyBitPattern. For instance, the following struct is not currently possible via the safer derive macro for AnyBitPattern:

#[repr(align(0x10))]
#[derive(AnyBitPattern)]
struct Stack<const N: usize>([MaybeUninit<u8>; N]);

Which results in the following error on the struct's tuple field:

the trait bound `core::mem::MaybeUninit<u8>: bytemuck::Pod` is not satisfied

... which complicates my current use case, of trying to safely allocate a Stack of uninitialized bytes (skipping the cost of first zeroing the stack, which isn't required for a process stack).

@Lokathor Lokathor requested a review from fu5ha July 16, 2025 14:41
@Lokathor Lokathor added semver-minor semver minor change semver-derive We need to update the main crate's use of the derive crate not yet released labels Jul 16, 2025
@Lokathor
Copy link
Copy Markdown
Owner

Lokathor commented Aug 22, 2025

@fu5ha ping! will you be available to review this any time soon?

@fu5ha
Copy link
Copy Markdown
Collaborator

fu5ha commented Aug 22, 2025

I think so! Setting a remider

fu5ha
fu5ha reviewed Aug 31, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fu5ha fu5ha left a comment

Choose a reason for hiding this comment

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

This makes sense to me, but I think we could actually broaden this to instead be

#[cfg(feature = "min_const_generics")]
#[cfg_attr(
  feature = "nightly_docs",
  doc(cfg(
    feature = "min_const_generics"
  ))
)]
unsafe impl<T, const N: usize> AnyBitPattern for [T; N] where
  T: AnyBitPattern
{
}

Which would then get [MaybeUninit<T>; N] for free.

Also this is making me think that we could broaden the existing impl<T> AnyBitPattern for MaybeUninit<T> where T: AnyBitPattern to drop the T: AnyBitPattern bound. MaybeUninit<T> is in general AnyBitPattern no matter what is the inner T, unless I'm missing something 🤔

@zachs18
Copy link
Copy Markdown
Contributor

zachs18 commented Aug 31, 2025

This makes sense to me, but I think we could actually broaden this to instead be

#[cfg(feature = "min_const_generics")]
#[cfg_attr(
  feature = "nightly_docs",
  doc(cfg(
    feature = "min_const_generics"
  ))
)]
unsafe impl<T, const N: usize> AnyBitPattern for [T; N] where
  T: AnyBitPattern
{
}

This would conflict with the existing impl<T: Pod> Pod for [T; N] (due to the blanket impl<T: Pod> AnyBitPattern for T).

In a future (major) version of bytemuck, we could "flip" the blanket impl, and have impl<T: AnyBitPattern + NoUninit> Pod for T, and have users only implement AnyBitPattern and NoUninit, and then this would work (since we would have impl<T: AnyBitPattern> AnyBitPattern for [T; _] and impl<T: NoUninit> NoUninit for [T; _] which would together mean T: Pod implies [T; _]: Pod)

Also this is making me think that we could broaden the existing impl<T> AnyBitPattern for MaybeUninit<T> where T: AnyBitPattern to drop the T: AnyBitPattern bound. MaybeUninit<T> is in general AnyBitPattern no matter what is the inner T, unless I'm missing something 🤔

This has possible issues with interior mutability. In particular, if T has interior mutability, then so does MaybeUninit<T>, and AnyBitPattern currently requires that the type not have interior mutability, so impl<T> AnyBitPattern for MaybeUninit<T> would be unsound. We could restrict it to impl<T: Copy> AnyBitPattern for MaybeUninit<T> since there are currently no interior-mutable Copy types, but it is not clear if T: Copy is guaranteed to never have interior mutability. More info here: #249 (comment)

@zdivelbiss
Copy link
Copy Markdown
Contributor Author

zdivelbiss commented Sep 1, 2025 •
edited
Loading

I think this implementation is acceptable for the current state of the language specifications regarding soundness. I'm not exactly opposed to expanding the impl, but the reason I made this PR was that the implementation utilized existing ("known-good") soundness, and did not require any new soundness rules to be evaluated.

@fu5ha
Copy link
Copy Markdown
Collaborator

fu5ha commented Sep 5, 2025

This would conflict with the existing impl<T: Pod> Pod for [T; N] (due to the blanket impl<T: Pod> AnyBitPattern for T).

Ah right, I was afraid of that. I guess that's why these blanket impls didn't already exist in the first place.

OK, well, since we've thought about the more general options I think it makes sense to still get this minimal version in now. It doesn't exclude the possibility of making it broader in the future.

@fu5ha fu5ha merged commit f0fbcbd into Lokathor:main Sep 5, 2025
14 checks passed
@fu5ha
Copy link
Copy Markdown
Collaborator

fu5ha commented Sep 5, 2025

Can put this on the list for a patch release when you get the time @Lokathor

@Lokathor Lokathor added semver-patch semver patch change and removed semver-minor semver minor change labels Sep 5, 2025
@Lokathor Lokathor removed the semver-derive We need to update the main crate's use of the derive crate label Oct 3, 2025
@Lokathor
Copy link
Copy Markdown
Owner

Lokathor commented Oct 3, 2025

released in 1.24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fu5ha fu5ha fu5ha approved these changes

Assignees

No one assigned

Labels

semver-patch semver patch change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zdivelbiss @Lokathor @fu5ha @zachs18