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

huonw commented Mar 24, 2014

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

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

@erickt

erickt commented Mar 24, 2014

Copy link
Copy Markdown
Contributor

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

@huonw

huonw commented Mar 24, 2014

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

wycats commented Mar 24, 2014

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

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

#[deriving(Decodable)]
struct Bar {
  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 = match Decodable::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:

enum Foo {
  Bar(~str, ~str)
}

impl<D: Decoder, E> Decodable<D, E> for Foo {
  fn decode(&self, d: &mut D) -> 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
Contributor Author

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

@seanmonstar

Copy link
Copy Markdown
Contributor Author

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

@erickt r?

Comment thread src/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
Contributor Author

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

erickt commented Mar 27, 2014

Copy link
Copy Markdown
Contributor

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

@seanmonstar

Copy link
Copy Markdown
Contributor Author

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

@seanmonstar

Copy link
Copy Markdown
Contributor Author

@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.

fixes rust-lang#12292
@seanmonstar

Copy link
Copy Markdown
Contributor Author

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
@bors bors closed this Mar 28, 2014
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 this pull request may close these issues.

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

6 participants