Unnecessary unsafe in anstyle-parse's osc_dispatch?

[Ortham]

anstyle-parse has:

    #[inline]
    fn osc_dispatch<P: Perform>(&self, performer: &mut P, byte: u8) {
        let mut slices: [MaybeUninit<&[u8]>; MAX_OSC_PARAMS] =
            unsafe { MaybeUninit::uninit().assume_init() };

        for (i, slice) in slices.iter_mut().enumerate().take(self.osc_num_params) {
            let indices = self.osc_params[i];
            *slice = MaybeUninit::new(&self.osc_raw[indices.0..indices.1]);
        }

        unsafe {
            let num_params = self.osc_num_params;
            let params = &slices[..num_params] as *const [MaybeUninit<&[u8]>] as *const [&[u8]];
            performer.osc_dispatch(&*params, byte == 0x07);
        }
    }

this caught my eye because I wasn't sure if the initialisation of slices is sound, but after some thought it's not clear why MaybeUninit is needed at all - I don't really understand it, but is there any reason why you can't instead create an array of slices that is initialised using zero-length slices, and then overwrite them where possible? I.e.

    #[inline]
    fn osc_dispatch<P: Perform>(&self, performer: &mut P, byte: u8) {
        let mut slices: [&[u8]; MAX_OSC_PARAMS] = [&[]; MAX_OSC_PARAMS];

        for (i, slice) in slices.iter_mut().enumerate().take(self.osc_num_params) {
            let indices = self.osc_params[i];
            *slice = &self.osc_raw[indices.0..indices.1];
        }

        performer.osc_dispatch(&slices[..self.osc_num_params], byte == 0x07);
    }

[danjl1100]

I just ran across this MaybeUnint::uninit.assume_init() which looks iffy (but is probably technically OK?) in a cargo-vet review. Glad to see I'm not alone in having some pause when reading this code 🙂

It looks like this was changed from mem::uninitialized() to MaybeUninit::uninit().assume_init() in this commit c093a67, but not sure why uninitialized was needed in the original addition of this function (3149ed0).

I suppose it's in the hot-loop, where maybe even zeroing [&[u8]; 16] has a measurable impact?

[danjl1100]

At the very least, it seems like the creation of the [MaybeUninit; MAX] could be safe.

For the second unsafe block (casting to params) the recently stabilized slice function assume_init_ref in 1.93 would clarify the intent a bit better. I'm not sure on the MSRV policy for this crate/project though...

I don't see any change in the emitted assembly for this small mockup https://godbolt.org/z/Ea51z9hKj

Illustrating both changes:

     #[inline]
     fn osc_dispatch<P: Perform>(&self, performer: &mut P, byte: u8) {
         let mut slices: [MaybeUninit<&[u8]>; MAX_OSC_PARAMS] =
-            unsafe { MaybeUninit::uninit().assume_init() };
+            [MaybeUninit::uninit(); MAX_OSC_PARAMS];

         for (i, slice) in slices.iter_mut().enumerate().take(self.osc_num_params) {
             let indices = self.osc_params[i];
             *slice = MaybeUninit::new(&self.osc_raw[indices.0..indices.1]);
         }

+        let params;
         unsafe {
             let num_params = self.osc_num_params;
-            let params = &slices[..num_params] as *const [MaybeUninit<&[u8]>] as *const [&[u8]];
+            params = &slices[..num_params].assume_init_ref();
-            performer.osc_dispatch(&*params, byte == 0x07);
         }
+        performer.osc_dispatch(params, byte == 0x07);
     }

[Ortham]

in a cargo-vet review.

That's also what I was doing when I saw this, I'm glad it's not just me wondering about this!

Illustrating both changes:

I think I'd initially thought of suggesting the same (except keeping the use of as instead of assume_init_ref() since Cargo.toml has the MSRV being 1.66), but then decided to instead ask why unsafe was used when it doesn't seem strictly necessary - I think it's the sort of thing that could do with a comment explaining why (e.g. if it does have a performance impact).

[epage]

This started as a fork of vtparse but I've not kept up with all of the latest vtparse changes.

https://github.com/wezterm/wezterm/blob/577474d89ee61aef4a48145cdec82a638d874751/vtparse/src/lib.rs#L621-L638