3 ms·

Yeah, I made this mistake in trust-dns. Went back and thought better on it, and replaced all unwraps with proper checks. After that no more random crashes, but

Yeah, I made this mistake in trust-dns. Went back and thought better on it, and replaced all unwraps with proper checks. After that no more random crashes, but you're correct that it's easy even in Rust to say, "I'm sure this is never None in this case", and be very wrong. The Rust linter (clippy) does perform a bunch of checks on unwrap usage now. For example, a common mistake before TryFrom was added to the stdlib was to implement From between two types and do an unwrap on conversion (think String -> enum variant), and clippy will now suggest TryFrom as a replacement since the unwrap is hiding a fallible case. The fact that folks use unwrap so much in tests and examples can also mislead people new to the language that this is a common practice.

tptacek 6y ago
I thought about that --- particularly for Firecracker, which has so much test code --- and no, it's in the actual code too. I think Rust does more than any other mainstream language to mitigate this problem. I'm just saying, it still exists in Rust; it's just called a panic on unwrap/expect, instead of "null pointer exception".
saagarjha 6y ago
Using unwraps is often the “proper check”. Trying to come up with error handling in cases where invariants are not met is usually done wrong.
- bluejekyll 6y ago
  That's not a check at that point, though, it's an assertion that the None case is invalid. I'm not saying you should never use unwrap, but I've been burned by it when treating it a little too nonchalantly. If you're implementing a lower level library, like a dns stub resolver, panicking will bring down the software using your library... which is definitely not something people are generally happy about. Error handling isn't always the right thing, sometimes just returning Option is the correct thing.
- ufmace 6y ago
  Yeah it may be valid, barring careful consideration of the exact situation. My canonical example is a web server. In the context of handling a web request, never unwrap, because a web server should never die no matter what kind of weird or crazy thing a client somewhere sends it. Check every error for everything in that context, always at the very least drop back to a code path that returns a 500 and keeps the server running. In the context of server init though, unwrap is good. If anything in the server init process goes wrong, that means I as the developer / deployment engineer did something wrong. The server should then blow up to let me know that something is very wrong and needs fixing. The server definitely should not try to rattle to life anyways and run in some sort of degraded state where it can't handle requests correctly.
  - richardwhiuk 6y ago
    Usually `.expect` is better, as you can explain the error.

tptacek 6y ago

I thought about that --- particularly for Firecracker, which has so much test code --- and no, it's in the actual code too. I think Rust does more than any other mainstream language to mitigate this problem. I'm just saying, it still exists in Rust; it's just called a panic on unwrap/expect, instead of "null pointer exception".

saagarjha 6y ago

Using unwraps is often the “proper check”. Trying to come up with error handling in cases where invariants are not met is usually done wrong.

bluejekyll 6y ago
That's not a check at that point, though, it's an assertion that the None case is invalid. I'm not saying you should never use unwrap, but I've been burned by it when treating it a little too nonchalantly. If you're implementing a lower level library, like a dns stub resolver, panicking will bring down the software using your library... which is definitely not something people are generally happy about. Error handling isn't always the right thing, sometimes just returning Option is the correct thing.
ufmace 6y ago
Yeah it may be valid, barring careful consideration of the exact situation. My canonical example is a web server. In the context of handling a web request, never unwrap, because a web server should never die no matter what kind of weird or crazy thing a client somewhere sends it. Check every error for everything in that context, always at the very least drop back to a code path that returns a 500 and keeps the server running. In the context of server init though, unwrap is good. If anything in the server init process goes wrong, that means I as the developer / deployment engineer did something wrong. The server should then blow up to let me know that something is very wrong and needs fixing. The server definitely should not try to rattle to life anyways and run in some sort of degraded state where it can't handle requests correctly.
- richardwhiuk 6y ago
  Usually `.expect` is better, as you can explain the error.