Review of array initialization

code review
1

I have a trait to deserialize data from a little endian byte stream:

impl<T, const SIZE: usize> FromLeStream for [T; SIZE]
where
    T: FromLeStream,
{
    fn from_le_stream<I>(mut bytes: I) -> Option<Self>
    where
        I: Iterator<Item = u8>,
    {
        let mut array = [const { MaybeUninit::uninit() }; SIZE];

        for element in &mut array {
            element.write(T::from_le_stream(&mut bytes)?);
        }

        Some({
            // SAFETY:
            // - All elements have been initialized.
            // - `MaybeUninit` guarantees the same size and layout as `T`.
            #[allow(unsafe_code)]
            unsafe {
                array.as_ptr().cast::<[T; SIZE]>().read()
            }
        })
    }
}

My question is whether

  1. This code is sound, i.e. does not cause UB.
  2. This code is optimal, i.e. there's no faster and / or simpler way to do this on stable which is sound.

I am aware of e.g. try_from_fn in std::array - Rust but it hasn't been stabilized yet.
I also don't want to use third-party libraries to do this.

2

Your code leaks if deserializing an element returns None or when it panics.

3 Likes
3

(What CodesInChaos says with more words)

If T implements Drop and T::from_le_stream(&mut bytes) returns None, the destructor of every previously successfully initialised instances of T will not be called. I assume (I don't know much about proper unsafe API design) this is at the very least confusing and at worst leads to bugs. I think you should properly drop every successfully initialised T, before early-returning None, if T::from_le_stream(&mut bytes) returns None.

4

Using heapless::Vec should solve aforementioned issues without changing performance, as well as remove unsafe code

2 Likes
5

Why so? Isn't MaybeUninit supposed to prevent that if it's uninit?

6

Note that dropping a MaybeUninit<T> will never call T’s drop code. It is your responsibility to make sure T gets dropped if it got initialized.

~ MaybeUninit in std::mem - Rust

1 Like
7

Yeah, but heapless is behind a feature gate in my library, which I don't want to force upon the user if they just need stuff from core.

8

And for the performance, you really should hope that llvm will be able to see through the iterator enough to break the dependency chains between bytes of different elements, because if it manages, cpu can compute all elements of the array in parallel

1 Like
9

Well, what if you copy paste a bit of code inside your lib? Just to have unsafe code in a single module.

10

I'd consider creating your own try_from_fn and implementing this on top of that. That way it'll be easier to switch to the std implementation once it stabilizes.

Though you'll probably need to write a specific implementation for Option and/or Result, instead of using unstable traits to handle both at once.

Unrelated: Why does this trait return Option instead of Result? The latter seems like a better match semantically.

You can define a fitting unit error type, like FromStreamError or UnexpectedEndOfStream and use it in Result.

11

Because the only possible error is a premature end of the stream, which will cause building a complete T to fail. Thus any error type would be a singleton or () and Result<T, ()> is equivalent to Option<T>.

FWIW I use an error in another trait method where there may be multiple possible reasons to fail.

12

Oh, that early return may ruin it by the way. Maybe keeping a flag would be more performant. Needs testing. And a follow up, you should inspect the generated assembly if llvm was able to see through iterators and if not, maybe changing the interface in a way to allow it.

1 Like
13

Well, it was a nice try. I reverted back to a safe and possibly less efficient version:

14

It looks like you're trying to make ArrayVec<T, SIZE> from arrayvec - Rust

Just use it. That's what the compiler does.

As an aside, this example in the library sounds like what you're doing:

2 Likes