Error handling when parsing a data structure is too verbose

code review
1

I have a data structure used to make a tree representing data brought in from an existing serialized format.

#[derive(Debug, Clone, PartialEq, EnumAsInner)]
pub enum LLSDValue {
    Undefined,
    Boolean(bool),
    Real(f64),
    Integer(i32),
    UUID(uuid::Uuid),
    String(String),
    Date(i64),
    URI(String),
    Binary(Vec<u8>),
    Map(HashMap<String, LLSDValue>),
    Array(Vec<LLSDValue>),
}

Parsing into that format is straightforward, as is seralizing it. I've written a crate for that.

Getting data from that data structure, though, is a a pain. That requires code like this:

    let id = innermap
        .get("ID").ok_or_else(|| anyhow!("Expected 'ID'"))?
        .as_binary().ok_or_else(|| anyhow!("Expected ID byte Array"))?
        .clone()
        .try_into().map_err(|e| anyhow!("Expected 16 byte array, error{:?}",e))?;

All this is doing is taking a value of the form
Map("ID" : [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16])
and returning an array of 16 bytes. I didn't want to use "unwrap" because then mis-formatted external data could crash the program. I'm using the enum_as_inner crate to create the accessor .as_binary().

The problem is that there's nothing concise like "?" for functions such as hashmap.get() which return None on failure. So I have to write out the whole "ok_or_else" boilerplate, plus a closure, plus a call to "anyhow!". ok_or_else is supposed to be lazily evaluated. I hope.

The "json" crate has a less verbose approach. All their accessors return something like "JsonNone" on fail, and "JsonNone" accepts all the accessors. So JsonNone propagates all the way to the end of the expression, where it can be detected and ejected.

That requires building more machinery into the type. This is a very simple enum type, and accessing it should be equally simple. The verbosity above works fine, but seems excessive.

2

Option<T> does support the ? operator, so if you wrap this in a function or closure that returns Option (or, in the future, a try block), you can simplify this to:

let id = innermap.get("ID")?.as_binary()?.clone().try_into().ok()?;

It also implements the anyhow::Context trait if you prefer returning an error with a custom message attached:

use anyhow::Context;

let id = innermap
        .get("ID").context("Expected 'ID'")?
        .as_binary().context("Expected ID byte Array")?
        .clone()
        .try_into().context("Expected 16 byte array")?;
5 Likes
3

The type HashMap which returns for a failure is not convertible via "?" to std::error::Error.

error[E0277]: the trait bound `NoneError: std::error::Error` is not satisfied
 --> src/main.rs:5:31
  |
5 |     let found = items.get(key)?;
  |                               ^ the trait `std::error::Error` is not implemented for `NoneError`
  |
  = note: required because of the requirements on the impl of `From<NoneError>` for `anyhow::Error`
  = note: required by `from`
4

Well, looking at the problem from a bird's eye view, it appears that you are doing exactly what you are not supposed to do in a strongly-typed language: you are validating, not parsing. In particular, you are putting the deserialized data into a dynamically-typed Value data structure containing a plain hash map, and then you are trying to extract from it what is apparently statically-known information, using a literal string as a key.

Is there any particular reason why you didn't write a Serde Deserializer? In the Serde model, you would instead define the data types you want to serialize and deserialize upfront, and then you would drive the deserializer from those types, converting the byte or text stream directly into a type that you can work with immediately, instead of having to perform many chained fallible lookups. If the data types don't match up, Serde will cooperate with the deserializer, giving you an appropriate error message in one go, without user code needing to manually handle lookup and type errors at every possible level of nesting.

6 Likes
5

As I said, you'll need to wrap the code in a function or closure that returns Option (or a try block, on nightly) if you want to use the ? operator directly, e.g.:

fn getitem(items: HashMap<String, String>, key: &str) -> Option<String> {
    let found = items.get(key)?;
    Some(found.clone())
}
6

No, it doesn't.

the trait `std::error::Error` is not implemented for `NoneError`

It mgiht be nice if it did, but it doesn't.

With enough wrapping, you can do anything, of course.

7

It does, but only in functions that return an Option.

2 Likes
8

This is HashMap's "get", which does return an Option.

pub fn get<Q: ?Sized>(&self, k: &Q) -> Option<&V>

9

Yes, but that's not what I meant. I was referring to the return type of the function that contains the question mark.

3 Likes
10

Here's a slightly more complete example of how you might mix functions returning Option and functions returning anyhow::Result, using ? in both of them:

(I'm looking forward to try expressions, which will make it easier to do both in the same function.)

2 Likes
11

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.