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

Replace fn backtrace() with Provider API in Error derive #200

Merged
merged 8 commits into from Oct 4, 2022

Conversation

ilslv
Copy link
Contributor

@ilslv ilslv commented Aug 31, 2022

Fixes #195 similar to dtolnay/thiserror#182

@ilslv ilslv changed the title Replace fn backtrace() with Provider API Replace fn backtrace() with Provider API in Error derive Aug 31, 2022
@ilslv ilslv marked this pull request as ready for review August 31, 2022 09:33
@tyranron
Copy link
Collaborator

tyranron commented Sep 8, 2022

@JelteF would you be so kind to move requirement from Test Rust 1.36.0 CI jobs to Test Rust 1.56.0 in master branch protection settings?

Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@ilslv hmm...regarding the CI failure. I thought it was because of my changes, but I can reproduce the same test failures localy on your last commit in 2022-09-08 nightly Rust. Could you look into it?

@ilslv ilslv requested a review from tyranron September 8, 2022 12:18
tyranron
tyranron previously approved these changes Sep 9, 2022
@tyranron tyranron requested a review from JelteF September 9, 2022 09:09
doc/error.md Show resolved Hide resolved
.then(|| {
let backtrace_expr = &self.data.members[backtrace];
quote! {
demand.provide_ref::<std::backtrace::Backtrace>(&#backtrace_expr);
Copy link
Owner

@JelteF JelteF Sep 10, 2022

Choose a reason for hiding this comment

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

Food for thought: One of the reasons for this provider API (afaict) is so more types can be provided that just Backtrace, without needing to add more methods to the Error trait.

So probably we want to allow this too, e.g. to embed some additionaly context such as trace IDs in there for distributed tracing. This would probabyl be better as a separate PR. I think it makes sense to keep dedicated and easy to use support for the Backtrace type.

Another idea is to also provide the source through the provider API, like is done in the rust docs

impl std::error::Error for Error {
    fn provide<'a>(&'a self, demand: &mut Demand<'a>) {
        demand
            .provide_ref::<MyBacktrace>(&self.backtrace)
            .provide_ref::<dyn std::error::Error + 'static>(&self.source);
    }
}

Source: https://doc.rust-lang.org/nightly/std/error/trait.Error.html

Again this would be better in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JelteF I've already raised question of what should be provided and this implementation mirrors thiserror. As Provider API is quite new and source() default impl still returns None, I think that we shouldn't rush with providing dyn Error and wait how Provider API will be used throughout the ecosystem.

Copy link
Owner

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

Looks good overall. but docs need to be updated. Thanks for all the work on this.

@JelteF JelteF added this to the 1.0.0 milestone Sep 11, 2022
@ilslv ilslv requested a review from JelteF September 12, 2022 06:34
@tyranron
Copy link
Collaborator

@JelteF ping

@JelteF JelteF merged commit 42a4b8c into JelteF:master Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

backtrace method was removed by Rust
3 participants