Skip to content

Add section on common message styles for Result::expect #96033

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

Merged
merged 12 commits into from
May 26, 2022
3 changes: 3 additions & 0 deletions library/core/src/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,9 @@ impl<T> Option<T> {
/// let x: Option<&str> = None;
/// x.expect("fruits are healthy"); // panics with `fruits are healthy`
/// ```
///
/// **Note**: Please refer to the documentation on [`Result::expect`] for further information
/// on common message styles.
#[inline]
#[track_caller]
#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
74 changes: 74 additions & 0 deletions library/core/src/result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,80 @@ impl<T, E> Result<T, E> {
/// let x: Result<u32, &str> = Err("emergency failure");
/// x.expect("Testing expect"); // panics with `Testing expect: emergency failure`
/// ```
///
/// # Common Message Styles
///
/// There are two common styles for how people word `expect` messages. Using
/// the message to present information to users encountering a panic
/// ("expect as error message") or using the message to present information
/// to developers debugging the panic ("expect as precondition").
///
/// In the former case the expect message is used to describe the error that
/// has occurred which is considered a bug. Consider the following example:
///
/// ```should_panic
/// // Read environment variable, panic if it is not present
/// let path = std::env::var("IMPORTANT_PATH").unwrap();
/// ```
///
/// In the "expect as error message" style we would use expect to describe
/// that the environment variable was not set when it should have been:
///
/// ```should_panic
/// let path = std::env::var("IMPORTANT_PATH")
/// .expect("env variable `IMPORTANT_PATH` is not set");
/// ```
///
/// In the "expect as precondition" style, we would instead describe the
/// reason we _expect_ the `Result` should be `Ok`. With this style we would
/// prefer to write:
///
/// ```should_panic
/// let path = std::env::var("IMPORTANT_PATH")
/// .expect("env variable `IMPORTANT_PATH` should be set by `wrapper_script.sh`");
/// ```
///
/// The "expect as error message" style does not work as well with the
/// default output of the std panic hooks, and often ends up repeating
/// information that is already communicated by the source error being
/// unwrapped:
///
/// ```text
/// thread 'main' panicked at 'env variable `IMPORTANT_PATH` is not set: NotPresent', src/main.rs:4:6
/// ```
///
/// In this example we end up mentioning that an env variable is not set,
/// followed by our source message that says the env is not present, the
/// only additional information we're communicating is the name of the
/// environment variable being checked.
///
/// The "expect as precondition" style instead focuses on source code
/// readability, making it easier to understand what must have gone wrong in
/// situations where panics are being used to represent bugs exclusively.
/// Also, by framing our expect in terms of what "SHOULD" have happened to
/// prevent the source error, we end up introducing new information that is
/// independent from our source error.
///
/// ```text
/// thread 'main' panicked at 'env variable `IMPORTANT_PATH` should be set by `wrapper_script.sh`: NotPresent', src/main.rs:4:6
Copy link
Member Author

@yaahc yaahc Apr 30, 2022

Choose a reason for hiding this comment

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

In an ideal world this example would either use .map_err(std::error::Report::new).expect(...) to show the proper error message rather than the Debug output of this source, or, once we land error in core and have landed an expect equivalent that requires E: Error we could automatically format this correctly.

For now I'm just letting it print poorly rather than complicating the example with an adapter type to convert Display to Debug, which would still be incorrect as a general recommendation if the wrapped error type had a source that needs to be printed.

/// ```
///
/// In this example we are communicating not only the name of the
/// environment variable that should have been set, but also an explanation
/// for why it should have been set, and we let the source error display as
/// a clear contradiction to our expectation.
Copy link
Member

Choose a reason for hiding this comment

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

I like the strong language here, because it also fits well with a more general thing about saying that .expect is generally not supposed to be for user-visible stuff (though of course it might happen) since it's for things that you have a reason to expect won't happen, and that reason is what goes in the text. (Said otherwise, result errors says what went wrong, expect messages are for saying why you think it shouldn't have happened.)

///
/// For programs where panics may be user facing, either style works best
/// when paired with a custom [panic hook] like the one provided by the CLI
/// working group library, [`human-panic`]. This panic hook dumps the panic
/// messages to a crash report file while showing users a more friendly
/// "Oops, something went wrong!" message with a suggestion to send the
/// crash report file back to the developers. Panic messages should be used
/// to represent bugs, and the information provided back is context intended
/// for the developer, not the user.
///
/// [panic hook]: https://doc.rust-lang.org/stable/std/panic/fn.set_hook.html
/// [`human-panic`]: https://docs.rs/human-panic
#[inline]
#[track_caller]
#[stable(feature = "result_expect", since = "1.4.0")]
Expand Down