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

Why Serializer and Deserializer implementors are missing in public API? #242

Closed
zrkn opened this issue Jul 31, 2018 · 9 comments
Closed

Why Serializer and Deserializer implementors are missing in public API? #242

zrkn opened this issue Jul 31, 2018 · 9 comments

Comments

@zrkn
Copy link

@zrkn zrkn commented Jul 31, 2018

Majority of serde compatible data format crates export their Serializer/Deserializer implementors as opaque struct. Also serde documentation seems to mention that this is desired https://serde.rs/conventions.html

As i can see, currently the only way to obtain Serializer/Deserializer is through hidden SerializerAcceptor/DeserializerAcceptor which should not be considered stable API and also are not very convenient. Though for majority use cases user can be satisfied by wrapper functions, sometimes access to underlying trait implementors is needed(for example if you need to wrap in in some Serializer/Deserializer adapter with custom logic).

@TyOverby
Copy link
Collaborator

@TyOverby TyOverby commented Jul 31, 2018

The reason for this is because the bincode Serializer and Deserializer are generic over many different "behaviors" for performance reasons. These behaviors currently include "is there a byte limit" and "which endianness should I use", but bincode was designed to support more of them.

The reason that these options are done as generic parameters is because it's way faster than branching on every deserialization (pay only for what you use). Unfortunately, because these parameters are generic, I can't make the structure public because then adding more parameters would be a breaking change. I do plan on adding more parameters such as "size of enum tags" and "null terminate strings".

For these reasons, the raw internal Deserializer / Serializer can not be exported without making every change a breaking change. However, it seems possible to use impl trait to return an impl Deserializer or impl Serializer. This is technically possible, but not feasible until you can write impl Trait inside of another trait definition, so I'm waiting for that to land before I make any more changes to the API.

@Xaeroxe
Copy link

@Xaeroxe Xaeroxe commented Sep 17, 2018

This is causing a problem for me as I'd like bincode to be compatible with my WIP crate serde_dyn. Perhaps we could provide a wrapper type that internally uses a trait object to hide all the generics, i.e.

pub struct DeserializerWrapper {
    internal: Box<dyn Deserializer>,
}

As a user of bincode I'm not super concerned about branching performance personally because most of the time when I'm serializing and deserializing I'm sending it out to disk or the network, and both of those are much worse bottle necks than CPU branching.

@Xaeroxe
Copy link

@Xaeroxe Xaeroxe commented Jan 2, 2019

Hm so I just tried to implement my idea and it's kind of a non-starter because even specifying the Serialize trait in use by this crate is border line impossible due to all the type specifications on it, which are all populated with types that have generics. So making a trait object with Serialize is unreasonably difficult.

Unfortunately, the same problem also exists with the impl Trait approach as it won't give a consistent return type for the Config struct. impl Trait doesn't allow the return type to be anything, like a trait object would, it just allows the type to not be explicitly specified. So we're stuck. We can't expose a Serializer and Deserializer without overhauling the code to use way less generics.

@Dr-Emann
Copy link

@Dr-Emann Dr-Emann commented Jun 16, 2019

This also makes using serde_transcode impossible.

@TimonPost
Copy link

@TimonPost TimonPost commented Mar 19, 2020

I would love to use erased_serde with bincode, but I can't because of this.

@ZoeyR
Copy link
Collaborator

@ZoeyR ZoeyR commented Mar 19, 2020

@TimonPost it looks like the workaround posted in #301 (using the currently unstable and undocumented serializeracceptor) would fix the issue. Would you be able to test that workaround to see if it solves your issue?

@TimonPost
Copy link

@TimonPost TimonPost commented Mar 19, 2020

@ZoeyR thanks !! I saw that solution today as well. I am not sure yet. I will try it soon. And update this post if it does work.

@ZoeyR
Copy link
Collaborator

@ZoeyR ZoeyR commented Mar 19, 2020

Great! I'm working in parallel on a draft PR that would resolve this in a more traditional manner. If that ends up panning out we won't need the SerializerAccepter

@ZoeyR
Copy link
Collaborator

@ZoeyR ZoeyR commented Apr 16, 2020

Closed as of #310

@ZoeyR ZoeyR closed this Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.