concordium-rust-smart-contracts icon indicating copy to clipboard operation
concordium-rust-smart-contracts copied to clipboard

Remove the `Default` constraint for the `Err` associated type of the Write trait

Open abizjak opened this issue 2 years ago • 4 comments

The concordium_contracts_common::Write has a Default constraint.

This is not really essential, and I think the only reason it is there is simplicity. It is used in the write_all implementation, where it can and should just be removed and the inner error should be propagated.

It is also used in schema.rs for some functions, such as serializing size length. There it should be replaced by custom enums that will also provide better error messages that will point to a precise source of an error.

abizjak avatar Dec 03 '22 13:12 abizjak

So I briefly looked into this. The challenge with removing Default in write_all is that it has the additional check for the number of bytes written is more than 0. So we would have to change behavior to remove it

limemloh avatar Mar 16 '23 09:03 limemloh

We can't change that behaviour. It's quite essential because some implementations, for example

/// This implementation overwrite the contents of the slice and updates the
/// reference to point to the unwritten part. The slice is (by necessity) never
/// extended.
/// This is in contrast to the `Vec<u8>` implementation which always extends the
/// vector with the data that is written.
impl Write for &mut [u8] {
    type Err = ();

    #[inline]
    fn write(&mut self, buf: &[u8]) -> Result<usize, Self::Err> {
        let to_write = core::cmp::min(buf.len(), self.len());
        let (overwrite, rest) = core::mem::replace(self, &mut []).split_at_mut(to_write);
        overwrite.copy_from_slice(&buf[..to_write]);
        *self = rest;
        Ok(to_write)
    }
}

It would be incorrect to change the write_all behaviour here. Of course we could change the default implementation, and override it here, but having a dangerous default implementation is not very good I think.

abizjak avatar Mar 16 '23 10:03 abizjak

A fancy solution could be to have write_all in its own WriteAll trait, define a WriteAllError and require Err in the trait to implement From<WriteAllError>.

limemloh avatar Mar 17 '23 07:03 limemloh

We could also do

trait Write {
   type Err

    ....

    fn write_all(&mut self, data: &[u8]) -> Result<...> where Self::Err: Default {

   }

But this also requires a lot of changes since write_all is used a lot.

abizjak avatar Mar 17 '23 07:03 abizjak