Quick draft "Result::expect" rfc

[thepowersgang]

See also rust-lang/rust#25359 for implementation

Rendered

[nagisa]

Not a drawback per se.

[gkoz]

The bound should probably be Display.
The output of unwrap and Debug isn't meant to reach the user, in that context adding a function that shows a message seems pointless. If it's Display, this would remove the need to do panic!("error: {}", e); (example).

[thepowersgang]

unwrap and expect are very similar methods. expect is intended to also to not be for user consumption (it panics after all).
The intent is to provide a version of unwrap that provides context on the error.

[gkoz]

it panics after all

Yeah, command line utilities (like rustc) have the luxury of using panics for error handling. It would be disappointing to have expect that can't be used for that.

[nagisa]

In rustc an occurence of panic is ICE which should be fixed, so that’s a bad example.

[gkoz]

A panic caused by an ICE gets caught and then the code goes on to panic. Are you suggesting the latter should be fixed?

Both of you seem to be implying that a panic is not a valid way for a utility to die on a fatal error. Any citation for that?

[quantheory]

According to RFC 236, which is focused on the standard library but is intended to be a guideline for Rust libraries in general, panics should only be used for bugs (not user facing) or when there is nothing else that you can meaningfully do (e.g. out of memory). This is why unwrap uses a Debug bound currently, AFAIK. Using unwrap or expect is basically an assertion that you will never get an Err value (unless there is a bug in your program, of course), and thus that a user will never see this error.

It is absolutely the case that whenever rustc panics, it is due to a bug. The code you linked should execute rarely or never in a release, but it exists in order to assist the user in filing a bug report when there is an error. There's no expectation that the user will be able to understand anything about the output, other than that they should send it to the Rust developers in a bug report.

My understanding is that, if you want a utility to be able to die in "normal" circumstances (i.e. not a bug in the program itself), it's preferable not to use an uncaught panic, but to print a message yourself and then call std::process::exit. We don't have any hard documentation that I know of about this, but I think that's in part because std::process::exit was only stabilized recently (a month or so ago, during the beta/release rush?).

If you want destructors to run, you do have to return up to main (or whatever level has the destructors you need) before exiting, which is an inconvenience in the current design. I think #1100 is intended to address this in part by allowing panics to be done more "quietly" and then caught at a higher level (but it is still very coarse).

[bluss]

panics should only be used for bugs (not user facing) or when there is nothing else that you can meaningfully do

Isn't this view too narrow for a general programming language? That kind of rule works for a mature application, but does it work on the way there? Or for tests, examples, wild experiments? I think we need to think of that kind of code too, and allow some easy escapes like .unwrap() or .expect().

[quantheory]

To be clear:

a) I'm just giving my interpretation of the vague current conventions, not something that I have 110% support for.

b) The idea is not that you should avoid unwrap/expect, it's that they act as assertions that should only fail during testing/development, when a developer is the one encountering the error rather than a user. Therefore it's fine to use Debug instead of Display. So for "tests, examples, wild experiments," and new projects, having a lot of panics is not a problem, because in theory the people encountering these errors are either developers or relatively advanced users testing an experimental/beta version. (If Debug output is impossible even for a developer to understand, that's a whole separate problem.)

(This discussion reminds me of a rant I read about enabling the assert macro in release builds of C applications a while back. Though the situation is not exactly analogous, since in this case the assertion is still "there" in the release code, we just don't expect it to ever trigger, regardless of how bad the inputs to the program are.)

[gkoz]

@quantheory thanks for the elaboration.

If Debug output is impossible even for a developer to understand, that's a whole separate problem.

It's more that the Debug output is sometimes less helpful in the case of unwrapping Results:

• If this is a Result<_, String> we get a string with newlines rendered as \n instead of just text.
• If this is an io::Result, we get Os(some_number) instead of the actual error message.

So if expect is to provide some convenience over unwrap, a middle ground between just spitting out a line number of the failed assert and using proper error handling, utilizing Display feels like better balance to me.

[bluss]

Seems logical to me. After you get to know .expect() on Option, you'll expect to find the method on Result too.

[Manishearth]

@Gankro @sfackler @huonw This needs a sheriff

[nielsle]

This would be great for quick and dirty scripts. It would also make one of the first examples in the rust book a little easier to read:

   io::stdin().read_line(&mut guess)
        .ok()
        .expect("Failed to read line");

https://doc.rust-lang.org/book/guessing-game.html

[cmr]

I dislike this signature, and much prefer:

impl<T, U: Debug, V: Display> Foo<T, V> for Result<T, U> {
    fn expect(self, msg: V) -> T {
        match self {
            Ok(v) => v,
            Err(e) => panic!("Error: {} ({:?})", msg, e)
        }
    }
}

[cmr]

(And I think Option should be the same)

[thepowersgang]

Ooh, interesting idea. This would allow passing format_args! for formatted messages

[grissiom]

I think .expect("Failed to read line") mean to be "expect to Fail..." which is logically inverse to what it is done. I think .on_error("Failed to read line") should be more logical. Am I the only one thinking this way?...

[thepowersgang]

@grissiom Possibly, this RFC just brings the avaliable methods into line with Option's methods.

[gsingh93]

@grissiom, I completely agree. I think this should at least be discussed before accepting this RFC so we'd only have to deprecate one method, not two, if this were accepted.

[thepowersgang]

@grissiom @gsingh93 The .expect() method is part of the stable API for Option, and thus cannot be changed without incrementing the language's major version.

[gsingh93]

Yes, but it can be deprecated.

[grissiom]

@gsingh93 @thepowersgang OK, when I have time, I will open an other RFC to discuss Option::expect and reference this RFC. Thanks.

[alexcrichton]

This RFC is now entering the week-long final comment period.

[cmr]

I still REALLY think that this should take msg: Display, but that can be added after the fact. I'm happy with this RFC in its entirety (including the format string).

[gkoz]

I don't think the point about Debug formatting for errors being inferior to Display has been addressed.

Which of these are more helpful?
Error { repr: Os(2) } vs No such file or directory (os error 2)
JoinPathsError { inner: JoinPathsError } vs path segment contains separator ':'

Maybe this issue should be raised elsewhere though because it's not a problem with expect per se.

[BurntSushi]

Maybe this issue should be raised elsewhere though because it's not a problem with expect per se.

I suspect this is the right way to think about it. For better or worse, Option::unwrap, Option::expect, Result::unwrap and Result::unwrap_err are all bounded with Debug. Making Result::expect use Display instead seems pretty surprising to me.

[Gankro]

The text as written now is a bit unclear. I'd really like it to explicitly provide the signature and implementation (since it should be trivial to provide, and is the entire point of the RFC).

[thepowersgang]

@gkoz That should be raised somewhere, probably on the offending error type, making Debug include the error text as well as the number.

[sfackler]

Maybe this issue should be raised elsewhere though because it's not a problem with expect per se.

Agreed. We could change the conventions for error types to have more informative Debug output, or, when specialization is implemented, use Display output for objects implementing Debug+Display and Debug output for objects only implementing Debug as a couple of options.

[bluss]

Edit: see below.

@BurntSushi: I think the cases are different. The Debug bound is on the value inside the Result. The Display bound would be on the message you pass to .expect(). Display is a (natural?) generalization of passing just a &str for a message. Debug is not a generalization of passing a string at all.

Also, we can demand a rarer trait like Display when the user must consciously choose to pass in a fitting message. We need Debug for the other cases to be able to work with a broader range of values contained in the Result.

[gkoz]

@bluss but that part of discussion was about the value inside the Result, not the message.

[bluss]

@gkoz Oops! I'm sorry for the noise.

[alexcrichton]

The consensus among the libs team is to accept this RFC now. The matter of generalizing the argument of expect to T: Display can be handled at a later date (and should also affect Option as well). Additionally, using Debug instead of Display for the error type in the result follows the precedent of the unwrap method, which while without downsides covers the most possible types today.

Thanks for the RFC @thepowersgang!