I don't think it's a good default. It introduces a whole bunch of friction. There's tons of example code out there using unwrap() that will trigger lints whenever someone tries to use it, even when it's not necessary or desirable to remove the calls to unwrap(). (See my blog for when unwrap is totally acceptable as a means of error handling. It isn't just limited to tests.)
6 Likes
It is incorrect to say that unwrap was the problem that Cloudflare had. It was a cascade of problems that culminated on a Rust service failing an assertion and causing it to get stuck in a restart loop. The appropriate layer for handling this was with monitoring and getting paged when the panic occurred. Either they didn't have this for this system, or it got drowned by the rest of the noise in their dashboards during the outage. If the service had continued by either returning the an error or logging and using a default value, I suspect the service would have gotten itself in a worse state and made it harder to debug what in the hell was happening while in the middle of the outage. On distributed web services you want simultaneously resiliency and for things that are wrong to be loudly so.
The conversation on whether using unwrap is reasonable is the conversation on whether assertions are reasonable.
Regarding renaming, as much as I want the language to be as easy to learn and understand as possible, I don't think the naming really masks the underlying behavior once it has been taught, and the strife caused by such a rename would be high. If there was agreement that the renaming should take place, then now is better than later (but I'm not convinced that even now that cost is worth it).
19 Likes
You've seen one case where a panic backfired terribly, but you haven't seen the other cases where loud panics helped catch bugs during development, testing, and internal dogfooding/canary releases. Panics (assertions and other "this should never happen" unwinds) can be very useful.
In this case the unwrap() was only a symptom of an already bad state causing an error that the service couldn't recover from. This would have been as much of an unrecoverable error if it was reported in any other way. The mechanisms needed to either prevent it or recover are much more nuanced than just whether it's an unwrap or Result.
Using Result instead isn't universally better. For example:
if let Ok(Some(cached)) = memcache.get() {
this is a reasonable pattern, because communication with the cache server is going to be inherently flaky, and treating it as a cache miss is a trivial way to recover.
…except when you have a cache client that is too old, and doesn't support a certain protocol feature, which makes it systematically fail on a subset of queries. You have no idea, because all users of it discard the Err, and Rust has no good way of enforcing that a certain Error variant must be handled (it boils down to undroppable types).
Mixing of critical "this should never happen" cases with Results that often carry noisy and trivial/expected errors (caused by bad user input, flaky I/O, timeouts, etc.) risks losing the important error in the noise. When a service sees some "missing required http header" trivial error thousands of times per second and a "the core config is all wrong" every 5 minutes, it will be easy to lose the important one in the noise.
27 Likes
I firmly believe we should rename std::io::Write::write() (but not unwrap()).
1 Like
There's an argument here that this should really be multiple methods. Just like todo!(), unimplemented!() and panic!() actually do the same thing, but are used for different purposes (the difference between first two being subtle), we could have multiple methods that do the same as .unwrap(), but are used in different situations. Like
-
a method to mean that right now I am unwrapping, but I intend to do something else later. This is useful for linting, to avoid accidentally shipping unfinished code
-
a method to say that I am unwrapping because
None/Errshould be actually impossible. This is similar to an assert. If this ever panics, we will be 100% sure this is a bug, so this is valuable for debugging. -
a method to say that this error can legitimately happen, but it's not intended to be treated - instead this should be viewed as a fatal error. This will put this unwrap in the same bucket as memory allocation failures - in some environment they may happen, but there isn't much we can do in most cases
If we cover all possibilities for unwrap in separate methods, people can use a more specific method from now on. We don't need to actually go through the churn of changing every single .unwrap() or even making it warn or error by default to reap the benefits
18 Likes
Not sure what you mean by older version? The Frontline written in PHP, from what I gather, was happy to continue robusting along, causing delays, and false positives.
In general, any large-scale rename needs to come with large upsides, given all the downsides. I do not see this being the case at all. I don't think there are any problems with unwrap today that a rename would fix.
As with any proposal, we must always start with the following question: "what is the problem that people are having with Rust today?"
4 Likes
Bikeshedding:
.todo().assert().expect()
I believe these are "obvious" and could easily be added on Option and Result (well, .expect() already exists).
2 Likes
To quote the post mortem:
Customers on our old proxy engine, known as FL, did not see errors, but bot scores were not generated correctly, resulting in all traffic receiving a bot score of zero. Customers that had rules deployed to block bots would have seen large numbers of false positives. Customers who were not using our bot score in their rules did not see any impact.
4 Likes
If we're going to invent new names with new semantics we need to not recycle any of the old names, so that people can easily tell whether code was written assuming the old or the new semantics. I like @dlight's three-way split though. How about
.err_todo()and.none_todo()- takes no arguments, means "I'll deal with the possibility of this being Err/None later".assert_ok("reason"),.assert_some("reason")- "Because of <reason>, it should be impossible for this to be Err/None"..ok_or_panic("message"),.some_or_panic("message")- "If this is Err/None, treat as a fatal error, with panic message <message>".
9 Likes
I think that .unwrap() is a fine name. Every Rust programmer knows it can panic, so that shouldn't be the issue. It's often used in places when the programmer thinks it is safe or when for prototyping. Just renaming it to something else won't change the usage.
What I instead suggest is, that .unwrap() should only be allowed in dev builds for prototyping. And when building release version, the compiler could reject .unwrap(). Even an .expect("") is a better choice and there should never be a reason to use .unwrap() instead.
2 Likes
Imo .expect("") is a bad sign and shouldn't be used, as it's basically an .unwrap() but with a worse error message (: vs. called `Result::unwrap()` on an `Err` value: followed by the error value for a result, for an option .expect("") just straight-up gives an empty message).
Personally, I use .unwrap() for "if this fails, then there's an error local to this function" vs .expect() with a detailed message if it can fail for anything not matching expectations (like function arguments being wrong, or disk/network reads not having the expected values, etc.) if I've decided to promote the problem to an immediate crash.
4 Likes
Oh, about .expect(""), I didn't mean to just use empty String. I just put empty string as a placeholder for the post. But I guess even if this was not intentional by me, it shows it could be misused as an .unwrap() later.
1 Like
I don't want Regex("...").expect("") and Duration::from_hours(24).checked_mul(7).expect("") and mutex.lock().expect("") scattered all throughout my code. Among many, many other examples.
5 Likes
What about, per @zackw's naming, Regex("...").assert_ok(), Duration::from_hours(24).checked_mul(7).assert_some(), mutex.lock().ok_or_panic()?
(Maybe it should be shorter, not longer than unwrap)
2 Likes
This is something where I think we might want different guidelines for const (compile time) vs runtime.
There is nothing wrong with unwrap or expect at compile time. You're compiling the code, if there's an error you're in the position to look at the code that errored.
To the extent that we want any guidelines or lints helping people avoid some uses of unwrap, those should distinguish between compile time and runtime.
(Separate from that, for the specific case of mutex, I think we should solve that by removing poison so that you don't need .lock().unwrap().)
2 Likes
Regex::new isn't const though. It may never be. I think I could come around to your perspective if const were more expressive, but it's a non-starter today IMO. There's just too many things that can't reasonably be const.
3 Likes
The err_todo() and .none_todo() cases are only slightly longer than .unwrap() - and (I think) that would be the most common use case where .unwrap() is preferred over .expect("[with_error_context]").
The one other thing I am curious about, is that the methods that have none or err in their naming schemes seem to follow [verb]_[object] (e.g..is_err(), .is_none(), .expect_err()).
Does .todo_err() and/or .todo_none() seem more consistent, or too clunky? And what about just .todo()?
There are lots of unwraps that are not these very intentional ones, though, and probably should be linted against. And lots of unwraps that are these very intentional ones. And some unwraps that are essentially always fine (regex/lock) that no lint should fire against. And they're all spelled about the same right now.
I would hope/expect than any lint intended to be enabled by default would be tested against some fairly high quality crates and found to be not too noisy. I expect there are some minority of crates for which any unwrap lint fires like crazy, and those would have to #![allow(mad_unwraps)]. Perhaps a survey of unwrap usage is in order to find out what is typical.
I wasn't trying to be consistent with any existing naming convention, because to me Result and Option are already a big mess of random method names, and anyway it's more important to make each new name make sense in the context of its typical usage than to make it seem consistent with other stuff near it in the docs.
Thus, for instance, I prefer mutex.lock().err_todo() to mutex.lock().todo() because the former tells readers who haven't memorized the manual what's still to do (error handling), without their having to go look at the docs, and the latter doesn't.
2 Likes