Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

json_string and embedded struct #53

Open
v1gnesh opened this issue Jul 17, 2023 · 10 comments
Open

json_string and embedded struct #53

v1gnesh opened this issue Jul 17, 2023 · 10 comments

Comments

@v1gnesh
Copy link

v1gnesh commented Jul 17, 2023

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
Copy link
Member

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
Copy link
Author

v1gnesh commented Jul 17, 2023

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
Copy link
Author

v1gnesh commented Jul 17, 2023

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
Copy link
Author

v1gnesh commented Jul 17, 2023

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
Copy link
Member

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
Copy link
Author

v1gnesh commented Jul 18, 2023

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

@hecatia-elegua
Copy link

hecatia-elegua commented Jul 26, 2023

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
Copy link
Author

v1gnesh commented Jul 26, 2023

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
Copy link
Member

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
Copy link
Author

v1gnesh commented Jul 26, 2023

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

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

No branches or pull requests

3 participants