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

Can no longer have a backtrace field when using source(from) where the original type is not an error (breaking change in 0.7.2) #364

Closed
jnicholls opened this issue Oct 19, 2022 · 10 comments · Fixed by #366
Labels
bug Something isn't working found in the field A user of SNAFU found this when trying to use it

Comments

@jnicholls
Copy link

jnicholls commented Oct 19, 2022

I had an error roughly defined as such:

#[derive(Debug, Snafu)]
enum Error {
    Variant1 {
        #[snafu(source(from(String, Into::into)))]
        source: Box<dyn std::error::Error + Send + Sync>,
    },

    // ...
}

fn return_error() -> Result<(), String> {
    Err("nope!".to_string())
}

return_error().context(Variant1Snafu)?;

In 0.7.1 this was acceptable. In 0.7.2, it is now saying that String does not implement std::error::Error which is a requirement for AsErrorSource. I believe the derive macro is somehow using the from type as the binding for the context, which would be equivalent to trying to have a source: String field directly, which I know has never worked.

@shepmaster shepmaster added bug Something isn't working help wanted Extra attention is needed found in the field A user of SNAFU found this when trying to use it labels Oct 19, 2022
@shepmaster
Copy link
Owner

Repro

main.rs

use snafu::prelude::*;

#[derive(Debug, Snafu)]
enum Error {
    Variant1 {
        #[snafu(source(from(String, Into::into)))]
        source: Box<dyn std::error::Error + Send + Sync>,
    },
}

fn return_error() -> Result<(), String> {
    Err("nope!".to_string())
}

fn demo() -> Result<(), Error> {
    return_error().context(Variant1Snafu)
}

fn main() {}

Cargo.toml

[dependencies]
snafu = "=0.7.1"
snafu-derive = "=0.7.1"

Output

Upgrading snafu-derive to 0.7.2 causes the error

error[E0599]: no function or associated item named `generate_with_source` found for trait object `dyn GenerateImplicitData` in the current scope
    --> /Users/shep/.cargo/registry/src/github.com-1ecc6299db9ec823/snafu-0.7.1/src/lib.rs:1391:17
     |
1391 | #[derive(Debug, Snafu)]
     |                 ^^^^^ function or associated item not found in `dyn GenerateImplicitData`
     |
     = note: this error originates in the derive macro `Snafu` (in Nightly builds, run with -Z macro-backtrace for more info)

@shepmaster
Copy link
Owner

no function or associated item named generate_with_source found

This occurs if you have snafu = 0.7.1 and snafu-derive = 0.7.2, which is something I intended to work but don't have tests for. That being said...

it is now saying that String does not implement std::error::Error which is a requirement for AsErrorSource

This isn't the error I produced — can you please create a fully contained reproduction?

@jnicholls
Copy link
Author

jnicholls commented Oct 19, 2022

Sorry for not sharing it sooner. This is the message below, built against 0.7.2 of both snafu and snafu-derive.

error[E0599]: the method `as_error_source` exists for struct `std::string::String`, but its trait bounds were not satisfied
  --> src/concatenator.rs:62:17
   |
62 | #[derive(Debug, Snafu)]
   |                 ^^^^^ method cannot be called on `std::string::String` due to unsatisfied trait bounds
   |
   = note: the following trait bounds were not satisfied:
           `std::string::String: StdError`
           which is required by `std::string::String: AsErrorSource`
           `str: Sized`
           which is required by `str: AsErrorSource`
           `str: StdError`
           which is required by `str: AsErrorSource`
   = note: this error originates in the derive macro `Snafu` (in Nightly builds, run with -Z macro-backtrace for more info) 

This is partially my enum:

#[derive(Debug, Snafu)]
pub enum Error {
    #[snafu(display("Unable to build an HDD: {source}"))]
    HddBuild {
        backtrace: Option<Backtrace>,
        #[snafu(source(from(String, Into::into)))]
        source: Box<dyn StdError + Send + Sync>,
    },
}

@shepmaster
Copy link
Owner

Complete example

use snafu::{prelude::*, Backtrace};

#[derive(Debug, Snafu)]
pub struct HddBuildError {
    backtrace: Option<Backtrace>,

    #[snafu(source(from(String, Into::into)))]
    source: Box<dyn std::error::Error + Send + Sync>,
}

fn return_error() -> Result<(), String> {
    Err("nope!".to_string())
}

fn demo() -> Result<(), HddBuildError> {
    return_error().context(HddBuildSnafu)
}

fn main() {}

@shepmaster shepmaster changed the title Breaking changing 0.7.2 Can no longer have a backtrace field when using source(from) where the original type is not an error (breaking change in 0.7.2) Oct 19, 2022
@shepmaster
Copy link
Owner

I'll need to dig into the code, but I'm hoping that the fix here is to call the generate_with_source method with the error after the transformation.

@shepmaster
Copy link
Owner

Yeah, we generate this:

fn into_error(self, error: Self::Source) -> HddBuildError {
    HddBuildError {
        backtrace: {
            use ::snafu::AsErrorSource;
            let error = error.as_error_source();
            ::snafu::GenerateImplicitData::generate_with_source(error)
        },
        source: (Into::into)(error),
    }
}

Should need to move the (Into::into)(error) earlier, save it as error and pass that to generate_with_source

@jnicholls
Copy link
Author

Great sleuthing! Thanks for jumping on this one so quickly. 🕵️

@shepmaster
Copy link
Owner

I want to clean up the commit, but can you try #366 locally and see if that works for your original case?

@shepmaster shepmaster removed the help wanted Extra attention is needed label Oct 20, 2022
shepmaster added a commit that referenced this issue Oct 20, 2022
Previously, we only converted the source in the source field
initializer. However, `GenerateImplicitData::generate_with_source`
also needs access to the post-converted value.

Fixes #364
@jnicholls
Copy link
Author

Sorry for the delay. #366 works! Thank you.

@shepmaster
Copy link
Owner

Released in 0.7.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working found in the field A user of SNAFU found this when trying to use it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants