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

Expose types implementing serde::Serializer and Deserializer #729

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

fjarri
Copy link

@fjarri fjarri commented Oct 27, 2024

This is sort of a proof-of-concept PR, I would like your input on it even though it's still in draft.

The goal here is to expose types implementing serde::Serializer and Deserializer so that bincode could be used with https://docs.rs/erased-serde. Serializer is straightforward, but Deserializer can be exposed in two ways:

  • Expose some type D, where for<'a> &'a mut D: Deserializer<'de>. This is done e.g. in postcard. The problem here is that using such types generically involves passing around an ugly bound on lifetimes, which cannot be encapsulated in a trait. Also, dealing with such types in erased_serde is somewhat complicated (albeit possible).
  • A much more convenient API is exposing a type D: Deserializer<'de>. The downside of this is that in the format implementations (like bincode) serde::Deserializer is generally implemented for &mut Something, or a wrapper of it (SerdeDecoder in bincode's case), because it has to be passed to nested list/struct/etc deserializers. So creating another implementation for D involves writing down the massive trait impl one more time.

So, I would like your input on the way to proceed with this. I see the following options:

  1. This is not something you want to see in bincode at all. Feel free to close the PR then.
  2. Expose a type D, where for<'a> &'a mut D: Deserializer<'de>. This requires minimum changes, but creates problems for the users, as described above.
  3. Expose a type D: Deserializer<'de>, writing down another Deserializer impl (of which there are already two in the codebase).
  4. Expose a type D: Deserializer<'de>, relying on a helper crate that I made (https://docs.rs/serde-persistent-deserializer), which is implemented in this PR. The reason I put it there is that I want to create a similar PR for other formats, and I would like to avoid writing Deserializer impls in each of them. I understand if you want to minimize the number of dependencies, then the contents of that crate can be brought in verbatim.

(This PR currently does not expose Serializer, but that's easy, and I'll add it later if you decide not to go with Option 1).

A few things about the PR itself.

  • I would like to remove the Deserializer impl in de_borrowed, and it's almost possible by just replacing de_borrowed::SerdeDecoder with de_owned::SerdeDecoder , but a single test fails because these impls are slightly different. I am not sure if it's possible to merge them.
  • I am not sure I understand the comment in de_owned::decode_from_slice() ("Note that this does not work with borrowed types like &str or &[u8]. For that use [borrow_decode_from_slice].") Seems to be working fine, as I can just call de_borrowed::borrow_decode_from_slice() from it.

@fjarri fjarri force-pushed the public-deserializer branch from e0cf3e6 to f286ab3 Compare October 30, 2024 22:17
@fjarri fjarri marked this pull request as ready for review October 30, 2024 22:17
@fjarri
Copy link
Author

fjarri commented Oct 30, 2024

After some experimentation, I decided to not use serde-persistent-bytes at this level and make it the user's responsibility (although its use can be added later, making API a little more convenient).

The PR exposes two new entities: BorrowedSerdeDecoder and OwnedSerdeDecoder, with as_deserializer() methods allowing one to get access to an object implementing serde::Deserializer.

@VictorKoenders
Copy link
Contributor

Sorry for the late response, the last month has been very busy. We're still thinking on if we want this change in bincode or not. Thank you for your patience 🙏

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

Successfully merging this pull request may close these issues.

2 participants