For context, I implemented a (hopefully) safe vector backed up by a static array as a functional block for a bigger project, but as you may imagine, popping or pushing values can fail when it's either empty or full.
I chose to use assert!s to sanitize these calls instead of creating try_...() methods since the code is dense with such interactions and filling it with unwrap() didn't look useful to me, however without bugs even these checks would be unnecessary.
Should I introduce fallible methods and maybe a newtype called PanicVec or something?
Thanks for you time.
Can you show your current interface? I don't quite understand your question, and it would help. For example std::vec::Vec::<T>::pop returns Option<T>. What does your implementation return, and how do you want to use it?
Thanks for your response, here it is.
struct ArrayVec<T, const N: usize> {
values: [MaybeUninit<T>; N],
len: usize,
}
impl<T, const N: usize> ArrayVec<T, N> {
fn new() -> Self {
Self {
values: unsafe { MaybeUninit::uninit().assume_init() },
len: 0,
}
}
fn push(&mut self, value: T) {
assert_ne!(self.len, usize::MAX);
(self.values[self.len as usize]).write(value);
self.len += 1;
}
fn pop(&mut self) -> T {
assert_ne!(self.len, 0);
self.len -= 1;
unsafe { ptr::read(mem::transmute(self.values.as_ptr().add(self.len))) }
}
fn last(&self) -> Option<&T> {
let index = self.len - 1;
self.get(index)
}
fn last_mut(&mut self) -> Option<&mut T> {
let index = self.len - 1;
self.get_mut(index)
}
fn get(&self, index: usize) -> Option<&T> {
if index < self.len {
Some(unsafe { self.values[index as usize].assume_init_ref() })
} else {
None
}
}
fn get_mut(&mut self, slot: usize) -> Option<&mut T> {
assert!(slot < self.len);
Some(unsafe { mem::transmute(&mut self.values[slot as usize]) })
}
fn iter(&self) -> std::slice::Iter<'_, T> {
unsafe { slice::from_raw_parts(self.values.as_ptr() as *const T, self.len) }.iter()
}
}
Your new function is UB i think. You can't do MaybeUninit::uninit().assume_init(). there are a lot of examples that even for integers this is UB. You should look at MaybeUninit::uninit_array() which is nightly but you can just copy the implementation as it is really short.
This is undefined behavior. You can learn more about at: MaybeUninit - Initialization invariant
How to do it correctly is literally explained in the MaybeUninit documentation Initializing an array element-by-element, too:
fn new() -> Self {
Self {
values: [const { MaybeUninit::uninit() }; N],
len: 0,
}
}
The assert_ne is wrong:
Correct is to use N:
fn push(&mut self, value: T) {
assert_ne!(self.len, N);
...
}
Method last() and last_mut() is wrong:
When there are no values, there is an integer overflow. First check for at least one element, then subtract:
fn last(&self) -> Option<&T> {
if self.len > 0 {
Some(unsafe { self.values[self.len - 1].assume_init_ref() } )
} else {
None
}
}
There is no need to use mem::transmute(). You can use the methods from MaybeUninit in a much safer way. Checkout assume_init_read():
fn pop(&mut self) -> T {
assert_ne!(self.len, 0);
self.len -= 1;
unsafe { self.values[self.len].assume_init_read() }
}
Regarding your original question: Most of your methods check for valid state and are returning Options. So what is special about pop()? Returning an Option would be very consistent with the rest of your code.
However, if you really would like to provide a shortcut, so a caller can avoid unwrap() at his own risk, maybe a variant unchecked_pop() which is marked unsafe will do fine?
3 Likes
You can, in the special case that all-bits-uninitialized is a valid state of the contained type. This pattern was explicitly recommended in the docs until recently.
We haven’t been shown any manual Drop implementation, but the automatic drop glue here shouldn’t be a problem— MaybeUninit doesn’t call drop() on its contents because it doesn’t do any tracking about whether or not it contains initialized data.
The assert is entirely superfluous here because the indexing operator will bounds-check and panic on its own.
6 Likes
You're right. I had it mixed up with zeroed().
Have you considered using the arrayvec crate?
1 Like
As for the original question: it's ok to use assert! to ensure safety. It always runs (in release mode too), and guarantees to unwind or abort when the assertion fails.
From performance perspective, it's almost always better to use Option or Result and not use any panics. This is because a possibility of a panic can prevent optimizer from moving or changing the code, since it's a major side effect. Regular function return is often cheaper and doesn't prevent optimizations as much.
10 Likes
I thank everyone for your good catches, I've been rather careless in fixing this code before showing it, mainly because it was not meant to be reviewed in that sense, however I invite someone to double check before claiming errors.
Just to minimize the amount of unsafe code that you have to audit and get right, consider writing methods that return &[T] and &mut [T] which you then use internally for anything that doesn't require changing len. You could also consider Derefing to a slice (like Vec does) so that you don't have to manually forward to all of the various slice methods you're interested in:
pub struct ArrayVec<T, const N: usize> {
values: [MaybeUninit<T>; N],
len: usize,
}
impl<T, const N: usize> ArrayVec<T, N> {
pub fn new() -> Self {
Self {
// Old way, but still valid
// values: unsafe { MaybeUninit::uninit().assume_init() },
// New preferred method
values: [const { MaybeUninit::uninit() }; N],
len: 0,
}
}
pub fn push(&mut self, value: T) {
self.values[self.len].write(value);
self.len += 1;
}
pub fn pop(&mut self) -> Option<T> {
self.len = self.len.checked_sub(1)?;
Some(unsafe { self.values[self.len].assume_init_read() })
}
pub fn as_slice(&self) -> &[T] {
unsafe { slice::from_raw_parts(self.values.as_ptr() as *const T, self.len) }
// Or, with #![feature(maybe_uninit_slice)] on the nightly compiler...
// unsafe { MaybeUninit::slice_assume_init_ref(&self.values[..self.len]) }
}
pub fn as_mut_slice(&mut self) -> &mut [T] {
unsafe { slice::from_raw_parts_mut(self.values.as_mut_ptr() as *mut T, self.len) }
// Or, with #![feature(maybe_uninit_slice)] on the nightly compiler...
// unsafe { MaybeUninit::slice_assume_init_mut(&mut self.values[..self.len]) }
}
/* Provided via Deref / DerefMut below
fn last(&self) -> Option<&T> {
self.as_slice().last()
}
fn last_mut(&mut self) -> Option<&mut T> {
self.as_mut_slice().last_mut()
}
fn get(&self, index: usize) -> Option<&T> {
self.as_slice().get(index)
}
fn get_mut(&mut self, slot: usize) -> Option<&mut T> {
self.as_mut_slice().get_mut(slot)
}
fn iter(&self) -> std::slice::Iter<'_, T> {
self.as_slice().iter()
}
*/
}
impl<T, const N: usize> Drop for ArrayVec<T, N> {
fn drop(&mut self) {
let len = self.len;
self.len = 0;
for i in 0..len {
unsafe { self.values[i].assume_init_drop() };
}
}
}
impl<T, const N: usize> std::ops::Deref for ArrayVec<T, N> {
type Target = [T];
fn deref(&self) -> &[T] {
self.as_slice()
}
}
impl<T, const N: usize> std::ops::DerefMut for ArrayVec<T, N> {
fn deref_mut(&mut self) -> &mut [T] {
self.as_mut_slice()
}
}
2 Likes
Thanks for the truly enriching tips.