8 ms·
It seems people have a blind spot for unwrap, perhaps because it's so often used in example code. In production code an unwrap or expect should be reviewed exac
by wrs 11mo ago
It seems people have a blind spot for unwrap, perhaps because it's so often used in example code. In production code an unwrap or expect should be reviewed exactly like a panic.
It's not necessarily invalid to use unwrap in production code if you would just call panic anyway. But just like every unsafe block needs a SAFETY comment, every unwrap in production code needs an INFALLIBILITY comment. clippy::unwrap_used can enforce this.
- dist1ll 11mo ago> every unwrap in production code needs an INFALLIBILITY comment. clippy::unwrap_used can enforce this. How about indexing into a slice/map/vec? Should every `foo[i]` have an infallibility comment? Because they're essentially `get(i).unwrap()`.
- danielheath 11mo agoI mean... yeah, in general. That's what iterators are for.
- tux3 11mo agoUsually you'd want to write almost all your slice or other container iterations with iterators, in a functional style. For the 5% of cases that are too complex for standard iterators? I never bother justifying why my indexes are correct, but I don't see why not. You very rarely need SAFETY comments in Rust because almost all the code you write is safe in the first place. The language also gives you the tool to avoid manual iteration (not just for safety, but because it lets the compiler eliminate bounds checks), so it would actually be quite viable to write these comments, since you only need them when you're doing something unusual.
- dist1ll 11mo agoFor iteration, yes. But there's other cases, like any time you have to deal with lots of linked data structures. If you need high performance, chances are that you'll have to use an index+arena strategy. They're also common in mathematical codebases.
- wrs 11mo agoI didn't restate the context from the code we're discussing: it must not panic. If you don't care if the code panics, then go ahead and unwrap/expect/index, because that conforms to your chosen error handling scheme. This is fine for lots of things like CLI tools or isolated subprocesses, and makes review a lot easier. So: first, identify code that cannot be allowed to panic. Within that code, yes, in the rare case that you use [i], you need to at least try to justify why you think it'll be in bounds. But it would be better not to. There are a couple of attempts at getting the compiler to prove that code can't panic (e.g., the no-panic crate).
- kibwen 11mo agoIndexing is comparatively rare given the existence of iterators, IMO. If your goal is to avoid any potential for panicking, I think you'd have a harder time with arithmetic overflow.
- echelon 11mo agoCargo needs to grow a label for crates that provably do not panic. (Neverminding allocations and things outside our control flow.) I want to ban crates that panic from my dependency chain. The language could really use an extra set of static guarantees around this. I would opt in.
- phire 11mo agoI think I'd prefer a compile-time guarantee. Something that allows me to tag annotate a function (or my whole crate) as "no panic", and get a compile error if the function or anything it calls has a reachable panic. This will allow it to work with many unmodified crates, as long as constant propagation can prove that any panics are unreachable. This approach will also allow crates to provide panicking and non panicking versions of their API (which many already do).
- aw1621107 11mo agoI think the most common solution at the moment is dtolnay's no_panic [0]. That has a bunch of caveats, though, and the ergonomics leave something to be desired, so a first-party solution would probably be preferable. [0]: https://github.com/dtolnay/no-panic https://github.com/dtolnay/no-panic
- 10000truths 11mo agoYes? Funnily enough, I don't often use indexed access in Rust. Either I'm looping over elements of a data structure (in which case I use iterators), or I'm using an untrusted index value (in which case I explicitly handle the error case). In the rare case where I'm using an index value that I can guarantee is never invalid (e.g. graph traversal where the indices are never exposed outside the scope of the traversal), then I create a safe wrapper around the unsafe access and document the invariant.
- dist1ll 11mo agoIf that's the case then hats off. What you're describing is definitely not what I've seen in practice. In fact, I don't think I've ever seen a crate or production codebase that documents infallibility of every single slice access. Even security-critical cryptography crates that passed audits don't do that. Personally, I found it quite hard to avoid indexing for graph-heavy code, so I'm always on the lookout for interesting ways to enforce access safety. If you have some code to share that would be very interesting.
- hansvm 11mo ago> graph-heavy code Could you share some more details, maybe one fully concrete scenario? There are lots of techniques, but there's no one-size-fits-all solution.
- dist1ll 11mo agoSure, these days I'm mostly working on a few compilers. Let's say I want to make a fixed-size SSA IR. Each instruction has an opcode and two operands (which are essentially pointers to other instructions). The IR is populated in one phase, and then lowered in the next. During lowering I run a few peephole and code motion optimizations on the IR, and then do regalloc + asm codegen. During that pass the IR is mutated and indices are invalidated/updated. The important thing is that this phase is extremely performance-critical.
- 11mo ago
- dehrmann 11mo agoIt's the same blind spot people have to Java's checked exceptions. People commonly resort to Pokemon exception handling and either blindly ignoring or rethrowing as a runtime exception. When Rust got popular, I was a bit confused by people talking about how great Result it's essentially a checked exception without a stack trace.
- Terr_ 11mo ago"Checked Exceptions Are Actually Good" gang, rise up! :p I think adoption would have played out very different if there had only been some more syntactic-sugar. For example, an easy syntax for saying: "In this method, any (checked) DeepException e that bubbles up should immediately be replaced by a new (checked) MylayerException(e) that contains the original one as a cause. We might still get lazy programmers making systems where every damn thing goes into a generic MylayerException, but that mess would still be way easier to fix later than a hundred scattered RuntimeExceptions.
- bigstrat2003 11mo agoI'm with you! Checked exceptions are actually good and the hate for them is super short sighted. The exact same criticisms levied at checked exceptions apply to static typing in general, but people acknowledge the great value static types have for preventing errors at compile time. Checked exceptions have that same value, but are dunked on for some reason.
- Terr_ 11mo agoYeah, in both cases it's a layering situation, where it's the duty of your code to decide what layers of abstraction need to be be bridged, and to execute on that decision. Translating/wrapping exception-types from deeper functions is the same as translating/wrapping return-types the same places. I think it comes down to a psychological or use-case issue: People hate thinking about errors and handling them, because it's that hard stuff that always consumes more time than we'd like to think. Not just digitally, but in physical machines too. It's also easier to put off "for later."
- speed_spread 11mo agoPet peeve: unwrap() should be deprecated and renamed or_panic(). More consistent with the rest of stdlib methods and appropriately scarier.
- echelon 11mo agoA lot of stuff should be done about the awful unwrap family of methods. A few ideas: - It should not compile in production Rust code - It should only be usable within unsafe blocks - It should require explicit "safe" annotation from the engineer. Though this is subject to drift and become erroneous. - It should be possible to ban the use of unsafe in dependencies and transitive dependencies within Cargo.
- kibwen 11mo agoThe `unsafe` keyword means something specific in Rust, and panicking isn't unsafe by Rust's definition. Sometimes avoiding partial functions just isn't feasible, and an unwrap (or whatever you want to call the method) is a way of providing a (runtime-checked) proof to the compiler that the function is actually total.
- echelon 11mo agoPanics should be explicit, not implicit. unwrap() should effectively work as a Result<> where the user must manually invoke a panic in the failure branch. Make special syntax if a match and panic is too much boilerplate. This is like an implicit null pointer exception that cannot be statically guarded against. I want a way to statically block any crates doing this from my dependency chain.
- bigstrat2003 11mo agounwrap is explicit.
- speedgoose 11mo ago
- _zagj 11mo ago> It seems people have a blind spot for unwrap Not unlike people having a blind spot for Rust in general, no?
- littlestymaar 11mo ago> In production code an unwrap or expect should be reviewed exactly like a panic. An unwrap should never make it to production IMHO. It's fine while prototyping, but once the project gets closer to production it's necessary to just grep `uncheck` in your code and replace those that can happen with a proper error management and replace those that cannot happen with `expect`, with a clear justification of why they cannot happen unless there's a bug somewhere else.
- wrs 11mo agoI would say, sure, if you feel the same way about panic calls making to production. In other words, review all of them the same way. Because writing unwrap/expect is exactly the same as writing “if error, panic”.
- littlestymaar 11mo agoI don't understand your point: panic! is akin to expect: you think about it consciously, use it explicitly and you write down a panic message explaining its rational. unwrap isn't like that.
- wrs 11mo agoIt should be. If you aren’t treating it exactly the same as panic and expect, that’s what I’m calling the “blind spot”. And why should you have to make up a message every time when the backtrace is going to tell you what was wrong?
- littlestymaar 11mo ago> And why should you have to make up a message every time when the backtrace is going to tell you what was wrong? The message isn't really here to be displayed during a crash (since the crash should never happen in the first place), it's here to communicate the invariant in the code, to the developer reading and modifying it later on.
- bombela 11mo agoThis thread warms my heart. Rust has set a new baseline that many and myself now take for granted. We are now discussing what can be done to improve code correctness beyond memory and thread safety. I am excited for what is to come.
- StopDisinfo910 11mo agoAlternatively you can look at actually innovative programming languages to peek at the next 20 years of innovation. I am not sure that watching the trendy forefront successfully reach the 1990s and discuss how unwrapping Option is potentially dangerous really warm my heart. I can’t wait for the complete meltdown when they discover effect systems in 2040. To be more serious, this kind of incident is yet another reminder that software development remains miles away from proper engineering and even key providers like Cloudfare utterly fail at proper risk management. Celebrating because there is now one popular language using static analysis for memory safety feels to me like being happy we now teach people to swim before a transatlantic boat crossing while we refuse to actually install life boats. To me the situation has barely changed. The industry has been refusing to put in place strong reliability practices for decades, keeps significantly under investing in tools mitigating errors outside of a few fields where safety was already taken seriously before software was a thing and keeps hiding behind the excuse that we need to move fast and safety is too complex and costly while regulation remains extremely lenient. I mean this Cloudfare outage probably cost millions of dollars of damage in aggregate between lost revenue and lost productivity. How much of that will they actually have to pay?
- JuniperMesos 11mo agoLet's try to make effect systems happen quicker than that. > I mean this Cloudfare outage probably cost millions of dollars of damage in aggregate between lost revenue and lost productivity. How much of that will they actually have to pay? Probably nothing, because most paying customers of cloudflare are probably signing away their rights to sue Cloudflare for damages by being down for a while when they purchase Cloudflare's services (maybe some customers have SLAs with monetary values attached, I dunno). I honestly have a hard time suggesting that those customers are individually wrong to do so - Cloudflare isn't down that often, and whatever amount it cost any individual customer by being down today might be more than offset by the DDOS protection they're buying. Anyway if you want Cloudflare regulated to prevent this, name the specific regulations you want to see. Should it be illegal under US law to use `unwrap` in Rust code? Should it be illegal for any single internet services company to have more than X number of customers? A lot of the internet also breaks when AWS goes down because many people like to use AWS, so maybe they should be included in this regulatory framework too.
- brabel 11mo agoYes, I always thought it was wrong to use unwrap in examples. I know, people want to keep examples simple, but it trains developers to use unwrap() as they see that everywhere. Yes, there are places where it's ok as that blog post explains so well: https://burntsushi.net/unwrap/ https://burntsushi.net/unwrap/ But most devs IMHO don't have the time to make the call correctly most of the time... so it's just better to do something better, like handle the error and try to recover, or if impossible, at least do `expect("damn it, how did this happen")`.
- inferiorhuman 11mo agoDunno, I think the alternatives have their own pretty significant downsides. All would require front loading more in-depth understanding of error handling and some would just be quite a bit more verbose. IMO making unwrap a clippy lint (or perhaps a warning) would be a decent start. Or maybe renaming unwrap.
- jandrewrogers 11mo agoThis strikes me as a culture issue more than one of language. A tenet of systems code is that every possible error must be handled explicitly and exhaustively close to the point of occurrence. It doesn’t matter if it is Rust, C, etc. Knowing how to write systems code is unrelated to knowing a systems language. Rust is a systems language but most people coming into Rust have no systems code experience and are “holding it wrong”. It has been a recurring theme I’ve seen with Rust development in a systems context. C is pretty broken as a language but one of the things going for it is that it has a strong systems code culture surrounding it that remembers e.g. why we do all of this extra error handling work. Rust really needs systems code practice to be more strongly visible in the culture around the language.
- empath75 11mo agoUnwrap _is_ explicitly handling an error at the point of occurrence. You have explicitly decided to panic, which is sometimes a valid choice. I use it (on startup only) when server configs are missing or invalid or in CLI tools when the options aren't valid. Crashing a pod on startup before it goes Ready is a valid pattern in k8s and generally won't cause an outage because the previous pod will continue working.
- quotemstr 11mo ago> people have a blind spot for unwrap It's not about whether you should ban unwrap() in production. You shouldn't. Some errors are logic bugs beyond which a program can't reasonably continue. The problem is that the language makes it too easy for junior developers (and AI!) to ignore non-logic-bug problems with unwrap(). Programmers early in their careers will do practically anything to avoid having to think about errors and they get angry when you tell them about it.
- mamcx 11mo agoThe problem here is that for a service "fail fast" is mostly the best default.