Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mneumann opened this issue Feb 15, 2014 · 9 comments · Fixed by #13107
Closed

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

mneumann opened this issue Feb 15, 2014 · 9 comments · Fixed by #13107

Comments

@mneumann
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

huonw commented Feb 15, 2014

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

@mneumann
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

wycats commented Mar 7, 2014

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
Copy link
Contributor

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
Copy link
Contributor

erickt commented Mar 11, 2014

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
Copy link
Contributor

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.

bors added a commit that referenced this issue Mar 28, 2014
All of Decoder and Encoder's methods now return a Result.

Encodable.encode() and Decodable.decode() return a Result as well.

fixes #12292
@bors bors closed this as completed in f1739b1 Mar 28, 2014
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 25, 2022
…n, r=jonas-schievink

internal: Bump extension version
flip1995 pushed a commit to flip1995/rust that referenced this issue Feb 26, 2024
…ATTR, r=flip1995

 Add new lint `DEPRECATED_CLIPPY_CFG_ATTR`

As discussed [on zulip](https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Is.20.60--cfg.20feature.3Dcargo-clippy.60.20deprecated.20or.20can.20it.20be.3F).

This lint suggests to replace `feature = "cargo-clippy"` with `clippy`.

r? `@flip1995`

changelog:  Add new lint `DEPRECATED_CLIPPY_CFG_ATTR`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants