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

{Result,Option}::expect() hurt the readability of Rust code #35083

Closed
liigo opened this issue Jul 28, 2016 · 6 comments
Closed

{Result,Option}::expect() hurt the readability of Rust code #35083

liigo opened this issue Jul 28, 2016 · 6 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@liigo
Copy link
Contributor

liigo commented Jul 28, 2016

Some bad smell code samples in tree:

in RawVec::with_capacity():

cap.checked_mul(elem_size).expect("capacity overflow");

It maybe read as the author expect that capacity overflows.

in BTreeMap Index impl:

self.get(key).expect("no entry found for key")

It maybe read as the author expect that no entry found for key.

in Rc tests:

Rc::downgrade(&a)::upgrade().expect("upgrade of live rc failed");

It maybe read as the author expect ...(something)... failed?

in MIR:

self.terminator.as_ref().expect("invalid terminator state")

It maybe read as the author expect an invalid terminator state?

Now in Rust community, expect is widely used. There are too many samples to list all of them here.


These code are quite counter intuitive, they hurt the readability of Rust code.

The root source of all confusing come from the meaning of the 'msg' arg of expect().

Currently, it is interpreted as 'the error message on unexpected state'.

If we could change it to 'the message expounds expected behavior', that would be great.

And that will improve the readability significantly:

.expect("no capacity overflow");            // old: expect("capacity overflow")
.expect("at least found an entry for key"); // old: expect("no entry found for key")
.expect("upgrade of live rc successfully"); // old: expect("upgrade of live rc failed")
.expect("valid terminator state");          // old: expect("invalid terminator state")

(The implementation of expect() requires to be changed slightly.)


But, is this a BREAKING-CHANGE?

Yes, and No.

The meaning of an arg of a stable method changed significantly, this IS a breaking-change. But the code logic don't change, program's behavior is not touched. What really changes is just the panic message.

If we do this change, all expect() caller side code should be reviewed carefully, almost all msg arg strings should be edited. Now i must admit honestly, that is a BIG breaking-change!

If we treat this as a soundness issue like RFC 1214, we can and we should fix it, whatever it's breaking-change or not.

Maybe the easiest way is create another expect2() (or any creative name), and deprecated the current expect()?

What do you think about this, Rustaceans?

@chris-morgan
Copy link
Member

Renaming expect to unwrap_or_panic would make it all perfectly clear. I have a feeling that the name of expect was discussed at length some time before 1.0 but evidently it was not changed at that point.

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 25, 2017
@Swoorup
Copy link

Swoorup commented Sep 6, 2017

I agree with this as well. What about using unexpected instead of expect 😄

@pengshao
Copy link

Actually I think it means literally what "expect" means: regard sth. as likely to happen. Notice that it does not necessarily mean the subject "hopes", or "wishes" sth to happen subjectively.

As in this code piece, it means the author of the code is expecting something to go wrong and to cause a panic, therefore s/he intentionally leaves a message explaining what the expected error is.

@owwk
Copy link

owwk commented Jul 28, 2021

I really think that expect should have been called something like or_panic.

Would not be a breaking change, just create a new alias and add a deprecation warning for expect.

self.get(key).expect("no entry found for key")

would then become

self.get(key).or_panic("no entry found for key")

which is far more logical and readable, in my opinion.

@pkcinna01
Copy link

pkcinna01 commented Jan 4, 2022

"expect" clashes with every language I can think of... python has "except" and "raise", rx libraries have .catchException(), other languages have try/catch/throw. Javascript promises have .then().catch(). The method "expect" feels like a rite of passage into Rust because you must know the history to understand the name choice. Clarity and conciseness should trump Rust specific historic motivations.

@workingjubilee
Copy link
Member

It seems this issue involves people proposing two things:

  • Strongly recommending a message style that steers people away from bad patterns.
  • Renaming expect or otherwise churning the name.

Now, regarding the latter:

  • The RFC to churn this name was opened.
  • The RFC to churn this name was rejected.

Regarding the former:

So it seems this has been resolved to the extent possible. Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants