deku icon indicating copy to clipboard operation
deku copied to clipboard

RFC/feat: Add `ctx_nested` attribute to top level

Open sharksforarms opened this issue 3 years ago • 7 comments

Closes: https://github.com/sharksforarms/deku/issues/217

This allows clients to pass context arguments down to all fields.

Example

use deku::prelude::*;

struct MyCtx {}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
#[deku(ctx = "ctx: MyCtx", ctx_nested = "ctx")]
pub struct Child {
    data: u32,
}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
#[deku(ctx = "ctx: MyCtx", ctx_nested = "ctx")]
pub struct Parent {
    child: Child,
}

impl<'a> DekuRead<'a, MyCtx> for u32 {
    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,
    {
        // Custom u32 reader
        todo!()
    }
}

impl DekuWrite<MyCtx> for u32 {
    fn write(
        &self,
        output: &mut deku::bitvec::BitVec<deku::bitvec::Msb0, u8>,
        ctx: MyCtx,
    ) -> Result<(), DekuError> {
        // Custom u32 writer
        todo!()
    }
}

sharksforarms avatar Jun 22 '21 13:06 sharksforarms

@RReverser @v1gnesh @constfold @wcampbell0x2a We can continue the discussion here. Would a ctx_nested attribute be sufficient?

sharksforarms avatar Jun 22 '21 13:06 sharksforarms

Apologies in advance if what I'm about to say makes little sense. I'm a pretty basic programmer... If you have time, can you please help me understand how I can use this in the context of this comment - https://github.com/sharksforarms/deku/issues/213#issuecomment-841948929

So if I have a struct like this:

#[deku(endian="little")]
pub struct H {
  a: u16,
  #[deku(map="morph", bytes_read="4")]
  b: String,
  #[deku(map="morph", bytes_read="4")]
  c: String,
  #[deku(map="morph", bytes_read="4")]
  d: String,
  #[deku(map="morph", bytes_read="64")]
  e: String,
}

I can now do this as:

struct MyCtx {
  boits: [4, 4, 4, 64]
  funcs: ["morph", "morph", "morph", "morph"]
}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
#[deku(ctx = "ctx: MyCtx", ctx_nested = "ctx")]
#[deku(endian="little")]
pub struct H {
  a: u16,
  b: String,
  c: String,
  d: String,
  e: String,
}

and

impl<'a> DekuRead<'a, MyCtx> for String {
    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,
    {
        // Custom String reader
        // interate/zip through ctx.boits and ctx.funcs and apply that to the `input`
        // ... what would the above impl look like?
    }
}

v1gnesh avatar Jun 22 '21 14:06 v1gnesh

@RReverser @v1gnesh @constfold @wcampbell0x2a We can continue the discussion here. Would a ctx_nested attribute be sufficient?

Yeah looks exactly like what I had in mind in https://github.com/sharksforarms/deku/issues/225#issuecomment-864578411:

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

Seems sufficient for defining custom type structure. I wonder though, how does it interact with other ctx? Is it possible to still add extra ctx on fields and pass them down or would they conflict with ctx_nested on the struct itself?

RReverser avatar Jun 22 '21 15:06 RReverser

Any thoughts on #225 (comment) ?

I don't like this idea at my first feeling. Changing the beheviour of build-in types is somewhat strange for me. But it is indeed a solution. If I must choose one between this and the as/with, I will vote for the latter one.

Does an attribute named ctx_nested make sense?

Maybe? Maybe ctx_for_all?

constfold avatar Jun 22 '21 15:06 constfold

If you have time, can you please help me understand how I can use this in the context of this comment - #213 (comment)

This PR woudln't address this usecase. The ctx_nested will expand and add each variable in ctx_nested to ctx for every field.

I wonder though, how does it interact with other ctx? Is it possible to still add extra ctx on fields and pass them down or would they conflict with ctx_nested on the struct itself?

This will not interfere with existing ctx or other attributes such as endian, but there are some "gotchas" .... for example, if using a foreign type:

use deku::{ctx::Endian, prelude::*};

#[derive(Copy, Clone)]
struct MyCtx {}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
#[deku(
    ctx = "endian: Endian, ctx: MyCtx",
    ctx_nested = "ctx",
    endian = "endian", // endian works with the context system
)]
pub struct Child {
    data: u32,
}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
#[deku(ctx = "ctx: MyCtx", ctx_nested = "ctx")]
pub struct Parent {
    #[deku(endian = "big")]
    child: Child,
}

// ERROR, Endian is foreign
impl<'a> DekuRead<'a, (Endian, MyCtx)> for u32 {
    fn read(
        input: &'a deku::bitvec::BitSlice<deku::bitvec::Msb0, u8>,
        (endian, ctx): (Endian, MyCtx),
    ) -> Result<(&'a deku::bitvec::BitSlice<deku::bitvec::Msb0, u8>, Self), DekuError>
    where
        Self: Sized,
    {
        // Custom u32 reader
        todo!()
    }
}

/// ...


error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
  --> examples/overide_defaults.rs:24:1
   |
24 | impl<'a> DekuRead<'a, (Endian, MyCtx)> for u32 {
   | ^^^^^^^^^-----------------------------^^^^^---
   | |        |                                 |
   | |        |                                 `u32` is not defined in the current crate
   | |        this is not defined in the current crate because tuples are always foreign
   | impl doesn't use only types from inside the current crate
   |
   = note: define and implement a trait or new type instead

But this is is ok because it doesn't affect the trait impl:

use deku::{ctx::Endian, prelude::*};

#[derive(Copy, Clone)]
struct MyCtx {}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
#[deku(
    ctx = "field: u8, ctx: MyCtx",
    ctx_nested = "ctx",
)]
pub struct Child {
    data: u32,
}

#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
#[deku(ctx = "ctx: MyCtx", ctx_nested = "ctx")]
pub struct Parent {
    field: u8,
    #[deku(ctx = "*field")]
    child: Child,
}

impl<'a> DekuRead<'a, MyCtx> for u32 {
    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,
    {
        // Custom u32 reader
        todo!()
    }
}

sharksforarms avatar Jun 22 '21 15:06 sharksforarms

https://github.com/sharksforarms/deku/issues/225#issuecomment-864477322 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?

@RReverser Is there a reason why you wouldn't want to abstract this into a separate type?

struct Leb128 {
    // ...
}

impl DekuRead for Leb128 { ... }
impl DekuWrite for Leb128 { ... }

#[derive(DekuRead, DekuWrite)]
struct ParseMe {
    field1: Leb128,
    // ...
}

sharksforarms avatar Jun 22 '21 16:06 sharksforarms

@RReverser Is there a reason why you wouldn't want to abstract this into a separate type?

Leb128 can be used for variety of types (u8, u16, u32, usize, i8, ...) and while they all share same encoding method, they still need to be reflected as distinct types in the final struct.

Similar happens for f32 vs f64, where encoding in both cases is just "a set of bytes in little-endian order" but now both types need to be replaced with wrappers too.

For me, the whole point of using a custom derive (whether the one I use in wasmbin or the one deku provides) is being able to use "natural" types in the final representation.

Thanks for the examples, I think this ctx_nested solution reduces the boilerplate significantly for those cases!

RReverser avatar Jun 22 '21 22:06 RReverser