12 ms·
`unwrap()` is "handling" errors by crashing the program, so it shouldn't really be used. It's common in examples, because it's usually the shortest/laziest code
by pornel 7y ago
`unwrap()` is "handling" errors by crashing the program, so it shouldn't really be used. It's common in examples, because it's usually the shortest/laziest code possible. However, all practical uses of `.unwrap()` have better alternatives, e.g. instead of
if foo.is_some() {
let unwrapped = foo.unwrap();
}
you write:
if let Some(unwrapped) = foo {
}
There are plenty of combinators like .map_or(), .ok_or()?, .filter_map() that deal with optional values gracefully.
- gdxhyrd 7y agoIn libraries, yes. In applications, no. A lot of errors are fatal.
- cesarb 7y agoOne of the commits in one of the first MRs in this article shows an example of a legitimate use of `unwrap()`... pub fn new(mut streams: Vec<TokenStream>) -> TokenStream { match streams.len() { 0 => TokenStream::empty(), 1 => streams.pop().unwrap(), _ => TokenStream::Stream(Lrc::new(streams)), } } Since `streams.pop()` will never return `None` when `streams.len()` is `1`. But I agree, most of the time using `match` (or its simplified form `if let`) or one of these combinators instead of `unwrap()` is better.
- asdkhadsj 7y agoAs a sidenote, I've been thinking about instituting an internal styling guide to discourage `unwrap()` in your scenario, in favor of `expect("impossibly missing")` or etc. The message helps a bit more, and I also want to avoid debug unwraps that were forgotten about. Though, the more I think about it the more a macro makes sense to me, purely for code search. If we used a macro, something like `impossible!(streams.pop())` then I can code search for `.unwrap` and `.expect`. Hmm
- rrobukef 7y agoA compiler error if the code cannot be eliminated during optimization. Kind-of hacky, but it could be valuable as a new function.
- jashmatthews 7y ago> `unwrap()` is "handling" errors by crashing the program, IIUC, panicking via unwrap() or expect() IS graceful error handling. It unwinds the stack and releases resources. We should be clearer because it's confusing to people less familiar with Rust who will confuse this with actually crashing and immediately terminating. Personally, I think if there's nothing you can actually do to handle this error then unwrap() or expect() are the correct choices. There's no point writing more code just to bubble an error that you can't handle.
- galangalalgol 7y agoIf the filesystem you are trying to open a log file on fails, only your logging thread needs to give up, not your whole program. If there is anything you can still accomplish, don't panic, carry on. Tesla let the logging exception prevent charging of the vehicle.
- lmm 7y ago> If there is anything you can still accomplish, don't panic, carry on. This is often poor advice, because continuing in a corrupt state can often be worse (and harder to debug) than a clean panic. E.g. it's better to crash than to overwrite a save file with corrupt data, or transfer money to the wrong account, or show one user data belonging to another user.
- galangalalgol 7y agoIf we are talking microservices I agree, take down the service. I have written a lot of safety critical stuff, and the advice for a given address space (virtual or physical) has been throw on inputs that are out of range or otherwise bad. This makes you fail fast and notice stuff, but i makes the conglomerate of address spaces as a whole be horribly unreliable, all coming down all the time. As we have bevome less monolithic with little persistent data that is shared between concerns, it has become possible to kill and restart only the offending thread, service, hardware etc. The result is a degraded functionality with an alert that it has become so. Tesla shouldn't have just stopped, they should have alerte the user that maintenance was necessary and given up on logging those errors to the now failed flash.
- devit 7y agoIt should be used when you can prove that the error never happens, although unfortunately the compiler can't check that.
- burntsushi 7y ago> `unwrap()` is "handling" errors by crashing the program, so it shouldn't really be used This isn't the right advice to give, and is only going to confuse beginners. Panicking via unwrap/expect is perfectly fine, whether in a library or an application, only when the panic represents a bug. For example, accessing an element of a Vec with an incorrect index because of a logic error. If a panic occurs, then it should reflect a bug that is intended to be fixed. Stated differently, it should be impossible for an end user to cause a Rust application to panic. If they can, then it's a bug. This advice permits use of unwrap/expect anywhere, so long as its occurrence is never expected.
- jayflux 7y agoI’m pretty sure clippy “encourages” you to use .get() for Vectors returning an Option<> for you to deal with so accessing a bad index is avoidable in idiomatic rust. I’m not sure that was the best example but I agree with your overall point.
- GrayShade 7y agoIt doesn't: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=850feb616235ea3cfc2f841629937eee https://play.rust-lang.org/?version=stable&mode=debug&editio... (click Tools / Clippy).
- burntsushi 7y agoIt does not. I know because I advocated against it. :-) https://github.com/rust-lang/rust-clippy/issues/1300 https://github.com/rust-lang/rust-clippy/issues/1300 Also, Clippy's defaults are developed to be quite opinionated. It makes many suggestions that I (and many others) disagree with. So Clippy's defaults should not be used as a bludgeon for what's idiomatic and correct.
- steveklabnik 7y ago(I also do not use clippy very aggressively due to disagreeing with a lot of lints)