IDL Deserialization for Rust

[chenyan-dfinity]

TODO:

• deserialize multiple arguments
• lifetime problem for deserialize_str
• newtype support (this requires changes to ser.rs)
• test cases for missing/extra fields
• limit recursion depth

[hansl]

We end up not using serde, since serde cannot derive type information, and we cannot easily sort the field index in struct and enum by the idl_hash.

I'm using a simple state machine in my Request ID hasher for this. It makes things very easy. I don't think deriving type information is necessary for this. It doesn't seem like a good enough reason to not support serde serialization/deserialization.

[chenyan-dfinity]

Let's sync on this. I would be very interested to learn how we can get around this without custom derive. I would love to live in the serde ecosystem if at all possible!

[hansl]

#42 is my PR for calculating Request ID with Serde Serialization. It uses a basic state machine (as most serializer have to) for keeping the fields then sorting them once the structure is done.

It would be trivial to add types to it. And it would be trivial to do recursive embedded structures (using a stack instead of an Option<_>).

[chenyan-dfinity]

You skipped enum. Yes, everything is possible under serde if you don't support enum.

[nomeata]

I managed to use this library in my unofficial experiments of building canisters using rust: https://github.com/dfinity-lab/dev/pull/1114. :-)

[hansl]

@chenyan-dfinity I don't have to explicitly support enums in my use case, but I thought serialize_unit_variant would cover that case.

[hansl]

We should not merge this PR (and should not have merged the preceding one) without cargo fmt and cargo clippy passing. We also need CI.

[chenyan-dfinity]

@chenyan-dfinity I don't have to explicitly support enums in my use case, but I thought serialize_unit_variant would cover that case.

In enum, you can only get the type of the current variant, and the index cannot be sorted beforehand either. Let's consider a simpler example: serialize None of type Option<i32>. In serde, you get dispatched to serialize_none, which takes no arguments. How do you know this None is of type Option<i32>?

[hansl]

How do you know this None is of type Option?

Serde's own implementation says that you know this during deserialization (here). I haven't faced a wire format that made a difference between None of i32 and None of str.

[chenyan-dfinity]

I haven't faced a wire format that made a difference between None of i32 and None of str.

hmm, this is a good point. If the Some variant never appears, we can safely assign it as an arbitrary Option<T> type. Not sure if this will cause problems in the deserialization. Let me finish this "precise" type approach first, hopefully we will have a more clear picture by then.

[paulyoung]

The doc comment for the deserialize_option function at the link above says this though:

// As commented in `Serializer` implementation, this is a lossy
// representation. For example the values `Some(())` and `None` both
// serialize as just `null`. Unfortunately this is typically what people
// expect when working with JSON. Other formats are encouraged to behave
// more intelligently if possible.

[hansl]

You can get the type when deserializing Some(), you just can't get it from None. ⊥ has no type, so its serialization probably shouldn't have any type.

JSON is very limited. For example, there is no difference between f32, u8 or i64, or Some(()) and None. But serde itself do support differentiating those two, so it's a limit of the wire format, not the serializer (you could serialize to JSON with something like { type: "()", value: {} } and deserialize properly).

[hansl]

Please merge master when you have time to. There has been a lot of changes with CI and Rust.

[matthewhammer]

Not sure what's the best way to return multiple values. I plan to return them in a tuple. Any other suggestions?

Yeah, that seems right: Use a tuple.

BTW: I'm looking at this PR now, and will try it shortly myself.

[hansl]

@chenyan-dfinity Is this ready for review?

1. Could you separate this in commits that make sense? Seems you're changing a lot of code across libraries. I know this is not "normal" practice at DFINITY but it makes the review faster, easier, and the intent and documentation clearer (commit messages are documentation too)
2. The TODO in the description of this PR is still empty. What's done and what's still to finish?
3. The PR is in Draft mode. If this is ready to review and merge then please open up the PR. It's a simple button at the bottom.

I'm looking at the code right now but these would help.

[hansl]

Initial comments.

[matthewhammer]

Update: I want to understand all of this code eventually, but admittedly, I'm currently just trying to understand how to use it for now.

On that front, I was able to write some simple unit tests that exercise both the serialization and deserialization support, and check that in each case they agree. Those tests all work as expected.

[hansl]

Can we add a fuzzy test for this? I feel it is something we should do; generate random structured and typed data, serialize and deserialize it, then verify it's equal to original data.

I'm okay making this a separate PR.

[chenyan-dfinity]

@chenyan-dfinity Is this ready for review?

Ready for early feedback and use for CLI, not ready for merging to master

1. Could you separate this in commits that make sense? Seems you're changing a lot of code across libraries. I know this is not "normal" practice at DFINITY but it makes the review faster, easier, and the intent and documentation clearer (commit messages are documentation too)

I will address the TODOs before split the PR.

2. The TODO in the description of this PR is still empty. What's done and what's still to finish?

TODOs listed in the PR description is not done.

[chenyan-dfinity]

Can we add a fuzzy test for this? I feel it is something we should do; generate random structured and typed data, serialize and deserialize it, then verify it's equal to original data.

Of course! Not this PR though. Let's do it one at a time :)

[hansl]

@chenyan-dfinity Please fix CI's issues and we'll merge. Looks good to me.

[chenyan-dfinity]

CI fixed. PTAL

[matthewhammer]

👍