Add TryFromBytes::try_read_from_io

[joshlf]

Makes progress on #158

gherrit-pr-id: Ia15467145b06dd57e4dfd9e127fdd6d7a313ecba

[jswrenn]

Getting the return type of this right is an interesting rabbit hole! My initial thinking was io::Result<Result<Self, ValidityError<???, Self>>>, but how to fill in the ??? source parameter? We cannot yet return an [u8; size_of::<Self>()] as the source, because such a type is impossible to define. We could return CoreMaybeUninit<Self> as the source, but doing so by-value would destroy the initialization invariant; it'd be a dangerous type to return.

We can't return a &mut CoreMaybeUninit<Self>, because the buffer is allocated in the stack of try_read_from_io, but perhaps this points at the heart of the problem: Should we allow the user to pass in a reusable buffer?

If we change the signature to something like:

fn try_read_from_io<R>(
    mut src: R,
    buffer: &mut CoreMaybeUninit::<Self>
) -> io::Result<Result<Self, ValidityError<&mut CoreMaybeUninit::<Self>, Self>>>
where
    Self: Sized,
    R: io::Read,
{ ... }

two questions arise:

1. Can we avoid the cost of zeroing the buffer on each invocation?
2. Can we drop the Self: Sized bound?

I think the answer to both of these questions is yes. We can achieve the first by defining a ReadBuffer type:

pub struct ReadBuffer<'a, Dst>(Pin<&'a mut MaybeUninit<Dst>>);

impl<'a, Dst> ReadBuffer<'a, Dst> {
    pub fn from_maybe_uninit(src: Pin<&'a mut MaybeUninit<Dst>>) -> Self {
        use zerocopy::FromZeros;
        ReadBuffer(unsafe {
             src.map_unchecked_mut(|src| { src.zero(); src })
        })
    }
}

...and we can achieve the second by using our KnownLayout::MaybeUninit machinery.

EDIT: Actually, I think this is unsound under reverse transmutation.

[jswrenn]

I'm still working on exploring the problem space, presently by considering what a try_mut_from_io would look like. I think we could provide this API:

fn try_mut_from_io<R>(mut source: R, buffer: &mut CoreMaybeUninit<Self>) -> io::Result<Result<&mut Self, ValidityError<&[u8], Self>>>
    where
        Self: KnownLayout + Sized,
        R: io::Read,

Note the lack of an IntoBytes bound! If we can find a way to express this, them I'm less concerned about maximizing the expressivity of try_read_from_io. The ideal API there is one in which we can return a Initialized<T> in the error type, and until that happens, we'll just have to be satisfied with a sub-optimal API. Those wishing to have access to the buffer in the error branch or who wish to pass a buffer in can just use try_mut_from_io.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 34.48276% with 19 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (da442b4) to head (97ce7a8).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lib.rs	34.48%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2619      +/-   ##
==========================================
- Coverage   88.72%   88.40%   -0.33%     
==========================================
  Files          20       20              
  Lines        5332     5356      +24     
==========================================
+ Hits         4731     4735       +4     
- Misses        601      621      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[joshlf]

Why not just ValidityError<(), Self>? [u8; 0] seems a bit unnecessary. I feel like () conveys "we're not providing a source value."

[jswrenn]

We'll want it to be something that's Deref so if it's embedded in a ConvertError, the whole package implements Error. So how about &'static ().

[joshlf]

Eh, between &'static () and [u8; 0], I'd prefer [u8; 0].

[jswrenn]

It would need to be &'static [u8; 0]

[jswrenn]

Which I think is worse, tbh.

[joshlf]

Oh I see. Yeah, agreed that &'static () is better.