Add section on common message styles for Result::expect

[yaahc]

Based on a question from rust-lang/project-error-handling#50 (comment)

One thing I haven't decided on yet, should I duplicate this section on Option::expect, link to this section, or move it somewhere else and link to that location from both docs?: I ended up moving the section to std::error and referencing it from both Result::expect and Option::expect's docs.

I think this section, when combined with the similar update I made on std::panic! implies that we should possibly more aggressively encourage and support the "expect as precondition" style described in this section. The consensus among the libs team seems to be that panic should be used for bugs, not expected potential failure modes. The "expect as error message" style seems to align better with the panic for unrecoverable errors style where they're seen as normal errors where the only difference is a desire to kill the current execution unit (aka erlang style error handling). I'm wondering if we should be providing a panic hook similar to human-panic or more strongly recommending the "expect as precondition" style of expect message.

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• a stabilization of a library feature
• introducing new or changes existing unstable library APIs
• changes to public documentation in ways that create new stability guarantees

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[wonderfulspam]

Is there not a third style here that combines the two, using the "expect as error message" style for the message and simply writing the precondition in a comment similar to a // Safety: comment before an unsafe block?

I'm struggling to see the advantages of the "expect as precondition" style given that one would almost certainly look at the offending line when debugging an issue encountered by a user (or oneself) and thus come across the comment explaining the precondition. It removes one layer of indirection (the need to read through the code) but that seems like a fairly small win. I'd be much happier if expect messages would converge towards one style across the Rust ecosystem.

[yaahc]

Is there not a third style here that combines the two, using the "expect as error message" style for the message and simply writing the precondition in a comment similar to a // Safety: comment before an unsafe block?

I'm struggling to see the advantages of the "expect as precondition" style given that one would almost certainly look at the offending line when debugging an issue encountered by a user (or oneself) and thus come across the comment explaining the precondition. It removes one layer of indirection (the need to read through the code) but that seems like a fairly small win. I'd be much happier if expect messages would converge towards one style across the Rust ecosystem.

I hadn't seen this style before. I wouldn't want to recommend using "Safety" personally, because I think that should be reserved for unsafe code, but the general idea sounds reasonable to me. I'm certain there are probably other styles beyond even these three, so I imagine this will be a living document as more people come forward and add context that we currently lack.

As for advantages, I think it comes down to how you're using expect. So long as you're using expect exclusively for situations where you legitimately don't expect to ever encounter that error, you're presumably not able to anticipate exactly why any given expect would encounter an error, so your error message wouldn't be providing more useful context on what must have failed, beyond the basic "encountered unexpected Err variant", which is implicit in expect and maybe should become part of the default output some day. Part of the issue though is that expect formats errors via Debug, rather than via Error, so it often discards context that is relevant, not to mention that many of the errors we export from std include very little context by default. This is already something I'm working on fixing via the effort to move error into core, which would then allow us to have an alternative expect API for Error types, or if specialization ever becomes a thing that we can use, possibly even a specialization within expect.

For comparison, I setup a couple test cases for each approach and I tossed in the experimental std::error::Report type to produce an actual error message for the source, rather than just the Debug output:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=fe0e1167eae5a2dcd452d8e12d48fe42

IMO, the only useful information being added in the former style is that it is mentioning the env variable's name, though both styles express this info so it's not an advantage necessarily. Beyond that all the information being expressed is a duplicate of information already present in the original source error.

Looking at this though I think I want to go back and apply your second suggestion for how you fixed the typos in my expect precondition message. The "should be" framing seems to fit a lot better than the "is always" one. I think this might be closer to my ideal usage / output, assuming we had the specialized expect that uses Error and a slightly changed default output:

let path = std::env::var("IMPORTANT_PATH")
    .expect("env variable `IMPORTANT_PATH` should always be set by `wrapper_script.sh`");

With an output something like this... (note: extremely strawman output, I have no idea if we would even be okay with changing the default output of the expect method, though if we have to duplicate it to handle a lack of specialization making drastic changes to the default output seem much more in reach, and I've decided to dream big rather than cargo cult the existing style)

thread 'expect_as_precondition' encountered an unexpected `Result::Err` at src/lib.rs:16:10

Expected:
    env variable `IMPORTANT_PATH` should always be set by `wrapper_script.sh`

Error:
    environment variable not found

[yaahc]

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.

[scottmcm]

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.)

[scottmcm]

I like all this text, though I had one pandora note.

One thing that's not super obvious to me, though, is whether this makes sense in a method's documentation. That's a long comment, and I'm not sure that having to scroll past it to see all of Result's methods makes sense.

Maybe there's a way to put a paragraph in the actual method, and merge the long-form discussion into a book somewhere?

[yaahc]

I like all this text, though I had one pandora note.

One thing that's not super obvious to me, though, is whether this makes sense in a method's documentation. That's a long comment, and I'm not sure that having to scroll past it to see all of Result's methods makes sense.

Maybe there's a way to put a paragraph in the actual method, and merge the long-form discussion into a book somewhere?

I'm not opposed to putting it in the book, and I want this reachable from as many places as possible, but I have an alternative idea. I've been thinking about reclaiming the std::error module docs and turning those into long form docs about all of the pieces of error handling provided by libs/lang. Then I could cross link to that from both the Result/Option docs and the book.

[scottmcm]

This looks basically good, but it looks like there's an issue in the PR build? If you get that fixed I'll do a last once over and approve.

[yaahc]

This looks basically good, but it looks like there's an issue in the PR build? If you get that fixed I'll do a last once over and approve.

yea, working on that and adding more links in the generated html, I'll push a new copy shortly

[yaahc]

@rustbot ready

[scottmcm]

Looks good!

@bors r+ rollup

[bors]

📌 Commit ef879c6 has been approved by scottmcm