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

Decode context #710

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants