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.

Sign up for GitHub

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

Quick draft "Result::expect" rfc #1119

Merged
merged 1 commit into from
Jun 10, 2015
+40 −0
Merged

Quick draft "Result::expect" rfc #1119

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions text/0000-result-expect.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
- Feature Name: `result_expect`
- Start Date: 2015-05-13
- RFC PR: (leave this empty)
- Rust Issue: (leave this empty)

# Summary

Add an `expect` method to the Result type, bounded to `E: Debug`
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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


# Motivation

While `Result::unwrap` exists, it does not allow annotating the panic message with the operation
attempted (e.g. what file was being opened). This is at odds to 'Option' which includes both
`unwrap` and `expect` (with the latter taking an arbitrary failure message).

# Detailed design

Add a new method to the same `impl` block as `Result::unwrap` that takes a `&str` message and
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

(And I think Option should be the same)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

returns `T` if the `Result` was `Ok`. If the `Result` was `Err`, it panics with both the provided
message and the error value.

The format of the error message is left undefined in the documentation, but will most likely be
the following
```
panic!("{}: {:?}", msg, e)
```

# Drawbacks

- It involves adding a new method to a core rust type.
Copy link
Member

Choose a reason for hiding this comment

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

Not a drawback per se.

- The panic message format is less obvious than it is with `Option::expect` (where the panic message is the message passed)

# Alternatives

- We are perfectly free to not do this.
- A macro could be introduced to fill the same role (which would allow arbitrary formatting of the panic message).

# Unresolved questions

Are there any issues with the proposed format of the panic string?