Skip to content

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

Closed
@liigo

Description

@liigo

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?

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-enhancementCategory: An issue proposing an enhancement or a PR with one.T-libs-apiRelevant to the library API team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions