Add backtrace support to derive-Error macro

[ffuugoo]

Implementation is mostly done, but there are 3 unresolved issues:

• The way State process attributes need some reorganization. Right now I've worked around it by sticking conditional on "deriving Error" right into State code.
• I'm not sure what is the proper way to (unstable-)feature-gate macro-generated code.
• I'm not sure what is the proper way to enable/disable tests for unstable features (e.g., backtrace) [1] [2].

[ffuugoo]

@JelteF @tyranron

[ffuugoo]

Had to change file name, as I've moved tests to separate module and Rust won't allow me to have mod error in error.rs.

[JelteF]

For the last two questions, you can revert part of this commit:
ca23e47
That does automatic detection of nightly. I think for tests t's needed for sure. For actual library code I don't know if we want that at all, because as long as you don't have a backtrace field or Backtrace type you won't generate backtrace code. That way if you try to generate backtrace code with a non-nighthly compiler you will get a compiler error saying you should run nightly.

For your first issue I think there's two issues:

1. Your example in the original issue should work without the source attribute. The Backtrace field should be ignored by default as as a source.
2. With three fields (e.g E(Error, i32, Backtrace)) the issue still stands and there should be a solution. I think your solution is slightly hacky but does the job. I'll think a bit more if I can think of a more elegant solution.

FYI I won't have time to review this until mid december.

[ffuugoo]

as long as you don't have a backtrace field or Backtrace type you won't generate backtrace code

I thought the same at first, but that's not entirely true. Consider this situation:

#[derive(Error)]
struct MyError {
    source: SomeOtherError,
}

Now, consider, that SomeOtherError has a backtrace method that actually returns some backtrace. If we completely discard generating backtrace method if no backtrace-field specified, then we would have to explicitly mark source field with #[error(backtrace)] attribute, if we want to generate backtrace method.

It's not the biggest problem, but if there is a way to properly feature-gate generated backtrace method (and I'm not sure it's possible), then I'd prefer to generate feature-gated method if we have either source-field and/or backtrace-field.

[ffuugoo]

Your example in the original issue should work without the source attribute. The Backtrace field should be ignored by default as as a source.

Seems reasonable. I'll improve this part.

But the issue still stands even for two fields case. Consider:

#[derive(Error)]
struct MyError(#[error(not(backtrace))] SomeOtherError, Backtrace);

Specifying #[error(not(backtrace))] on the source-field explicitly disables backtrace-chaining (i.e., when we try to retrieve backtrace from source-field and if it returns None return backtrace stored in structure/enum-variant itself).

However, in this case, specifying valid attribute on a source-field will mark all other fields as "disabled" and we'll get nothing.

[ffuugoo]

I think your solution is slightly hacky but does the job.

It's absolutely hacky. I just put it there as a temporary solution, as reworking State (and other macro) is a big task in itself and I wanted to more-or-less finalize my solution on Error.

[ffuugoo]

Your example in the original issue should work without the source attribute.

Fixed.

For tuple-structs/variants with 1 field

Only infer field as a source-field if it's not "inferrable" as backtrace-field.

#[derive(Error)]
// This field WON'T be inferred as source.
struct MyError(#[error(not(backtrace))] Backtrace); 

#[derive(Error)]
// This field WILL be inferred as source.
struct MyError(#[error(not(backtrace))] AnyOtherType); 

#[derive(Error)]
// You can always mark any field as source explicitly.
struct MyError(#[error(source)] Backtrace);

For tuple-structs/variants with 2 fields

If there is a backtrace-field found (either inferred, or explicitly specified via attribute), infer other field as a source-field, unless it's explicitly marked with [#error(not(source))] attribute.

#[derive(Error)]
// Infers AnyOtherType as source-field
struct MyError(AnyOtherType, Backtrace);

#[derive(Error)]
// Also infers AnyOtherType as source-field
struct MyError(AnyOtherType, #[error(backtrace)] MyBacktrace);

#[derive(Error)]
// Infers Backtrace as source-field
// Kinda unexpected, but the whole case is strange itself
// As far as I see, it's the only such case for 2-fields tuple-struct/variant
struct MyError(Backtrace, #[error(backtrace)] MyBacktrace);

[ffuugoo]

I've experimented a bit with generating feature-gated code and come to conclusion that it's impossible to do properly in our case.

It's possible to generate feature-gated code. And using rustversion crate it's even possible (not at all trivial, though) to feature-gate by compiler channel. But in the end it's impossible to feature-gate by enabled unstable feature. So, we can't generate implementation that would automatically become available when #![feature(backtrace)] is enabled.

I suppose we should add separate error-backtrace (or just backtrace) feature and apply it in implementation instead of nightly.

And tests should stay as is (i.e., feature-gated by nightly and #![cfg_attr(feature = "nightly", feature(backtrace)]).

And then we can also optionally revert rustc_version check in build.rs and force-enable nightly feature from there so that full test-suite is guaranteed to run on nightly.

[JelteF]

Hmm, this indeed sounds like a shitty situation. One thing that would solve it though afaict, is if the default implementation of backtrace would forward to self.source() instead of always returning None. I think that would be a very reasonable thing to do. Could you create a PR for this on rust itself? This should still be possible to change there, since it is still an unstable API.

…

On Fri, 22 Nov 2019, 19:52 Roman Titov, ***@***.***> wrote: I've experimented a bit with generating feature-gated code and come to conclusion that it's impossible to do properly in our case. It's possible to generate feature-gated code. And using rustversion crate it's even possible (not at all trivial, though) to feature-gate by compiler channel. But in the end it's impossible to feature-gate by *enabled unstable feature*. So, we can't generate implementation that would automatically become available when #![feature(backtrace)] is enabled. I suppose we should add separate error-backtrace (or just backtrace) feature and apply it in implementation instead of nightly <https://github.com/JelteF/derive_more/pull/110/files#diff-9a269209bed069ffb36d5565a91c9840R241> . And tests should stay as is (i.e., feature-gated by nightly and #![cfg_attr(feature = "nightly", feature(backtrace)]). And then we can also optionally revert rustc_version check in build.rs and force-enable nightly feature from there so that full test-suite is *guaranteed* to run on nightly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#110?email_source=notifications&email_token=AAI3YJTH6OPLIUTTODFK5NTQVAS65A5CNFSM4JQF4UR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE6Q6WY#issuecomment-557649755>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAI3YJTI3IJ4J34XKURPTDLQVAS65ANCNFSM4JQF4URQ> .

[ffuugoo]

@JelteF

I've seen your questions on RFC 2504. What's your current opinion on this PR?

I've had a discussion with @tyranron and we think, as std::backtrace::Backtrace is still unstable and until some idioms on proper error handling established (e.g., should we source-chain backtraces or not), current solution is good enough.

I'll re-review the PR and probably try to chart all implicit/explicit source/backtrace permutations, to ensure we know what expected behaviour is in each case.

Considering the issue I've explained in this comment, I think we should just accept it and go with "no backtrace by default, enabled with explicit #[error(backtrace)] attribute". As it's the simplest and safest way to do it.

[ffuugoo]

I'll re-review the PR and probably try to chart all implicit/explicit source/backtrace permutations

I've refreshed my context. Seems like tests I've made are good enough specification of expected behaviour.

---

Considering my proposal to "not generate backtrace method implementation implicitly when there is only source-field, but no backtrace-field":

We have to change implementation (and tests) in such way, so:

• all *_no_backtrace_source_with_backtrace_explicitly_disabled tests are removed (as it would be default behaviour and explicit disabling won't make any difference)
• all *_no_backtrace_source_with_backtrace test cases would return None as backtrace (as no backtrace-method implementation would be generated and default implementation returns None)
• a set of *_no_backtrace_source_with_backtrace_explicitly_enabled test cases would be added and all of them would return Some(Backtrace) as backtrace

We also have to make sure that we do actually chain backtrace calls in cases when:

• there is a source attribute without explicit #[error(not(backtrace))] attribute specified
• there is a backtrace-field (inferred or explicitly specified)

But current test suite should catch such errors.

As far as I can see, this should not be too difficult to implement and it also won't introduce any "impossible-to-specify" or "ambiguous" attribute combinations.

[JelteF]

Thanks for picking this up again. Based on the discussion I've had on RFC 2504 I actually think that it would be better if we don't implement any backtrace forwarding. This is an antipattern that I don't think this library should try to make easy.

So i think we should only support a backtrace field, inferred or explicitely specified in cases where it cannot be inferred.

[ffuugoo]

This is an antipattern that I don't think this library should try to make easy.

I have mixed ideas in this regard.

It may be an antipattern, but:

• so far the only compelling case I've seen when such behaviour may be harmful is when an 'error with backtrace' have an 'error with backtrace from another thread' as its source

  • though I'd assume it's less common case

  • backtrace forwarding can be disabled with attributes, so derive-Error still can be used for such errors with very moderate discomfort

• backtrace forwarding is, probably, the most common case of all

• std::backtrace::Backtrace is still unstable and it's unclear for me if community would settle for 'backtrace forwarding as usual default' or 'std::error::Error::backtrace never forward, forwarding implemented with extension-traits'

  • and, if backtrace forwarding happens to be widespread, being a purist against a pattern used by most of community seems impractical

• dtolnay decided to implement backtrace forwarding in thiserror, so, at least one respected Rust developer thinks it's not that bad

  • 'argument from authority' is a logical fallacy, but still...

• we can always change it later, if it would prove to be a faulty decision

  • though, I recognize, that breaking changes for a crate with 1.5 millions downloads may be a potentially questionable decision

@tyranron may also have some valid arguments.

[JelteF]

I think the responses of @mitsuhiko explain why it's an anti pattern pretty clearly here: rust-lang/rust#53487 But for completeness I'll reword it here too. The main reason why it's an anti pattern, is that it causes problems if you want to show the whole error chain (which is usually what you want). In that case you suddenly cannot show all the stacktraces in the chain, because there will be duplicate ones in the chain. Users don't really care about easy backtrace forwarding. They care about easy printing of an error chain with backtraces. As long as you don't implement backtrace forwarding, this function is very easy to implement. If you don't it becomes much harder/impossible. I'd like this library to help bring the ecosystem in a good state, where we have error chains that are easy to display nicely. Not the other way around. So especially with regards to backwards compatibility: I think we should remove backtrace forwarding. Removing it later would be a breaking change. Adding it later should be no problem, if it turns out that we do need forwarding.

…

On Tue, 14 Jan 2020, 20:54 Roman Titov, ***@***.***> wrote: This is an antipattern that I don't think this library should try to make easy. I have mixed ideas in this regard. It *may* be an antipattern, but: - so far the only compelling case I've seen when such behaviour may be harmful is when we have an *'error with backtrace'* have an *'error with backtrace from another thread'* as its source - though I'd assume it's less common case - backtrace forwarding can be disabled with attributes, so derive-Error still can be used for such errors with very moderate discomfort - backtrace-chaining is, probably, the most common case of all - std::backtrace::Backtrace is still unstable and it's unclear for me if community would settle for *'backtrace chaining as usual default'* or *'std::error::Error::backtrace never chains, chaining implemented with extension-traits'* - and, if backtrace-chaining happens to be widespread, being a purist against a pattern used by most of community seems impractical - dtolnay decided to implement backtrace-chaining in thiserror, so, at least one respected Rust developer thinks it's not that bad - 'argument from authority' is a logical fallacy, but still... - we can always change it later, if it would prove to be a faulty decision - though, I recognize, that breaking changes for a crate with 1.5 millions downloads may be a potentially questionable decision @tyranron <https://github.com/tyranron> may also have some valid arguments. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#110?email_source=notifications&email_token=AAI3YJQQAMYT7VKUKCMF3U3Q5YJ6BA5CNFSM4JQF4UR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI542II#issuecomment-574344481>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAI3YJS2TI7OFWNMVO7WNMLQ5YJ6BANCNFSM4JQF4URQ> .

[tyranron]

@JelteF I understand why backtrace forwarding is an anti-pattern when is applied blindly and by default. I agree. Let's just not apply any backtrace forwarding by default. But I cannot understand why we should remove the ability when library user really requires backtrace forwarding in his case? Let's make it just opt-in: apply the forward behavior only if #[error(backtrace(forward))] attribute is applied to the field explicitly. If both attributes #[error(backtrace(forward))] and #[error(backtrace)] are applied to different fields then f1.backtrace().or(f2) logic is applied to generated code.
This way we have all the possible behaviors in a convenient way to use, while not pushing forward harmful anti-patterns by default. Even more, we can explain the reasoning about backtrace forwarding in docs and make an explicit warning about blind applying it everywhere. So, we're not restrict library users in their capabilities, while preferring and pushing forward good practices by default.

[JelteF]

Do you want to use the forwarding feature yourself? If so in what case?

Because if not, I'd much rather have only features that are recommended in derive_more. As far as I know there's no good reason to use the forwarding feature. That's why I don't want to give people the option to choose something bad. And also I don't want to have code to handle a feature that no-one should use.

[JelteF]

So, we're not restrict library users in their capabilities, while preferring and pushing forward good practices by default.

I think it's good to restrict people in their capabilities of shooting themself and the rust ecosystem in the foot.

[tyranron]

@JelteF OK then... I'll try to explain in details our position and errors usage in our codebase.

The main reason why it's an
anti pattern, is that it causes problems if you want to show the whole
error chain (which is usually what you want). In that case you suddenly
cannot show all the stacktraces in the chain, because there will be
duplicate ones in the chain.

In our codebase (~30K CLOC) we have zero cases of showing the whole error chain separately error-by-error, which is quite illustrative that we do not need it at all. What we do instead, is:

• Every error represents an enum and implements Display the way that it renders source error if it's present (with derive_more it's really convenient even for generic errors).
• In most situations we forward the innermost Backtrace up the error chain. In rare situations we prefer the immediate Backtrace instead.
• This way we separate the error displaying and backtracing.
  • To display the whole chain of messages we do not need to dig into source()s at all. We just call Display::fmt and it conveniently renders all the messages chain down the way. It's a well fit for us in all our use cases (displaying in logs, sending stuff to Sentry, output to a client in a debug mode, etc).
  • We do not display Backtrace in Display::fmt implementation. We do not need it for regular error messages. But where/when we need it, it's convenient for us to call backtrace() at the topmost error and receive the innermost Backtrace of the whole chain without digging into source()s. The innermost Backtrace already contains the whole callstack where errors wrapping happens, so there is no need for us to call backtrace() for the all errors in chain, and there is always a single Backtrace only for a single error happened.
• If we need to inspect the errors chain, we rarely do it with source() methods, but rather go with pattern matching as all our errors are pub well-documented enums.

I understand that this way of "doing errors" may be somewhat different from what @mitsuhiko explained, because the problems he has described are non-problems for us. But I cannot call our way of "doing errors" non-idiomatic or bad, just because it plays too well for us. And vice versa: I don't see any reason to propose our way of "doing errors" as the most idiomatic and best one, because it plays quite well only for our backend app case, and may be quite inconvenient for library cases or apps of different kinds.

Do you want to use the forwarding feature yourself? If so in what case?

So, yes. We definitely wanna use forwarding for our codebase. We will use it anyway, but we want derive_more to simplify that for us instead of bare hand copy-paste for almost all errors.

And that's why I'm talking about giving that ability for derive_more users, even if it's not encouraged as a default way for error handling.

[JelteF]

Thanks a lot for the clear explanation. One problem I see with your current approach as is that if you will get a problem if a an Error type from one of your dependencies implements source, but not backtrace forwarding on an inner error of its own (as is recommended).
In that case you will not get the backtrace by calling backtrace() on the outer most error. This is why the community should use the same error handling, so you don't get these problems.

If you would replace your .backtrace() calls with calls to the following function, you should get your desired backtrace behaviour (pseudo code, not tested):

fn first_backtrace(err: Error) -> Option<Backtrace> {
    while err.backtrace() != None {
         if err.source() == None {
             return None
         }
         err = err.source()
    }
    return err.backtrace()
}

If you use that you also don't need backtrace forwarding on your own errors.

So this has the 3 advantages:

1. It works with all dependency errors
2. you don't have to add #[error(backtrace(forward))] to your errors (less lines of code)
3. Your errors don't follow an antipattern

Also in your specific case you even have a bonus advantage:

1. You follow the advice of @mitsuhiko, which is one of the main developers of Sentry. So your sentry integration will probably get better automatically.

[JelteF]

@tyranron Does my suggestion not fix your usecase for #[error(backtrace(forward))] for some reason?

[JelteF]

@tyranron @ffuugoo I'm planning to remove the backtrace(forward) support from this and merge this in. If you give me a reason why my solution above doesn't work for you then I'm fine with adding it in in a separate PR.

[tyranron]

@JelteF sorry for the quite long limbo state of the answer. 😱

First of all:
I'm totally OK with adding backtrace(forward) support in a separate PR. I like well-separated PRs 🙃

And regarding the question itself:
I understand the friction around backtrace forwarding for Sentry in the way they want to use it: each error in chain is printed separately, containing its own backtrace and refering to the source. The situation with backtrace forwarding is really nasty for thins. But I'm arguing that the Sentry way of handling this is far from being the only way. Even in our case/apps, we're OK and want to use Sentry in slightly different manner, which plays totally well for us (we don't need the all chain, we have that Displayed already, we need only the most meaningful Backtrace). So having convenient general-purpose library tools is very desirable (at least for us).

Why the alternatives, described above, won't play well for us:

• If you would replace your .backtrace() calls with calls to the following function, you should get your desired backtrace behaviour (pseudo code, not tested):

  Yup, but the problem here is that such function is quite often required on different layers of app. So, either these layers should depend on some common util (which is bad idea), or some my_std::ErrorExt should be introduced in codebase, which handles .backtrace() semantics differently. While also, backtrace handling logic may vary depending on the error type itself, so this goes too complex instead of having additional attributes for deriving.

• One problem I see with your current approach as is that if you will get a problem if a an Error type from one of your dependencies implements source, but not backtrace forwarding on an inner error of its own (as is recommended).
  In that case you will not get the backtrace by calling backtrace() on the outer most error. This is why the community should use the same error handling, so you don't get these problems.

  I don't see this as a problem. The boundary is well controlled in our code and boundary-level errors will treat it correctly. There is no friction with the fact community/library uses another semantics/conventions for error handling. This happens quite often and not only with error handling. You can't eliminate this totally, but it's easy to make adapters between.

Beside the our situation:
There are situations where backtrace forwarding deriving is still quite helpful. One of examples out of my head is transparent error wrappers, like the one:

enum DbError {
    Postgres(postgres::Error),
    MySql(mysql::Error),
}

Here we do not want the DbError error have its own Display message, or even represent a some kind of self-contained error, but rather be just a transparent enumeration wrapper for the inner errors. And having backtrace(forward) here helps to not write additional code manually, while explicitly states how the error acts with its properties (which is good).

That's why:
I'm very motivated to have backtrace(forward) supported by derive_more out-of-the-box. This makes the library flexible for user needs, so convenient to use. Even more, I think it's a good idea to have backtrace(with = "func_name") where the custom logic may be handled (like serde has for serializing/deserializing) without manual trait implementation.
I'm also think that this question should be explicitly and well overviewed in documentation.

[tyranron]

@ffuugoo please, polish the PR and prepare to merge it without backtrace forwarding.

[JelteF]

Thanks for the response @tyranron (no worries on it taking a bit long). Looking forward to the updated PR without the #[error(backtrace(forward)]

There are situations where backtrace forwarding deriving is still quite helpful. One of examples out of my head is transparent error wrappers

I think this would actually be a separate case where you would want to forward all Error methods with something like #[error(forward)].

I think the reasons you describe why my solution doesn't work make quite a lot of sense and your usecase seems quite common. As far as I can tell the only real solution would be to have the first_backtrace function be a method on the Error trait with a default implementation. Since it's such a simple function I think it makes sense to try to get that to happen. Could you make a PR to rustc adding that method? (please link the PR here as well) I think having this functionality in the Error trait itself makes more sense than "hacking" around it in this derive crate.

If that PR is not accepted I indeed think it makes sense to add this functionality in this crate and I would accept a PR for for backtrace(forward). In that case we would definitely need quite a bit of explanation in the docs of when and when not to use it. Especially that backtrace(forward) isn't suitable for libraries.

[ffuugoo]

@JelteF @tyranron Removed backtrace forwarding support. Messed up with force pushing a bit first time. Repushed properly now.

[tyranron]

@ffuugoo please, consider CI complains.

[ffuugoo]

@ffuugoo please, consider CI complains.

@JelteF @tyranron Done.

[JelteF]

Awesome work! Especially all the tests you added. I left some small comments, on the code. Once you resolve those this can be merged.

[JelteF]

This is now released in version 0.99.5