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

Error Handling #2

Merged
merged 23 commits into from
Mar 16, 2020
Merged

Error Handling #2

merged 23 commits into from
Mar 16, 2020

Conversation

FintanH
Copy link
Collaborator

@FintanH FintanH commented Feb 25, 2020

This PR proposes how to make decisions around error handling in Rust and which packages to use for making our lives easier.

@FintanH FintanH requested a review from a team February 25, 2020 11:09
Copy link
Contributor

@kim kim left a comment

Choose a reason for hiding this comment

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

I think thiserror is what I actually wanted for librad. A few questions, though:

  • why is anyhow better suited to work in conjunction with thiserror?
  • I'm not sure about the assertion point: is this about being able to write assert!(matches!(...)) or?
  • I think it could be spelled out a bit more concretely what the recommendations / guidelines are for using this combo of libraries. To me, errors-as-values always boils down to the basic question: do I want the caller to implement control flow based on the error variant or not? In the latter case, how would I express this with anyhow + thiserror? Is the recommendation to use Box<dyn Error> (or a generic Error(String) variant) in libraries, and anyhow::Error in application code? What about unifying a Box<dyn Error> at a higher level? MyError::DunnoButItBroke(Box<dyn Error>)?

@FintanH FintanH marked this pull request as ready for review February 26, 2020 16:39
@FintanH
Copy link
Collaborator Author

FintanH commented Feb 26, 2020

@kim: Thanks for the feedback! I'll address the 3rd point by fleshing out the recommendation section more. Regarding the other two points, here's my thoughts.

why is anyhow better suited to work in conjunction with thiserror?

In reality, any library that has interoperability with the std::Error trait will work with thiserror, since this is what the library provides as well as the display stuff, "This library provides a convenient derive macro for the standard library's std::error::Error trait.". So, failure will work fine since it provides AsFail.

However, I think something to keep in mind is that thiserror and anyhow are developed by the same author so they're meant to complement each other.

Otherwise, it's down to which set of functionalities we would prefer and anyhow seems to squeeze out more in this area.

What do you think? Also let me know if any of this info is useful for the proposal proper.

I'm not sure about the assertion point: is this about being able to write assert!(matches!(...)) or?

The assertion point is about the macros used for early return syntax rather than assert!. They both have ensure!and bail!, but anyhow allows you to return an enum variant, example in the docs

@FintanH FintanH requested a review from a team February 26, 2020 17:04
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

Sick first draft. Left a bunch of suggestions for typos and a general question about the understanding of Option.

I agree with @kim - examples of the recomnended stack, one for a lib and one for an app would help a lot.

proposals/0001.md Show resolved Hide resolved
proposals/0001.md Show resolved Hide resolved
proposals/0001.md Outdated Show resolved Hide resolved
proposals/0001.md Outdated Show resolved Hide resolved
proposals/0001.md Outdated Show resolved Hide resolved
proposals/0001.md Outdated Show resolved Hide resolved
FintanH and others added 4 commits February 27, 2020 10:05
Co-Authored-By: Alexander Simmerl <a.simmerl@gmail.com>
Co-Authored-By: Alexander Simmerl <a.simmerl@gmail.com>
Co-Authored-By: Alexander Simmerl <a.simmerl@gmail.com>
Co-Authored-By: Alexander Simmerl <a.simmerl@gmail.com>
@garbados
Copy link

garbados commented Mar 2, 2020

Looks good to me so far -- I third the point that code examples of using the suggested stack in our context would be helpful.

Copy link

@geigerzaehler geigerzaehler left a comment

Choose a reason for hiding this comment

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

Great work! Nothing to add

`struct` will be created to represent the custom error type. In the case of creating a library,
these can be left public (`pub`) if they are fine to expose. In the case of opaque errors for which
the library author wishes to prevent the end-user from doing any control flow, a combination of
a `pub struct` and hidden fields is recommended. I believe this gives maximal control to the library
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also #[non_exhaustive], which I think is also a good recommendation: always force library consumers to be prepared for new variants / fields.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh interesting 🤔 I'll add this to the recommendation.

example of a file error but also introduce a deserialization error.

```rust
pub struct DeserializationError(SomeDeserializationError);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm ok. I think I would in practice have an internal error enum, and unify that at a higher level in a variant ContentsOfSausage(Box<dyn Error>). Because, with this solution you would end up with a lot of standalone structs only for the purpose of hiding the underlying error, or no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well the struct can hide a private enum of opaque errors. So whatever is in the opaque struct is up to you. Whether it's dyn Error or something else, all it needs is the Error derivation.

My point for having this opaque struct method is to still allow the library to handle the errors if it thinks it can, otherwise it'll eventually become a String.

Would you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I agree, but I suspect that I’d be too lazy to exercise this diligently XD.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lol, we can always amend ;)

Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

Fine write-up, @FintanH ✌️

I would recommend you run this file through grammarly.com, mostly to add missing commas that make the text a bit harder to read at times.

String>` you can chain errors further by returning another `Err("Something went wrong")`. `String`
can almost be seen as an "unstructured error" since we are able to place whatever we like inbetween
the double-quotes. We can reverse pros and cons in contrast to structured errors. We avoid
"structural hell" but we can't inspect any of the information (unless you parse, but please don't)
Copy link
Contributor

Choose a reason for hiding this comment

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

unless you parse, but please don't

this!

proposals/0001.md Outdated Show resolved Hide resolved
FintanH and others added 2 commits March 13, 2020 10:44
Co-Authored-By: Nuno Alexandre <nunombalexandre@gmail.com>
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

📚 📀 🛎 ⌚️

@FintanH
Copy link
Collaborator Author

FintanH commented Mar 16, 2020

Thanks, everybody ❤️ 🌱

@FintanH FintanH merged commit 9b8c3dd into master Mar 16, 2020
@FintanH FintanH deleted the fintan/error-handling branch March 16, 2020 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants