algebra_core::io module inconsistent with std::io

[tsunrise]

When the std feature is on, algebra_core chooses to reexport the std::io library instead of using its own io package. However, some of the implementation in the io package is inconsistent with the std::io library. It might cause some confusion and user has to handle both cases when inconsistency happens.

For example, when user tries to use the Error struct in the io package, when in no_std mode, it is fine to return Err(algebra_core::io::Error), while when the std feature is on it won't compile.

It is because the IO Error implemented by algebra_core is

pub struct Error;

while Error in std::io is

pub struct Error {
    repr: Repr,
}

It might be better to make algebra_core::io more consistent to the standard library, or just use the io package instead of reexporting std::io package regardless of whether std feature is on or off to avoid confusion.

[weikengchen]

I agree that it is unnatural (I need to handle this specifically when writing my code).

Some thinking:

There is a downside for "use the io package instead of reexporting std::io package regardless of whether std feature is on or off". In this case, our Read or Write would not naturally compose with other packets using std::io.

[Pratyush]

std::io is not available in no_std (there is no equivalent in core or alloc, so we can't just re-export it. If you could post your code snippet here that would be great.

[tsunrise]

I'm now using cfg switch in my code like this:

impl<F: Field> ToBytes for XZZPS19PMsg<F> {
    #[cfg(not(feature = "std"))]
    fn write<W: Write>(&self, writer: W) -> IOResult<()> {
        match self.serialize(writer) {
            Ok(()) => Ok(()),
            Err(_e) => Err(IOError),
        }
    }
    #[cfg(feature = "std")]
    fn write<W: Write>(&self, writer: W) -> IOResult<()> {
        match self.serialize(writer) {
            Ok(()) => Ok(()),
            Err(_e) => Err(IOError::new(
                ErrorKind::InvalidData,
                "Cannot serialize message. ",
            )),
        }
    }
}

It might be better if there's no need to handle both cases manually.

[Pratyush]

For the time being, to make the code shorter, you could change it to something like

impl<F: Field> ToBytes for XZZPS19PMsg<F> {
   
    fn write<W: Write>(&self, writer: W) -> IOResult<()> {
 		#[cfg(not(feature = "std"))]
        self.serialize(writer).map_err(|_| IOError)

   		#[cfg(feature = "std")]
        self.serialize(writer).map_err(|_| IOError::new(
                ErrorKind::InvalidData,
                "Cannot serialize message. ",
            ))
    }
}

[Pratyush]

But also, what is the serialize method from? Is it from CanonicalSerialize? If so, I can try to add a Into<io::Error> impl on serialize::Error.

[Pratyush]

(Eventually, I plan on getting rid of to_bytes! and ToBytes and FromBytes; these conversions are currently unprincipled and should be replaced by CanonicalSerialize/CanonicalDeserialize)

[tsunrise]

Make sense. There may be some other crates that need ToBytes and FromBytes trait, so I believe it's good to impl Into<io::Error> on serialize::Error.

[burdges]

At present, there is little option besides using some "serialization framework" for no_std, so CanonicalSerialize/CanonicalDeserialize here.

It's plausible the new error handling project group rust-lang/rfcs#2965 selects some error type and trait that work without std, like https://github.com/yaahc/nostd-error-poc or maybe rust-lang/rust#48331 An obvious approach is some SmallBox<dyn Error> type ala https://internals.rust-lang.org/t/idea-fixed-sized-traits/12179/8?u=jeffburdges so any T: Error always works, even without alloc, provide T is smaller than 8 bytes, and larger T panics without alloc. After an error type exists that works with OS errors, native crate errors, and does not strictly require alloc, then either the traits in std::io become core::io or some similar fork appears.

I've no idea if functionality like canonical serialization should work like std::io but it might become possible down the road.

[Pratyush]

This is fixed now in ark-std. (For common APIs)