unpack_bytes_verified does not advance the underlying reader

[kodemartin]

Bug description

The unpack_bytes_{un,}verified methods do not advance the underlying slice while unpacking slices of bytes.

To reproduce:

use packable::unpacker::SliceUnpacker;
use packable::{Packable, PackableExt};

fn main() {
    let src: Vec<u8> = (0..10).collect();
    // This advances the underlying reader
    let mut unpacker = SliceUnpacker::new(src.as_slice());
    for byte in &src {
        let unpacked = u8::unpack(&mut unpacker, None).unwrap();
        assert_eq!(*byte, unpacked);
    }
    // This does not advance the underlying reader
    let bytes = src.as_slice();
    for byte in &src {
        let unpacked = u8::unpack_bytes_verified(bytes, &()).unwrap();
        // will panic after first iteration
        assert_eq!(*byte, unpacked);
    }
}

This is because of the use of bytes.as_ref that copies the underlying slice address into a new address. Hence when we pass slice: &mut bytes.as_ref() into a function that at some point calls *slice = new_slice, the original bytes slice is not affected. This could help illustrate the argument:

let src: Vec<u8> = (0..10).collect();
let bytes = src.as_slice();
let new_bytes = bytes.as_ref();
assert!(format!("{:p}", &bytes) != format!("{:p}", &new_bytes));

If this is the intended behavior, I reckon it should be documented.

Otherwise you could consider using &[u8] instead of AsRef<u8> to ensure that the underlying reader is advanced, which seems like the expected behavior.

Rust version

1.76

[DaughterOfMars]

You can't advance a slice, so I'm not sure what you expect from this. You need the cursor to do so.

[kodemartin]

You can't advance a slice, so I'm not sure what you expect from this. You need the cursor to do so.

That is certainly true.

The motivation for this report comes from dealing with the 0.4.0 version of the library, that implemented Unpacker for &mut &[u8] allowing one to use both Packable and PackableExt directly on a mutable slice with inconsistent side-effects.

I came across this issue while going over Thoralf's (@Thoralf-M) first take on the stardust-snapshot parser. In particular, I couldn't figure out why this part does not introduce parsing errors, since the vector of Outputs comes immediately after the FullSnapshotHeader.

With mut slice: &[u8] one would very much expect that unpack::<_, _>(&mut slice.as_ref()) wrapped in the PackableExt would have the same side effects as unpack::<_, _>(&mut slice). But this is not the case for the reasons I discussed in the issue's description.

Nevertheless, the introduction of the SliceUnpacker in later versions overcomes indeed this problem and as you point out one shouldn't expect any side effects on the slice itself when using PackableExt. Feel free to close the issue.