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

backtrace method was removed by Rust #195

Closed
JelteF opened this issue Aug 20, 2022 · 9 comments · Fixed by #200
Closed

backtrace method was removed by Rust #195

JelteF opened this issue Aug 20, 2022 · 9 comments · Fixed by #200
Assignees
Labels
Milestone

Comments

@JelteF
Copy link
Owner

JelteF commented Aug 20, 2022

@tyranron @ilslv The tests started failing with the most recent rust nightlies because of this: rust-lang/rust#99431

Could you spend some time to fix it and use the Provider API instead? rust-lang/rust#96024

I wanted to release version 1.0.0 of derive_more but this blocks me from doing that.

@tyranron
Copy link
Collaborator

cc @ilslv

@JelteF will do in a couple of days.

@JelteF
Copy link
Owner Author

JelteF commented Aug 22, 2022

If it's hard to get the provider API working, e.g. because it's not ready yet. Then I think we can temporarily remove backtrace functionality from derive_more until we can create a good implementation.

@tyranron
Copy link
Collaborator

@JelteF btw, would you consider to release 0.100.0 version instead of 1.0.0. I think there are more breaking things to be done before going 1.0.0. The main of them is making this crate consisting of two: derive_more_codegen containing proc macro implementations + derive_more non-procmacro crate re-exporting macros from derive_more_codegen and providing some type-level glue to aid those macros in their expansion. Such type level glue may improve macro ergonomics and cleverness. One of such cases is this one.

@JelteF
Copy link
Owner Author

JelteF commented Aug 22, 2022

Maybe I'm missing something, but a quick look at thiserror suggests that shouldn't be a breaking change. Th derive_more non-procmacro crate can simply re-export the derives like this: https://github.com/dtolnay/thiserror/blob/master/src/lib.rs#L214

@tyranron
Copy link
Collaborator

@JelteF makes sense. Even changes like #[display(fmt = "{}", "expression.to.execute()")] -> #[display(fmt = "{}", expression.to.execute())] (using tokens directly instead of stringifying them) we still may do in backward-compatible manner, allowing both cases until the next major release. Then OK.

@ilslv
Copy link
Contributor

ilslv commented Aug 30, 2022

@JelteF @tyranron I'm not really sure, how to make transition to Provider API here. Basically now we have provide() method instead of the backtrace(), which uses Demand to provide some "context". As this API is quite new and there is no practice of using it inside the Rust's ecosystem, I'm not really sure how to approach it:

enum MyError {
    First(FirstError),
    Second { source: SecondError, backtrace: Backtrace }
}

impl Error for MyError {
    fn provide<'a>(&'a self, demand: &mut Demand<'a>) {
        match self {
            First(first) => {
                // What should we do here?
                // 1. Just try to call provide on `first`?
                first.provide(demand);
                // 2. Attach it as a concrete type?
                demand.provide_ref::<FirstError>(first);
                // 3. Attach it as `dyn Error`?
                demand.provide_ref::<dyn Error>(first);
                // Or all 3 of them?
            }
            Second { source, backtrace } => {
                // All questions above are still valid for `source`.
                // But with `backtrace`, it's clear to me, that we should
                // attach it.
                demand.provide_ref::<Backtrace>(backtrace);
            }
        }
    }
}

I propose to wait until there is more understanding of Provider API role in Rust's error handling ecosystem. Especially when new approaches that leverage Provider are surfacing, like error-stack. And until then we can just remove backtrace() impls.

thiserror has this issue too.

@tyranron
Copy link
Collaborator

Then I think we can temporarily remove backtrace functionality from derive_more until we can create a good implementation.

@ilslv ☝️

@ilslv
Copy link
Contributor

ilslv commented Aug 31, 2022

@tyranron It looks like thiserror fixed this issue: dtolnay/thiserror#182

enum MyError {
    First(#[error(backtrace)] FirstError),
    Second { #[error(source)] source: SecondError, #[error(backtrace)] backtrace: Backtrace }
}

impl Error for MyError {
    fn provide<'a>(&'a self, demand: &mut Demand<'a>) {
        match self {
            First(first) => {
                first.provide(demand);
            }
            Second { source, backtrace } => {
                source.provide(demand);
                demand.provide_ref::<Backtrace>(backtrace);
            }
        }
    }
}

I think it looks reasonable and we should mirror the implementation.

@tyranron
Copy link
Collaborator

@ilslv 👍

JelteF pushed a commit that referenced this issue Oct 4, 2022
Fixes #195 similar to dtolnay/thiserror#182

Co-authored-by: tyranron <tyranron@gmail.com>
@tyranron tyranron removed the priority label Oct 5, 2022
@tyranron tyranron added this to the 1.0.0 milestone Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants