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

#[deku(as = "")]/#[deku(with = "")] attributes #225

Open
coolreader18 opened this issue Jun 12, 2021 · 6 comments · May be fixed by #226
Open

#[deku(as = "")]/#[deku(with = "")] attributes #225

coolreader18 opened this issue Jun 12, 2021 · 6 comments · May be fixed by #226
Assignees
Labels
enhancement New feature or request

Comments

@coolreader18
Copy link

coolreader18 commented Jun 12, 2021

Sort-of related to #174, but more general.

I'm looking into converting RustPython's bytecode format to use deku over bincode (RustPython/RustPython#2362 (comment)), and I'm running into a few issues cause I want to keep the DekuRead/Write structs roughly as they are, i.e. ergonomic to use from Rust. For example, not adding separate length fields for deserializing vectors, and also I'd like to use LEB128 to represent most integers as our existing bincode impl does. I understand why those aren't defaults in deku, but it'd be a lot easier to customize deku to meet our needs if there was something like serde's with attribute, or even serde_with's as attribute. Would you accept a PR that adds Deku{Read/Write}As traits and an as attribute to the derive macros so that individual field [de]serializing can be customized? I might just base it off serde_with's design, since I think that seems to work well.

@constfold
Copy link
Contributor

I'd like to use LEB128 to represent most integers as our existing bincode impl does

Same when writing the luajit bytecode parser. I'd appreciate if we have the as/with mechanism in deku.

@coolreader18 coolreader18 linked a pull request Jun 13, 2021 that will close this issue
@sharksforarms sharksforarms added the enhancement New feature or request label Jun 15, 2021
@RReverser
Copy link

RReverser commented Jun 19, 2021

I'm doing something very similar to what deku does in wasmbin - self-generating parser/serializer for Wasm - where I'm using custom derive implementation. Example: https://github.com/GoogleChromeLabs/wasmbin/blob/3508d13398c59a337d42eaa8a49f9f02434fee9d/src/sections.rs#L197-L237

I was thinking of what would it take to try and reimplement it on top of deku instead, and I think lack of as/with or some other mechanism for defining own implementations for built-in types would be the biggest hurdle.

I think I'd like something even more general though - ideally I'd want to be able to define custom parsers for built-in types (e.g. LEB128 for integers, custom parser for floats, etc.) somewhere in a single place, and then somehow tell Deku to use those implementations for parsing, rather than putting as/with on each single field. I don't have a good idea in mind for how this would work though... Maybe some sort of context object to pass around?

UPD: I see DekuRead and DekuWrite accept Ctx as 2nd type param, perhaps I can build off that...

@v1gnesh
Copy link

v1gnesh commented Jun 20, 2021

I think I'd like something even more general though - ideally I'd want to be able to define custom parsers for built-in types (e.g. LEB128 for integers, custom parser for floats, etc.) somewhere in a single place, and then somehow tell Deku to use those implementations for parsing, rather than putting as/with on each single field. I don't have a good idea in mind for how this would work though... Maybe some sort of context object to pass around?

Something like this, perhaps?
The suggestion there was to build one's own proc macro lib that'd do it's thing...

EDIT: Can you show an example please of what you have in mind when you say you can get by with ctx for the above requirement?

@RReverser
Copy link

EDIT: Can you show an example please of what you have in mind when you say you can get by with ctx for the above requirement?

My idea was to use a custom marker to implement custom parsing for built-in types. Thanks to DekuRead / DekuWrite accepting Ctx, this is allowed under Rust trait implementation rules:

struct MyCtx;

impl DekuRead<'_, MyCtx> for u8 {
    fn read(
        input: &'a deku::bitvec::BitSlice<deku::bitvec::Msb0, u8>,
        ctx: MyCtx,
    ) -> Result<(&'a deku::bitvec::BitSlice<deku::bitvec::Msb0, u8>, Self), DekuError>
    where
        Self: Sized {
        todo!()
    }
}

impl DekuRead<'_, MyCtx> for u16 { ... }

impl DekuRead<'_, MyCtx> for u32 { ... }

Then, I can derive DekuRead on all my structures using this context like

#[derive(DekuRead)]
#[deku(ctx = "_: MyCtx")]
struct MyStruct {
    a: u8,
    b: u16,
    y: u32,
}

This ensures that my structures get impl DekuRead<'_, MyCtx> instead of regular impl DekuRead<'_, ()>.

At this point the only missing bit is making sure that the same context is passed to all fields, so that entire type tree uses same context type consistently. Unfortunately, at this step I've realised that currently the only way to do so is to specify it on every field:

#[derive(DekuRead)]
#[deku(ctx = "ctx: MyCtx")]
struct BBB {
    #[deku(ctx = "ctx")]
    a: u8,
    #[deku(ctx = "ctx")]
    b: u16,
    #[deku(ctx = "ctx")]
    y: u32,
}

So, while this helps with boilerplate for types, we're back to square one in terms of specifying something for all fields and probably need a custom macro like suggested in #217...

It's not very complicated to write one, but I wonder if it would make sense to add ctx_nested to Deku itself.

@sharksforarms
Copy link
Owner

@RReverser @v1gnesh these are interesting suggestions, I never really thought about an override for built-in types. I like the solution above but it is verbose and kinda a hack way around it. I think we can do better. I'll give it some thought and experimentation this week.

@v1gnesh
Copy link

v1gnesh commented Jun 21, 2021

That's great to hear, thank you!
Please conisder scenarios from both #217 and #213, i.e.,

  • .conf file to put project-wide settings
  • struct/enum-wide setting, but filterable (to specific types or field names perhaps)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants