serialize::Decodable decode() should return a IoResult/DecodeResult?

[mneumann]

Decoding can fail in the same way as any IO operation or any read_xxx() function in trait Reader. So should the serialize::Decodable trait, no? Otherwise, we again depend on using task::try for decoding data structures.

My proposal is to change the signature of trait Decodable to something like:

pub type DecodeResult<T> = Result<T, &str>;

pub trait Decodable<D: Decoder> {
    fn decode(d: &mut D) -> DecodeResult<Self>;
}

Likewise, the trait serialize::Decoder must be changed to return DecodeResults as well.

This of course comes with a slight performance penalty in the non-error case.

[mneumann]

I am volunteering to provide a patch (as I am also working on the rust-msgpack and rust-toml libraries, which needs to be changed as well), if we can agree upon a DecodeResult type (is &str enough?).

[huonw]

It seems like Encodable should do something similar if we do do this.

[mneumann]

Yes, so in case the underlying Encoder fails with an IoError, we can propagate it back. The big question is only, what will the type of EncodeResult/DecodeResult be?

[mneumann]

For performance reasons, I don't like to bloat up the return values of each read_xxx method very much. Especially as errors are very rare, so I'd prefer something like

pub enum DecodeError = ...
pub type DecodeResult<T> = Result<T, ~DecodeError>

which would only add one extra word (but of course incurrs an allocation in the error case).

[mneumann]

As the underlying error can be manifold (it can be an IoError or a protocol error), it would be better to just signal an error condition, and being able to retrieve the exact error condition (e.g. the IoError) from the Decoder or Encoder struct. Also the error heavily depends on the underlying Encoder/Decoder. For example a Decoder which is based on a MemReader would not need an IoError.

[wycats]

I agree with this. I was implementing a Decoder and the only real way to propagate errors at the moment is to store the error information in the Decoder and move on. However, this means you're forced to continue parsing a source that you already know is invalid (or explicitly handle this in read_struct_field).

In my experience so far, IoError is actually not the predominant kind of error. Instead, a common problem is that the struct indicates that it wants a uint but the source gave you a string.

For example, if you had a struct like this: struct Article { title: ~str, body: ~str } and you were deserializing a JSON like: { "title": 1 }, you would have two errors (missing a mandatory body field and incorrect type provided for title).

These kinds of errors need to be handled by virtually all decoders, and should be supported out of the box rather than requiring an error field in the struct plus extra work in the handlers.

Also, the current approach requires each of the handlers to still return a dummy value (0 for uint, for example), even though there was no actual value in the source. This is a lot of overhead when building a Decoder that could be eliminated by allowing a Result to be returned.

[arjantop]

Every encoder/decoder will deal with errors differently so my suggestion is to change traits to this:

trait Decoder<E> {
    fn read_uint(&self) -> Result<uint, E>;
}

trait Decodable<E, D: Decoder<E>> {
    fn decode(d: &mut D) -> Result<Self, E>;
}

impl<E, D: Decoder<E>> Decodable<E, D> for uint {
    fn decode(d: &mut D) -> Result<uint, E> {
        d.read_uint()
    }
}

trait Encoder<E> {
    fn emit_uint(&self, v: uint) -> Result<(), E>;
}

trait Encodable<E, S: Encoder<E>> {
    fn encode(&self, s: &mut S) -> Result<(), E>;
}

impl<E, S: Encoder<E>> Encodable<E, S> for uint {
    fn encode(&self, s: &mut S) -> Result<(), E> {
        s.emit_uint(*self)
    }
}

[erickt]

I feel @arjantop's solution is the right one. We'll need some way of communicating malformed values. The excess typarams is a bit of a shame though. I long for the day associated types (#5033) gets implemented.

[arjantop]

The other solution would be using IoResult but i'm not convinced it's the right type for this. I think custom types with apropriate error information are the best way. For decoding only EndOfFile, InvalidInput and OtherIoError are useful and any error information must be included in message string.