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

Automatically convert to external errors w/ ensure! and bail! #95

Merged

Conversation

j-baker
Copy link
Contributor

@j-baker j-baker commented May 2, 2023

A pattern documented by thiserror is

pub enum MyError {
  ...

  #[error(transparent)]
  Other(#[from] anyhow::Error), // source and Display delegate to anyhow::Error
}

It'd be nice for this to work with ensure! but right now, that macro returns an eyre error. There's the 'ScienceError' example but that is more for the case where you want to pick a specific enum variant.

With this PR, the macro additionally runs an .into(). In the case that the return type is an eyre error, obviously .into() will do nothing and be compiled away. In the case that there is a from method, the wrapping will occur. This enables eyre to be used for ergonomic 'implementation detail error' in a thiserror using system which has contractual errors. In the above case, one could write:

fn foo() -> Result<(), MyError> {
  ensure!(false, "this will be wrapped up in a MyError");
  Ok(())
}

To the best of my knowledge, this is a non-breaking change. I also considered modifying eyre! to add the .into(), however that seemed more likely to be a breaking change (in particular, breaking eyre!("foo").into()!).

@j-baker j-baker changed the title Automatically convert to external errors Automatically convert to external errors w/ ensure! and bail! May 2, 2023
@thenorili thenorili force-pushed the jbaker/auto_convert_to_external_errors branch from 73c0582 to acd7891 Compare November 25, 2023 03:40
@thenorili
Copy link
Contributor

thenorili commented Nov 25, 2023

Testing revealed a pattern that this change breaks.

     // Tests single-argument `ensure!`
-    let f = || {
+    let f = || -> Result<()> {^M
         ensure!(v + v == 1);
         Ok(())
     };
    assert_eq!(
        f().unwrap_err().to_string(),
        "Condition failed: `v + v == 1`",
    );

This test passed before this change, but afterwards it needs a return type on the closure.

The compiler warning was surprisingly unhelpful. It suggested something like Ok::<(), E> for a generic specified in the function header instead of a return type on the closure.

We should at the very least keep an eye out for any issues opened after this change mentioning a pattern like that so we can tell them how to fix it. I'm not quite sure how to document that behavior though.

Copy link
Contributor

@thenorili thenorili left a comment

Choose a reason for hiding this comment

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

Git diff is broken on test_macros.rs because somehow I've added a carriage return to every line. I'll work on doing something about that before I commit. The actual changes are just here.

    // Tests single-argument `ensure!`
    let f = || -> Result<()> {
        ensure!(v + v == 1);
        Ok(())
    };
    assert_eq!(
        f().unwrap_err().to_string(),
        "Condition failed: `v + v == 1`",
    );

    // Tests automatically converting to external errors with ensure!()
    let f = || -> Result<(), SomeWrappingErr> {
        ensure!(false, "this will fail");
        Ok(())
    };
    assert!(f().is_err());
}

#[allow(dead_code)]
struct SomeWrappingErr {
    err: eyre::Error,
}

impl From<eyre::Error> for SomeWrappingErr {
    fn from(err: eyre::Error) -> Self {
        SomeWrappingErr { err }
    }

j-baker and others added 2 commits November 24, 2023 21:51
A pattern documented by thiserror is

```
pub enum MyError {
  ...

  #[error(transparent)]
  Other(#[from] anyhow::Error), // source and Display delegate to anyhow::Error
}
```

It'd be nice for this to work with ensure! but right now, that macro returns
an eyre error.

With this PR, the macro additionally runs an .into(). In the case that
the return type is an eyre error, obviously .into() will do nothing and
be compiled away. In the case that there is a from method, the wrapping
will occur. This enables eyre to be used for ergonomic 'implementation
detail error' in a thiserror using system which has contractual errors.

Since this conversion adds more flexibility to the result of these
macros, this could break code which relies on the narrowness to inform
type inference. If this is the case, update your code to specify the
result type you would like to use.
By adding the possibility for polymorphism from an eyre error, the
previous commit breaks a previous eyre test for single-argument
ensure!().

This change fixes that test and adds `allow(dead_code)` to the struct
used for the test for automatically converting to external errors.
@thenorili thenorili force-pushed the jbaker/auto_convert_to_external_errors branch from acd7891 to 9b6a4f6 Compare November 25, 2023 05:54
@thenorili
Copy link
Contributor

Personally I think this is value enough for a very small break. I added a note in the commit message and I'll bring this up with folks before the next release to see how they feel about it, but for now this looks good to merge.

@thenorili thenorili merged commit a795c97 into eyre-rs:master Nov 25, 2023
29 checks passed
@ileixe
Copy link

ileixe commented Dec 12, 2023

FYI: this PR seems to break API. Now eyre 0.6.10 is not compatible with eyre 0.6.8 as the type is mismatched when user is using bail() in closure.

@yaahc
Copy link
Collaborator

yaahc commented Dec 12, 2023

well shit, okay, we'll yank that version for now to give us time to talk about whether or not we want to make that breaking change and pick the right version for the re-release accordingly. Thanks for letting us know.

@thenorili thenorili added the S-controversial Status: Controversial Change label Dec 13, 2023
@yaahc yaahc mentioned this pull request Dec 13, 2023
yaahc added a commit that referenced this pull request Dec 13, 2023
@thenorili thenorili added breaking change Non-urgent breaking changes, probably delay to the next release and removed S-controversial Status: Controversial Change labels Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Non-urgent breaking changes, probably delay to the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants