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

Make Report compatible with std::error::Error based types #1746

Closed
mayabyte opened this issue Dec 26, 2022 · 7 comments · Fixed by #1749
Closed

Make Report compatible with std::error::Error based types #1746

mayabyte opened this issue Dec 26, 2022 · 7 comments · Fixed by #1749
Assignees
Labels
area/libs > error-stack Affects the `error-stack` crate (library) category/enhancement New feature or request

Comments

@mayabyte
Copy link

Related Problem

Let's say I have a library crate that uses error-stack for all its errors, and a pre-existing user-facing program that wants to use this library but doesn't want to do a big-bang migration of all its own error handling to use error-stack and may not ever care to - let's say it just uses Box<dyn Error> for all its errors.

Integrating the library's error types into the application is currently very clumsy and weird, requiring us to either pull the Contexts out of the Reports manually, stringify the errors, or some other tedious and undesirable pattern.

Proposed Solution

Other error handling libraries provide some conversion options for integrating with std Error that would solve this issue for error-stack. By way of example, eyre has the following:

  • impl AsRef<dyn Error + 'static> for Report
  • impl Deref<Target = dyn Error + Send + Sync + 'static> for Report
  • impl From<Report> for Box<dyn Error + 'static>

Alternatives

One possible alternative is implementing Error directly on Report. I can't tell whether this is deliberately being avoided, but neither eyre or anyhow do this for their error types so I assume there's a good reason not to choose this option.

Additional context

No response

@mayabyte mayabyte added area/libs > error-stack Affects the `error-stack` crate (library) C-enhancement labels Dec 26, 2022
@indietyp
Copy link
Member

Thanks for creating the issue! I just wanted to note that it may take a bit longer to resolve/respond to the issue.

Most of the people at HASH (and other contributors) are currently on Christmas/New Years vacation.🎄

Once everyone is back from enjoying some time with their close ones we'll be sure to take a closer look and respond properly!

Sorry for the inconvenience.

@TimDiekmann
Copy link
Member

Hi @mayabyte and thanks for filing this issue!

We’ve been looking into ways to improve the ergonomics of using Report in an environment that heavily relies on the usage of Error. From the start, we’ve considered implementing Error on Report. But we erred on the side of not doing it as it would allow you to construct a Result<T, Report<Report<C>> by using into_report() on Result<T, Report<C>>. In our own preliminary testing that ended up being quite frustrating, and easy to accidentally do.

We’ve experimented with a few other approaches and wonder if providing ways to turn the Report into an Error would be enough. Something like this:

fn into_error(self) -> impl Error + Send + Sync + 'static;
fn as_error(&self) -> &impl Error + Send + Sync + 'static;

Do you think this would help in situations such as yours?

@mayabyte
Copy link
Author

mayabyte commented Jan 2, 2023

I definitely think those would help, but having to sprinkle .into_error() everywhere would still be unfortunate IMO. Implementing From<Report> for Box<dyn Error> or similar seems like the lowest friction option in my case since I think that would Just Work with the ? operator, but I wonder if that would lead to a similar problem to the one you described with implementing Error directly on Report...

In the absence of other ideas, that problem with making Result<T, Report<Report<C>>s on accident sounds like exactly the kind of thing that could be solved by specialization, so going with into_error() to reduce friction for now and waiting is definitely an option.

@TimDiekmann
Copy link
Member

The implementation of From<Report<C>> for Box<dyn Error> works as expected with the ? operator. I tested this locally and have a branch more or less ready and will submit a PR to implement this. I have not found any problems with this approach.

Yes, this could theoretically be solved by specialization, but there are a few drawbacks here. Specialization with a default associated type does not work yet and to my knowledge, there is no solution to this problem, currently, specialized associated types have to be handled as opaque. However, if the problem can be solved with specialization (which might be possible) we currently avoid the specialization feature to ensure the same behavior on the nightly and stable toolchain. Also, the behavior would be unexpected since you would still be able to call into_report on Result<T, Report<C>>, but that should be a compile error. The correct solution to this problem would most likely be a negative trait that implements Error on Report, but not From.

@TimDiekmann
Copy link
Member

The changes landed on main and will be available with the next release of error-stack. Thanks again for opening this issue!

@mayabyte
Copy link
Author

mayabyte commented Jan 3, 2023

Awesome! Thanks so much for your work on this 😄

@TimDiekmann
Copy link
Member

error-stack v0.3 was just released 🙂

@vilkinsons vilkinsons added category/enhancement New feature or request and removed C-enhancement labels Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/libs > error-stack Affects the `error-stack` crate (library) category/enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

5 participants