Fix the Error trait

[withoutboats]

Rendered
Tracking issue

[matklad]

Alternative to source:

fn caused_by(&self) -> Option<&dyn Error + 'static>

[algesten]

Or

fn reason(&self) -> Option<&dyn Error + 'static>

[Nemo157]

Has no_std support for Backtrace been considered? The primary reason for Error being in std instead of core seems to be coherency issues, and I would love to be able to have it moved to core at some point in the future.

[ben0x539]

I'm gonna be extremely sad if we end up with an awkward synonym for cause.

[mitsuhiko]

I don’t want to chime in too much on the bikeshedding but with regards to naming here is what others do:

• .NET: InnerException for cause. Source is an application name hint there
• PHP: getPrevious()
• Python: __cause__ and __context__
• Java: getCause()
• Ruby: cause
• Go: Cause()

Cause is the most common but obiously not available. For the non cause uses they are all different.

However one other thing that comes to mind is that since we are already fixing this it might be reasonable to consider aggregate exceptions as well. A fanout can collect multiple errors that might make sense to capture. In that case the name causes() could become available and return no cause, a single cause or a slice of causes. Thoughts?

[withoutboats]

Has no_std support for Backtrace been considered?

In failure backtrace just always act like the backtrace variable is off when compiled in no_std. We could possibly do the same thing with core someday.

In that case the name causes() could become available and return no cause, a single cause or a slice of causes. Thoughts?

Not sure what you were thinking but &[dyn Error + 'static] is not a valid type because &[T] requires T: Sized.

[epage]

1. Should the provided backtrace automatically chain?

fn backtrace(&self) -> Option<&Backtrace> {
    self.source().map(|s| s.backtrace())
}

2. How much implementation flexibility should there be for Backtrace?

This is more hypothetical. I am not completely sure on motivations. Possible motivations:

• Python templating languages mix template backtraces with python backtraces
• Separating out the no_std from std logic

Possible ways of doing this

• Making it a trait and have the function return Box<dyn Backtrace>
  • But requires alloc
• Backtrace is a container only and the way of gathering backtraces is decoupled
  • Requires stablizing a lot of the API

3. One of the selling points to failure was no_std but I didn't see it called out in the RFC. Any relevant no_std parts carried over?

I can't remember how it was good for no_std and if they was Fail vs Error or failure vs error-chain.

[tmandry]

Does this mean Backtrace::new should return an Option, or that it will simply not be called by any code inside std (and always produces a backtrace when called)?

[bstrie]

The problem with the existing cause API is that the error it returns is not 'static. This means it is not possible to downcast the error trait object, because downcasting can only be done on 'static trait objects (for soundness reasons).

For what reason was cause's return type not marked 'static to begin with?

The backtraces provided by the standard library are designed for user display purposes only

Given that the text makes a prior distinction "end-user display message" and "programmer debug message", I think the quoted section intends to suggest that backtraces are for programmers rather than end-users, yes?

[sfackler]

It seems kind of weird for it to be impossible to get a backtrace at all if the right environment variable isn't set. It seems reasonable to have a constructor that returns an Option<Backtrace> by checking the environment, but I think Backtrace::new should just unconditionally produce a backtrace.

[mitsuhiko]

I agree. I effectively have to force this envvar on all the time now on startup which seems like a weird way to control the system.

[epage]

@withoutboats

the behavior of a Backtrace is based on whether or not that environment variable is set.

Personally, I feel like that policy should be decoupled from being able to get a Backtrace. We should have an aligned default policy but we should make it possible for people to create policies to suit their needs. For example, this RFC is bringing up a change in policy:

From RFC

Two additional variables will be added: RUST_PANIC_BACKTRACE and RUST_STD_BACKTRACE: these will independently override the behavior of RUST_BACKTRACE for backtraces generated for panics and from the std API.

How will Backtrace know its calling context to use these two variables?

@withoutboats

If it isn't, its a null type that doesn't collect any backtrace information and prints nothing when displayed.

And in Errors case, is there a reason to use null objects rather than Option like the API?

[sfackler]

For what reason was cause's return type not marked 'static to begin with?

It was an oversight - Error used to have a 'static bound but when that was removed we forgot about the implications for cause.

[withoutboats]

@tmandry @sfackler To clarify, the way that error-chain and failure both work, Backtrace::new returns a Backtrace unconditionally, but the behavior of a Backtrace is based on whether or not that environment variable is set. If it isn't, its a null type that doesn't collect any backtrace information and prints nothing when displayed.

[aturon]

cc @rust-lang/libs

[mitsuhiko]

Not sure what you were thinking but &[dyn Error + 'static] is not a valid type because &[T] requires T: Sized.

I was actually thinking it would return some sort of wrapped [&dyn Error + 'static]. However I suppose since this means the return value needs to be an enum representing a single cause, no cause or multiple it adds quite a bit of potential overhead (and probably needs a smallvec or heap right away for the multiple case).

I do not completely want to lose track of the aggregation case though because with async code that will come up sooner or later.

[alexcrichton]

Along the lines of creating a backtrace, I agree with @sfackler's comment as well! I wonder, would something like this suffice?

impl Backtrace {
    // always captures a backtrace, unless platform
    // does not support it, in which case `None` is returned
    pub fn new() -> Option<Backtrace>;

    // optionally capture a backtrace, dictated by environment variables
    // returns `None` if env vars say to not capture a backtrace 
    // or on platforms that don't support it
    pub fn maybe_new() -> Option<Backtrace>;
}

That way if a tool wants to unconditionally capture a backtrace they have a means of doing so. At the same time though we'd provide an idiomatic constructor for crates like failure to use which I think is still effectively serving the same purpose?

[bluetech]

Two additional variables will be added: RUST_PANIC_BACKTRACE and RUST_STD_BACKTRACE: these will independently override the behavior of RUST_BACKTRACE for backtraces generated for panics and from the std API.

Is it possible to make panic use std::backtrace in some fashion? Maybe add it to std::panic::PanicInfo? I don't know if it is possible, but if "panic backtraces" and "std backtraces" were the same thing, that sounds better.

[mitsuhiko]

@alexcrichton agreed with this a whole bunch. I would generally not make new capture a backtrace however but add an explicit function. The reason for this is that there are lots of reasons why code might want to create a synthetic backtrace (sending to other processes for instance) so it would be great not to close that option for later.

Instead of maybe_new one could also imagine a version that takes an enum to handle the error vs panic case:

impl Backtrace {
    // capture a backtrace unless unsupported.
    pub fn capture() -> Option<Backtrace>;

    // capture a backtrace if possible and wanted for the given situation.
    pub fn capture_for(c: BacktraceConsumer) -> Option<Backtrace>;
}

pub enum BacktraceConsumer {
    Panic,
    Error,
}

(I started a spec for a better backtrace object a while back but never finished it because I ran into limitations with the symbolication support in backtrace.rs that makes the API I envisioned really hard to get. If there is interest I can unearth what I wrote together though and remove those bits).

[withoutboats]

I'm uncertain what the benefit of any -> Option<Backtrace> constructor is. How is a user supposed to handle the None case there? I can't think of any way to handle it other than carrying an Option<Backtrace> indefinitely, ultimately checking for the None case when you print it out; in other words, the same behavior that having nullity be a permitted backtrace behavior. I also notice that there is no API in which backtrace is actually present unconditionally; it seems more correct and more user friendly to incorporate this nullity into the behavior of the backtrace type instead.

I don't know if it is possible, but if "panic backtraces" and "std backtraces" were the same thing, that sounds better.

The distinction is an intentional feature; I received a lot of feedback on failure that users want the ability to control the backtraces they get from panics differently from the backtraces created by their libraries, because they can't afford to have backtraces in library code but want to have backtraces when things catastrophically fail.

[mitsuhiko]

Right now I as a user check against an empty string return from display. Hardly any better :)

…

On Fri, Jul 20, 2018 at 2:06 PM boats ***@***.***> wrote: I'm uncertain what the benefit of any -> Option<Backtrace> constructor is. How is a user supposed to handle the None case there? I can't think of any way to handle it other than carrying an Option<Backtrace> indefinitely, ultimately checking for the None case when you print it out; in other words, the same behavior that having nullity be a permitted backtrace behavior. I also notice that there is no API in which backtrace is actually present unconditionally; it seems more correct and more user friendly to incorporate this nullity into the behavior of the backtrace type instead. I don't know if it is possible, but if "panic backtraces" and "std backtraces" were the same thing, that sounds better. The distinction is an intentional feature; I received a lot of feedback on failure that users want the ability to control the backtraces they get from panics differently from the backtraces created by their libraries, because they can't afford to have backtraces in library code but want to have backtraces when things catastrophically fail. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2504 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAc5NVDzzXACXD8TydHh-OLep7KACBoks5uIcfagaJpZM4VWUn_> .

[withoutboats]

@mitsuhiko What do you do when the string is empty? That's what I'm trying to get at: for most use cases, I don't know how you would handle a null backtrace. Backtrace::capture().unwrap_or(/* ????? */)

(I think the better solution would be to have an API on backtrace that tells you if its null or not.)

[mitsuhiko]

@withoutboats in the trivial case i render differently (eg: i render out "stacktrace:\n{}" vs "set the envvar to get stacktraces). More importantly though I currently also need to regex parse the backtrace since there is no api to access so there is more to this. With a backtrace API I could also see that I did not get any frames.

For me the main difference between Option<Backtrace> and Backtrace without frames is that the former indicates that no backtrace was requested and the latter that the backtrace was empty (limitations). For me those are different things and I want to communicate them to the user.

[withoutboats]

For me the main difference between Option and Backtrace without frames is that the former indicates that no backtrace was requested and the latter that the backtrace was empty (limitations).

I don't know what you mean by "no backtrace was requested," but I'm having trouble understanding how this comment squares with the API you proposed above, in which both constructors return an Option<Backtrace>. Isn't Backtrace::capture returning None because of platform limitations?

[mitsuhiko]

I don't know what you mean by "no backtrace was requested,"

The usecase that comes up for me is that right now some APIs (like panic handlers, or errors with stacktraces bubbled up to main) can show backtraces to the user but only if the envvar is set. So I want to hint to the user when to turn it on. Not requested was that the envvar was not set.

in which both constructors return an Option. Isn't Backtrace::capture returning None because of platform limitations?

It would maybe make sense to have the Backtrace::capture method always return a backtrace, but it might be empty. I'm not sure what the behavior there should be. For me it would be fine that capture and capture_for have different failure modes.

[jonhoo]

How about:

enum Reason {
    Envvar,
    NotSupported,
}

enum Backtrace {
    Captured(RealBacktrace),
    NotCaptured(Reason)
    Synthetic
}

impl Default for Backtrace {
    fn default() {
        Backtrace::Synthetic
    }
}

impl Backtrace {
    fn capture() -> Backtrace() {
        unimplemented!("actually capture a backtrace here, NotSupported if not supported")
    }

    fn new() -> Backtrace {
        match envvar {
            true => Backtrace::capture(),
            false => Backtrace::NotCaptured(Reason::Envvar),
        }
    }
}

Then any user can place the desired printing on top of that.

[withoutboats]

An example of why including the nullity in the backtrace is useful: if we don't, presumably errors will be storing Option<Backtrace>; when failure's Error type goes to check if the error you're casting into it has a backtrace, it will get None, and try to create another backtrace; this is wrong, since the backtrace will fail again.

I think it makes sense for backtrace to report its status somehow; I initially imagined a boolean, but probably we want it to be to give more information, that is, something like:

#[non_exhaustive]
enum BacktraceStatus {
    Captured,
    Disabled,
    Unsupported,
}

impl Backtrace {
    pub fn status(&self) -> BacktraceStatus
}

You could match on this status to convey to users, e.g. "turn the env var on," or "backtraces are unsupported on this platform"

[epage]

@withoutboats how are you planning on handling this requirement:

Two additional variables will be added: RUST_PANIC_BACKTRACE and RUST_STD_BACKTRACE: these will independently override the behavior of RUST_BACKTRACE for backtraces generated for panics and from the std API.

I have some thoughts on the API but I feel like the answer to the above might also help shape it and want to make sure its included.

[joshtriplett]

Under "Drawbacks", could you please explicitly document whether there's any functionality drawback to adding the 'static bound on source? Is there any error pattern that that would prevent? And if not, could you state that explicitly?

[mitsuhiko]

@GuillaumeGomez "With this trait, you don't have the choice anymore". I do not see how this trait changes anything. You have the same choice now as you had before.

[dekellum]

Please reviewers, bias toward action (merging this) unless you find substantive compatibility problems not covered in The transition plan section of this RFC.

[gnzlbg]

Does creating a backtrace allocate?

I suppose it might allocate a String but then Error probably cannot be used anywhere close to an allocator or one must be careful doing so. This limitation is ok, but I am not sure what the scope of this limitation is.

Also, Backtrace contains a backtrace to what exactly? Where is the “leaf” stack frame of this backtrace? Is it some implementation detail of the Backtrace machinery? The stack frame of the caller method? Platform / optimization dependent (e.g. inlining)?

I’d expect that for the backtrace to be useful it must point to the caller stackframe and all stackframes from the Backtrace implementation must have been pruned, but I haven’t found any information about this in the RFC or the comments. Sorry if this was already asked or discussed.

[withoutboats]

@gnzlbg @GuillaumeGomez This RFC proposes to add this default method to the trait Error:

fn backtrace(&self) -> Option<&Backtrace> {
     None
}

Any error which doesn't include a backtrace just doesn't override this method; in other words, nothing about that experience changes. What has changed is that we have created a standardized API for accessing the backtrace of an error which does have one.

[GuillaumeGomez]

I'm just afraid that on the opposite to std lib, you won't be able to disable backtraces when using crates. Maybe I'm just worrying about things that won't happen but still...

[mitsuhiko]

There is no difference to today. There are already crates that carry backtraces.

[DDOtten]

enum BacktraceStatus {
    Captured,
    Disabled,
    Unsupported,
}

Small tweak: I would take Enabled instead of Captured for consistency with Disabled.

[GuillaumeGomez]

There is no difference to today. There are already crates that carry backtraces.

And this is part of why I don't use them.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[Centril]

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#53487

[gnzlbg]

@withoutboats

Any error which doesn't include a backtrace just doesn't override this method; in other words, nothing about that experience changes. What has changed is that we have created a standardized API for accessing the backtrace of an error which does have one.

From the first line of the RFC:

Introduce a backtrace module to the standard library to provide a standard interface for backtraces.

In particular, this RFC adds an API, Backtrace::capture, that creates a backtrace.

My two questions are:

• What backtrace does this API create (a backtrace to where exactly)?
• Does creating one allocate memory? take some locks? etc.

The RFC doesn't even say what a backtrace is, and when it states: "Capture the backtrace for the current stack if it is supported on this platform" I think it is important to state in which state the current stack is when its backtrace is captured. E.g. if I write:

fn foo() {
   let b = Backtrace::capture();
   println!("{}", b);
}

what am I going to see ?

...
...
foo

or

...
...
foo
Backtrace::capture
Backtrace::barbaz
os::raw:..
...

?

I find the meaning of:

The backtraces provided by the standard library are designed for user display purposes only, and not guaranteed to provide a perfect representation of the program state, depending on the capabilities of the platform.

unclear. What does "user display purposes" mean ?

The answers to all these questions are the difference between getting a backtrace that I can just pass to panic to abort a process, or whether I can actually parse the backtrace to do something useful.