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

Best way to consolidate error types #391

Closed jondot closed 1 year ago

jondot commented 1 year ago

Hi, I'd like to consolidate error types, meaning, say I have:

[caching providers]
RedisProvider [emits TCPError, IOError]
MemcacheProvider [emits IOError]

[db providers]
MysqlProvider [emits MySqlConnectionError, MySqlAccessError]
SqliteProvider [emits PoolError, IOError]

And I'd like my crate, which hides the various providers, emit just the following errors:

CacheError
DatabaseError

where each of these can but not must also include a source.

So far what I'm doing is:

// at the side of mapping a provider
map_err(|e| Box::from(e)).context(CacheSnafu{ message } )?;

// at the error definition side:
enum Error {
    Cache {
        message: String,
        source: Box<dyn std::error::Error + Send + Sync>,
    },
...

but I feel it's too much friction.

first note i know Whatever maps everything and can be used, but I feel it's too much "catch all", and I can't have more than 1 Whatever in a single enum derived with Snafu.

second note, i might as well just do this, in which case I don't see how Snafu is helping me save my efforts, maybe only for display and such:

    .map_err(|e| Error::Cache {
        message: e.to_string(),
        source: Box::from(e),
    })? as usize;

Any elegant solution for this?

Enet4 commented 1 year ago

Snafu can (and probably should) take care of encasing the source error for you.

enum Error {
    Cache {
        message: String,
        #[snafu(source(from(Box<dyn std::error::Error + Send + Sync>))]
        source: Box<dyn std::error::Error + Send + Sync>,
    },
}

With this you should only need to write this:

do_things().context(CacheSnafu{ message })?;

Error transformation is documented here. This also works if you set up more specific source error types, which would be more typical and more aligned with the design of Snafu errors.

jondot commented 1 year ago

Tried that, but looks like from is expecting some more things:

error: expected `,`
  --> src/chain/mod.rs:12:69
   |
12 |         #[snafu(source(from(Box<dyn std::error::Error + Send + Sync>)))]
   |                                                                     ^
Enet4 commented 1 year ago

Hmm, it's probably missing the constructor parameter, Box (or Some if we were to put it in an Option).

enum Error {
    Cache {
        message: String,
        #[snafu(source(from(Box<dyn std::error::Error + Send + Sync>, Box))]
        source: Box<dyn std::error::Error + Send + Sync>,
    },
}
jondot commented 1 year ago

Yep tried putting a Box::new there as well, And I'm getting into a Sized rabbit hole:

error[E0277]: the size for values of type `dyn snafu::Error + std::marker::Send + std::marker::Sync` cannot be known at compilation time
 --> src/chain/mod.rs:7:17
  |
7 | #[derive(Debug, Snafu)]
  |                 ^^^^^ doesn't have a size known at compile-time
  |
  = help: the trait `std::marker::Sized` is not implemented for `dyn snafu::Error + std::marker::Send + std::marker::Sync`
  = help: the trait `snafu::Error` is implemented for `std::boxed::Box<T>`
  = note: required for `std::boxed::Box<dyn snafu::Error + std::marker::Send + std::marker::Sync>` to implement `snafu::Error`
  = note: required for the cast from `std::boxed::Box<std::boxed::Box<dyn snafu::Error + std::marker::Send + std::marker::Sync>>` to `std::boxed::Box<dyn snafu::Error + std::marker::Send + std::marker::Sync>`
  = note: this error originates in the derive macro `Snafu` (in Nightly builds, run with -Z macro-backtrace for more info)
Enet4 commented 1 year ago

There's probably a missing formula for turning an arbitrary error into Box<dyn std::error::Error + Send + Sync>. I only happen to know this conversion for an Option<Box<dyn std::error::Error + Send + Sync>> @shepmaster Any idea? Maybe we can also extend the documentation with examples such as these.

jondot commented 1 year ago

Yea, I saw some discussion about error mapping <code errors>:<snafu error>

shepmaster commented 1 year ago

And I'm getting into a Sized rabbit hole:

That's the usual error text for having a Box<dyn std::error::Error> and trying to treat it as a type that implements std::error::Error — which the trait object does not. This is a perpetual gotcha of the trait.

use std::error::Error;

fn demo(e: Box<dyn Error>) -> impl Error {
    e
}
error[E0277]: the size for values of type `dyn std::error::Error` cannot be known at compilation time
 --> src/lib.rs:3:31
  |
3 | fn demo(e: Box<dyn Error>) -> impl Error {
  |                               ^^^^^^^^^^ doesn't have a size known at compile-time
  |
  = help: the trait `Sized` is not implemented for `dyn std::error::Error`
  = help: the trait `std::error::Error` is implemented for `Box<T>`
  = note: required for `Box<dyn std::error::Error>` to implement `std::error::Error`

Instead, you'd want something like:

use snafu::prelude::*;

#[derive(Debug, Snafu)]
struct OuterError {
    #[snafu(source(from(InnerError, Box::new)))]
    source: Box<dyn snafu::Error>,
}

#[derive(Debug, Snafu)]
struct InnerError;

Unfortunately, you won't be able to add a second from mapping for another type. Even if the derive macro didn't stop you from attempting, you'd get a compiler error like

conflicting implementations of trait `IntoError<OuterError>` for type `OuterSnafu`

Under the hood, SNAFU generates some code like

impl ::snafu::IntoError<OuterError> for OuterSnafu
where
    OuterError: ::snafu::Error + ::snafu::ErrorCompat,
{
    type Source = InnerError;

    #[track_caller]
    fn into_error(self, error: Self::Source) -> OuterError {
        let error: Box<dyn snafu::Error> = (Box::new)(error);
        OuterError { source: error }
    }
}

This maps a (source error, context selector) to a (output error).

turning an arbitrary error into Box<dyn std::error::Error + Send + Sync>

You'd need to be able to write code like

impl<E> ::snafu::IntoError<OuterError> for OuterSnafu
where
    E: ::snafu::Error,
{
    type Source = E;

The problem with that is that you run into

error[E0207]: the type parameter `E` is not constrained by the impl trait, self type, or predicates
  --> src/main.rs:70:6
   |
70 | impl<E> ::snafu::IntoError<OuterError> for OuterSnafu
   |      ^ unconstrained type parameter

I don't see how to implement that trait for this case.

For the case of multiple from and maybe blanket implementations, a bigger change would be to move away from associated types and use another generic (e.g. something like trait IntoError<Source, Selector>. I don't see it in the git history, but I have a fuzzy recollection that doing so had some annoying type inference outcomes. I suppose now that SNAFU is more widely used, someone could make that change locally and run our mini-crater tool to see what happens.


Something that you could do today would be to add this to your code

trait ResultExt<T, E> {
    fn boxed(self) -> Result<T, Box<dyn snafu::Error + 'static>>
    where
        E: snafu::Error + 'static;
}

impl<T, E> ResultExt<T, E> for Result<T, E> {
    fn boxed(self) -> Result<T, Box<dyn snafu::Error + 'static>>
    where
        E: snafu::Error + 'static,
    {
        self.map_err(|e| Box::new(e) as Box<dyn snafu::Error>)
    }
}

You would then call .boxed() as needed

#[derive(Debug, Snafu)]
struct OuterError {
    source: Box<dyn snafu::Error>,
}

#[derive(Debug, Snafu)]
struct Inner1Error;

#[derive(Debug, Snafu)]
struct Inner2Error;

fn inner1() -> Result<(), Inner1Error> {
    Ok(())
}

fn outer1() -> Result<(), OuterError> {
    inner1().boxed().context(OuterSnafu)
}

fn inner2() -> Result<(), Inner1Error> {
    Ok(())
}

fn outer2() -> Result<(), OuterError> {
    inner2().boxed().context(OuterSnafu)
}

fn meta_outer() -> Result<(), OuterError> {
    outer1()?;
    outer2()?;
    Ok(())
}

We could probably even add that boxed to SNAFU, but we'd have to think about if we want Send or Sync bounds. I'd probably copy what futures does with FutureExt::boxed vs FutureExt::boxed_local.


123 may be related.

shepmaster commented 1 year ago

something like trait IntoError<Source, Selector>

That would look roughly like this

trait IntoError<S, C> {
    fn into_error(source: S, context: C) -> Self;
}

// ---------- Two specific sources, one context
// error1().context(AlphaSnafu)?;
// error2().context(AlphaSnafu)?;

struct S1;
struct S2;

struct C1;

struct OuterError;

impl IntoError<S1, C1> for OuterError {
    fn into_error(source: S1, context: C1) -> Self {
        OuterError // Would have conversion logic here
    }
}

impl IntoError<S2, C1> for OuterError {
    fn into_error(source: S2, context: C1) -> Self {
        OuterError // Would have conversion logic here
    }
}

// ---------- Any possible error (using Display as a placeholder)

use std::fmt;

struct C2;

struct AbsorbingError(String);

impl<S> IntoError<S, C2> for AbsorbingError
where
    S: fmt::Display,
{
    fn into_error(source: S, context: C2) -> Self {
        AbsorbingError(source.to_string())
    }
}

impl fmt::Display for AbsorbingError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        "Absorbing".fmt(f)
    }
}
jondot commented 1 year ago

Thanks for this very thorough explanation! I believe that if boxed where in Snafu it would standardize the pattern that I've seen in many places already, so I feel that it would be a welcome addition, including some section of how to deal with N:1 mapping of errors in the docs.