Decode context

[branchseer]

trafficstars

Why

With the current Decode/BorrowDecode, it's impossible to implement them on a type that borrows something other than the input data. A concrete example would be a Vec<'bump, T: 'bump> which is allocated by an arena allocator.

How

This PR introduces the following changes:

• Add a decode function that accepts a decode context: decode_from_slice_with_ctx<Ctx, D: de::Decode<Ctx>, C: Config>(src: &[u8], config: C, ctx: Ctx). In that arena Vec example, the decode context would be the allocator &Bump.
• The context is attached to DecoderImpl and exposed to Decode implements via Decoder::ctx method.
• Decode trait is changed to Decode<C>, which allows types to implement Decode only under some specific context. (Vec<'bump, T: 'bump> would implement Decode<&'bump Bump>)
• The derive macro Decode is changed to generating impl<Ctx> Decode<Ctx> for ... instead of impl Decode for .... A container attribute decode_context is added to allow deriving Decode with a concrete context type. (for example #[bincode(decode_context = "&'bump Bump")])
• All changes above are also similarly done to BorrowDecoder.

You can see how these changes work together in tests/ctx.rs.

I know this is a big change and would break existing 2.0.0 users. But it's still rc so I guess it's not too late to discuss it? ;)

Alternative Design

Leaves (Borrow)Decode untouched, and add (Borrow)DecodeWithContext<C>.

Pros:

• Less impactful. Non-breaking to (Borrow)Decode.
• Less cognitive overload to implementors who don't care about decode contexts.

Con:

• Trait explosion: https://github.com/bincode-org/bincode/issues/578#issuecomment-1233071687.
• Needs a separate derive macro.

TODOs

This PR hasn't been polished to a mergeable state yet. I will do that if the overall design is accepted.

• Namings are not consistent (C?Ctx?Context? )
• Docs not updated
• virtue is updated to allow generating impl<Ctx>. This would need a separate PR.
• Current derive macro Decode allows Ctx to be either a concrete type or totally unbounded. How about allowing Ctx: ...? Is it useful enough to be implemented?

[VictorKoenders]

I like this concept a lot, but I have a vague memory I had a discussion about this with @ZoeyR a while back and we decided not to do this.

We'll get back to you

[VictorKoenders]

I had a discussion with @ZoeyR and we decided that this is a useful feature and we're willing to break open the API for this.

Could you get this in a state where the CI succeeds? Specifically test --no-default-features, as this tends to fail a lot.

As for the TODOs:

• Namings are not consistent (C?Ctx?Context? )

I like writing out things fully, so Context please

• virtue is updated to allow generating impl<Ctx>. This would need a separate PR.

I maintain virtue so feel free to make a PR to that

• Current derive macro Decode allows Ctx to be either a concrete type or totally unbounded. How about allowing Ctx: ...? Is it useful enough to be implemented?

Let's start with the minimum effort you need to do to get this merged, we can enhance the macros later

Thanks a lot for your proposal!

[branchseer]

That's great to hear. Thanks! I'll work on it as soon as I'm back from vacation.

[VictorKoenders]

Pinging @branchseer we'd love to have this in bincode 2

[branchseer]

Pinging @branchseer we'd love to have this in bincode 2

Ah sorry I totally forgot! Will work on it this weekend!

[branchseer]

Hi @VictorKoenders, let's start with https://github.com/bincode-org/virtue/pull/90. It's for generating impl<Context> Decode<Context>

[lcnbr]

It seems the derive macros aren't always doing the right thing. If I derive Decode, it fails because the generated code fails with: : failed to resolve: could not find result in core. Additionally when specifying the context, it only fills in the specified type in one place, missing it in the impl body. However as a concept and in usage it is really nice!

[VictorKoenders]

Bincode derive should be outputting the files in target/bincode, can you paste an example of a mis-derived struct?

[VictorKoenders]

Would be great if we can get this merged before releasing 2.0

[ZoeyR]

Closing this since I pulled in the changes in #749

[alexisfontaine]

Congratulations on the release! What do you think about making the unit type the default for the Context generic?