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

Support more serialization/deserialization targets: Deserialize into owned strings if "alloc" feature is enabled #175

Closed
wants to merge 3 commits into from

Conversation

jsdw
Copy link

@jsdw jsdw commented May 5, 2022

Currently bitvec will attempt to deserialize several things into borrowed strings, which will fail if the Deserializer calls visit_string() instead of visit_borrowed_string() for these things. This is the case with things like serde_json and serde_value, and probably any deserializers that themselves hold owned strings.

This PR tweaks the behaviour so that when the alloc feature is enabled, we deserialize into owned strings instead, allowing bitvec to be deserialized from popular formats like serde_json. When the alloc feature isn't enabled, we keep the existing behaviour roughly as-is.

What do you reckon @myrrlyn? :)

Closes #167

@@ -40,6 +40,9 @@ rust-version = "1.56"

[features]
alloc = [
# If the serde feature is also enabled, enable alloc for it
# so that we can deserialize Strings.
"serde?/alloc"
Copy link
Author

@jsdw jsdw May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the min supported rustc version for Bitvec is 1.56, this "?" features isn't available yet.
Removing it would mean that enabling the "alloc" features also enabled the "serde" feature. Any thoughts on how best to tackle this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which version did this stabilize? While I had hoped to refrain from MSRV raises until I'm able to release 2.0 with the BitArray redesign, my MSRV policy does state that I may freely do so in the minor series.

If raising the MSRV to support optional transitive features really is sufficient (with the rest of this PR) to resolve the de/ser problem, then please raise MSRV in rust-toolchain.toml and README.md, and update to 1.1.0 in Cargo.toml.

I am going to continue working through my backlog in the 1.0 series. Thank you for your patience as I've ignored the project, and I look forward to releasing your 1.1 soon!

@myrrlyn
Copy link
Collaborator

myrrlyn commented Jul 1, 2022

While the text representation of type parameter tokens is very much a hack to try to ensure round-trip correctness, I would prefer to keep it if able. I don't have a better way offhand to encode this information (while type_name::<T>() isn't guaranteed to be stable across compiler versions, I don't believe type_id::<T>() is even guaranteed to be stable across compiler invocations), and I certainly don't want to try to enforce a global registry of BitOrder implementors within a compilation unit (especially since this wouldn't help in the case of two separate programs exchanging data with each other!).

I believe the easiest resolution is therefore just to accept this PR, lifting the MSRV and minor version as described in my response to your comment in Cargo.toml.

@jsdw
Copy link
Author

jsdw commented Jul 4, 2022

Thanks for your feedback!

I bumped the MSRV to 1.60 anywhere I found it mentioned, and added a changelog entry and version bump for 1.1.0 :)

Lemme know if you'd like anything else tweaking before merging and I'll get on that!

@myrrlyn
Copy link
Collaborator

myrrlyn commented Jul 10, 2022

It looks like @/dtolnay fixed this in #185 without an MSRV bump by fixing misunderstandings I'd made about how serde operates. Thank you for your efforts and I apologize for taking so long to check my inbox that this got superseded.

@myrrlyn myrrlyn closed this Jul 10, 2022
@jsdw
Copy link
Author

jsdw commented Jul 13, 2022

Oh not at all; that's excellent, and his PR looks far more comprehensive a fix than mine, so brilliant stuff!

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.

Deserializing with serde yields invalid type error
2 participants