Rename {Option, Result}::expect to unwrap_or_panic by chris-morgan · Pull Reques...

BurntSushi commented 8 days ago

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 commented 8 days ago

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

shepmaster commented 7 days ago

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 commented 6 days ago

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 commented 4 days ago

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 commented 4 days ago

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

RalfJung commented 4 days ago

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

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 commented 4 days ago

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 commented 3 days ago

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

programmerjake commented 3 days ago

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

jongiddy commented 3 days ago •

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 commented 3 days ago

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 commented 3 days ago

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 commented 2 days ago

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?