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

Reconsider MapSink and SeqSink #6

Closed
mitsuhiko opened this issue Feb 5, 2022 · 1 comment
Closed

Reconsider MapSink and SeqSink #6

mitsuhiko opened this issue Feb 5, 2022 · 1 comment
Labels
defect Design defect

Comments

@mitsuhiko
Copy link
Owner

mitsuhiko commented Feb 5, 2022

Currently a Sink has two sub sinks MapSink and SeqSink which are returned from map and seq respectively. The challenge with these is that their lifetime has to be constrained to the sink itself which means that the MapSink and SeqSink returned can't reference data which is not on &mut self.

This seems like a non issue but actually is problematic for where a in direction is needed during deserialization. For greater flexibility our Deserialize::deserialize_into method returns a SinkHandle which can also contain a boxed Sink. This means that the following piece of code does not work:

impl<T: Deserialize> Sink for SlotWrapper<Option<T>> {
    fn map(&mut self, state: &DeserializerState) -> Result<Box<dyn MapSink + '_>, Error> {
        **self = Some(None);
        Deserialize::deserialize_into(self.as_mut().unwrap()).map(state)
    }
}

This fails as deserialize_into when it returns a SinkHandle::Owned cannot be held on to. The following code would compile:

impl<T: Deserialize> Sink for SlotWrapper<Option<T>> {
    fn map(&mut self, state: &DeserializerState) -> Result<Box<dyn MapSink + '_>, Error> {
        **self = Some(None);
        let handle = Deserialize::deserialize_into(self.as_mut().unwrap());
        match handle {
            SinkHandle::Borrowed(handle) => handle.map(state),
            SinkHandle::Owned(_) => panic!(),
        }
    }
}

The challenge now is how does one come up with a better design which retains the general usefulness of the SinkHandle. Currently this complexity is largely pushed into the highly unsafe internals of the Driver but for quite a few sinks this complexity is resurfacing. What's worse though is that there is really no safe way in which one can fulfill the contract of a MapSink + '_ while holding on to a SinkHandle::Owned without violating some rules.

@mitsuhiko mitsuhiko added the defect Design defect label Feb 5, 2022
@mitsuhiko
Copy link
Owner Author

I definitely can't come up with a better solution here. However thankfully the problem turns out to be not massive thankfully as one can just return an alternative sink upon deserialization that wraps the entire sink handle.

This is an example of how this is done for optionals now: 9c8efc9

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

No branches or pull requests

1 participant