Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the Error trait #2504

Merged
merged 3 commits into from
Aug 19, 2018
Merged

Fix the Error trait #2504

merged 3 commits into from
Aug 19, 2018

Conversation

withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Jul 19, 2018

@withoutboats withoutboats added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jul 19, 2018
@matklad
Copy link
Member

matklad commented Jul 19, 2018

Alternative to source:

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

@algesten
Copy link

Or

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

@Nemo157
Copy link
Member

Nemo157 commented Jul 19, 2018

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
Copy link

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

@mitsuhiko
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

epage commented Jul 19, 2018

  1. Should the provided backtrace automatically chain?
fn backtrace(&self) -> Option<&Backtrace> {
    self.source().map(|s| s.backtrace())
}
  1. 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
  1. 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.


Today, the `RUST_BACKTRACE` controls backtraces generated by panics. After this
RFC, it also controls backtraces generated in the standard library: no backtrace
will be generated when calling `Backtrace::new` unless this variable is set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

bstrie commented Jul 19, 2018

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?


Today, the `RUST_BACKTRACE` controls backtraces generated by panics. After this
RFC, it also controls backtraces generated in the standard library: no backtrace
will be generated when calling `Backtrace::new` unless this variable is set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member

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
Copy link
Contributor Author

withoutboats commented Jul 19, 2018

@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
Copy link
Member

aturon commented Jul 19, 2018

cc @rust-lang/libs

@mitsuhiko
Copy link
Contributor

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
Copy link
Member

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
Copy link

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
Copy link
Contributor

mitsuhiko commented Jul 20, 2018

@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
Copy link
Contributor Author

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
Copy link
Contributor

mitsuhiko commented Jul 20, 2018 via email

@withoutboats
Copy link
Contributor Author

withoutboats commented Jul 20, 2018

@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
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

jonhoo commented Jul 20, 2018

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
Copy link
Contributor Author

withoutboats commented Jul 20, 2018

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
Copy link
Contributor

epage commented Jul 20, 2018

@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.

methods will be encouraged (by warnings) to change their code, and library
authors will need to revisit whether they should override one of the new
methods.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@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
Copy link

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

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 17, 2018

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor

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

@DDOtten
Copy link

DDOtten commented Aug 18, 2018

enum BacktraceStatus {
    Captured,
    Disabled,
    Unsupported,
}

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

@GuillaumeGomez
Copy link
Member

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
Copy link
Collaborator

rfcbot commented Aug 19, 2018

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Aug 19, 2018
@Centril Centril merged commit 5d4b752 into master Aug 19, 2018
@Centril
Copy link
Contributor

Centril commented Aug 19, 2018

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#53487

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 20, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error-handling Proposals relating to error handling. A-traits-libstd Standard library trait related proposals & ideas disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.