simd-lite / simd-json-derive

high performance Serialize and Deserialize derives
Apache License 2.0
30 stars 7 forks source link

json_string and embedded struct #53

Open v1gnesh opened 11 months ago

v1gnesh commented 11 months ago

bitfield is supposed to return the parsed struct. Instead it just returns input array (bytes). (fn from_bytes([u8; 1]) -> Self) Ref: https://docs.rs/modular-bitfield/latest/modular_bitfield/#generated-implementations

To reproduce:

#![allow(dead_code)]

use std::io::Cursor;
use binrw::BinRead;
use modular_bitfield::prelude::*;
use simd_json_derive::Serialize;

#[bitfield]
#[derive(Debug, BinRead, Serialize)]
#[br(map = Self::from_bytes)]
pub struct Flags {
    r1: B3,
    v1: B1,
    v2: B1,
    v3: B1,
    v4: B1,
    r2: B1,
}

#[derive(Debug, BinRead, Serialize)]
#[br(big)]
pub struct Outside {
    y0: u16,
    y1: u16,
    f1: Flags,
}

fn main() {

    let rec = vec![0x01, 0xB0, 0x00, 0x00, 0x1e];

    let value = Outside::read(&mut Cursor::new(rec)).unwrap();

    println!("\n{}", value.json_string().unwrap());
    // println!("\n{:?}", value);
}
println!("\n{:?}", value);

Outside { y0: 432, y1: 0, f1: Flags { r1: 6, v1: 1, v2: 1, v3: 0, v4: 0, r2: 0 } }
println!("\n{}", value.json_string().unwrap());

{"y0":432,"y1":0,"f1":{"bytes":[30]}}

So it looks like json_string is acting before binrw can derive those bytes. When trying to manually impl Serialize for Flags, self. only shows self.bytes. This explains why json_string just dumps the bytes as is.

Any idea how to make this right?

Somehow, Serialize should run after binrw's map.

But placing it like this (innocently) doesn't help:

#[br(map = Self::from_bytes)]
#[derive(Serialize)]
Licenser commented 11 months ago

I think that's something the binrw people have to answer, since if the struct only shows bytes there is nothing the derive can do about that.

v1gnesh commented 11 months ago

It's not supposed to (show only bytes) though... because the value struct parses out ok. It's only when invoking value.json_string() that this happens.

v1gnesh commented 11 months ago

I've just run cargo-expand on it, and see BinRead impl show up first, then simd-json-derive, then bitfield. I don't know if the order of these is relevant... but if so, it explains why sjd doesn't see the struct as parsed by bitfield. After these 3, the impl for fmt::Display shows up... if the ordering is important, this also explains why {:?} printing the struct shows up correctly.

v1gnesh commented 11 months ago

Can I collect all structs and then manually serialize them into JSON? Doing this as the last step should help avoid this out-of-order macro expansion/execution.

Licenser commented 11 months ago

you could try changing the order of the macros, I think that determines what gets executed first, so if br comes before derive it might act the way you expect

v1gnesh commented 11 months ago

I tried this... it doesn't :(

hecatia-elegua commented 11 months ago

Macro expansion starts with the top-most attribute, yes. What happens to the outputs and cargo expand with the below? (also try putting Debug below #[bitfield])

#[derive(Debug, BinRead, Serialize)]
#[br(map = Self::from_bytes)]
#[bitfield]
pub struct Flags { ... }

bitfield converts the struct into Flags { bytes: [u8; 1] } and overwrites the derive(Debug) with their own implementation using their own field accessors, since you can't access the fields of Flags directly if they don't exist. So {:?} will show you the fields, but they aren't there for real, it's just a byte array.

If you add #[bitfield] to struct Outside for a moment, the json_string() thing will also just return {"bytes":[...]}. It's basically compressed. If you really want these fields, uncompressed, in json, it would need Serialize support for modular_bitfield. Well or in bilge you or we could add a SerializeBits macro.

v1gnesh commented 11 months ago

Funny story, I am indeed using a custom Serialize impl like this but for the bitfield.

Although the fields aren't available, the functions to get to them were (b1(), b2(), etc.); so I just called them within the custom Serialize impl. The ordering of the bits is (in my case) the other way around though.

Licenser commented 11 months ago

json objects aren't sorted so order is probably not important

As to an implementation, I feel like it would have a better home in the bitfield crate. Perhaps they're open to a PR to implement it in the bitfield crate. The reasoning is simple there are way fewer crates doing serialization then there are data structure crates. So if the serialization creates implements all the serializers then they quickly become unwieldy and large with a ton of dependencies and feature flags on the other hand if data structure crates implement the handful of serializer, it's not too much of a addition to each crate.

v1gnesh commented 11 months ago

Absolutely, no worries. I was pointing @hecatia-elegua to this issue for the example of how I'm using bitfields, nothing more :)