Add Rkyv support - #110
Conversation
|
Thanks, this looks great. Could you please setup automated tests (see e.g. #111 which does this). Happy to take it then |
|
Added the CI job and synced to main, not sure if you wanted to squash or not so I rebased -> forced the changes |
| #[cfg_attr( | ||
| feature = "rkyv", | ||
| derive(rkyv::Archive, rkyv::Deserialize, rkyv::Serialize) | ||
| )] |
There was a problem hiding this comment.
Actually, this is problematic: rkyv::Deserialize can deserialize anything without range-checks. This would allow you to (for example) produce a u7 with a value of 200, where the max is 127.
The right approach is to implement deserialize manually and add a range check. See the serde implementation for an example.
It would also be good to have a test for that specifically (e.g. construct a u127, serialize that, mess with the bytes, deserialize and verify it got rejected properly)
There was a problem hiding this comment.
Doesn't rkyv use bytecheck? So our implementation of bytecheck::Verify should catch invalid objects
There was a problem hiding this comment.
Yes it does, it even lives under the rkyv org. I didn't see your PR that added support. I'm going to add a dep on the rkyv feature on bytecheck.
I've got a small issue with implementing the validation logic.
Int and UInt are both transparent, so we can actually implement rkyv::Portable on them. This should allow removing the wrapper Archived... types if we combine with rkyv(as = ), giving us free validation on the already existing bytecheck::verify.
The problem is that I'm not managing to find the right type for the rkyv(as).
Right now I've got:
#[cfg_attr(
feature = "rkyv",
derive(rkyv::Archive, rkyv::Deserialize, rkyv::Serialize, rkyv::Portable)
// rkyv(as = ??)
// No idea on what to put there, Self? But then it complains
)]
pub struct UInt...
fn invalid_out_of_range() {
let bytes = rkyv::to_bytes::<RkyvError>(&u127::MAX).unwrap();
let actual = rkyv::access::<UInt<u8, 1>, RkyvError>(&bytes[..]).is_err();
assert!(actual); // Works :D
let actual = rkyv::access::<ArchivedUInt<u8, 1>, RkyvError>(&bytes[..]).is_err();
assert!(actual); // Doesn't :(
}Archived type can't be made private and even then it would be more of a hack than anything. I'm almost certain it can be done with as but I can't figure it out.
|
I did a merge from main so that tests run as they were broken on main (they're fixed now) - feel free to overwrite that with a git push -f yourself again |
|
Ok so not completely fixed, because I've asked on the Rkyv discord, I'll keep you updated when I get an answer |
In the same vein as Serde, add Rkyv support with an optional feature.
An example (and also my) use case is for writing savestates for an emulator. Rkyv is really good at making small ser file, and combining it with arbitrary-int for type system invariant fits well.
The code seems weirdly simple too but the tests cases works fine.