sharksforarms / deku

Declarative binary reading and writing: bit-level, symmetric, serialization/deserialization
Apache License 2.0
1.05k stars 54 forks source link

update attribute not working on internal struct? #369

Open yanshay opened 8 months ago

yanshay commented 8 months ago

I'm trying to use update, and in my tests it doesn't run on internal structs, it works only on top level. Is there a way to get it working also on internal structs? I can manually run it on the internal field but it means I need to know the implementation details while I'd like all updates to propagate through the container structure.

Here is my test code:

use deku::prelude::*;
use std::convert::TryInto;

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
struct IpAddr {
    #[deku(update = "9")]
    a: u8,
    b: u8,
    c: u8,
    d: u8,
}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
struct Container {
    ip_addr: IpAddr,
}

pub fn main() {
    let mut onlyip = IpAddr {
        a: 1,
        b: 2,
        c: 3,
        d: 4,
    };
    onlyip.update().unwrap();
    let onlyip_buf: Vec<u8> = onlyip.try_into().unwrap();
    println!("{:?}", onlyip_buf);

    let mut container = Container {
        ip_addr: IpAddr {
            a: 1,
            b: 2,
            c: 3,
            d: 4,
        },
    };
    container.update().unwrap();
    let container_buf: Vec<u8> = container.try_into().unwrap();
    println!("{:?}", container_buf);
}

This is the output it generated:

[9, 2, 3, 4]
[1, 2, 3, 4]

As seen, the update doesn't run in the second case

wcampbell0x2a commented 8 months ago

Recursive update seems like a good thing to add. Anyway, here is the "requires Clone + Copy and is really just a hack" code you can use:

#[derive(Debug, PartialEq, DekuRead, DekuWrite, Clone, Copy)]
struct IpAddr {
    #[deku(update = "9")]
    a: u8,
    b: u8,
    c: u8,
    d: u8,
}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
struct Container {
    #[deku(update = "{ let mut a = self.ip_addr; a.update()?; a }")]
    ip_addr: IpAddr,
}
yanshay commented 8 months ago

This works for this case, however I also have a Vec in the relevant structure, and for this to work it is required for IpAddr implement Clone and Copy, but Vec doesn't implement Copy, so it can't work. Is there a way to bypass it?

What I did was to simply implement my own update function on Container that calls the IpAddr update. Not sure why the update through Deku requires IpAddr to implement Copy when my own function doesn't (maybe because it accepts &mut self)

wcampbell0x2a commented 8 months ago

deku emits the following for updating, for my example:

impl DekuUpdate for Container {
    fn update(&mut self) -> core::result::Result<(), ::deku::DekuError> {
        use core::convert::TryInto;
        self
            .ip_addr = ({
            let mut a = self.ip_addr;
            a.update()?;
            a
        })
            .try_into()?;
        Ok(())
    }
}

There isn't a reason that deku couldn't in the future support emitting this:

impl DekuUpdate for Container {
    fn update(&mut self) -> core::result::Result<(), ::deku::DekuError> {
        self.ip_addr.update()?;
        Ok(())
    }
}

Maybe allow an empty update for this behavior? I'll admit I don't use the update attribute much.

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
struct Container {
    #[deku(update)]
    ip_addr: IpAddr,
}