RFC: `From` for boxed errors

[nrc]

In TiKV we often have very large error types (mostly due to including protobuf data with errors) and these errors appear on hot paths (because we try to recover from them). We found that boxing our errors improves our performance by a non-trivial amount.

A small tweak that makes our error handling much more ergonomic is to generate From impls from the unboxed type. Since we often convert error types from external crates into boxed errors, this lets us use a simple ?, rather than mapping the error.

E.g.,

#[derive(Error, Debug)]
pub enum Error {
    #[error(transparent)]
    ProtoErr(#[from] Box<protobuf::Error>),
    #[error(transparent)]
    Other(anyhow::Error),
}

impl From<protobuf::Error> for Error {
    fn from(e: protobuf::Error) -> Error {
        Error::ProtoErr(Box::new(e))
    }
}

I suppose that for backwards compatibility such a thing should be opt-in, e.g., using #[from(unbox)] or using a Cargo feature.

I have a prototype implementation which was straightforward (though I didn't implement any kind of opt-in).

[dtolnay]

Wouldn't it usually be better to box the outer error type, not the variants?

#[derive(Error, Debug)]
#[error(transparent)]
pub struct Error(Box<ErrorKind>);

impl<E> From<E> for Error where ErrorKind: From<E> {
    fn from(err: E) -> Self {
        Error(Box::new(ErrorKind::from(err)))
    }
}

#[derive(Error, Debug)]
#[error(transparent)]
pub enum ErrorKind {
    ProtoErr(#[from] protobuf::Error),
    Other(#[from] anyhow::Error),
}

[nrc]

Wouldn't it usually be better to box the outer error type, not the variants?

That's what we've been doing at the moment in TiKV (using Failure or quick_error). I've been looking into doing it better. I'd prefer to avoid having an outer error type. We often match on nested errors, and it adds an extra layer. It would be nice too (though not essential) to only box the large error types and keep the small ones by value (or avoid 'double boxing' for already boxed error types).

For example, I'd like to simplify code like this: https://github.com/tikv/tikv/blob/master/src/storage/errors.rs#L174-L177

[dtolnay]

Ah, that's a nasty one. You are right that if errors are frequently destructured then layers are unpleasant. FWIW I am hopeful that we can eventually eliminate box keyword and make patterns like Error(ErrorInner::Engine(...)) automatically look inside the box, just as it would currently automatically look past a & or &mut.

Overall I am hesitant to build this into thiserror right now. But our From support is all opt-in and optional so it should be possible to use a different derive macro for the From impls; I think derive_more has generally been on board with building in all kinds of customization.