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

Tracking issue for RFC 2504, "Fix the Error trait" #53487

Open
Centril opened this issue Aug 19, 2018 · 111 comments
Open

Tracking issue for RFC 2504, "Fix the Error trait" #53487

Centril opened this issue Aug 19, 2018 · 111 comments

Comments

@Centril
Copy link
Contributor

@Centril Centril commented Aug 19, 2018

This is a tracking issue for the RFC "Fix the Error trait" (rust-lang/rfcs#2504).

Steps:

Unresolved questions:

  • The choice to implement nullability internal to backtrace may prove to be a mistake: during the period when backtrace APIs are only available on nightly, we will gain more experience and possible change backtrace's constructors to return an Option<Backtrace> instead.

Current status:

#53487 (comment)

@L-as
Copy link

@L-as L-as commented Sep 26, 2018

What parts haven't been implemented?

@withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Sep 26, 2018

I've updated the tracking issue to reflect that, but right now the source method has been implemented, but the backtrace related APIs have not been implemented

@L-as
Copy link

@L-as L-as commented Sep 26, 2018

Perhaps I could try implementing this, is it OK to just use the backtrace crate?

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Oct 2, 2018

Implementing backtraces in the standard library will probably not use the backtrace crate but rather the backtrace API that's already in the standard library for RUST_BACKTRACE. We can perhaps unify these implementations in the long run, but for now it's infeasible to do so.

The current platform-independent backtrace code lives here, mostly in the _print function. Implementing the API defined in the RFC will likely involve reusing these interfaces to define a struct which carries along the relevant information.

@laaas if you'd like to implement this please feel free! Also feel free to reach out with any questions.

@L-as
Copy link

@L-as L-as commented Oct 2, 2018

Thanks! Will definitely try implementing it.

@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Nov 22, 2018

Thanks! Will definitely try implementing it.

@laaas Did you make any progress? Need any help?

@L-as
Copy link

@L-as L-as commented Nov 22, 2018

@frewsxcv Actually not!

I haven't really looked at it much, so if you want to work on it, go ahead!

@aturon
Copy link
Member

@aturon aturon commented Feb 21, 2019

cc @rust-lang/libs @withoutboats @mitsuhiko would be really nice to make progress here

@mitsuhiko
Copy link
Contributor

@mitsuhiko mitsuhiko commented Feb 22, 2019

I would move than love to spearhead this.

@aturon
Copy link
Member

@aturon aturon commented Feb 22, 2019

@mitsuhiko go go go! if you need any help around the backtrace stuff @alexcrichton can get you set up.

@mitsuhiko
Copy link
Contributor

@mitsuhiko mitsuhiko commented May 26, 2019

Just an update from my side about why there is no progress: I tried a few things to expose a sensible backtrace object and with the current system that is in place it's all too likely that the outcome is just a bad workaround. I'm currently leaning towards waiting for the backtrace support in rust to be based on backtrace-rs.

@idubrov
Copy link

@idubrov idubrov commented May 30, 2019

Is migration to backtrace-rs is something that is already happening? Any issue to track progress?

@sfackler
Copy link
Member

@sfackler sfackler commented May 30, 2019

That landed ~4 days ago: #60852

@est31
Copy link
Contributor

@est31 est31 commented Jun 13, 2019

Does this RFC also include backtrace printing for ? in tests? Currently, when you switch to ? from unwrap, the backtraces are very poor. The only useful info those backtraces convey (amidst a mountain of spam) is the test that failed along with its line, nothing more.

@joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Jun 3, 2020

@yaahc

Because we could stabilize std::backtrace::Backtrace without stabilizing fn backtrace, and you can then use anyhow's own fn backtrace for extracting the backtrace if you need to access it directly when reporting errors to sentry. Would this be sufficient?

This seems like a great idea to me. (Bonus if we can have appropriate mechanisms to control whether errors get backtraces taken or not, given the cost and the size complexity.)

@withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Jun 4, 2020

Anyhow does not support backtraces without the backtrace feature, which only works on nightly Rust. It depends on both the std Backtrace type and the Error::backtrace method, because it uses the Error::backtrace method to determine if a backtrace needs to be generated when constructing an anyhow::Error. Maybe @dtolnay would be willing to make anyhow generate a redundant backtrace on stable, but this would be a degredation in the quality of the anyhow crate.

Backtrace does not block moving Error into core because its so much easier to solve than the coherence issue (which I am personally unhappy to see "solved" with more hacks on our coherence system instead of solving the underyling issue). We could move backtrace into core by creating hooks of some kind for std to provide the actual backtrace behavior, and without those hooks implemented it would do nothing, just like we have working for panicking, just like we would require to add hooks for std to add a special inherent impl for dyn Error. Stabilizing Error::backtrace should not be blocked on Backtrace being core.

We are not prioritizing the short term over the long term by stabilizing a method that's been in development for nearly 3 years (from when it first appeared in failure). If the Backtrace type is ready to stabilize, so is Error::backtrace.

@withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Jun 4, 2020

I've created a stabilization proposal here: #72981. I've tried to cover all of the issues discussed. Other people who have concerns about stabilizing backtrace should leave comments on this PR!

@aloucks
Copy link
Contributor

@aloucks aloucks commented Aug 1, 2020

One minor nit: Perhaps RUST_STDLIB_BACKTRACE would be better than RUST_LIB_BACKTRACE. When I first saw this I found myself asking the question "is this something library authors are supposed to honor or otherwise deal with". I'm pretty sure the answer is "no" and the suggested name makes that more obvious.

@joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Aug 2, 2020

@aloucks That seems like a good idea to me.

@Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Oct 1, 2020

I'm not sure if was brought up anywhere, but I think it would be useful to have a Backtrace::empty() function which unconditionally creates a no-op backtrace (like Backtrace::capture() with both environment variables unset). This would allow users to re-implement Backtrace::capture with different environment variables or checks, and avoid the need to use Option<Backtrace>.

@yaahc
Copy link
Contributor

@yaahc yaahc commented Oct 2, 2020

I'm not sure if was brought up anywhere, but I think it would be useful to have a Backtrace::empty() function which unconditionally creates a no-op backtrace (like Backtrace::capture() with both environment variables unset). This would allow users to re-implement Backtrace::capture with different environment variables or checks, and avoid the need to use Option<Backtrace>.

like this? #74490

:D I added this back in july https://doc.rust-lang.org/nightly/std/backtrace/struct.Backtrace.html#method.disabled, it's even const fn, tho that is making it slightly more complicated to move Backtrace into core, lol.

@omarabid
Copy link

@omarabid omarabid commented Oct 13, 2020

I'm a bit confused to be honest. If backtrace is not stable yet, then why the Failure crate got deprecated? People who relied on Failure and tries to move now will either have to compile on nightly or drop backtracing.

Is there any ETA for the stabilization of backtrace?

@yaahc
Copy link
Contributor

@yaahc yaahc commented Oct 14, 2020

Is there any ETA for the stabilization of backtrace?

yes, kinda, no specific eta but there are updates over in the "stabilize backtrace" PR #72981 (comment)

@KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Nov 12, 2020

Status Update: 2020-11-13

  • Stabilization of the initial formattable Backtrace API has entered FCP: #72981 (comment)
  • We've just got one remaining question on Debug formatting in #65280 about supporting the precision flag, but also noticed a difference in Display formatting with panic! backtraces in #71706
  • Work has started on an API to get frames from a backtrace in #78299

I've marked the internal nullability open question as resolved, since that's what we've all agreed to stabilize.

@jhpratt
Copy link
Contributor

@jhpratt jhpratt commented Nov 15, 2020

This is probably tangential to this issue, but is there any plan to support returning a non-static errors from the source method? I just ran into that myself, and it unfortunately requires me to return None, despite the fact that there is a better value.

@yaahc
Copy link
Contributor

@yaahc yaahc commented Nov 16, 2020

@jhpratt, my expectation is that no, we don't intend to go back to non 'static errors. Technically the Error trait still supports that via the deprecated cause method, but most of the ecosystem has moved away from using that method, so you'd not be able to use most error handling libraries with errors focusing on the cause fn.

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Dec 2, 2020

I randomly stumbled upon #58520 (comment) and I wonder if this should be encoded in the default implementation of ‘backtrace‘:

fn backtrace(&self) -> Option<&Backtrace> {
  self.source.and_then(Error::backtrace)
}
@yaahc
Copy link
Contributor

@yaahc yaahc commented Dec 3, 2020

The only reason I can think of to not do this is to avoid obfuscating the origin of a backtrace, either because someone needs to identify the exact error the backtrace came from or they need to handle multiple backtraces in a single chain of errors. It shouldn't be too hard to figure out the exact origin of the backtrace or to differentiate between actually unique backtraces though, since you can still use std::ptr::eq to compare the references.

@yaahc
Copy link
Contributor

@yaahc yaahc commented Apr 16, 2021

Updated the description to reflect the current status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet