WIP: conditionally read Option<T>

[vhdirk]

So, I really like the readability of bin-proto's internals. But feature-wise, it's not where I'd need it to be to replace deku.

For one, I'd like the ability to conditionally read an Option, based on an external field. Kinda like externally length prefixed, but for Option.

I started making it, but I quickly realized that, in order for it to be compatible with other attributes, like bits, it start's getting somewhat complicated. And from what I can tell, with the current infrastructure, I'd need a trait Conditional and ConditionalWithBitfield or something alike? That feels somewhat clunky.

I guess it can be easily solved using the new Ctx system, but I'd like for it to be a first-class citizen.

What's your opinion on this?

[wojciech-graj]

First of all, I'd like to say that this would be a good addition, so thanks for taking the time to work on it!
I would argue that making it a first-class citizen (i.e. a new trait) doesn't really make much sense. The ExternallyLengthPrefixed and BitField traits exist because their read/write functions take additional context, but in the case of a conditional, the decision to codec or not to codec is made beforehand, and is not part of that value's codec process - if we decide to conditionally skip it, we simply don't call the read/write function. My preferred approach would be to, within the codegen, wrap the entire read/write of that value in an if statement with arbitrary conditions from attribute macros, similar to serde's skip_serializing_if, and some sort of default_if for decoding.
The only issue with this approach is that, if the condition is true, the Option would still require a set bit/byte at the start, since that's how impl Protocol for Option is written. I'm not sure about this, so feel free to pitch any ideas, but maybe we could impl ExternallyLengthPrefixed for Option as well, and do something like this:

#[protocol(default_if = "...")]
#[protocol(skip_if = "self.optional.is_none()")]  // This would actually be redundant in this scenario, but I'm leaving it for clarity
#[protocol(length = "1")]
optional: Option<T>

[vhdirk]

I'm not sure I like the idea of implementing ExternallyLengthPrefixed for Option, that seems like a rather weird thing to do. Unless it is renamed to something more universal?
The basic idea is indeed pretty much the same. Rather than having the boolean flag for the Option just before it, it should be possible to relocate that field, or base it on an expression.
The same would also be true for enum discriminants. The protocol that I'm implementing makes heavy use of placing these at other locations. Possibly even in parent structs.

So, just thinking out loud here:
What if BitField and ExternallyLengthPrefixed could (somehow) instead pass the required values in Ctx, rather than needing custom read/write functions? Although that again doesn't solve enum discrimintants being relocated, but I'm not quite sure if we can be generic over that at all.

I'm going to read some more into Deku to see how they solve it. Deku is nice, but its api is really rather complex once you start doing stuff outside its scope.

[wojciech-graj]

I'm not sold on using Ctx for any of the types that bin-proto traits are impl'd on by the library, as it would require a user to add additional fields or traits to their custom context structs, and it doesn't really translate cleanly to enums.

However, I see a lot of potential in using ExternallyLengthPrefixed but making it more universal with a different name. What if, like how when impl'ing TryFrom you have to specify an error type, we specify the type of the length prefix, so we could make the prefix a usize for Vec, and a bool for Option. There's also the option of making the length prefix type a generic on ExternallyLengthPrefixed, but that might just complicate the codegen without actually providing any additional value - I'm not sure.

I think we could apply a very similar solution to enums, by allowing either Protocol or ExternallyLengthPrefixed to be derived for an enum depending on the user's needs. Or maybe always derive both for enums, with the Protocol implementation simply reading/writing the discriminant then calling ExternallyLengthPrefixed's read/write.

The length prefixes were already getting quite clunky, so I was thinking working on an attribute macro that prepends the length prefix to the field without requiring a length field to be explicitly present, so if you wanted to keep using the Option in the same way that you currently do, you could, but you also have the option of having it be external. But, we could maybe take a similar approach to my suggestion for enums, where we impl Protocol for types like Vec, Option, etc. using ExternallyLengthPrefixed internally.

[vhdirk]

That does seem like a clean solution, indeed!

[wojciech-graj]

This seems like it will be quite a big change, so are you willing to implement it?

[vhdirk]

I'll see what I can do, but I can't guarantee anything; after looking what I use from deku, I realize that I'd need a lot more than this. So I may just stay with deku :/

[wojciech-graj]

Alright, just let me know if you end up not implementing this, as it would be quite a valuable addition, so I can add it instead. Good luck!

[wojciech-graj]

Hey @vhdirk, I had a free day today so I decided to implement this more generic tagging. You can now do the following to have a tag with any type prepended directly to your type:

#[protocol(tag(type = "u16", write_value = "self.prefixed_arr.len() as u16"))]
prefixed_arr: Vec<u8>,

If you want it separately, you can do so too:

#[protocol(write_value = "self.opt.is_some()")]
flag: bool,
#[protocol(tag = "flag")]
opt: Option<u32>,

And this applies for enums too.

You said you would need a bunch of other functionality from deku as well to switch, but I've run out of ideas, so I'd love to hear what else you'd want to see.

[vhdirk]

Hi. Thanks for your hard work!

I'm regarding this PR as closed then, and opening a new one :)