Skip to content

serialize: use Result - #13107

Merged
bors merged 1 commit into
rust-lang:masterfrom
seanmonstar:encoder-errors
Mar 28, 2014
Merged

serialize: use Result#13107
bors merged 1 commit into
rust-lang:masterfrom
seanmonstar:encoder-errors

Conversation

@seanmonstar

Copy link
Copy Markdown
Contributor

All of Decoder and Encoder's methods now return a Result.

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

fixes#12292

@huonw

Copy link
Copy Markdown
Contributor

Yay! ❤️

The next person to take a snapshot will get to have fun with rm \o/

(Also, this needs a rebase.)

@seanmonstar

Copy link
Copy Markdown
ContributorAuthor

Rebase already? Hah, I rebased just before submitting :-)

@erickt

Copy link
Copy Markdown
Contributor

This is great work. Once this clears travis, r=me.

@huonw

Copy link
Copy Markdown
Contributor

(cc @wycats: a magical @seanmonstar fixed all/some of your problems for you.)

@alexcrichton

Copy link
Copy Markdown
Member

I'm a little worried about the explosion of generics when dealing with serialization things. Dealing with decodable/encodable already has a few type parameters which makes signatures a bit unwieldy, and adding another for an error seems a little sad.

Is it crucial that all encoders/decoders need their own error type? Could this all possibly return IoResult?

@wycats

Copy link
Copy Markdown
Contributor

My decoders definitely want their own Error type. I think this is just a side effect of not having yet nailed down the expected patterns for Result propagation.

@seanmonstar

Copy link
Copy Markdown
ContributorAuthor

There is definitely errors that could occur besides IoError. Take this example:

#[deriving(Decodable)]structBar{foo:uint}let json_str = "{\"foo\":false}";// assume we've merged Parser and Decoder (a future PR)let decoder = json::Decoder::from_str(json_str);let bar = matchDecodable::decode(decoder){Ok(b) => b,Err(e) => fail!("{}", e)};

This isn't an IoError. from_str would create MemReader, and those don't return errors. The error would be ExpectedError(~"Number", ~"false").

With Encoders, it's certainly possible that they could only return IoError. However, it's also possible they could return their own errors. For instance, if someone impl Encodable manually, and they do it wrong, or they emit a value that a certain format can't encode, it wouldn't be an IoError. Example:

enumFoo{Bar(~str, ~str)}impl<D:Decoder,E>Decodable<D,E>forFoo{fndecode(&self,d:&mutD) -> Result<(),E>{match*self{Bar(ref a,ref b) => {// i forget to use `emit_enum_variant`
d.emit_enum(|d| {
try!(d.emit_enum_variant_arg(0, |d| d.emit_str(a)));
d.emit_enum_variant_arg(1, |d| d.emit_str(b))})}}}}

A Decoder could be keeping track of a state, and notice that you emitted values wrongly, and return some DecodeError or something.

@erickt As for Travis: it seems it failed downloading llvm dependencies? Can anyone give it a kick?

@alexcrichton

Copy link
Copy Markdown
Member

I kicked off the relevant travis build, and it sounds like custom errors is necessary. Thanks for the explanation!

@seanmonstar

Copy link
Copy Markdown
ContributorAuthor

Oh wow, I somehow missed a bunch of encode stuff in rustc/metadata. Working on it...

@seanmonstar

Copy link
Copy Markdown
ContributorAuthor

there we go, had missed a bunch of in rustc/middle/astencode as well. Sorry for the unwrap_ mess... looks like travis is happyish

@seanmonstar

Copy link
Copy Markdown
ContributorAuthor

@erickt r?

Comment threadsrc/librustc/driver/driver.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should do krate.encode(&mut json).unwrap() here, so we'll fail instead of silently ignoring an error.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was just to remove the unused result warning, because stage0 has it returning (). I could add in the staged unwrap_ functions here as well.

@erickt

Copy link
Copy Markdown
Contributor

Epic work. r=me with my minor comments addressed.

@seanmonstar

Copy link
Copy Markdown
ContributorAuthor

@erickt just force pushed up all the FIXMEs and an unwrap_ in rustc/driver.

@seanmonstar

Copy link
Copy Markdown
ContributorAuthor

@erickt i had missed a doc example, I've fixed and ran them locally to be sure.

All of Decoder and Encoder's methods now return a Result.
Encodable.encode() and Decodable.decode() return a Result as well.
fixesrust-lang#12292
@seanmonstar

Copy link
Copy Markdown
ContributorAuthor

something else landed before bors got to this, requiring another rebase against master.

bors added a commit that referenced this pull request 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
@borsbors closed this Mar 28, 2014
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

6 participants

@seanmonstar@huonw@erickt@alexcrichton@wycats@bors