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

Rename {Option, Result}::expect to unwrap_or_panic #3218

Closed
wants to merge 1 commit into from

Conversation

chris-morgan
Copy link
Member

@BurntSushi
Copy link
Member

The first question is then whether it’s worth the bother of changing. I think the answer is yes, because we’ve made such change easy, and this makes the standard library more consistent and helps newcomers, without meaningfully inconveniencing oldtimers.

Very strongly disagree. I absolutely do not think such a change is easy. It's only easy if you don't look at its impact on the ecosystem, including documentation (outside of official docs) and blogs and printed materials.

expect is too widely used to abandon it IMO. I agree that unwrap_or_panic is more descriptive, although its verboseness feels pretty bad to me. Regardless, it is not anywhere close to "better enough" to warrant this change IMO. Similarly, expect is also not bad enough to warrant its renaming at all, IMO.

@SOF3
Copy link

SOF3 commented Jan 9, 2022

I can imagine that the existence of unwrap_or_panic could potentially mislead people briefly into thinking unwrap doesn’t panic.

That's my first reaction too. If anything, expect seems to have stronger meaning of panicking then unwrap does since the verb itself implies some condition, while unwrap intuitively means something similar to what you'd describe with "flatten" (for me).

@ehuss ehuss added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jan 9, 2022
@shepmaster
Copy link
Member

I'm sympathetic to the fact that expect isn't a perfect name (I've got my own issues as well 1). That being said, unwrap_or_panic isn't a better name to me as it suggests that unwrap doesn't panic. If we went down this road, it seems like unwrap should be renamed to unwrap_or_panic and then expect could become unwrap_or_panic_with.

I agree that a large bar needs to be cleared to make this change, but also want to point out that Makefiles use tabs because the author didn't want to break compatibility for tens of users, later affecting many more. If Rust continues to grow and this affects people enough, it's worth considering inconveniencing the current (relatively smaller) group in favor of improving the situation for a future (relatively larger) group.

Footnotes

  1. My personal gripe is that I read it as "I expect that $some_variable is ...":

    // I want to read this as "I expect that `age` was `Some(...)`"
    age.expect("age was Some");
    

    However, the error message is the opposite polarity:

    thread 'main' panicked at 'age was Some'

@eddyb
Copy link
Member

eddyb commented Jan 11, 2022

IMO a big mistake we made with Option method naming was using the same unwrap word even in non-panic cases.
unwrap_or, unwrap_or_else, unwrap_or_default could've used get or some other word instead of unwrap (if inconsistencies are allowed, the last one could be even just or_default).
I've even seen people suggest unwrap itself could've been named get_or_panic or just or_panic.

As a result, I feel like this RFC is going in the wrong direction. If .unwrap() was .or_panic(), then maybe .expect() could've been .or_panic_msg("...") or something similar. The difference between them is only the message, so it would be nice if the name reflected that.

@dlight
Copy link

dlight commented Jan 13, 2022

expect is a bad name (because of the polarity issue raised by @shepmaster) but my trouble with unwrap_or_panic is that unwrap also panics.

@gulshan
Copy link

gulshan commented Jan 13, 2022

May be unwrap_with_panic_msg or unwrap_with_err would be more consistent. How about just .panic("...")?

@RalfJung
Copy link
Member

IMO a big mistake we made with Option method naming was using the same unwrap word even in non-panic cases.

I agree using unwrap for all of them is not great. IMO the panic cases should involve the term assert.

But that's water down the bridge...

@loafofpiecrust
Copy link

I definitely agree with other folks that unwrap_or_panic implies that unwrap doesn't panic. If we are willing to change expect at all then I would personally be in favor of reversing the polarity of the message to match assert. Every time I see/use expect I get confused about what the message means especially in someone else's code. Otherwise for a new name, perhaps unwrap_with("operation failed")? This one has less semantic baggage for me.

@ssokolow
Copy link

I'm not sure how viable it is in an absolute sense (how many people are parsing the human-readable output?) but wouldn't it be more viable to keep .expect() and change the textual output to something like this:

`thread 'main' panicked. Expected 'age was Some'

@SOF3
Copy link

SOF3 commented Jan 14, 2022

with seems to imply a closure should be passed. See anyhow::Context::with_context.

@programmerjake
Copy link
Member

well, if we really have to pick a new name, how about something like unwrap_msg?

@jongiddy
Copy link

jongiddy commented Jan 14, 2022

I'd suggest Result::ok_or_panic, Result::err_or_panic, Option::some_or_panic. And I would have liked the same pattern for all the unwrap_or... methods.

However, I do think expect is here to stay. And for consistency with the current methods, this RFC should either add unwrap_or_panic as an alias for expect or do nothing.

@ssokolow
Copy link

And for consistency with the current methods, this RFC should either add unwrap_or_panic as an alias for expect or do nothing.

...but that wouldn't be consistent with current methods so long as unwrap still exists and panics, because it'd encourage the intuitive impression among newcomers that "clearly, unwrap must do something other than panic because unwrap_or_panic also exists".

@jongiddy
Copy link

If unwrap_or_panic is unacceptable, so anything introduced will be inconsistent with the existing unwrap_or... methods, then I affirm my initial suggestions: Result::ok_or_panic, Result::err_or_panic, Option::some_or_panic.

@tanriol
Copy link

tanriol commented Jan 15, 2022

This is more of a long-term thought, but if at some point Rust gets optional arguments (on wishlist in #323), would it be possible to make unwrap take an optional argument and thus combine unwrap and expect into one function?

@joshtriplett
Copy link
Member

@rfcbot close

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 25, 2023

Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels Feb 25, 2023
@joshtriplett
Copy link
Member

@ssokolow If you'd like to propose the message change you mentioned as a separate PR, I'd be happy to see that happen, and would be happy to r+ it.

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Feb 25, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Feb 25, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@ssokolow
Copy link

@joshtriplett What kind of detailed guides are available on the DOs and DON'Ts of making such a PR?

I'm game, but things are a bit crazy right now, so I don't really have time for research/self-directed learning on a contribution process I've never participated in before.

@joshtriplett
Copy link
Member

@ssokolow There's the rustc-dev-guide, but in the context of a simple message change, I think a simple PR changing that one line should suffice. Remember to update any tests that check for the old message, and run ./x.py fmt.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Mar 7, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 7, 2023

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This is now closed.

@rfcbot rfcbot added closed This FCP has been closed (as opposed to postponed) and removed disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels Mar 7, 2023
@rfcbot rfcbot closed this Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed This FCP has been closed (as opposed to postponed) finished-final-comment-period The final comment period is finished for this RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.