Added SmallVec::push_mut andSmallVec::insert_mut

[mematthias]

The push_mut feature (rust-lang/rust#135974) has already been merged and is expected to be included in the Rust 1.95 stable release. For this reason, we should also add push_mut and insert_mut here.

[emilio]

Would be nice to split the formatting into a separate (preliminary?) PR.

[emilio]

Maybe just &mut *ptr? Or why would as_mut().unwrap_unchecked() be preferred?

[mematthias]

That is just my writing style; it makes it easier to search.
I simplified it anyway

[emilio]

Miri seems to be complaining.

[balt-dev]

miri seems to be getting confused on the provenance?

[balt-dev]

as the guy who did push_mut in std, i feel like the change just made kind of defeats the point? the whole point of push_mut is that you're not rederiving the pointer
i say the old version is fine, we just need some way to tell miri it's okay

[mematthias]

@balt-dev While I understand the concerns raised, I am currently unable to resolve the Miri report. That said, this should not block us from shipping the feature. At the moment, users neither benefit from nor are harmed by it. But by making it available now, we ensure that once the optimization is in place, existing users can immediately take advantage of it without any disruption to their experience.

One possible (but dirty) solution would be to use set_len directly (inline) within the push_mut function:

#[inline]
pub fn push(&mut self, value: T) {
    _ = self.push_mut(value);
}

#[inline]
#[must_use]
pub fn push_mut(&mut self, value: T) -> &mut T {
    let len = self.len();
    if len == self.capacity() {
        self.reserve(1);
    }
    // SAFETY: both the input and output are within the allocation
    let ptr = unsafe { self.as_mut_ptr().add(len) };
    // SAFETY: we allocated enough space in case it wasn't enough, so the address is valid for
    // writes.
    unsafe { ptr.write(value) };

    // unsafe { self.set_len(len + 1) };
    {
        let new_len = len + 1;
        debug_assert!(new_len <= self.capacity());
        let on_heap = self.len.on_heap();
        self.len = TaggedLen::new(new_len, on_heap);
    }

    unsafe { &mut *ptr }
}

[alejandro-vaz]

@mematthias @balt-dev While I understand the concerns raised, I am currently unable to resolve the Miri report. That said, this should not block us from shipping the feature. At the moment, users neither benefit from nor are harmed by it. But by making it available now, we ensure that once the optimization is in place, existing users can immediately take advantage of it without any disruption to their experience.

One possible (but dirty) solution would be to use set_len directly (inline) within the push_mut function:

#[inline]
pub fn push(&mut self, value: T) {
    _ = self.push_mut(value);
}

#[inline]
#[must_use]
pub fn push_mut(&mut self, value: T) -> &mut T {
    let len = self.len();
    if len == self.capacity() {
        self.reserve(1);
    }
    // SAFETY: both the input and output are within the allocation
    let ptr = unsafe { self.as_mut_ptr().add(len) };
    // SAFETY: we allocated enough space in case it wasn't enough, so the address is valid for
    // writes.
    unsafe { ptr.write(value) };

    // unsafe { self.set_len(len + 1) };
    {
        let new_len = len + 1;
        debug_assert!(new_len <= self.capacity());
        let on_heap = self.len.on_heap();
        self.len = TaggedLen::new(new_len, on_heap);
    }

    unsafe { &mut *ptr }
}

yeah

when set_len is called it takes a mutable reference into the full self and then miri can't prove that the reference is still pointing to valid data because the location it points to might have been mutated since and thus it might be uninitialized data (it might have been freed => returns null pointer to user => use after free). even though that isn't what happens in our code in any case

the TaggedLen / set_len model is not a very good abstraction for what we are trying to do

I honestly support setting the length inline with self.len = /*...*/

the TaggedLen thing should probably be redesigned anyway to be honest

but before hacking it we should change how length behaves, then we implement it properly