dtolnay / thiserror

derive(Error) for struct and enum error types
Apache License 2.0
4.53k stars 160 forks source link

Consider allowing extra fields in variants with #[from] attribute by specifying default values #288

Closed brunoczim closed 4 months ago

brunoczim commented 8 months ago

I am facing this situation:

#[derive(Debug, Error)]
pub enum Error {
    #[error(
        "{}{}{}",
        match .path { Some(ref path) => &path[..], None => "" },
        match .path { Some(_) => ": ", None => "" },
        .source,
    )]
    Tera {
        path: Option<String>,
        #[from]
        #[source]
        source: tera::Error,
    },
   // other variants omitted
}

Which yields this error:

error: deriving From requires no fields other than source and backtrace
  --> src/lib.rs:26:9
   |
26 |         #[from]
   |         ^^^^^^^

However, there is a pretty reasonable implementation of From<tera::Error> for Error: make this extra field path be constructed with None. Something like:

impl From<tera::Error> for Error {
    fn from(source: tera::Error) -> Self {
        Self::Tera { path: None, source }
    }
}

I could implement it manually, but it feels like it's a work that the procedural macro could be doing. Not only in my situation, but also in many similar situations, From could be automatically implemented constructing extra fields with some given default values. I'd propose this API:

#[derive(Debug, Error)]
pub enum Error {
    #[error(
        "{}{}{}",
        match .path { Some(ref path) => &path[..], None => "" },
        match .path { Some(_) => ": ", None => "" },
        .source,
    )]
    Tera {
        #[default(None)]
        path: Option<String>,
        #[from]
        #[source]
        source: tera::Error,
    },
   // other variants omitted
}

And also, to improve ergonomics, if the default value is omitted from the attribute, the proc-macro could generate Default::default() as the constructed value. For instance, since None == Default::default(), the code above would be equivalent to the code below:

#[derive(Debug, Error)]
pub enum Error {
    #[error(
        "{}{}{}",
        match .path { Some(ref path) => &path[..], None => "" },
        match .path { Some(_) => ": ", None => "" },
        .source,
    )]
    Tera {
        #[default]
        path: Option<String>,
        #[from]
        #[source]
        source: tera::Error,
    },
   // other variants omitted
}

I intend to implement this myself if I find some time to do it, but I need feedback first. Is this feature welcome? If I open a PR with this, would it be welcome?

dtolnay commented 4 months ago

I would prefer not to build this into thiserror. I think it's just fine to handwrite the simple From impl as you showed, or you can look for a more fully-featured derive(From) library that could generate this kind of impl based on attributes.