Be discouraging about unwrap() lints

[mulkieran]

Please see #1300 for previous discussion.

My position is in the comments, but it essentially means that programmers should use expect() to communicate verbally an invariant in their code which logically implies that the unwrap() should not fail.

Thus, the use of unwrap() should be discouraged. Programmers can use expect() to indicate logical impossibility of failure and return an error to handle the failure.

I would welcome suggestions about how to move this out of clippy into a more central repo like rust documentation. The examples of the use of expect() that I have seen in the docs are generally no more informative than a bare unwrap(), something like:

b.expect("This should work") which is identical in information content to b.unwrap() IMHO.

[oli-obk]

I often have used b.expect("checked above"), which isn't all that helpful either...

[Manishearth]

Yeah, I'm for making the ones that suggest expect over unwrap warn.

[oli-obk]

@mulkieran You're probably going to get few counter arguments judging from the other issue. Do you want to open a PR changing the level from allow to warn?

[llogiq]

I personally think that Result::unwrap() is ok, as it's often better than a generic message. I'm ok with warning on Option::unwrap() though, as long as it's not in test cases. At least for now. Once we can return T: Try in main() and tests (and I understand this work is underway), we should ask people to use ? instead for both Option and Result.

[mulkieran]

@llogiq Currently, I unwrap() like its going out of style in test cases, so I understand where you're coming from. Maybe I should re-evaluate, though.

[llogiq]

As I said, once we can use ? in tests like unwrap (well, perhaps with better output) I'm all for changing the code. For now, it'll only confuse or frustrate our users.

[mulkieran]

@oli-obk I don't mind waiting for a bit more input. Part of my position is that the way expect() is generally used in examples in the docs makes its existence virtually pointless (and that that's a shame). And that clippy might be a helpful agent of change.

@llogiq Why does not using Result::unwrap() lead to a generic error message?

[llogiq]

Let's say I have a Result<_, Box<Error>> (or any other type implementing the Error trait, but I'll box here for simplicity). Now if you .unwrap() you'll get the contained error description, whereas with .expect("this should not happen!") you just get "this should not happen!".

[oli-obk]

    Err::<(), &str>("foobar").expect("this should not happen");

yields

thread 'main' panicked at 'this should not happen: "foobar"',

[raindev]

I was surprised to not get any warnings from clippy when using Option::unwrap. E.g. in IntelliJ I would get a warning every time I try to call Java's Optional::get without guarding it by if (optional.isPresent). Option should be mapped over, methods that unwrap Option are the last resort, in my opinion. I think making the developer explicitly acknowledge the case that is not always safe with Option::expect is a good idea. Unsafe APIs should be more clumsy to use.

Another point is that the documentation frequently overuses and misuses both unwrap and expect methods. A good expect message would be one that explains why the invariant holds true. E.g. c.to_digit(10).expect("input to not allow non-digits"). Panic message could include "expect" prefix to make the error message clearer:

thread 'main' panicked at 'expect input to not allow non-digits'

[Lokathor]

So, the main reason that I sometimes don't use expect is that the text still shows up in the binary, and lately I've been working on the GBA, where you get 32MB total for everything, so spare text you don't really need is just junk data. So I just unwrap with a comment in the source.

[oli-obk]

So, the main reason that I sometimes don't use expect is that the text still shows up in the binary, and lately I've been working on the GBA, where you get 32MB total for everything, so spare text you don't really need is just junk data. So I just unwrap with a comment in the source.

That seems like enough of an edge case to turn off the lint, not to change the defaults.

[Lokathor]

Oh I'm all with you. I'm shocked that "don't use unwrap you dummy, at least use expect" wasn't the first lint that clippy ever implemented.

[camsteffen]

I don't see anything changing here. unwrap is perfectly sensible in certain cases and blanket warning by default would be too noisy.