Skip to content

Add Rkyv support - #110

Open
IzawGithub wants to merge 2 commits into
danlehmann:mainfrom
IzawGithub:rkyv
Open

Add Rkyv support#110
IzawGithub wants to merge 2 commits into
danlehmann:mainfrom
IzawGithub:rkyv

Conversation

@IzawGithub

Copy link
Copy Markdown

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.

@danlehmann

Copy link
Copy Markdown
Owner

Thanks, this looks great. Could you please setup automated tests (see e.g. #111 which does this). Happy to take it then

IzawGithub added a commit to IzawGithub/arbitrary-int that referenced this pull request Jul 28, 2026
@IzawGithub

Copy link
Copy Markdown
Author

Added the CI job and synced to main, not sure if you wanted to squash or not so I rebased -> forced the changes

Comment thread src/signed.rs
#[cfg_attr(
feature = "rkyv",
derive(rkyv::Archive, rkyv::Deserialize, rkyv::Serialize)
)]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't rkyv use bytecheck? So our implementation of bytecheck::Verify should catch invalid objects

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@danlehmann

Copy link
Copy Markdown
Owner

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

@IzawGithub

Copy link
Copy Markdown
Author

Ok so not completely fixed, because rkyv::access can still return ok when it's not, I think because it takes bytes as being independent of each other instead of being the same value? It's a bit weird and I'm just a basic rkyv user so idk about internal.

I've asked on the Rkyv discord, I'll keep you updated when I get an answer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants