shepmaster / snafu

Easily assign underlying errors into domain-specific errors while adding context
https://docs.rs/snafu/
Apache License 2.0
1.4k stars 60 forks source link

How to combine errors? #323

Closed bryanlarsen closed 9 months ago

bryanlarsen commented 2 years ago

The documentation talks about splitting a source error into two separate errors. In my experience the converse also often happens, sometimes you want to combine several source errors into a single one. However as a newbie, I've been unable to make this happen. I've attached an MRE of one attempt, but I'm not necessarily asking for how to make my MRE compile -- something completely different that matches my use case would be awesome. For example, I use the InnerError pattern and source(false) but a less verbose solution that doesn't need one or both of those likely would be even better.

use rand::Rng;
use snafu::prelude::*;

#[derive(Debug, Snafu)]
pub enum Error {
    /// Error in the construction of the NFT
    #[snafu(display("Parse Error on {field}: {source}"))]
    ParseError {
        field: String,
        #[snafu(source(false))]
        source: InnerParseError,
    },
}

#[derive(Debug, Snafu)]
pub enum InnerParseError {
    #[snafu(display("YAMLError {source}"))]
    YamlError {
        #[snafu(source(false))]
        source: serde_yaml::Error,
    },

    #[snafu(display("JSONError {source}"))]
    JsonError {
        #[snafu(source(false))]
        source: serde_json::Error,
    },

    #[snafu(display("{message}"))]
    MessageError { message: String },
}

fn main_wrapped() -> Result<String, Error> {
    let r: i32 = rand::thread_rng().gen_range(0..3);

    Ok(match r {
        0 => {
            let _ = serde_yaml::from_str("invalid yaml").with_context(|e| ParseSnafu {
                field: "0",
                source: YamlSnafu { source: e },
            })?;
            "0".to_string()
        }
        1 => {
            let _ = serde_json::from_str("invalid json").with_context(|e| ParseSnafu {
                field: "0",
                source: JsonSnafu { source: e },
            })?;
            "1".to_string()
        }
        _ => {
            ensure!(
                false,
                ParseSnafu {
                    field: "2",
                    source: InnerParseError::MessageError {
                        message: "blah".to_owned()
                    }
                }
            );
            "2".to_string()
        }
    })
}

fn main() {
    match main_wrapped() {
        Ok(s) => {
            panic!("#{s:?}");
        }
        Err(e) => {
            println!("#{e:?}");
        }
    }
}
[package]
name = "mre"
version = "0.1.0"
edition = "2021"

[dependencies]
rand = "0.8"
serde_yaml = "0.8"
serde_json = "1"
snafu = "0.7"

/home/blarsen/.cargo/bin/cargo run --manifest-path /work/gitnft/mre/Cargo.toml 
   Compiling mre v0.1.0 (/work/gitnft/mre)
error[E0271]: type mismatch resolving `<ParseSnafu<&str, YamlSnafu<&mut serde_yaml::Error>> as IntoError<Error>>::Source == serde_yaml::Error`
   --> src/main.rs:36:58
    |
36  |             let _ = serde_yaml::from_str("invalid yaml").with_context(|e| ParseSnafu {
    |                                                          ^^^^^^^^^^^^ expected struct `serde_yaml::Error`, found struct `NoneError`
    |
note: required by a bound in `snafu::ResultExt::with_context`
   --> /home/blarsen/.cargo/registry/src/github.com-1ecc6299db9ec823/snafu-0.7.0/src/lib.rs:529:26
    |
529 |         C: IntoError<E2, Source = E>,
    |                          ^^^^^^^^^^ required by this bound in `snafu::ResultExt::with_context`

error[E0277]: the trait bound `InnerParseError: From<YamlSnafu<&mut serde_yaml::Error>>` is not satisfied
   --> src/main.rs:36:71
    |
36  |               let _ = serde_yaml::from_str("invalid yaml").with_context(|e| ParseSnafu {
    |  __________________________________________________________------------_^
    | |                                                          |
    | |                                                          required by a bound introduced by this call
37  | |                 field: "0",
38  | |                 source: YamlSnafu { source: e },
39  | |             })?;
    | |_____________^ the trait `From<YamlSnafu<&mut serde_yaml::Error>>` is not implemented for `InnerParseError`
    |
    = note: required because of the requirements on the impl of `Into<InnerParseError>` for `YamlSnafu<&mut serde_yaml::Error>`
note: required because of the requirements on the impl of `IntoError<Error>` for `ParseSnafu<&str, YamlSnafu<&mut serde_yaml::Error>>`
   --> src/main.rs:4:17
    |
4   | #[derive(Debug, Snafu)]
    |                 ^^^^^
note: required by a bound in `snafu::ResultExt::with_context`
   --> /home/blarsen/.cargo/registry/src/github.com-1ecc6299db9ec823/snafu-0.7.0/src/lib.rs:529:12
    |
529 |         C: IntoError<E2, Source = E>,
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `snafu::ResultExt::with_context`
    = note: this error originates in the derive macro `Snafu` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0271]: type mismatch resolving `<ParseSnafu<&str, JsonSnafu<&mut serde_json::Error>> as IntoError<Error>>::Source == serde_json::Error`
   --> src/main.rs:43:58
    |
43  |             let _ = serde_json::from_str("invalid json").with_context(|e| ParseSnafu {
    |                                                          ^^^^^^^^^^^^ expected struct `serde_json::Error`, found struct `NoneError`
    |
note: required by a bound in `snafu::ResultExt::with_context`
   --> /home/blarsen/.cargo/registry/src/github.com-1ecc6299db9ec823/snafu-0.7.0/src/lib.rs:529:26
    |
529 |         C: IntoError<E2, Source = E>,
    |                          ^^^^^^^^^^ required by this bound in `snafu::ResultExt::with_context`

error[E0277]: the trait bound `InnerParseError: From<JsonSnafu<&mut serde_json::Error>>` is not satisfied
   --> src/main.rs:43:71
    |
43  |               let _ = serde_json::from_str("invalid json").with_context(|e| ParseSnafu {
    |  __________________________________________________________------------_^
    | |                                                          |
    | |                                                          required by a bound introduced by this call
44  | |                 field: "0",
45  | |                 source: JsonSnafu { source: e },
46  | |             })?;
    | |_____________^ the trait `From<JsonSnafu<&mut serde_json::Error>>` is not implemented for `InnerParseError`
    |
    = note: required because of the requirements on the impl of `Into<InnerParseError>` for `JsonSnafu<&mut serde_json::Error>`
note: required because of the requirements on the impl of `IntoError<Error>` for `ParseSnafu<&str, JsonSnafu<&mut serde_json::Error>>`
   --> src/main.rs:4:17
    |
4   | #[derive(Debug, Snafu)]
    |                 ^^^^^
note: required by a bound in `snafu::ResultExt::with_context`
   --> /home/blarsen/.cargo/registry/src/github.com-1ecc6299db9ec823/snafu-0.7.0/src/lib.rs:529:12
    |
529 |         C: IntoError<E2, Source = E>,
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `snafu::ResultExt::with_context`
    = note: this error originates in the derive macro `Snafu` (in Nightly builds, run with -Z macro-backtrace for more info)

Some errors have detailed explanations: E0271, E0277.
For more information about an error, try `rustc --explain E0271`.
shepmaster commented 2 years ago

If I understand what you want, I'd probably end up with

#[derive(Debug, Snafu)]
pub enum Error {
    #[snafu(display("Parse Error on {field}: {source}"))]
    ParseError {
        field: String,
        source: InnerParseError,
    },
}

#[derive(Debug, Snafu)]
pub enum InnerParseError {
    #[snafu(display("YAMLError {source}"))]
    YamlError { source: serde_yaml::Error },

    #[snafu(display("JSONError {source}"))]
    JsonError { source: serde_json::Error },

    #[snafu(display("{message}"))]
    MessageError { message: String },
}

fn main_wrapped() -> Result<String, Error> {
    let r: i32 = rand::thread_rng().gen_range(0..3);

    Ok(match r {
        0 => {
            let _ = serde_yaml::from_str("invalid yaml")
                .context(YamlSnafu)
                .context(ParseSnafu { field: "0" })?;
            "0".to_string()
        }
        1 => {
            let _ = serde_json::from_str("invalid json")
                .context(JsonSnafu)
                .context(ParseSnafu { field: "0" })?;
            "1".to_string()
        }
        _ => {
            return MessageSnafu { message: "blah" }
                .fail()
                .context(ParseSnafu { field: "2" })
        }
    })
}

Thoughts I had while reading your code:

  1. Why use source(false)? That seems to make your code harder to use and I'm not seeing the benefit.
  2. Why use a string error? While SNAFU has stringly-typed errors like Whatever, I don't like them and would encourage having explicitly listed errors.
  3. The field field appears to be wrong in the 1 match arm.
  4. ensure(false, ...) always produces an Err, so I'd prefer to use fail.
  5. It's debatable to include the source in the Display implementation message as the formatting tends to suck because everything is forced to be on one line.
  6. I don't see anything wrong with repeating the field in each variant of InnerParseError and flattening them out.

Another option, which avoids the repetition of the ParseSnafu, is to use an IIFE (or a try block in nightly Rust). I also switched field to be an i32:

fn main_wrapped() -> Result<String, Error> {
    let field: i32 = rand::thread_rng().gen_range(0..3);

    let tmp = (|| {
        Ok(match field {
            0 => {
                let _ = serde_yaml::from_str("invalid yaml").context(YamlSnafu)?;
                "0".to_string()
            }
            1 => {
                let _ = serde_json::from_str("invalid json").context(JsonSnafu)?;
                "1".to_string()
            }
            _ => return MessageSnafu { message: "blah" }.fail(),
        })
    })();

    tmp.context(ParseSnafu { field })
}

If you really don't have anything to add to the JSON / YAML errors and each can only occur exactly once, then you can remove all context from them:

#[derive(Debug, Snafu)]
pub enum InnerParseError {
    #[snafu(context(false), display("{source}"))]
    YamlError { source: serde_yaml::Error },

    #[snafu(context(false), display("{source}"))]
    JsonError { source: serde_json::Error },

    #[snafu(display("{message}"))]
    MessageError { message: String },
}

fn main_wrapped() -> Result<String, Error> {
    let field: i32 = rand::thread_rng().gen_range(0..3);

    let tmp = (|| {
        Ok(match field {
            0 => {
                let _ = serde_yaml::from_str("invalid yaml")?;
                "0".to_string()
            }
            1 => {
                let _ = serde_json::from_str("invalid json")?;
                "1".to_string()
            }
            _ => return MessageSnafu { message: "blah" }.fail(),
        })
    })();

    tmp.context(ParseSnafu { field })
}
bryanlarsen commented 2 years ago

What an awesome answer! It would be nice to see it in the examples. Calling context twice makes a lot of sense.

Why use source(false)? That seems to make your code harder to use and I'm not seeing the benefit.

That's an artifact of me flailing trying to get something to work. Just reducing the poorly-understood magic. I'm happy to ditch that.

Why use a string error? While SNAFU has stringly-typed errors like Whatever, I don't like them and would encourage having explicitly listed errors.

That's just an artifact of the MRE reductions. There are several more meaningful errors.

The field field appears to be wrong in the 1 match arm.

MRE cut & pasta

ensure(false, ...) always produces an Err, so I'd prefer to use fail.

Another simplification from my code, although I have had problems with fail. But that'd probably go away once I fix the other problems...

It's debatable to include the source in the Display implementation message as the formatting tends to suck because everything is forced to be on one line.

Good point. I'll test that.

I don't see anything wrong with repeating the field in each variant of InnerParseError and flattening them out.

That's a good segue into my next decision point. Do I make InnerParseError opaque or not? Your documentation on that point is pretty good, I'm not sure which is best. Obviously if it's opaque then the fields need to be in the parent. Otherwise flattening makes sense.

Another option, which avoids the repetition of the ParseSnafu, is to use an IIFE (or a try block in nightly Rust). I also switched field to be an i32:

Ooooh, that's a useful trick. Not sure it'll work where I MRE'd from, but it'll definitely be nice elsewhere in my code!

mplanchard commented 2 years ago

I'm trying out snafu, and something I ran into that seems potentially related to this issue is how to represent errors encountered while handling other errors, where I want to retain context about both. Currently I've wound up with something like:

#[derive(Debug, Snafu)]
enum MyInnerError {
    SomeVariant,
}

#[derive(Debug, Snafu)]
enum MyOuterError {
    WrapsInnerError {
        during: MyInnerError,
        source: std::io::Error,
    },
}

fn returns_my_error() -> std::result::Result<(), MyInnerError> {
    todo!()
}

fn returns_io_error() -> std::result::Result<(), std::io::Error> {
    todo!()
}

fn calls_stuff_that_errors() -> std::result::Result<(), MyOuterError> {
    match returns_my_error() {
        Ok(_) => todo!(),
        Err(e) => {
            // handle error by calling some other code that potentially errors
            returns_io_error().context(WrapsInnerSnafu { during: e })
        }
    }
}

But I was just wondering if there was a more standard way to do this. I feel like there should be something I can do by chaining errors .context() calls like in your example above, but I can't figure it out.

I hope you don't mind me hijacking this issue! Let me know if this is too different from the original question and I should open a new one.