feat: Add Ctx to SchemaRead

[cpubot]

Problem

With the removal of buffering methods on Reader, like fill_buf, peak, etc, certain "non-standard" SchemaRead implementations become very cumbersome to implement, or force the implementer to re-implement basic wincode functionality.

In particular, some implementations may require inspecting some set of bytes before choosing a deserialization path. Without the aforementioned buffer-style Reader methods, this is effectively impossible with the current set of wincode built-ins, unless the user re-implement core deserialization primitives themselves.

Here is one such example:
https://github.com/anza-xyz/agave/blob/fb34650653dbd84de5edddb46d7f8303cacac0d3/entry/src/block_component.rs#L519-L543

unsafe impl<'de, C: Config> SchemaRead<'de, C> for BlockComponent {
    type Dst = Self;

    fn read(mut reader: impl Reader<'de>, dst: &mut MaybeUninit<Self::Dst>) -> ReadResult<()> {
        // Read the entry count (first 8 bytes) to determine variant
        let count_bytes = reader.fill_array::<8>()?;
        let entry_count = u64::from_le_bytes(*count_bytes);

        if entry_count == 0 {
            // This is a BlockMarker - consume the count bytes and read the marker
            // SAFETY: fill_array::<8>() above guarantees at least 8 bytes are available
            unsafe { reader.consume_unchecked(8) };
            dst.write(Self::BlockMarker(<VersionedBlockMarker as SchemaRead<
                C,
            >>::get(reader)?));
        } else {
            let entries: Vec<Entry> =
                <WincodeVec<Entry, MaxDataShredsLen> as SchemaRead<'de, C>>::get(reader)?;

            if entries.len() >= Self::MAX_ENTRIES {
                return Err(wincode::ReadError::Custom("Too many entries"));
            }

            dst.write(Self::EntryBatch(entries));
        }

        Ok(())
    }
}

In this example, the first 8 bytes are effectively used as an ad-hoc discriminant. If they are 0u64, those bytes should be discarded and the function should proceed to deserialize a VersionedBlockMarker. Otherwise, those bytes represent the length of a Vec<Entry>. If the user were to use take_array::<8>(), the reader will advance by 8 bytes, and the subsequent deserialization of Vec<Entry> will fail or be incorrect due to a missing length. In this example, the only remediation is to re-implement Vec's deserialization logic at the call site.

Here is another example:
https://github.com/anza-xyz/solana-sdk/blob/556aa4442495047b8222cf4c921e367156d3a59d/transaction/src/versioned/mod.rs#L304-L329

let discriminator = reader.peek()?;
let mut builder = VersionedTransactionUninitBuilder::<C>::from_maybe_uninit_mut(dst);

if discriminator & MESSAGE_VERSION_PREFIX == 0 {
    // Legacy or V0 transaction

    builder.read_signatures(&mut reader)?;
    builder.read_message(reader)?;
    // ...
} else if *discriminator == V1_PREFIX {
    // V1 transaction

    builder.read_message(&mut reader)?;
    // ...
}

Here, the first byte is used as a discriminant to choose the appropriate execution path. That first byte is used as input in both paths. Similar to the previous example, if the user is forced to use take_byte(), they need to manually implement parts of the deserialization logic.

One (possible) solution

If we want to maintain the new, trimmed down, Reader API proposed in #213, one possible solution is a new trait, SchemaReadState. The API is identical to SchemaRead, but importantly, takes a &mut self receiver. This allows pre-populating some deserialization state before proceeding with deserialization. This abstraction generalizes to use-cases outside of just Vec, but the downside is that it does require more manual implementation work for the implementer.

The idea

The basic idea is to support deserialization use-cases where certain parts of the deserialization state is already read in an earlier phase of deserialization.

Consider the following type, which allows pre-populating a pre-read Vec length.

pub struct VecState<T> {
    len: usize,
    _marker: PhantomData<T>,
}

This is a generalization of the Vec SchemaRead implementation -- its implementation is parameterized by a length, rather than it being an implicit part of Vec's deserialization logic.

unsafe impl<'de, T, C: ConfigCore> SchemaReadState<'de, C> for VecState<T>
where
    T: SchemaRead<'de, C>,
{
    type Dst = vec::Vec<T::Dst>;

    fn read(
        &mut self,
        mut reader: impl Reader<'de>,
        dst: &mut MaybeUninit<Self::Dst>,
    ) -> ReadResult<()> {
        let len = self.len;
        let mut vec: vec::Vec<T::Dst> = vec::Vec::with_capacity(len);
        // ...
    }
}

This allows the Vec implementation to simply call it directly after deserializing the length:

unsafe impl<'de, T, Len, C: ConfigCore> SchemaRead<'de, C> for Vec<T, Len>
where
    Len: SeqLen<C>,
    T: SchemaRead<'de, C>,
{
    type Dst = vec::Vec<T::Dst>;

    #[inline]
    fn read(mut reader: impl Reader<'de>, dst: &mut MaybeUninit<Self::Dst>) -> ReadResult<()> {
        let len = Len::read_prealloc_check::<T::Dst>(reader.by_ref())?;
        let mut v: VecState<T> = VecState::new(len);
        v.read(reader, dst)
    }
}

An interesting possibility

An interesting possibility of this design is a more robust derive system for custom serialization schemes.

A hypothetical example:

#[derive(SchemaRead)]
struct PayloadSchema {
    data_len: u8,
    #[wincode(state_input = "data_len")]
    data: VecState<u8>,
}

where state_input indicates the annotated type is a SchemaReadState whose construction input is the name of the field.

The actual implementation details of this kind of functionality is likely fairly involved or less straightforward than the example code suggests, but interesting to contemplate nonetheless.

The downside

The downsides with this approach are increased API surface area and an increase in complexity for implementers. Rather than being able to have pretty much full control over conditional deserialization logic and payload inspection, implementers are confined to the boundaries of either SchemaRead or SchemaReadState. We likely would want to add *State variations for all collection types for completeness, and potentially other types that I haven't yet considered.

Summary

The problem described is a real problem that we will have to figure out how to deal with immediately if #213 goes in without any supplementing functionality.

I think SchemaReadState is a reasonable solution, but it does have downsides in terms of ergonomics and maintenance burden.

Another possible remediation would be to re-introduce Reader buffer-style methods, but make them optional, similar to what is already proposed with take_scoped. I personally lean this way because of the flexibility it offers for users. SchemaReadState may not work for certain edge cases not yet considered, and it requires a lot of setup / ceremony that the buffering methods don't. But, would like to hear other thoughts / ideas.

[kskalski]

One other solution is try to "give back" the consumed bytes, which is not practical in most implementations, but could be also achieved by chaining readers. Say:

let header = reader.take_array::<8>()?;
if heder_is_foo(&header) {
  variant1(reader);
} else {
  let backtracked = Reader::chain(header, reader);
  variant2(backtracked);
}

This might impose slight perf penalty though.

[cpubot]

One other solution is try to "give back" the consumed bytes, which is not practical in most implementations, but could be also achieved by chaining readers. Say:

let header = reader.take_array::<8>()?;
if heder_is_foo(&header) {
  variant1(reader);
} else {
  let backtracked = Reader::chain(header, reader);
  variant2(backtracked);
}

This might impose slight perf penalty though.

Yeah, this has the unfortunate consequence that the Chain impl will need to introduce branching in each method to check which is the active reader. Likely not a big deal for IO backends, but likely a non-trivial performance regression for the in-memory impls.

I think we either need to further flesh out the design proposed in this PR, revert deprecations on the buffering methods, or come up with another solution that doesn't impose a performance regression.

Also realizing now that serde has a similar feature, DeserializeSeed, so there is precedent for this kind of design. Seed is probably a better name than State that I used here

[kskalski]

One other idea is to just embed the seeded function in existing SchemaRead. It could actually take a typed value or simply raw bytes that were already read, so I'm considering:

trait SchmeaRead {
   type Seed = ();
   fn decode_seeded(seed: Self::Seed, reader: impl Reader) -> ReadResult<Self::Dst> {
       Self::decode(reader)
   }
}

or
simply fn decode_raw_seeded(seed: &[u8], reader: impl Reader) -> ReadResult<Self::Dst>;

[kskalski]

And for the name, I'm not sure if seed is that good, it sounds like something that pre-determines the whole value (like in seeded random generator), and here we are simply dealing with partially read value or more concretely a header. And if we think this will always be a prefix of the encoded bytes that are part of the "seed", then I would prefer to just use "header".

[cpubot]

One other idea is to just embed the seeded function in existing SchemaRead. It could actually take a typed value or simply raw bytes that were already read, so I'm considering:

trait SchmeaRead {
   type Seed;
   fn decode_seeded(seed: Self::Seed, reader: impl Reader) -> ReadResult<Self::Dst>;
}

or simply fn decode_raw_seeded(seed: &[u8], reader: impl Reader) -> ReadResult<Self::Dst>;

I like this direction, but making Seed an associated type means that every implementation has to define it because Rust doesn't support default associated types (breaking). Additionally, it restricts a type to supporting only a single Seed type. One can imagine a hypothetical scenario on a struct, with three fields for example, where one may want to seed the type with one or multiple fields, depending on the context.

We could add an additional generic "context" type parameter to SchemaRead with a default. E.g.,

pub unsafe trait SchemaRead<'de, C: ConfigCore, Ctx = ()> {
    // ...
    fn read_with_context(
        ctx: Ctx,
        reader: impl Reader<'de>,
        dst: &mut MaybeUninit<Self::Dst>,
    ) -> ReadResult<()> {
        Self::read(reader, dst)
    }
}

This has the following advantages:

1. We provide a default context type, (), so the change is non-breaking
2. The default means it is not required to specify
3. Users can provide multiple implementations over different contexts

Points 2 and 3 imply that even if one uses the derive macro (which will maintain the default), one can still provide a specific implementation with whatever context is appropriate for their use-case.

This means we can also provide a Vec implementation like the following without conflicts on the existing implementation:

unsafe impl<'de, T, C: ConfigCore> SchemaRead<'de, C, usize> for Vec<T>

Such that the following is possible:

Vec::get_with_context(usize)

[kskalski]

Having some form of context-enabled (or maybe stateful, with &mut decodes (!)) SchemaRead sounds interesting and might be useful, but I would give it a bit more time to bake / gather more use-cases, since it does have a cost. Adding new generic param to SchemaRead might be better for compatibility and ease of implementing the trait, but I think it increases the burden on calls when one needs to call <Type as SchemaRead<Config, Context>>::get, doesn't it (or we will need to provide twin APIs like we do with wincode vs wincode::config)?

A few more thoughts here:

• in rust memory layout, there is actually a counterpart to what we are discussing here - union can be one of several things and user decides on how to interpret the bytes (what denotes a discriminant) in given context / for given values - in block component we end up constructing enum, but at the point of deserialization we are clearly operating on union BlockComponent { marker: VersionedBlockMarket, entry_batch: Vec<Entry> } - possibly the best way to support that would be to enable deserializing unions with custom-specified logic on deciding variant in a general case - although this just generalizes the problem at hand, not making the solutions easier (we could probably do some more elaborate hacks if this all lived in the library / generated code)
• to solve the concrete block component example it might be enough to only expose a bit more of supporting APIs for how some built-in types are constructed - one such building block is clearly "deserialize collection of T values" - if we could abstract the operation to "populate given object with a series of length len for known T type", apart from solving the issue at hand, it might also make it easier to implement deserializing custom collections (nice win, since all collections in bincode format serialize in the same way)
• the header-reuse pattern in BlockComponent is specific enough that I'd hesitate to design a general state/context mechanism around it before we have more examples demanding it (i.e. re-use of len bytes with specific length encoding seem a bit arbitrary - it's easy to imagine it used 16 bytes covering len + first value, etc.) - not sure we really need to make all built-in collections enable providing len externally abstracted in generic API
• we could still go back to some of the "fill_buf" / "peek" APIs, maybe focusing on "peek" style function but for more bytes, e.g. peek_array::<8>() would clearly solve the block component example and though it still brings back lots of complexity for implementing buffered reader, we end up with a compromise similar to the chunking idea (i.e. size of the peeking can be bound to small sizes)

[kskalski]

wincode::schema::containers::Vec is a public, custom type providing wincode-related functionality. Could we for now simply add some public APIs there that will enable deserialzing alloc::Vec with a known len? If we hold on with elevating it to generic API, it can still serve immediate need, right?

[cpubot]

It can solve the Vec case, but doesn't help in the VersionedMessage case. We'll have to perform manual deserialization here

[kskalski]

Looking now at VersionedTransaction -> VersionedMessage case:

• context won't solve the issue of // Legacy or V0 transaction branch - note that you will have 1 byte consumed and signatures is a Vec<Signature>, the partial context of decoding vector in this case doesn't align with len, which needs more bytes. You will need to do manual parsing of len with 1 byte provided and others coming from the reader, and then use the context-aware parsing of Vec
• seems like this case can be solved by keeping peek_byte in the reader's API. This seems totally doable without compromising the buffered reader cleanup gains: 1 byte peek is available / easy to support in any imaginable Reader implementation
• VersionedMessage, unlike signatures vector, luckily do have a clear "context" matching the 1-byte that would be read independently (== 1 bytes denominator), but I think in general this example (with signatures in variant of transaction) highlights that flexible parsing relies on raw access to bytes at arbitrary (logic dependent) cutoff and not always on parsed units matching specific fields of the decoded struct (as shown in some derive-level proposals), so in practice context will sometimes be raw array of bytes - I guess this is ok, Context is a generic concept, which can be anything, it just makes the derive feature less attractive
• Interestingly in https://github.com/anza-xyz/solana-sdk/blob/556aa4442495047b8222cf4c921e367156d3a59d/message/src/versions/mod.rs#L430 we actually do what you mention as undesirable consequence of removing fill_buf / peek, the first byte is consumed, not peeked and then we indeed do manual deserialization of remaining fields. Possibly that code could be improved by peek_byte + use auto-generated deserialization of messages per-variant (there might be other issues I can't see, the logic is a bit complicated, but.. seems to be working https://github.com/anza-xyz/solana-sdk/pull/616/changes)

[cpubot]

• context won't solve the issue of // Legacy or V0 transaction branch - note that you will have 1 byte consumed and signatures is a Vec<Signature>, the partial context of decoding vector in this case doesn't align with len, which needs more bytes. You will need to do manual parsing of len with 1 byte provided and others coming from the reader, and then use the context-aware parsing of Vec
• seems like this case can be solved by keeping peek_byte in the reader's API. This seems totally doable without compromising the buffered reader cleanup gains: 1 byte peek is available / easy to support in any imaginable Reader implementation

Still solvable with context. We'd just need to add a context variant for ShortU16. But certainly true that requiring a buffered backend makes this simpler as we can just peek.

• VersionedMessage, unlike signatures vector, luckily do have a clear "context" matching the 1-byte that would be read independently (== 1 bytes denominator), but I think in general this example (with signatures in variant of transaction) highlights that flexible parsing relies on raw access to bytes at arbitrary (logic dependent) cutoff and not always on parsed units matching specific fields of the decoded struct (as shown in some derive-level proposals), so in practice context will sometimes be raw array of bytes - I guess this is ok, Context is a generic concept, which can be anything, it just makes the derive feature less attractive
• Interestingly in https://github.com/anza-xyz/solana-sdk/blob/556aa4442495047b8222cf4c921e367156d3a59d/message/src/versions/mod.rs#L430 we actually do what you mention as undesirable consequence of removing fill_buf / peek, the first byte is consumed, not peeked and then we indeed do manual deserialization of remaining fields. Possibly that code could be improved by peek_byte + use auto-generated deserialization of messages per-variant (there might be other issues I can't see, the logic is a bit complicated, but.. seems to be working https://github.com/anza-xyz/solana-sdk/pull/616/changes)

Yep -- all true. This is why I've been an advocate for requiring a buffered backend. There are a number of cases where it's just significantly simpler to be able to inspect bytes before consuming them. The downside with peeking is that you incur extra bounds checks that would otherwise be avoided if you parsed incrementally and leveraged already-read bytes in the form of the proposed context system or manual deserialization (e.g., with the builder in the VersionedMessage you pointed out) -- which in practice should be trivial in terms of performance vs simplicity trade-off. You also lose compatibility with a true streaming reader (e.g., std::io::Read) -- you need buffering, but I'm personally fine with that.

In summary I still think all cases we're discussing are solvable with the context system if we want to strip down the Reader trait as we've been discussing. The trade off is simpler, and more widely implementable Reader implementations and a modest removal of bounds-checks in some cases for much more flexiblity and reduced boilerplate in some cases

[cpubot]

Adding new generic param to SchemaRead might be better for compatibility and ease of implementing the trait, but I think it increases the burden on calls when one needs to call <Type as SchemaRead<Config, Context>>::get, doesn't it (or we will need to provide twin APIs like we do with wincode vs wincode::config)?

It actually wouldn't require changing the call-sites to <Type as SchemaRead<Config, Context>>::get. Ctx has a default value here, so calls like <Type as SchemaRead<Config>>::get can remain exactly as is. The only time passing an explicit Ctx type parameter is when you actually want to deserialize with a context. So, we wouldn't break anyone downstream or require users to necessarily even think about context if they aren't using it. The type-system is able to infer the appropriate context in cases where the destination is typed as well. E.g.,

wincode/wincode/src/schema/containers.rs

Lines 277 to 281 in e1312fa

	fn read(mut reader: impl Reader<'de>, dst: &mut MaybeUninit<Self::Dst>) -> ReadResult<()> {
	let len = Len::read_prealloc_check::<T::Dst>(reader.by_ref())?;
	<vec::Vec<T>>::read_with_context(context::Len(len), reader, dst)?;
	Ok(())
	}

• in rust memory layout, there is actually a counterpart to what we are discussing here - union can be one of several things and user decides on how to interpret the bytes (what denotes a discriminant) in given context / for given values - in block component we end up constructing enum, but at the point of deserialization we are clearly operating on union BlockComponent { marker: VersionedBlockMarket, entry_batch: Vec<Entry> } - possibly the best way to support that would be to enable deserializing unions with custom-specified logic on deciding variant in a general case - although this just generalizes the problem at hand, not making the solutions easier (we could probably do some more elaborate hacks if this all lived in the library / generated code)

I don't think union is appropriate here, unless perhaps I'm misunderstanding how you're thinking about using one here. The two types in question here don't occupy the same number of bytes in memory. In the VersionedBlockMarker case, the first 8 bytes are just totally discarded, whereas in the Vec<Entry> case they're the length of the Vec.

• to solve the concrete block component example it might be enough to only expose a bit more of supporting APIs for how some built-in types are constructed - one such building block is clearly "deserialize collection of T values" - if we could abstract the operation to "populate given object with a series of length len for known T type", apart from solving the issue at hand, it might also make it easier to implement deserializing custom collections (nice win, since all collections in bincode format serialize in the same way)

This is definitely a reasonable stopgap worth considering.

• the header-reuse pattern in BlockComponent is specific enough that I'd hesitate to design a general state/context mechanism around it before we have more examples demanding it (i.e. re-use of len bytes with specific length encoding seem a bit arbitrary - it's easy to imagine it used 16 bytes covering len + first value, etc.) - not sure we really need to make all built-in collections enable providing len externally abstracted in generic API

It actually does generalize to other cases. Here's another example:
https://github.com/anza-xyz/solana-sdk/blob/3275512d3320058db71cb200a92a8db68a4a4b85/message/src/versions/mod.rs#L425-L466

Here, the first byte is either a version discriminant or num_required_signatures of MessageHeader. This is a good use-case for the Ctx system. We have multiple areas in the SDK where being able to deserialize a MessageHeader with an already-read num_required_signatures would be useful. One of the sections mentioned in the PR description is another such case https://github.com/anza-xyz/solana-sdk/blob/556aa4442495047b8222cf4c921e367156d3a59d/transaction/src/versioned/mod.rs#L304-L329.

• we could still go back to some of the "fill_buf" / "peek" APIs, maybe focusing on "peek" style function but for more bytes, e.g. peek_array::<8>() would clearly solve the block component example and though it still brings back lots of complexity for implementing buffered reader, we end up with a compromise similar to the chunking idea (i.e. size of the peeking can be bound to small sizes)

I do think the context system we're discussing here is a good, generalized solution that is minimally invasive. It won't break downstream code or introduce additional complexity unless users want to opt into the functionality. Even then, the compiler is able to infer the right thing when the destination is typed, so doesn't necessarily always introduce typing complexity.

I also think your idea of providing a single utility function for deserializing a slice is a reasonable stop-gap as well. Though, it doesn't generalize to the num_required_signatures case.

[kskalski]

I don't think union is appropriate here, unless perhaps I'm misunderstanding how you're thinking about using one here. The two types in question here don't occupy the same number of bytes in memory. In the VersionedBlockMarker case, the first 8 bytes are just totally discarded, whereas in the Vec<Entry> case they're the length of the Vec.

Ah, right, the decoded bytes don't overlap. Technically then this specific case can be solved as

let entries: Vec<Entry> =
     <WincodeVec<Entry, MaxDataShredsLen> as SchemaRead<'de, C>>::get(reader)?;
if entries.is_empty() {
   // only len was deserialized and it turned to be 0
   <VersionedBlockMarker as SchemaRead<C>>::get(reader)?
} else {
   dst.write(Self::EntryBatch(entries));
}

union idea would probably fit other cases better where you do use the same bytes for two different things depending on some extra branching on those bytes.

It actually does generalize to other cases. Here's another example: https://github.com/anza-xyz/solana-sdk/blob/3275512d3320058db71cb200a92a8db68a4a4b85/message/src/versions/mod.rs#L425-L466

Here, the first byte is either a version discriminant or num_required_signatures of MessageHeader. This is a good use-case for the Ctx system. We have multiple areas in the SDK where being able to deserialize a MessageHeader with an already-read num_required_signatures would be useful. One of the sections mentioned in the PR description is another such case https://github.com/anza-xyz/solana-sdk/blob/556aa4442495047b8222cf4c921e367156d3a59d/transaction/src/versioned/mod.rs#L304-L329.

I see, though using context would only allow moving the manual deserialization of remaining parts to the dedicated implementation (from having it spread out in the branches of one function), not eliminate it, right?
Only with matching derive support you could designate some fields as being part of the context and get such impl derived.

One more supplementing idea here - maybe uninit builder should have a function like "read the rest" that will go over fields that were not yet initialized, in original order and deserialize them using provided reader (it would in fact be identical to the regular schema read deserialization except that each field reading would be gated by initialized bit).

• we could still go back to some of the "fill_buf" / "peek" APIs, maybe focusing on "peek" style function but for more bytes, e.g. peek_array::<8>() would clearly solve the block component example and though it still brings back lots of complexity for implementing buffered reader, we end up with a compromise similar to the chunking idea (i.e. size of the peeking can be bound to small sizes)

I do think the context system we're discussing here is a good, generalized solution that is minimally invasive. It won't break downstream code or introduce additional complexity unless users want to opt into the functionality. Even then, the compiler is able to infer the right thing when the destination is typed, so doesn't necessarily always introduce typing complexity.

Right, I agree it can express useful cases cleanly. I'm trying to weigh the net benefit though - the conditional deserialization that is done manually doesn't necessarily look too bad and I have an impression automating that part (flexible derive support) would be tricky.

I will analyze the provided examples a bit more.

[cpubot]

Ah, right, the decoded bytes don't overlap. Technically then this specific case can be solved as

let entries: Vec<Entry> =
     <WincodeVec<Entry, MaxDataShredsLen> as SchemaRead<'de, C>>::get(reader)?;
if entries.is_empty() {
   // only len was deserialized and it turned to be 0
   <VersionedBlockMarker as SchemaRead<C>>::get(reader)?
} else {
   dst.write(Self::EntryBatch(entries));
}

Yeah, you're right. Definitely could solve this case with the above.

I see, though using context would only allow moving the manual deserialization of remaining parts to the dedicated implementation (from having it spread out in the branches of one function), not eliminate it, right? Only with matching derive support you could designate some fields as being part of the context and get such impl derived.

Right. It would just allow localizing the partial deserialization with context into a dedicated mechanism. We could probably devise some derive attribute scheme that could generate them. Though, for v0.5 I think just having the context system would suffice to unblock. We can add more features to streamline once we have a good design for the derive macro.

I think the main challenge with derive macro attributes would be figuring out how to the right syntax for expressing context matricies. E.g.,

#[derive(SchemaRead)]
struct Foo {
    #[wincode(ctx)]
    bar: u32,
    #[wincode(ctx)]
    baz: bool,
}

Does this generate SchemaRead<'de, C, u32> and SchemaRead<'de, C, bool>, or SchemaRead<'de, C, (u32, bool)>, or all 3?

Or would we want to force expressing all desired permutations?

#[derive(SchemaRead)]
// Generate all 3
#[wincode(ctx(bar, baz, (bar, baz))]
struct Foo {
    bar: u32,
    baz: bool,
}

#[derive(SchemaRead)]
// Generate just for bar
#[wincode(ctx(bar))]
struct Foo {
    bar: u32,
    baz: bool,
}

^ This one actually seems fairly reasonable to me.

One more supplementing idea here - maybe uninit builder should have a function like "read the rest" that will go over fields that were not yet initialized, in original order and deserialize them using provided reader (it would in fact be identical to the regular schema read deserialization except that each field reading would be gated by initialized bit).

Only downside I see to this is that initialization bits are stored at runtime, so an implementation of "read the rest" would entail quite a bit of branching. Whereas the context system defines these as separate implementations at compile time

[kskalski]

Let's try to figure out if there are other blocking examples where it isn't enough to:

• peek one byte
• use custom logic that looks at data decoded so far to decide on decoding further data or terminating (as with entries.is_empty()) without intervention into internal parsing logic of the sub-struct / field

It feels like we are trying to decide if supporting LL(1)-style parsing (like in https://en.wikipedia.org/wiki/LL_parser) is enough or we need something that stores arbitrary context.

I think context could be a generic and useful concept for different purposes, for example using &mut context could be used for something resembling streaming compression where after seeing X items you may adjust the way you encode further items (say pick a cheaper var-int encoding). But for finalizing 0.4 and going for 0.5 I still wonder if we could avoid rushing this design.

[cpubot]

Let's try to figure out if there are other blocking examples where it isn't enough to:

• peek one byte
• use custom logic that looks at data decoded so far to decide on decoding further data or terminating (as with entries.is_empty()) without intervention into internal parsing logic of the sub-struct / field

It feels like we are trying to decide if supporting LL(1)-style parsing (like in https://en.wikipedia.org/wiki/LL_parser) is enough or we need something that stores arbitrary context.

I think context could be a generic and useful concept for different purposes, for example using &mut context could be used for something resembling streaming compression where after seeing X items you may adjust the way you encode further items (say pick a cheaper var-int encoding).

My sense is that peek and peek_array::<N> are likely sufficient for all cases we've observed.

But for finalizing 0.4 and going for 0.5 I still wonder if we could avoid rushing this design.

I'm not necessarily worried about "rushing" this design. It's robust enough to handle the cases we've been discussing and has the advantage that it's purely additive in terms of functionality (i.e., it doesn't necessarily add complexity for the typical use-case and doesn't break existing code or require a change to the derive macro). We're still pre v1.0, which gives us license to experiment, and doesn't lock us into a design long-term.

I agree in principle we shouldn't rush designs, but out of all the options we've explored to accommodate removal of implicit-buffering methods on Reader, this design appears to be the most robust.

1. It doesn't require additional Reader helpers like chaining that would negatively impact performance
2. It's totally opt-in, and doesn't break existing code or require changes to derive macros
3. It's polymorphic and can support multiple implementations on a single type
   • Each impl gets its own TypeMeta declaration (useful if we want to support context on fixed sized types where part of the payload has already been read and still preserve trusted capabilities)
   • It being fully polymorphic means it can support mutable receivers (for stateful parsing like you mentioned) or just scalar types to implement a sort of curried "partial application" scheme for already-read values (like the Vec with an already parsed length)
4. It can actually remove redundant work like already-performed bounds-checking by forcing implementations to make use of bytes that have already been read.
   • In contrast to permitting functions like peek_array, which will entail bounds checking the same bytes twice unless the user opts into manual parsing (e.g., with an UninitBuilder)

[kskalski]

It being fully polymorphic means it can support mutable receivers (for stateful parsing like you mentioned) or just scalar types to implement a sort of curried "partial application" scheme for already-read values (like the Vec with an already parsed length)

Good point, if Context could be &mut State it does seem pretty powerful.

I'm onboard with this change, but I think the reader's peeking functions are more straight-forward ways to achieve the typical use case.

Let's have this change in 0.5, ideally with some more motivating examples in form of:

• replacing peek_array where it would be hard to do otherwise
• gaining performance if passing the context would avoid bounds checks

[cpubot]

It being fully polymorphic means it can support mutable receivers (for stateful parsing like you mentioned) or just scalar types to implement a sort of curried "partial application" scheme for already-read values (like the Vec with an already parsed length)

Good point, if Context could be &mut State it does seem pretty powerful.

I'm onboard with this change, but I think the reader's peeking functions are more straight-forward ways to achieve the typical use case.

Let's have this change in 0.5, ideally with some more motivating examples in form of:

• replacing peek_array where it would be hard to do otherwise
• gaining performance if passing the context would avoid bounds checks

Ok, sounds good. We'll ship the minimal peek and peek_array methods, and we can introduce context in v0.5.