shepmaster / snafu

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

Update the changelog and upgrading guide in preparation for 0.8 #428

Closed shepmaster closed 8 months ago

netlify[bot] commented 9 months ago

Deploy Preview for shepmaster-snafu ready!

Name Link
Latest commit 1dc3d9500816833f0df1fb449ba60a6ca210b4f0
Latest deploy log https://app.netlify.com/sites/shepmaster-snafu/deploys/658c92cb9c104400083f5273
Deploy Preview https://deploy-preview-428--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 configuration.

shepmaster commented 9 months ago

I've run my mini-crater tool using the main branch. There were 170 crates (a) on crates.io (b) that use SNAFU (c) that I could build. 100% of those worked with the new version.

I've also tested with upgrading some of my binary projects and asked some friends to test with their private projects that use SNAFU. The biggest issue there was the change of location to no longer be automatically treated as implicit.

Anything else we should check before 0.8?

Enet4 commented 8 months ago

Do we have plans to change anything in light of https://github.com/shepmaster/snafu/issues/425? If not, I think we are ready to launch the release rocket.

shepmaster commented 8 months ago

I went ahead and added those deprecation attributes as it was easy. So... good to go then?

shepmaster commented 8 months ago

I found an unexpected problem in DICOM-rs when trying to migrate the code which depends on whatever_context. It cannot infer the type parameters, whereas it had no issues in snafu 0.7.

Odd; I can't immediately reason out why it should / would have worked before. There's a lot of type inference going on with whatevers... Bisecting leads to d826e6a71e58da06299b968814dfe39bc29fc6a1, which does add more From implementations.

The problem, if I get it correctly, is that whatever_context has some type S ("string") and then some E ("error") that can be constructed from the string. However, the ? operator then tries to convert that E into another error type via From / Into. With the new From impls, I guess it becomes too complicated for the type system to figure out what concrete type E should be. Before, I guess there was only ever the one type and that was solvable?

I think there's still only one concrete type that satisfies all the requirements, but maybe I'm missing something.

Annoyingly, you don't even really want the type conversion provided by ? here and if it were missing then this would all compile.


Some choices I see...

  1. Put those additional From implementations behind a feature flag, but that could easily cause crate A which uses the feature to break crate B which does not use the feature.
  2. Remove the new implementations. They aren't essential to any current functionality, but allow experimenting with do yeet.
  3. Live with the new type inference problems and make people add annotations. If we went down this path, we could potentially change the signature to use impl trait to make turbofishing easier:

    fn whatever_context<S, E>(self, context: Into<String>) -> Result<T, E>
    where
        E: FromString,
Enet4 commented 8 months ago

All that it took to hit this edge case was calling whatever_context in a function returning a Result<_, SpecificCustomError>. As such, I would be inclined with option 2, because the loss of ergonomics is overwhelming, especially against the given benefits of allowing experimentation with a feature which is not even stabilized. We might come up with better alternatives in the meantime.

shepmaster commented 8 months ago

Alright; reverted and removed from the changelog. Anything else?

Enet4 commented 8 months ago

So yes, the type inference errors are gone!

For curiosity sake, I noticed that one of my error types AccessError, increased in size upon this update, from 48 bytes to 56. Any idea why? Is it because Backtrace from std can be larger?

shepmaster commented 8 months ago

Is it because Backtrace from std can be larger?

Looks like it...

use snafu::prelude::*;

#[derive(Debug, Snafu)]
struct Error {
    backtrace: snafu::Backtrace,
}

fn main() {
    dbg!(std::mem::size_of::<Error>());
}
version features size
0.7.5 0
0.7.5 backtraces 32
0.7.5 backtraces-impl-std 48
0.7.5 backtraces-impl-backtrace-crate 32
master 48
master backtraces-impl-backtrace-crate 32