mitsuhiko / deser

Experimental rust serialization library
https://docs.rs/deser
Apache License 2.0
287 stars 8 forks source link

Redesign ignore #11

Closed mitsuhiko closed 2 years ago

mitsuhiko commented 2 years ago

deser currently inherits the design for ignore from miniserde. It involves creating a mutable reference to some static Sink. Miri complains about this and it does sound dodgy. I definitely get miri failures from this when I use ignore and I was also able to reproduce the same issue in miniserde: https://github.com/dtolnay/miniserde/issues/24

The solution for deser would be to embed a zero sized type Ignore directly in the SinkHandle like so:

pub enum SinkHandle<'a> {
    Borrowed(&'a mut dyn Sink),
    Owned(Box<dyn Sink + 'a>),
    Null(Ignore),
}

Ignore can stay internal and SinkHandle gets a new method to create it (SinkHandle::null()) to replace SinkHandle::to(ignore()). This is also more convenient to use for a user.

Size of the enum should stay the same I think.

mitsuhiko commented 2 years ago

It would appear that this is okay for zero sized types: https://github.com/rust-lang/rust-memory-model/issues/44

That said, I'm not convinced this unsafe monkey business is actually needed in this case as keeping the Ignore directly in the SinkHandle is possible. At the very least I vastly prefer calling SinkHandle::null over having to make a SinkHandle manually and calling ignore().

mitsuhiko commented 2 years ago

I changed this now to SinkHandle::null().