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

feature: improve error_handling exercises #772

Merged
merged 3 commits into from
Jun 24, 2021

Conversation

tlyu
Copy link
Contributor

@tlyu tlyu commented Jun 7, 2021

Move result1 to errors4 and put it in the error handling section. Fixes #719.

Add new exercises errors5 and errors6, to introduce boxed errors and
custom error enums more gently. Delete errorsn, because it tried to do
too much too soon. Fixes #756.

tlyu added 2 commits June 6, 2021 23:08
Also put it in the ERROR HANDLING section where it probably belongs.
Add new exercises errors5 and errors6, to introduce boxed errors and
custom error enums more gently. Delete errorsn, because it tried to do
too much too soon.
Comment on lines 46 to 47
CreationError::Negative => "Number is negative",
CreationError::Zero => "Number is zero",

This comment was marked as outdated.

Comment on lines 26 to 27
PositiveNonzeroInteger::new(x)
.or(Err(ParsePosNonzeroError::CreationError))

This comment was marked as outdated.

// This is a custom error type that we will be using in `parse_pos_nonzero()`.
#[derive(PartialEq, Debug)]
enum ParsePosNonzeroError {
CreationError,

This comment was marked as outdated.

@tlyu
Copy link
Contributor Author

tlyu commented Jun 9, 2021

Thanks for the review! I agree with most of your changes.

I was planning to have map_err in one of the advanced exercises, but now that we've talked about passing functions instead of closures to map_err (because closures aren't used yet at this point in the exercise sequence), I guess it could go in this exercise.

Is there a consensus for error enum variants with payloads? I think I've seen both 1-tuple structs and structs with named fields. Are there tradeoffs involved? (I suspect there are, involving public vs private visibility of fields, but I'm not sure.)

std::num::ParseIntError does have a payload, though it's a private one at the moment, so I guess we shouldn't wrap that?

@yaahc
Copy link
Member

yaahc commented Jun 9, 2021

Is there a consensus for error enum variants with payloads? I think I've seen both 1-tuple structs and structs with named fields. Are there tradeoffs involved? (I suspect there are, involving public vs private visibility of fields, but I'm not sure.)

I don't think there is a clear consensus. There should be no difference in field visibility since all fields in enums are pub. The main difference is how the constructor is exposed. For a struct variant it uses the constructor syntax, but for unit variants the compiler essentially adds a function with the same name for the constructor. And I think I can see where you're going with this question.

enum WrappingError {
    Inner(InnerError),
}

struct InnerError;

fn main() {
    // The turbofish is only needed here because rustc can't infer the `T` parameter here.
    // This just set's it to () so the compiler leaves me alone, most code wouldn't need this turbofish
    Err::<(), _>(InnerError).map_err(WrappingError::Inner);
}

This is possible with tuple variants but not with struct variants, so it will probably be easiest to teach the tuple version since it provides the needed conversion function out of the box.

@yaahc
Copy link
Member

yaahc commented Jun 9, 2021

std::num::ParseIntError does have a payload, though it's a private one at the moment, so I guess we shouldn't wrap that?

I don't understand what you mean. The ParseInt payload is internal so we don't have to worry about it, but if anything the fact that they're not all identical and they have different runtime instantiations of that type means that we really need to wrap it, if we try to replace it with our own equivalent error type we will probably provide less information than the original error did.

@tlyu
Copy link
Contributor Author

tlyu commented Jun 9, 2021

I don't understand what you mean. The ParseInt payload is internal so we don't have to worry about it, but if anything the fact that they're not all identical and they have different runtime instantiations of that type means that we really need to wrap it, if we try to replace it with our own equivalent error type we will probably provide less information than the original error did.

Yeah, I guess you're right. Even if we can't make decisions based on those private variants, they're still relevant for display purposes.

@tlyu
Copy link
Contributor Author

tlyu commented Jun 9, 2021

This is possible with tuple variants but not with struct variants, so it will probably be easiest to teach the tuple version since it provides the needed conversion function out of the box.

I hadn't realized that tuple struct constructors actually were proper functions! I thought they were special struct expression syntax that happened to look like function calls. I'm not sure this exercise is the right place to introduce that concept. Or maybe it is? It seems likely to cause confusion if it hasn't been mentioned before.

@yaahc
Copy link
Member

yaahc commented Jun 9, 2021

I hadn't realized that tuple struct constructors actually were proper functions! I thought they were special struct expression syntax that happened to look like function calls.

hehe, yea. This comes up when you're re-exporting items in a library. If you a type alias to bring it in it will only import the type, but not the function, where as if you export it with a pub use it will bring in all items with the same name from all 3 namespaces.

I'm not sure this exercise is the right place to introduce that concept. Or maybe it is? It seems likely to cause confusion if it hasn't been mentioned before.

I think you're right on this, methods are less confusing and will more naturally lead into teaching about From impls.

Adjust error text and naming to conform with best practices.
Use `map_err()` instead of `or()`. Wrap lower-level errors instead of
ignoring their details.

Also, don't "cheat" by bypassing the `new()` function in tests.

Fix a dangling reference in the try_from_into hints.
Copy link
Member

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@shadows-withal shadows-withal left a comment

Choose a reason for hiding this comment

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

i trust your judgement @yaahc

@shadows-withal shadows-withal merged commit ec63cad into rust-lang:main Jun 24, 2021
ppp3 pushed a commit to ppp3/rustlings that referenced this pull request May 23, 2022
feature: improve error_handling exercises
dmoore04 pushed a commit to dmoore04/rustlings that referenced this pull request Sep 11, 2022
feature: improve error_handling exercises
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error-handling difficulty jump; teach custom error types Bug: error_handling/result1.rs order is wrong
3 participants