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

RFC/feat: Add ctx_nested attribute to top level #229

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sharksforarms
Copy link
Owner

@sharksforarms sharksforarms commented Jun 22, 2021

Closes: #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!()
    }
}

This allows clients to pass context arguments down to all fields
@sharksforarms sharksforarms marked this pull request as draft June 22, 2021 13:21
@sharksforarms sharksforarms changed the title feat: Add ctx_nested attribute to top level RFC/feat: Add ctx_nested attribute to top level Jun 22, 2021
@sharksforarms
Copy link
Owner Author

sharksforarms commented Jun 22, 2021

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

bits.as_ref(),
bytes.as_ref(),
ctx.as_ref(),
ctx_nested.as_ref(),
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be first?

impl DekuWrite<Endian, Size, MyCtx> for u32 {
vs
impl DekuWrite<MyCtx, Endian, Size> for u32 {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like MyCtx being first, since it's "non-default" deku usage and something you want to see first in impl error.

@v1gnesh
Copy link

v1gnesh commented Jun 22, 2021

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 -
#213 (comment)

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?
    }
}

@RReverser
Copy link

@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 #225 (comment):

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?

@constfold
Copy link
Contributor

constfold commented Jun 22, 2021

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?

@sharksforarms
Copy link
Owner Author

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

#225 (comment) 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,
    // ...
}

@RReverser
Copy link

RReverser commented Jun 22, 2021

@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!

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

Successfully merging this pull request may close these issues.

Request for Enhancement: Project-level Deku settings
5 participants