Implementing ByteSlice and ByteSliceMut for Vec<u8>

[shashitnak]

Adresses #992

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[jswrenn]

The safety comment needs to prove that the safety conditions of SplitByteSlice are totally satisfied. Unfortunately, I believe safety conditions are unsatisfiable in this case. It asks:

In particular, given B: SplitByteSlice and b: B, if b.deref() returns a byte slice with address addr and length len, then if split <= len, b.split_at(split) will return (first, second) such that:

• first's address is addr and its length is split
• second's address is addr + split and its length is len - split

Your implementation does not satisfy the second condition, because the two Vecs will almost certainly not be adjacent in memory.

[joshlf]

To add a bit of extra detail here: Your implementations of ByteSlice and ByteSliceMut are fine so long as you remove the implementation of SplitByteSlice.

[shashitnak]

@jswrenn @joshlf Pushed a naive solution where I somehow managed to make it work. Am I at least on the right path? Or is it unacceptable?

[shashitnak]

I think if we give up the restriction of only allowing pair of Self to be returned from SplitByteSlice::split_at, we can use a type similar to VecSlice, from my example, and can split a Vec for free. And then we can also implement SplitByteSlice for VecSlice and then we can split a Vec however many times we want at zero cost. Is there a reason we would not want to remove this restriction?

[joshlf]

IIUC this is still unsound - how do you prevent the CanDrop variant from being dropped before the DontDrop variant? If that happens, then the DontDrop variant points to freed memory. I could imagine solving this with lifetimes, but you might need to make SplitByteSlice even more complex to be able to carry those lifetimes (or maybe not? I'm genuinely not sure).

In the abstract I think this is an interesting idea, but it will take some work to rejigger the API to support it without adding a lot of complexity that other users (who aren't using Vec) will have to know about.

Is there a reason that your use case doesn't allow either just operating on slices (e.g. borrowing the Vec) or making a clone of the Vec into a RefCell<[u8]> (which we do support)?

[shashitnak]

Here's a new thing I tried that is probably better than the previous implementation. The VecSlice type now looks like the following

pub struct VecSlice {
    slice: VirtualVec,
    ghost: Rc<GhostVec>,
}

struct VirtualVec {
    ptr: *mut u8,
    len: usize,
    cap: usize,
}

struct GhostVec(Vec<u8>);

With every split, new VirtualVecs are created but the GhostVec is shared by all the splits and will only drop when the last VecSlice is dropped.

Although, now when I think about it, this is just a more sophisticated way of achieving what can easily be achieved by calling split_at on Vec not as Vec but as slice. This doesn't seem to provide much value I guess.

Regarding your question, I was just curious about if it was possible to split Vec at zero cost.

[shashitnak]

@joshlf removed all the unnecessary code. It now only contains implementations of ByteSlice and ByteSliceMut

[joshlf]

Can you add safety comments here and below? Our policy is not to introduce any new undocumented unsafe code while we burn down the existing TODOs.

[jswrenn]

On further reflection, I don't believe the safety conditions of ByteSlice are satisfiable:

Safety

Implementations of ByteSlice must promise that their implementations of Deref and DerefMut are "stable". In particular, given B: ByteSlice and b: B, b must always dereference to a byte slice with the same address and length. This is true for both b.deref() and b.deref_mut(). If B: Copy or B: Clone, then the same is also true of copies or clones of b. For example, b.deref_mut() must return a byte slice with the same address and length as b.clone().deref().

For Vec, a growable buffer, this cannot be the case: the length changes as elements are added and removed, and the address changes as elements are added.

[jswrenn]

On further reflection, I don't believe the safety conditions of ByteSlice are satisfiable:

Safety

Implementations of ByteSlice must promise that their implementations of Deref and DerefMut are "stable". In particular, given B: ByteSlice and b: B, b must always dereference to a byte slice with the same address and length. This is true for both b.deref() and b.deref_mut(). If B: Copy or B: Clone, then the same is also true of copies or clones of b. For example, b.deref_mut() must return a byte slice with the same address and length as b.clone().deref().

For Vec, a growable buffer, this cannot be the case: the length changes as elements are added and removed, and the address changes as elements are added.

[jswrenn]

@joshlf, in light of the above comment, I'm not certain that this satisfies our documented safety condition either. It's trivial to define a function that changes both the length and address of a &mut [u8] between calls to deref:

fn change_add_and_length(mut slice: &mut [u8]) {
    slice = &mut slice[1..];
}

I think this indicates that our safety documentation on ByteSlice is missing some critical nuance.

[joshlf]

Okay, now that #1215 and #1218 have landed, this can be rebased and the soundness issues should be addressable. In particular, we can implement ByteSlice and ByteSliceMut for Vec<u8>, but we can't implement any of the other traits.