shepmaster / snafu

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

Consider adding #[snafu(transparent)] #406

Closed shepmaster closed 9 months ago

shepmaster commented 1 year ago

While working on the playground recently, I found myself wanting a "transparent" error. Someone has mentioned this to me before, but we don't seem to have an issue for it.

The TL;DR of my case:

In the original playground implementation, the two data types shared one error type (e.g. enum Error { BadEdition, BadMode }) but that caused some annoying circular type problems and runs contrary to the normal Rust practice of one error per parse (e.g. ParseBoolError). Moving them to separate types leads to a catch — what do you put as the Display implementation on the outer error?

Concretely, code like

if let Err(e) = parse_run("moo", "debug") {
    println!("Couldn't parse `Run`: {e}");
}

Would print out:

Couldn't parse `Run`: Edition: 'moo' is not a valid edition

The Edition: is the auto-generated error message, but there's not really any better text to put here. Ideally I'd just completely remove that middle text and end up with Couldn't parse `Run`: 'moo' is not a valid edition.

Since we would want to use the source's Display, we probably shouldn't return the source as an error itself, which is why this sounds like "transparent" to me.

Rough suggested implementation

Adding #[snafu(transparent)] to an enum variant will delegate the implementation of Display and Error::source to the source field.

Open questions

Complete code example ```rust use snafu::prelude::*; use std::str::FromStr; // Data types enum Edition { Rust2015, Rust2018, } impl FromStr for Edition { type Err = ParseEditionError; fn from_str(s: &str) -> Result { Ok(match s { "2015" => Edition::Rust2015, "2018" => Edition::Rust2018, value => return ParseEditionSnafu { value }.fail(), }) } } #[derive(Debug, Snafu)] #[snafu(display("'{value}' is not a valid edition"))] struct ParseEditionError { value: String, } enum Mode { Debug, Release, } impl FromStr for Mode { type Err = ParseModeError; fn from_str(s: &str) -> Result { Ok(match s { "debug" => Mode::Debug, "release" => Mode::Release, value => return ParseModeSnafu { value }.fail(), }) } } #[derive(Debug, Snafu)] #[snafu(display("'{value}' is not a valid mode"))] struct ParseModeError { value: String, } // Requests #[allow(dead_code)] struct Run { edition: Edition, mode: Mode, } fn parse_run(edition: &str, mode: &str) -> Result { Ok(Run { edition: edition.parse()?, mode: mode.parse()?, }) } #[derive(Debug, Snafu)] enum ParseRunError { #[snafu(context(false))] Edition { source: ParseEditionError }, #[snafu(context(false))] Mode { source: ParseModeError }, } #[allow(dead_code)] struct Build { edition: Edition, mode: Mode, } fn parse_build(edition: &str, mode: &str) -> Result { Ok(Build { edition: edition.parse()?, mode: mode.parse()?, }) } #[derive(Debug, Snafu)] enum ParseBuildError { #[snafu(context(false))] Edition { source: ParseEditionError }, #[snafu(context(false))] Mode { source: ParseModeError }, } // Usage fn main() { if let Err(e) = parse_run("moo", "debug") { println!("Couldn't parse `Run`: {e}"); } if let Err(e) = parse_build("2018", "cow") { println!("Couldn't parse `Build`: {e}"); } } ```
Enet4 commented 1 year ago

I may have had some cases where the variant message was redundant in a similar fashion, though I can't find an example of this right now. There is also not a fully clear workaround to avoiding this redundancy (#[snafu(display("{source}"))] just duplicates the error message in the chain). As such, and seeing that #[snafu(transparent)] is to be made available as an enum variant, I'm all in for this. :+1:

This would often be paired with context(false) — would it always? Should transparent imply context(false)?

I don't think I ever used context(false). I don't hold a strong opinion about this, although this might be revealing cases where I could be using context(false) and saving some .context calls. In terms of API stability, both transparent and context(false) have potentially breaking implications, so not much to go from here either.

shepmaster commented 1 year ago

cases where I could be using context(false) and saving some .context calls.

Every time I use context(false) I have to pause for a bit. It's nice because the call-site is cleaner (just a ? — hooray!), but it's a bit of a subtle forwards-compatibility footgun. If I somehow do a second action with the same error, it's very tempting to copy-paste the existing ? code and then I've degraded my error a bit.

seritools commented 11 months ago

For reference, thiserror requires the equivalent of context(false) when using transparent thiserror's transparent has the same requirements as context(false):

error: #[error(transparent)] requires exactly one field

EDIT: Hmm, I guess you could still create the context selector for it, it just has the same requirements as context(false).

EDIT 2: Looking into it more, I'm not sure why you'd want the context selector anymore if you're trying to handle the inner error fully transparently.

nightkr commented 8 months ago

Maybe I'm missing something here, as far as I can tell this breaks downcasting into the wrapped error, right? Normally you'd be able to do something like err.iter_chain().find_map(|e| e.downcast_ref::<MyError>()) to see if the error chain contains MyError. However, with #[snafu(transparent)], .source() would skip over the wrapped error!

Ideally, I think the solution here would be for #[snafu(transparent)] to also forward is and the downcast* family of methods, but that wouldn't work since they're implemented on dyn Error rather than on Error itself...

shepmaster commented 8 months ago

Here's the code I used to explore:

use snafu::prelude::*;

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

#[derive(Debug, Snafu)]
#[snafu(transparent)]
struct MiddleError {
    source: InnerError,
}

#[derive(Debug, Snafu)]
struct OuterError {
    source: MiddleError,
}

fn main() {
    let source = InnerError;
    let source = MiddleError { source };
    let e = OuterError { source };

    snafu::ChainCompat::new(&e).for_each(|e| eprintln!("{e}"));
    eprintln!();

    snafu::ChainCompat::new(&e).for_each(|e| eprintln!("{e:?}"));
    eprintln!();

    let o = snafu::ChainCompat::new(&e).find_map(|e| e.downcast_ref::<OuterError>());
    eprintln!("{o:?}");

    let m = snafu::ChainCompat::new(&e).find_map(|e| e.downcast_ref::<MiddleError>());
    eprintln!("{m:?}");

    let i = snafu::ChainCompat::new(&e).find_map(|e| e.downcast_ref::<InnerError>());
    eprintln!("{i:?}");
}

Producing the output

OuterError
InnerError

OuterError { source: MiddleError { source: InnerError } }
MiddleError { source: InnerError }

Some(OuterError { source: MiddleError { source: InnerError } })
Some(MiddleError { source: InnerError })
None

First things first, I'll say that I rarely go the downcasting route. Often downcasting means that I haven't constructed my code correctly. Practically, the cases where I have downcast errors has been when using a third-party crate that structured errors in a way I didn't like. The proper solution there is more likely to talk to the upstream crate's maintainer, but downcasting is more immediate and completely in my control.

I'm not sure if there is a solution here. Like you said, I don't see a way to override the downcasting methods.

Looking at it from another direction, the inability could be considered a feature. While #[snafu(transparent)] is a new feature, the core behavior has been available via opaque errors:

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

This produces the same output as above. Using an opaque error is done purposefully so that the outside world can never get the concrete type and it's usually private.

Of additional note is that we behave the same as thiserror's usage of transparent:

use thiserror::Error;

#[derive(Debug, Error)]
#[error("InnerError")]
struct InnerError;

#[derive(Debug, Error)]
#[error(transparent)]
struct MiddleError {
    source: InnerError,
}

#[derive(Debug, Error)]
#[error("OuterError")]
struct OuterError {
    source: MiddleError,
}
nightkr commented 8 months ago

First things first, I'll say that I rarely go the downcasting route. Often downcasting means that I haven't constructed my code correctly.

Agreed. I'm not saying that should block the feature, just that maybe it warrants a caveat in the docs.