Lint idea: forbid all expect and unwrap use

[woodruffw]

What it does

It would be nice to have a restriction lint (i.e., totally opt-in) that allows clippy users to completely ban all expect and unwrap use from a codebase. Functions or other items that absolutely need expect and/or unwrap or that have been manually reviewed by a human could be marked explicitly with an attribute.

It might also make sense to check for panic!.

Categories (optional)

This would probably be a restriction lint, since it's not desirable for all codebases.

What is the advantage of the recommended code over the original code

expect and unwrap are a normal part of safe Rust programming, but are often an unintentional escape hatch for poor data validation or error handling. Their use doesn't represent a security vulnerability per se, but can result in denials-of-service and are also generally less readable than the equivalent error-handling approach. Programmers in high-assurance codebases might want to ensure that none of their routines (or their dependencies' routines) perform unwraps, which this lint would allow.

Drawbacks

None. As a restriction lint, users would have to explicitly opt into it.

Example

fn whatever() -> T {
  let thing = something().unwrap();
  thing.as_t()
}

would result in an linting error indicating that the user has requested unwrap be forbidden, and encouraging the user to add an attribute (something like #[allow(clippy::reviewed_unwrap)].

I'm capable of implementing this lint, if there's interest!

[giraffate]

FYI there are some restriction lints. expect_used and panic lints are seems to be good, but unwrap_used may not be suitable for this case due to suggesting use of expect.

• https://rust-lang.github.io/rust-clippy/master/index.html#expect_used
• https://rust-lang.github.io/rust-clippy/master/index.html#panic
• https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_used

[woodruffw]

Thanks for the pointers! Yeah, the current panic and expect_used lints look pretty close to what I'd want -- would anybody be opposed to a link that combines the two + similar behavior for unwrap into a single lint?

[giraffate]

IMHO, I like the new suggestion that just check use of unwrap, but I guess combining those into one seems not to be needed because those lints can be used at the same time.

[woodruffw]

I like the new suggestion that just check use of unwrap, but I guess combining those into one seems not to be needed because those lints can be used at the same time.

Makes sense! I'll work on a variant of unwrap_used that performs similarly to the current expect_used and panic checks.

[giraffate]

Yes, or just changing the message of unwrap_used may be good if it has same logic. I'm not a maintainer ( just a contributor ), so you may want to open a WIP pull request or ask a question of implementation on Zulip.

[woodruffw]

I played around with it a bit more, and the following actually does exactly what I wanted!

cargo clippy -- -D clippy::expect_used -D clippy::panic  -D clippy::unwrap_used

(unwrap_used does still recommend expect, but that's perfectly reasonable for my use case -- I'm more just interested in enumerating and manually reviewing each).

I'll leave this open just in case others get value out of it/want to pursue it further, but my particular use case is already solved. Thanks again to @giraffate and the clippy maintainers more generally for such an excellent tool 🙂

[schickst]

I would really love to have these enabled by default. Especially for people coming from less strict languages, this is useful to learn proper form and prevents from getting sloppy, or when going from a prototype to production ready code. People who don't care about it, probably won't use clippy in the first place.

[woodruffw]

I would really love to have these enabled by default.

IME, there are a variety of reasonable Rust patterns that result in Result and Option values that a human can verify, but that the compiler can't (in the general case). Flagging each and every single one of those would probably result in a lot of developer fatigue, especially for language newcomers. There are also plenty of patterns where it's more reasonable to unwrap or expect than to plumb a failure up to the user level, such as long data pipelines where such an error should result in a hard exit as close to the error site as possible.

[ChrisJefferson]

I genuinely not sure why you think expect() is "sloppy". If something goes badly wrong (can't read or write a file for example, or I find a list I expect to contain some value unexpectedly doesn't), I feel the best option for a (non-interactive) program is just to quit ASAP -- I can't reasonably recover from that, and the longer the program keeps going, the higher the risk something bad will happen.

[hbina]

@ChrisJefferson One use-case I've experience (albeit the circumstances itself is dumb, but this is the reality) is when the program is running in a black box and the only way to know something bad happen is to propagate this error all the way out of the system. Otherwise, it will just panic and you have to restart it. For example, an HTTP server that's running in a machine for which I don't have access to (so no logging or anything).

[dan-da]

I would like to see these lints on by default.

It is difficult to write "high assurance" software in rust because so many dependency crates may panic due to usage of unwrap and expect. And there is presently no way to indicate if a fn or crate may panic or not.

I think it should be strongly discouraged to use these, meaning that if you insist on it, then it should be marked with a clippy allow.

just my 2 cents.