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]

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

[mematthias]

@alejandro-vaz Should the same dirty workaround be used for insert_mut?

[alejandro-vaz]

yeah

the set_len method doesn't make much sense and just confuses the borrow checker by taking a mutable reference of the whole smallvec instead of just its len field

[alejandro-vaz]

please rebase the commit history onto servo:v2 to remove merge commits and have a linear history

[alejandro-vaz]

yeah this is good for now until we implement #451

[alejandro-vaz]

@mematthias I had to merge upstream changes, it annoyed rustfmt

please run cargo fmt --all so I can approve it

old review, changes already implemented