libserialize: add new error() to Decoder for Decodable objects to signal parse errors
#16130
Conversation
|
cc @erickt |
|
I will push a second commit which fixes |
| @@ -163,6 +163,9 @@ pub trait Decoder<E> { | |||
| fn read_map<T>(&mut self, f: |&mut Self, uint| -> Result<T, E>) -> Result<T, E>; | |||
| fn read_map_elt_key<T>(&mut self, idx: uint, f: |&mut Self| -> Result<T, E>) -> Result<T, E>; | |||
| fn read_map_elt_val<T>(&mut self, idx: uint, f: |&mut Self| -> Result<T, E>) -> Result<T, E>; | |||
|
|
|||
| // Failure | |||
| fn fail_here<T>(&mut self, err: &str) -> Result<T, E>; | |||
There was a problem hiding this comment.
I would have thought that this should be something along the lines of:
fn error(&mut self, err: &str) -> E;Generating a Result isn't necessarily what this is doing, and fail may mislead some into thinking that it invokes fail!()
There was a problem hiding this comment.
How about read_error? And agreed that it should return an E, Result is a confusing signature.
Edit: On second thought I agree error is good. I'll update the PR with that.
|
Could you also amend the first commit message in accordance with our breaking changes policy? Adding a new trait method will break all existing implementations. cc @aturon, @huonw, this seems like a reasonable strategy for #15036 for now. |
apoelstra
changed the title
Decodererror() to Decoder for Decodable objects to signal parse errors
|
@alexcrichton I'm fine with this solution for now. |
|
I'm already using this pattern in rust-serde, so this appoach looks fine to me. Can you change https://github.com/rust-lang/rust/blob/master/src/libserialize/json.rs#L296 to use this error as well? It's not perfect, but I guess we could use the A word a warning though, you're going to hit a merge conflict once #16107 lands. |
|
Hi @erickt, right now the new |
|
@apoelstra: you are right, let's save that for a future pr. |
A quick and dirty fix for rust-lang#15036 until we get serious decoder reform. Right now it is impossible for a Decodable to signal a decode error, for example if it has only finitely many allowed values, is a string which must be encoded a certain way, needs a valid checksum, etc. For example in the libuuid implementation of Decodable an Option is unwrapped, meaning that a decode of a malformed UUID will cause the task to fail. Since this adds a method to the `Decoder` trait, all users will need to update their implementations to add it. The strategy used for the current implementations for JSON and EBML is to add a new entry to the error enum `ApplicationError(String)` which stores the string provided to `.error()`. [breaking-change]
|
Rebase for EBML's move to librbml |
A quick and dirty fix for #15036 until we get serious decoder reform. Right now it is impossible for a `Decodable` to signal a decode error, for example if it has only finitely many allowed values, is a string which must be encoded a certain way, needs a valid checksum, etc. For example in the `libuuid` implementation of `Decodable` an `Option` is unwrapped, meaning that a decode of a malformed UUID will cause the task to fail.
A quick and dirty fix for #15036 until we get serious decoder reform.
Right now it is impossible for a
Decodableto signal a decode error, for example if it has only finitely many allowed values, is a string which must be encoded a certain way, needs a valid checksum, etc. For example in thelibuuidimplementation ofDecodableanOptionis unwrapped, meaning that a decode of a malformed UUID will cause the task to fail.