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

Allow the tape to be used in a standard serde::Deserialize #366

Closed

Conversation

bassmanitram
Copy link

@bassmanitram bassmanitram commented Jan 28, 2024

Issue #365

  • The new from_tape method on the Deserializer allows a tape to be extracted from a Deserializer via into_tape and then be used to rebuild a Deserializer. It is marked as unsafe because no structure verification is performed - it is up to the caller to make sure that the tape is correct (e.g. by extracting from a Deserializer, performing read-only operations, then rebuilding a Deserializer).

At the moment, however, nothing I try is able to allow access to that from within a standard serde::Deserializer.

  • Deserialize in a tap: really complex to implement
  • Extension Trait - specialization not allowed
  • ?

Copy link
Member

@Licenser Licenser left a comment

Choose a reason for hiding this comment

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

Looks good :) I was about to suggest that on the issue when you submitted the PR 👍 thanks a lot!

@Licenser
Copy link
Member

Oh I just saw, it'll need a cargo fmt before passing the CI

@bassmanitram
Copy link
Author

bassmanitram commented Jan 29, 2024 via email

@bassmanitram
Copy link
Author

Hold of merging for now - the larger goal is to figure out how to take the deserializer in a serde::de::Deserialize implementation's deserialize method, get the Tape from it, do my "type determinant" searches, then turn the Tape back into the deserializer and pass it onto the discovered types. Obviously this PR enables that last bit, but it's getting the Tape from the "anonymous" deserializer that has me a bit stuck at the moment.

@Licenser
Copy link
Member

I think a way of achieving this would be changing the implementation from Deserializer for Deserializer (yay naming ...) to Deserilizer for &[Node] or Deserializer for Vec<Node> that way the same implementation would work for Deserializer and Tape since both have the vec/slice?

@bassmanitram
Copy link
Author

bassmanitram commented Jan 29, 2024

Since Deserialize is foreign and so are Vec and any slice notation, we need a local Trait to cover them... thinking...

Can't think of anything - no matter what gyrations I think of, it comes down to needing to "know" that the anonymous deserializer can give me a reference to the tape so that I can "peek" at the tape and find the type determinants. I can't think of any way to do that (I'm constrained to using the serde Deserialize(er) in the framework to which I'm adding simd_json support)...

Ohhhh - wait - an extension trait! Testing...

Martin Bartlett added 2 commits January 29, 2024 17:57
The trait is an attempt to allow the deserializer to be "introspected" before a
serde::Deserialize::deserialize function implementation actually uses the deserializer.

Totally untested at the moment!
@bassmanitram bassmanitram changed the title Add unsafe from_tape associated function Implement an extension trait to allow the deserializer to be introspected. Jan 29, 2024
@bassmanitram bassmanitram changed the title Implement an extension trait to allow the deserializer to be introspected. Implement methods and an extension trait to allow the deserializer to be introspected. Jan 29, 2024
@bassmanitram
Copy link
Author

bassmanitram commented Jan 29, 2024

Ok, that's totally untested at the moment, but compiles and passes clippy.

My use case compiles fine - too late to test now, on it in the morning

Martin Bartlett added 3 commits January 29, 2024 18:07
The "into_tape" and reconstruction of the Deserializer after is far easier
to manage than all the lifetime stuff that gets involved when borrowing from
the deserializer, so that is what the extension trait now publishes - and my use case
compiles with it, even if it hasn't yet been tested.
@bassmanitram
Copy link
Author

You live and learn eh - can't "specialize", it seems - the default implementation is always invoked!

@bassmanitram
Copy link
Author

OK, lets try a deserializer for Node ... which of course means I have to start hacking another of your libraries - value_trait :)

@bassmanitram
Copy link
Author

Nope - that's fantastically complex too, it seems! This is becoming soul destroying!

@bassmanitram bassmanitram changed the title Implement methods and an extension trait to allow the deserializer to be introspected. Allow the tape to be used in a standard serde::Deserialize Jan 30, 2024
@Licenser
Copy link
Member

Just a few thought experiments would any of the following work:

  1. move the implementation of serde::Deserializer from simd_json::Deserializer to simd_json::Tape
  2. change the according functions that use the deserializer.deserialize() call to call deserializer.into_tape().deseriaze()

Or:

adding a as_tape(&self) -> &[Node] function and use that to inspect the type

@bassmanitram
Copy link
Author

OK, I leave this here - serde::de::DeserializeSeed "solves" the issue, but where I'm trying to implement it has serde_json so ingrained that swapping over to simd_json (with necessary "unsafe" stuff too) is just too onerous at the moment.

So the PR with only from_tape is ready, and some kind of solution to RawValue would be appreciated in the future.

@bassmanitram bassmanitram marked this pull request as ready for review January 31, 2024 18:08
@Licenser Licenser enabled auto-merge (rebase) August 3, 2024 12:52
auto-merge was automatically disabled August 3, 2024 13:02

Rebase failed

@Licenser
Copy link
Member

I manually merged this to avoid the conflicts :) thank you!

@Licenser Licenser closed this Aug 10, 2024
@bassmanitram bassmanitram deleted the feature/deserializer-from-tape branch October 28, 2024 16:32
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