Am I creating a correct `Pin` abstraction?

I've never used the Pin API before, just read up on it. I would appreciate if someone could take a look at this abstraction, tell me if it's correct/sound/idiomatic. :O)

use std::{
    pin::Pin,
    marker::Unpin,
    ptr::drop_in_place,
};

/// Wraps `Vec<T>` and disallows re-allocation.
pub struct PinBuffer<T> {
    vec: Vec<T>
}

impl<T> PinBuffer<T> {
    pub fn new(capacity: usize) -> Self {
        PinBuffer { vec: Vec::with_capacity(capacity) }
    }
    
    /// Current element length.
    pub fn len(&self) -> usize { self.vec.len() }
    
    /// Current element capacity.
    pub fn capacity(&self) -> usize { self.vec.capacity() }
    
    /// Whether capacity allows to push another element.
    pub fn can_push(&self) -> bool {
        self.len() < self.capacity()
    }
    
    /// Push an element. Panics if at capacity.
    pub fn push(&mut self, elem: T) {
        assert!(self.can_push(), "push to full PinBuffer");
        self.vec.push(elem);
    }
    
    /// Pop and drop the top element. 
    ///
    /// Return false if already empty.
    /// 
    /// Returning element would violate `Pin` variants.
    pub fn remove_top(&mut self) -> bool {
        let len = match self.vec.len() {
            0 => return false,
            l => l,
        };
        unsafe {
            // take special care to drop element without moving it
            let top: *mut T = &mut self.vec[len - 1];
            self.vec.set_len(len - 1);
            drop_in_place(top);
        }
        true
    }
    
    /// Pop and return the top element.
    ///
    /// Only possible if the element type is `Unpin`.
    pub fn pop(&mut self) -> Option<T> 
    where T: Unpin {
        self.vec.pop() 
    }
    
    /// Override an existing element.
    ///
    /// Quoting [std::pin::Pin](https://doc.rust-lang.org/std/pin/struct.Pin.html#method.set):
    /// 
    /// > This overwrites pinned data, but that is okay:
    /// > its destructor gets run before being overwritten,
    /// > so no pinning guarantee is violated.
    pub fn set(&mut self, index: usize, elem: T) {
        self.vec[index] = elem;
    }
    
    /// Get by index as pinned shared ref, or panic.
    pub fn idx_ref(&self, index: usize) -> Pin<&T> {
        unsafe { Pin::new_unchecked(&self.vec[index]) }
    }
    
    /// Get by index as pinned mutable ref, or panic.
    pub fn idx_mut(&mut self, index: usize) -> Pin<&mut T> {
        unsafe { Pin::new_unchecked(&mut self.vec[index]) } 
    }
    
    /// Get by index as pinned shared ref.
    pub fn get_ref(&self, index: usize) -> Option<Pin<&T>> {
        self.vec.get(index)
            .map(|r| unsafe { Pin::new_unchecked(r) })
    }
    
    /// Get by index as pinned mutable ref.
    pub fn get_mut(&mut self, index: usize) -> Option<Pin<&mut T>> {
        self.vec.get_mut(index)
            .map(|r| unsafe { Pin::new_unchecked(r) })
    }
}

you cannot pin elements themself.
It would make sense for static array, but not Vec where each push can re-allocated(and therefore invalidate your pins)

Instead if using vector underhood I suggest to just create static array of particular size (which is in fact what you have as you limit push to capacity)

1 Like

At a skim it seems sound. But it might be more obvious (though involve more unsafe code) to build on top of Box<[MaybeUninit<T>]> instead of Vec. As far as I know there is no real guarantee that calling push within the capacity will not reallocate.

2 Likes

From the std docs:

push and insert will never (re)allocate if the reported capacity is sufficient. push and insert will (re)allocate if len == capacity . That is, the reported capacity is completely accurate, and can be relied on.

However, OP can simplify things by using the heapless crate. heapless::Vec::push returns Err if the structure is at capacity, and never reallocates, so there's no need to track and check the capacity yourself.

But heapless is an inline storage, and thus not pinned unless its get_mut method were to take a pin in the form of Pin<&mut Self> itself. The problem is that the memory used for the vector storage needs to survive even leaking the Vec for the interface to be sound. Vec and Box do this, heapless and anything else that borrows stack memory (and exposed by value) does not.

1 Like

Yeah, you're right, he'd have to stick it in a Box or something.

you can make it static, that's actually the whole point

I considered backing a collection with something other than a vec. Thing is, since the actual number of elements included is variable, I would have to represent is as something like a usize length and a Box<[MaybeUninit<ManuallyDrop<T>>]>, then manually handle memory initialization, destruction, and bounds checking. After considering that, it seemed far simpler to just use a Vec and take care to never trigger a resize.

Hm it is not all that hard in all actuallity, but up to you.

You can use as reference more simple example of mine https://github.com/DoumanAsh/statiki/blob/master/src/vec.rs#L30-L368

1 Like

I wonder if something like a rope data structure could work here. The idea would be to have a vec of boxed arrays (or vecs with defined capacity), as more space is needed new arrays can be added to the collection, but old ones would stay where they were rather than being moved into a larger allocation. A drawback would be that you couldn’t provide continuous slices over rope segments, but iterators would still work.

http://abishov.com/xi-editor/docs/rope_science_00.html

FWIW, MaybeUninit "implies" ManuallyDrop-ness, so you can simplify it to Box<[MaybeUninit<T>]> (which, by the way, is actually a RawVec). You would still need to handle all the things you mentioned, so your approach is actually safer because simpler :slightly_smiling_face:

So your abstraction seems sound to me, good job!

To nitpick, I suspect (but am not sure of it) that the following code breaks provenance:

unsafe {
    // take special care to drop element without moving it
    let top: *mut T = &mut self.vec[len - 1]; // provenance: &mut self.vec
    self.vec.set_len(len - 1); // borrows &mut self.vec, so it invalidates the previous borrow and the pointer derived from it
    drop_in_place(top); // use of invalidated pointer
}

So I'd rather do:

let new_len = len - 1;
unsafe {
    // take special care to drop element without moving it
    self.vec.set_len(new_len);
    let top = self.vec.as_mut_ptr().add(new_len);
    drop_in_place(top);
}

just to be extra careful.

FWIW, MaybeUninit "implies" ManuallyDrop -ness, so you can simplify it to Box<[MaybeUninit<T>]>

Oh cool!

So your abstraction seems sound to me, good job!

Thank you!

To nitpick, I suspect (but am not sure of it) that the following code breaks provenance:

Interesting, I think I'll integrate that.

I wonder if something like a rope data structure could work here.

That is exactly the reason why I am creating this! I'll ping you when I publish to crates.io, if you like.

1 Like

Check it out!
https://crates.io/crates/pinvec

Cool, will do. I have a C API that is used to populate buffers, I can queue up buffers to populate and the API raises events when buffers are full. I think I can use a data structure like this to allow passing reference-like structs out to the rest of the application, which requeue the buffers on drop, while still allowing me to easily add buffers to the pool if the calling code doesn’t ever drop them.

1 Like