Introduce constructor functions to improve ergonomics of creating error contexts

[ChzenChzen]

Thank you for awesome crate! use it every day in every project.

now

 self.id.to_string().parse().map_err(|error| {
        ParseStrategyKindSnafu {
            strategy_id: self.id,
            error,
        }
        .build()
    })

more concise

 self.id.to_string().parse().map_err(|error| {
        ParseStrategyKindSnafu::build(self.id, error)
})

[ChzenChzen]

another example:

use std::str::FromStr;

use bar::Bar;
use snafu::prelude::*;

#[derive(Debug, Snafu)]
pub enum Error {
    #[snafu(display("foo {bar}"))]
    Foo {
        bar: String,
        source: bar::OtherError,
    },
}

mod bar {
    use super::*;
    #[derive(Debug, Snafu)]
    pub enum OtherError {
        #[snafu(display("context {context}"))]
        OtherError { context: String },
    }
    pub struct Bar;

    impl FromStr for Bar {
        type Err = OtherError;

        fn from_str(s: &str) -> Result<Self, Self::Err> {
            match s {
                "bar" => Ok(Bar),
                // now
                _ => OtherSnafu {
                    context: "some context".to_string(),
                }
                .fail(),
                // more concise
                _ => OtherSnafu::fail("some context"),
            }
        }
    }
}

fn main() -> Result<(), Error> {
    //now
    let b = "bar".parse::<Bar>().context(FooSnafu {
        bar: "some context".to_string(),
    })?;
    // more concise
    let b = "bar"
        .parse::<Bar>()
        .context(FooSnafu::new("some context"))?;

    Ok(())
}

[Enet4]

In the first example, the idiomatic form of creating context is this.

self.id.to_string().parse().context(ParseStrategyKindSnafu {
    strategy_id: self.id,
})

(For this to work you need to rename the field error to source or mark it as the error source explicitly.)

On the second example, context should work too:

        fn from_str(s: &str) -> Result<Self, Self::Err> {
            match s {
                "bar" => Ok(Bar),
                _ => OtherErrorSnafu {
                    context: "some context",
                }.fail(),
            }
        }

This should hopefully reduce the need for a selector constructor function. The few cases which would benefit from such a function are for error variants with a large number of variants, which are also unambiguous in their order (the literal struct construction syntax has better safeguards than function parameter listings).

[ChzenChzen]

@Enet4

use std::str::FromStr;

use snafu::prelude::*;

#[derive(Debug, Snafu)]
pub enum Error {
    #[snafu(display("foo {bar}"))]
    Foo { bar: String, source: String },
}
pub struct Bar;

impl FromStr for Bar {
    type Err = String;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        match s {
            "bar" => Ok(Bar),
            _ => Err("some context".to_string()),
        }
    }
}

fn main() -> Result<(), Error> {
    let _b = "bar".parse::<Bar>().context(FooSnafu {
        bar: "some context".to_string(),
    })?;

    Ok(())
}

error[E0599]: the method as_error_source exists for reference &String, but its trait bounds were not satisfied --> src/main.rs:5:17 | 5 | #[derive(Debug, Snafu)] | ^^^^^ method cannot be called on &String due to unsatisfied trait bounds | ::: /Users/fomotoshi/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/string.rs:362:1 | 362 | pub struct String { | ----------------- doesn't satisfy String: AsErrorSource or String: snafu::Error | = note: the following trait bounds were not satisfied: String: snafu::Error which is required by String: AsErrorSource &String: snafu::Error which is required by &String: AsErrorSource str: Sized which is required by str: AsErrorSource str: snafu::Error which is required by str: AsErrorSource = note: this error originates in the derive macro Snafu (in Nightly builds, run with -Z macro-backtrace for more info)

[ChzenChzen]

@Enet4 selector constructor function can be option in #[snafu(constructor(true))] or top-level under #[derive(Snafu, Debug)]

[Enet4]

In that failing example you seem to be using String as an error in <Bar as FromStr>::Err. Prefer a new custom error type, or a WhateverError which also accepts an arbitrary message.

use std::str::FromStr;

use snafu::prelude::*;

#[derive(Debug, Snafu)]
pub enum Error {
    #[snafu(display("foo {bar}"))]
    Foo {
        bar: String,
        source: ParseBarError
    },
}
pub struct Bar;

#[derive(Debug, Snafu)]
pub enum ParseBarError {
    /// totally not a bar
    NotBar,
}

impl FromStr for Bar {
    type Err = ParseBarError;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        match s {
            "bar" => Ok(Bar),
            _ => NotBarSnafu.fail(),
        }
    }
}

#[snafu::report]
fn main() -> Result<(), Error> {
    let _b = "derp".parse::<Bar>().context(FooSnafu {
        bar: "some context",
    })?;

    Ok(())
}

The output:

Error: foo some context

Caused by this error:
  1: totally not a bar

(note how some errors might not need extra fields if the variants are well outlined)

[Enet4]

One other problem I am seeing with this proposal is that the methods build and fail already exist, so the new associated functions would have to be named something else. Methods use the same namespace as associated functions, and there would not be a way to create multiple overloads to fulfill the use case described.

[ChzenChzen]

@Enet4 String Error common case in third party dependencies

[Enet4]

@Enet4 String Error common case in third party dependencies

That may then be a case for #103. In the meantime, if possible, one can suggest the third party dependency maintainers to make errors idiomatic.

[shepmaster]

one can suggest the third party dependency maintainers to make errors idiomatic.

Right. One question is "how much does this crate want to encourage poor practices from the rest of the ecosystem?" My immediate answer with little thought put into it is "not very much".

or a WhateverError

You can also use snafu::Whatever to wrap it:

use snafu::{prelude::*, FromString as _, Whatever}; // Add imports
use std::str::FromStr;

#[derive(Debug, Snafu)]
pub enum Error {
    #[snafu(display("foo {bar}"))]
    Foo { bar: String, source: Whatever }, // Switch from `String` to `Whatever`
}
pub struct Bar;

impl FromStr for Bar {
    type Err = String;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        match s {
            "bar" => Ok(Bar),
            _ => Err("some context".to_string()),
        }
    }
}

fn main() -> Result<(), Error> {
    let _b = "bar"
        .parse::<Bar>()
        .map_err(Whatever::without_source) // Convert `String` to a `Whatever` which implements `Error`
        .context(FooSnafu {
            bar: "some context".to_string(),
        })?;

    Ok(())
}

Perhaps snafu::Whatever should impl From<String> and then you'd be able to write that as .map_err(Into::into) or .map_err(Whatever::from)...

---

Also note that this is a bad practice:

FooSnafu {
    bar: "some context".to_string(),
}

This always constructs the String, even when no error occurs. Using a string slice is simpler to type and more performant. Using local variables with names that match the fields in the error make it even more streamlined:

let bar = "bar";

let _b = bar
    .parse::<Bar>()
    .map_err(Whatever::without_source)
    .context(FooSnafu { bar })?;