shepmaster / snafu

Easily assign underlying errors into domain-specific errors while adding context
https://docs.rs/snafu/
Apache License 2.0
1.4k stars 60 forks source link

Support the Provider API #344

Closed shepmaster closed 1 year ago

netlify[bot] commented 2 years ago

Deploy Preview for shepmaster-snafu ready!

Name Link
Latest commit b99ff885de5e10db690d7243ad37084e257b7f38
Latest deploy log https://app.netlify.com/sites/shepmaster-snafu/deploys/63305157f24d9b0008ae5fce
Deploy Preview https://deploy-preview-344--shepmaster-snafu.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

shepmaster commented 2 years ago

@Enet4 @hellow554 @ErichDonGubler @seritools @Stargateur @mikeando 👋 [^1]

This is a chunky new feature for SNAFU and I'd appreciate your feedback. I don't want to explain too much upfront because I hope that the documentation explains it (and if not, I'd love some suggestions on how to improve that!).

The TL;DR is that the (unstable) provider API is the replacement for the (unstable and now removed) Error::backtrace method. It's also much more powerful and can unlock a number of possibilities. I think this implementation is just about at a place where I'd feel comfortable releasing it as an unstable feature flag for SNAFU.

If you have the time, I'd love if you could read the docs and provide any feedback. If you have even more time, take a look at the tests. If you have a total abundance of time to spend on me, feel free to look at the implementation as well (and remember that proc-macro code is nasty 😢).

Thanks so much for all your time and help!

[^1]: I picked y'all because I know you and you've shown interest in helping SNAFU recently. If you'd rather I leave you alone, please let me know!

Stargateur commented 2 years ago

Disclaimer, I almost never use dyn and don't like use it. I play dump on purpose with this review:

My concern as user:

shepmaster commented 2 years ago

I almost never use dyn and don't like use it.

Same here. The good news is that you only really need to pop over to the trait object to call the request_* methods. I'm also double checking to see if there's a way that it could be added directly to std::error::Error, instead of on the trait object. It is possible to use a free function from the standard library and completely avoid the trait object:

let e = WebserverError;
std::any::request_value::<HttpCode>(&e);

I think example with an enum would better show what this api is capable of

I certainly should provide at least one example using an enum, but I'm not following why an enum example would show the capabilities better — can you expand on that point?

What happen when many error provide the same type ? the doc have an example with "the deepest backtrace" but that mean it fondamental unclear at runtime what I will get, imagine many of my error have an id, I would get the deepest id ? how snafu determine what is the deepest ?

The Provider API works by types and you can indeed only return one piece of data for a type. When there are multiple providers for the same type, the one that is provided first will win.

By default, SNAFU will gather the provided data from the source first, before providing any data from the current error. This can be overridden through the priority attribute, which will cause this error to provide that data first, before the source.

Any suggestions for how / where to work this information into the docs?

Do I need nightly to use this feature ?

Yes and no. You can add #[snafu(provide)] in any version of Rust that SNAFU supports. Only when the feature flag is enabled will that expand to useful code. This allows people to add provide to their code today and if something in the dependency tree enables the feature flag, errors just become better.

Automatically provided data and Explicitly provided data title doesn't match, shouldn't automatically be "implicit" ?

Yeah, probably. I also thought about "manually" as the counterpart to "automatically" — any opinions on that?

Do I pay for this feature if I don't use it (with the feature activated) ? How much cost this feature?

At compile time:

With the feature inactive, there's the time for parsing and manipulating the data inside the procedural macro for handling snafu(provide). With the feature active, there's also time taken to generate the Error::provide method call for each error and then further compilation of that.

At run time:

If someone calls request_ref / request_value, then it's more-or-less a O(N) walk through your error type and all the sources with each level doing a bunch of "are you looking for type X?`" checks.

Is that a good idea ? Practical use ? Should have follow guideline about what an typical error should provide, is that even exist ?

No, I believe that this is so very new that there's not a community consensus. I expect that backtraces will be the first primary use case, but others that I've thought about are annotating an error with an HTTP error code (e.g. HTTP 404 vs HTTP 500) or with some externally-defined error code (e.g. rustc's E0000).

I don't know exactly what SNAFU's role should be here — I show those examples in the docs, but I don't call attention to them as "best practice".

My actions

Stargateur commented 2 years ago

Any suggestions for how / where to work this information into the docs?

Maybe at the end of Automatically provided data.

Yeah, probably. I also thought about "manually" as the counterpart to "automatically" — any opinions on that?

I think explicit/implicit fit better, but manually/automatically is ok for me.

I certainly should provide at least one example using an enum, but I'm not following why an enum example would show the capabilities better — can you expand on that point?

Cause struct doesn't have variant, I could access to the property "at hand" it's in the struct after all, with enum you get variant that could or could not have the property that better reflect the incertitude that the property exist (also I mostly use snafu with enum).

shepmaster commented 2 years ago

@Stargateur I've attempted to address your points in this force-push

Stargateur commented 2 years ago

The doc is way more clear, my only concern is the enum example doesn't explain what happen when UserId is not provided.

let e = NetworkUnreachable { }.build();
let e = &e as &dyn std::error::Error;
// would panic
let _= e.request_ref::<UserId>().unwrap();

But that may be too detailed maybe just a sentence like "NetworkUnreachable doesn't have UserId if you unwrap e.request_ref::<UserId>() you will panic."

shepmaster commented 2 years ago

the enum example doesn't explain what happen when UserId is not provided.

I've amended the example a bit with:

match e.request_ref::<UserId>() {
    // Present when ApiError::Login or ApiError::Logout
    Some(user_id) => {
        println!("{user_id:?} experienced an error");
    }
    // Absent when ApiError::NetworkUnreachable
    None => {
        println!("An error occurred for an unknown user");
    }
}
shepmaster commented 1 year ago

For those watching along, I'm hoping to merge this sometime in the next few days, so if you have something you'd like to say, now would be a great time! ❤️

shepmaster commented 1 year ago

Force-pushed to address comments from @seritools to split the combined example for snafu(provide(...flags... into separate sections and examples that can be linked to.

shepmaster commented 1 year ago

Thanks all!