Skip to content
This repository was archived by the owner on Aug 15, 2025. It is now read-only.

Decode context#710

Closed
branchseer wants to merge 10 commits intobincode-org:trunkfrom
branchseer:decode_context
Closed

Decode context#710
branchseer wants to merge 10 commits intobincode-org:trunkfrom
branchseer:decode_context

Conversation

@branchseer
Copy link

@branchseer branchseer commented Apr 11, 2024

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:

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
Copy link
Contributor

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
Copy link
Contributor

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

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

@VictorKoenders VictorKoenders mentioned this pull request Sep 23, 2024
@VictorKoenders
Copy link
Contributor

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

@branchseer
Copy link
Author

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

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

@branchseer
Copy link
Author

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

@branchseer branchseer marked this pull request as draft November 9, 2024 02:46
@lcnbr
Copy link

lcnbr commented Dec 15, 2024

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
Copy link
Contributor

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

@VictorKoenders VictorKoenders mentioned this pull request Mar 2, 2025
@VictorKoenders
Copy link
Contributor

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

@ghost ghost mentioned this pull request Mar 3, 2025
@ghost
Copy link

ghost commented Mar 6, 2025

Closing this since I pulled in the changes in #749

@ghost ghost closed this Mar 6, 2025
@alexisfontaine
Copy link

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

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants