owengage / fastnbt

Fast serde serializer and deserializer for Minecraft's NBT and Anvil formats
MIT License
186 stars 34 forks source link

Value type should be reversible #28

Closed owengage closed 3 years ago

owengage commented 3 years ago

Problem

fastnbt currently has the Value type which is meant to capture any value stored in NBT. This works fine, but it is lossy.

If you deserialize an IntArray, the serde data model that you end up with is a seq of i32. This will happily get stored in something like a Vec<i32> or a Vec<i64>. If in future fastnbt wants to support serialization, we want to be able to 'round trip' Values, meaning that you can take some NBT data, deserialize it into a Value and then serialize it back to the original data. If we take the IntArray example, we do not know at serialization time if the NBT should be a NBT List of i32, or an IntArray.

This means the current Value type cannot become round-trip in future without breaking changes. It would be ideal to make these changes before minting fastnbt 1.0.

Potential solution

The advanced serde issue mentions TOML's datetime type, which maps into the serde data model.

We could take a similar approach fasnbt. This requires changes to the deserializer itself, in that when it encounters these array types, it encodes the needed extra information in the serde data model. For example, in pseudo-notation the model could look like

map{
    tag: IntArray,
    data: seq<i32>
}

for an IntArray.

This has the unfortunate side-effect that anyone writing their own types to deserialize will have to create structures that match this, or use ones we provide. In most formats serde supports you would expect to simply have to provide a normal Vec for this to work.

Notes

owengage commented 3 years ago

master now has this new value type. Need thorough tests for each variant to check they always deserialize to the exact type. In particular I want to check f32 and f64.