deriving Error

[JelteF]

@tyranron I thought some more about your idea for deriving only Error with derive(Error) in #25 and I think it's a great idea. I also saw the new thiserror crate coming by on reddit, which has some cool ideas. I put some ideas on how to add those to derive_more here: dtolnay/thiserror#2 (comment). I asked the author if he wanted to work together on this, but it seems he isn't a fan of that (which is completele fine ofcourse). Please let me know what you think of this syntax and if you're indeed going to pick it up.

For completeness here is the content of my comment with ideas (the rest of the issue thread is also useful for context):

That's definitely a good point. Some options I can think of:

// Specific solution for error, that does auto detection of backtrace field
#[derive(From, Debug)]
enum EnumError {
    #[from(error)] 
    Example{error: io::Error, backtrace: Backtrace),
    NotFromable(i32),
}

// More general, but more verbose
#[derive(From, Debug)]
enum EnumError {
    Example{
       error: io::Error, 
       #[from(default="Backtrace::capture()")] 
       backtrace: Backtrace,
    },
    NotFromable(i32),
}

Because of the way I've implemented attribute handling, the first one would also work easily for the big struct by putting the from attribute on the enum:

#[derive(From, Display, Error)]
#[display("E prefix: {}")
#[from(error)]
pub enum E {
    #[display('io error')]
    E0{source: io::Error, backtrace: Backtrace},
    E1{source: serde_json::Error, backtrace: Backtrace},
    E2{source: regex::Error, backtrace: Backtrace},
    E3{source: hyper::Error, backtrace: Backtrace},
    E4{source: anyhow::Error, backtrace: Backtrace},
    #[from(ignore)]
    #[source(ignore)] // maybe #[error(ignore)]
    #[display("wrong count {count}")]
    SomeCustomError{count: i32, backtrace: Backtrace},
}

I'll think a bit more about this, but I actually like the option above quite a bit.

[JelteF]

Thought a bit more about this and I think the best names for the different attributes are as follows:

// Error specific from derive that creates a backtrace if there's a field named backtrace
#[from(error)]
// Cancels error specific derive and does a normal from derive for this variant
#[from(noerror)]
// Doesn't derive from for this variant (already implemented)
#[from(ignore)]
// Indicate the specific field that is the source
#[error(source)]
// Indicate that this variant has no source
#[error(nosource)]
// Indicate the backtrace field
#[error(backtrace)]
// Indicate that this variant has no backtrace field
#[error(nobacktrace)]
// Indicate the backtrace field

These attributes are only used to change the default behaviour. The default behaviour should be:

• Field is called backtrace, this is returned by backtrace
• Field is called source, this is the source
• Variant is a tuple variant and has a single field, this is the source
• Variant is a struct variant and has a single field, that's not named source, this field is not inferred as source

Feel free to only implement the #[error(source/nosource)] part for now. Also, please look at the current From implementation, since that uses the utils code for handling attributes with enums.

[tyranron]

@JelteF I mostly agree with your design, but have some additions which I will illustrate as a serie of examples (so can be used for tests lately).

One of the main points for me: being clever about generics. This is why I use derive_more and love it so much 🙃

Sources

Default rules are the same as you've described:

• Tuple variant has a single field, this is the source();
• Field is called source, this is the source();
• Struct variant has a single field, that's not named source, this field is not inferred as source().

#[derive(Error)]
enum Tuples<A, B, C> {
    Anna(A),
    Boris(#[error(source)] B, u8),
    Calvin(#[error(not(source))] C)
}

// Expanded:
impl<A, B, C> Error for Tuples<A, B, C>
where A: Error,
      B: Error,
{
    fn source(&self) -> Option<&(dyn Error + 'static)> {
        match *self {
            Self::Anna(ref x) => Some(x as &(dyn Error + 'static)),
            Self::Boris(ref x, _) => Some(x as &(dyn Error + 'static)),
            _ => None,
        }
    }
}

#[derive(Error)]
enum Structs<A, B, C, D> {
    Anna {
        source: A,
    },
    Boris {
        #[error(source)] 
        boris: B,
        num: u8,
    }
    Calvin {
        #[error(not(source))]
        source: C,
    }
    David {
        david: D,
    }
}

// Expanded:
impl<A, B, C, D> Error for Structs<A, B, C, D>
where A: Error,
      B: Error,
{
    fn source(&self) -> Option<&(dyn Error + 'static)> {
        match *self {
            Self::Anna { source: ref x } => Some(x as &(dyn Error + 'static)),
            Self::Boris { boris: ref x, .. } => Some(x as &(dyn Error + 'static)),
            _ => None,
        }
    }
}

Backtraces

While for source() we're relying on field's name as a field type is arbitrary, for backtrace() we have a dedicated type Backtrace we can rely on. That's why defaults may not demand field to be named backtrace, but rather consider fields having Backtrace type.

So, default rules are the following:

• Variant has a source() field, then this is the backtrace().
• Variant has no source() field, but has a field with Backtrace type, then this is the backtrace().

#[derive(Error)]
enum Tuples {
    Anna(Backtrace),
    Boris(u8, Backtrace),
    Calvin(Backtrace, #[error(backtrace)] Backtrace),
    David(String, #[error(not(backtrace))] Backtrace),
    Erik(#[error(source)] AnotherError, Backtrace),
    Farkas(#[error(source)] AnotherError, #[error(backtrace)] Backtrace),
    Vilkas(#[error(source, not(backtrace))] AnotherError, Backtrace),
}

impl Error for Tuples {
    fn backtrace(&self) -> Option<&Backtrace> {
        match *self {
            Self::Anna(ref b) => Some(b),
            Self::Boris(_, ref b) => Some(b),
            Self::Calvin(_, ref b) => Some(b),
            Self::Erik(ref e, _) => e.backtrace(),
            Self::Farkas(_, ref b) => Some(b),
            Self::Vilkas(_, ref b) => Some(b),
            _ => None,
        }
    }
}

#[derive(Error)]
enum Structs {
    Anna {
        backtrace: Backtrace,
    },
    Boris {
        num: u8, 
        bt: Backtrace,
    },
    Calvin {
        foo: Backtrace, 
        #[error(backtrace)]
        bar: MyBacktrace,  // type alias
    },
    David {
        name: String, 
        #[error(not(backtrace))] 
        sup: Backtrace,
    },
    Erik {
        source: AnotherError,
        bt: Backtrace,
    },
    Farkas {
        #[error(source)]
        err: AnotherError,
        #[error(backtrace)]
        bt: Backtrace,
    },
    Vilkas {
        #[error(source, not(backtrace))]
        err: AnotherError,
        bt: Backtrace,
    },
}

impl Error for Structs {
    fn backtrace(&self) -> Option<&Backtrace> {
        match *self {
            Self::Anna{ backtrace: ref b } => Some(b),
            Self::Boris{ bt: ref b, .. } => Some(b),
            Self::Calvin{ bar: ref b, .. } => Some(b),
            Self::Erik { source: ref e, .. } => e.backtrace(),
            Self::Farkas{ bt: ref b, .. } => Some(b),
            Self::Vilkas{ bt: ref b, .. } => Some(b),
            _ => None,
        }
    }
}

Why not() predicate in attributes?

Well... as for me, it:

• reads more naturally;
• allows to refer multiple things, like #[error(not(backtrace, source))];
• is consistent with standard not() predicate in #[cfg()] attributes.

Further improvements

• Ability to use Optioned fields as source() and backtrace().
• Ability to use fields which derefers into Backtrace or smthng like that.

[JelteF]

Thanks for the suggestions:

• I like the not() idea
• checking for the Backtrace type. This does have one caveat which I think is okay, at parsing time you don't actually know that it's the standard library Backtrace. It could be any type that is named Backtrace. However, in that weird case you can use #[error(not(backtrace))]

One thing I would slightly change are these defaults for backtrace

So, default rules are the following:

• Variant has a source() field, then this is the backtrace().
• Variant has no source() field, but has a field with Backtrace type, then this is the backtrace().

I think they would be better like this:

1. Variant has a field with type name backtrace use this for backtrace()
2. Variant has a field with typeBacktrace, use this for backtrace()
3. Variant has a source() field, use that for backtrace.
4. Return None

I think this is better, because now you can add a backtrace to an error that doesn't have one by simply adding a Backtrace to the variant.

This will especially work well once we implement #[from(error)] that automatically populates this backtrace field when From is called.

[tyranron]

@JelteF

• checking for the Backtrace type. This does have one caveat which I think is okay, at parsing time you don't actually know that it's the standard library Backtrace. It could be any type that is named Backtrace. However, in that weird case you can use #[error(not(backtrace))]

Yeah, I thought about this as well, and came to a conclusion that the case were user has its own Backtrace type is especially rare and such user definitely will know what he is doing (even if not, type check will preserve him in this case), so the price of explicit #[error(not(backtrace))] in that rare case is negligible comparing to ergonomic improvements for the majority of cases.

I think this is better, because now you can add a backtrace to an error that doesn't have one by simply adding a Backtrace to the variant.

This will especially work well once we implement #[from(error)] that automatically populates this backtrace field when From is called.

I'm quite unsure about it. My point was in preferring longer backtraces as described here, which, again, from my point of view, represents majority of cases. In my experience you rarely attach Backtrace by hand to the error and this happens always somewhere in downstream errors. More often you just wrapping a downstream error holding its own Backtrace.

Btw... at this point the design for source() is approved and initial implementation can be rolled out.

[JelteF]

I'm quite unsure about it. My point was in preferring longer backtraces as described here, which, again, from my point of view, represents majority of cases. In my experience you rarely attach Backtrace by hand to the error and this happens always somewhere in downstream errors. More often you just wrapping a downstream error holding its own Backtrace.

I think you're making a good point, but the PR for that issue has the best default solution I think:

• if variant has source and Backtrace, return source.backtrace() if it's Some else return Backtrace.

[JelteF]

@tyranron do you still plan on working on this? Otherwise I might spend some time on implementing this (once I've finished updating docs).

[tyranron]

@JelteF yup, @ffuugoo already has some initial implementation and will create PR next week.

[JelteF]

Sounds awesome! @ffuugoo please make use of the State struct. For an example of how to use it with enums please check the From implementation: https://github.com/JelteF/derive_more/blob/master/src/from.rs

You would have to add the attributes required for error here: https://github.com/JelteF/derive_more/blob/HEAD@%7B2019-11-02T19:06:15Z%7D/src/utils.rs#L655-L661

[ffuugoo]

please make use of the State struct

My current implementation is rather complicated, but I've looked into State and From and, as far as I can see, it won't help simplifying Error much if at all. At least, not in any trivial way. What would you say if I open a PR with what I have and we'll see if and how we could simplify and generalise my current solution?

@JelteF

[ffuugoo]

@JelteF I've started working on backtrace support for derive-Error macro and I've encountered a problem with the way you process attributes.

So, in src/utils.rs, in State::with_attr_params you initialize defaults structure and the way you do it, as far as I understand, is that if any field in the struct has any kind of valid attribute on it, then all fields without any attributes are automatically "disabled".

This pose an instant problem for derive-Error. Consider:

#[derive(Debug, derive_more::Display, derive_more::Error)]
struct MyError(#[error(source)] MyOtherError, std::backtrace::Backtrace); 

Specifying #[error(source)] is mandatory, as for tuple-structs we infer source-field only when tuple-struct contains exaclty one field. At the same time, we can infer backtrace-field by it's type. But in this case, as there is a field in the struct with valid attribute defined, all other fields are marked as "disabled", and so backtrace-field does not appear in State::enabled_fields result (and, obviously, I only use fields returned by State::enabled_fields for inferring).

I did not yet dig too deep on this, but it seems, that the way defaults structure is used in State::with_attr_params is a shortcut to implement some other macro (cause if I just force enabled field in defaults struct to always be true I start getting errors from other macro). If that is true, then, I believe, the way it should be done is that these other macro should get State::enabled_fields and then filter these fields on attributes they actually depend on, not just expect State::enabled_fields to have single field.

So, what should I do? I, probably, can figure how to fix it myself, but if you can give some guidance off top of your head, that would be nice. Also, should I open separate PR for this fix, or just include it into "derive-Error Backtrace support" PR?

(Notifying @tyranron on the issue as well.)

[JelteF]

Closing this now, since it has been merged.