You Must Fix Your Asserts
21 points by gulbanana
21 points by gulbanana
I agree that just crashing (/ crashing a task, like Rust panics) on assert is often the best behaviour. I can’t agree that using asserts as optimization guides is necessarily better than just compiling them out though. this is for two reasons:
first, arbitrary asserts are often not great for optimization. a lot of conditions are not directly usable by the optimizer. unless you’re asserting something like “this branch will never be taken” directly, performance benefits from peppering random assumptions all over the code are probably not huge.
second, turning asserts into assumptions greatly amplifies the blast radius of a mistake. imagine you have a system that processes some data partitioned by some key: maybe the work is completely separated by project, or by user. let’s say we have an assert in the middle of a computation function that catches some state that should not have been possible. we did some profiling and decided that we can’t afford to keep it in release builds, because the performance impact is too large. if we just disable it, any possible damage to the data would be limited to one project / user, and might even be caught by some other check down the line. on the other hand, if we turn it into UB, the computation might jump into some random code, arbitrarily corrupt the memory, and potentially damage data for every project.
essentially, by choosing unsafe assertions as the default in release builds we’re prematurely optimizing random places in our code in exchange for reducing the chance of localizing the damage if something goes wrong in release.
I think Rust gets it right: it has assert!() that always panics, debug_assert!() that only panics in debug mode, and assert_unchecked() that panics in debug and turns into an optimization hint in release. you use the first by default, the second when the performance impact of panicking is too large, and the last when you actually have demonstrable gains from doing that.
My argument is not against disabling individual asserts, it's about turning them off wholesale as a generally recommended practice.
we did some profiling and decided that we can’t afford to keep it in release builds, because the performance impact is too large.
Not only this is perfectly reasonable, but also an assert that is computationally heavy to evaluate is almost certainly an assert that would not result in a performance improvement, going back to your previous point.
I have a few of those myself in Zine:
essentially, by choosing unsafe assertions as the default in release builds we’re prematurely optimizing random places in our code in exchange for reducing the chance of localizing the damage if something goes wrong in release.
There is no "default release mode" in Zig. You have to always pick what you want to do with regards to asserts and the two "wholesale" options you're given are crash or optimize, neither of which is more 'default' than the other.
my argument is that this makes much more sense as a per-assert choice rather than per-project choice. in particular, I’d like unsafe assumptions to be locally explicit, probably with a comment explaining why it is needed and linking to some performance measurements.
in particular, I’d like unsafe assumptions to be locally explicit, probably with a comment explaining why it is needed and linking to some performance measurements.
That would make sense for an application but, when you're writing library code, the optimization potential of your asserts is not fixed (nor is the blast radius of deviating from the spec) and will depend on how the library is used. In this case it shouldn't be up to the library author to decide what to do with asserts because what's optimal for one user might not be for another.
In Zig libraries can not only be built using whatever mode the application author wants, but then usage code can use @setRuntimeSafety (+ function call inlining) for more fine-grained control. Related to this, @setRuntimeSafety is about to be replaced with @optimizeFor which adds more control to what to optimize for (safety vs performance vs code size, like ReleaseSafe vs ReleaseFast vs ReleaseSmall).
To be clear, I think Rust's model is generally good, and certainly strictly superior to C/C++ assert macros.
that requires a library to be very carefully written to work with ReleaseFast, because (as matklad points out in a sibling thread) ReleaseFast makes a lot of operations very unsafe. I would consider “you can build application code in ReleaseFast, and library code in ReleaseSafe” to be one of the best arguments for ReleaseFast.
the optimization potential of your asserts is not fixed
while true in principle, optimization potential for an assumption is strongest in local code; the farther away you get from an assumption, the lesser the probability it would help. for a lot of assumptions, I would find them affecting optimizations for outer code to be very surprising.
I would consider “you can build application code in ReleaseFast, and library code in ReleaseSafe” to be one of the best arguments for ReleaseFast.
That's probably an interesting cultural difference between different ecosystems. In an ecosystem where you normally depend on a bunch of small third party libraries your statement makes perfect sense.
So far in the Zig ecosystem that has not been the case, and when you do have dependencies, it tends to be big things, like battle hardened C projects. In the blog post I mentioned media codecs and sqlite for my discord alternative, but the same is also true for my static site generator:
I do have a few dependencies, but most of them are my own code just split up into a separate package. What remains are mostly things like wuffs, cmark-gfm, and tree-sitter, which I would want to build with all optimizations on regardless.
That being said, I think your point will probably become more relevant for the Zig ecosystem going forward. I'll try to keep it in mind.
turning asserts into assumptions greatly amplifies the blast radius of a mistake.
Note that if you are concerned about blast radius of mistakes, you should use ReleaseSafe, rather than ReleaseFast.
I don’t think this is a binary. I might be willing to trade off some safety for some performance, the question is how much safety I give up and how much performance I get. in my view, “turning all asserts into assumptions” gets a bad score on this tradeoff, for reasons outlined above.
This is precisely why you should use ReleaseSafe: ReleaseFast expresses your preferences for 100% performance. a+b, a[b], @intCast(a), @ptrCast(a), a.? all come with unsafe assumptions, there's no any kind of granularity.
The granularity is achieved by compiling your code in ReleaseSafe, disabling specific costly assertions with, e.g., if (constants.verify) assert(costly()), and flipping individual blocks to @setRuntimeSafety(false).
ReleaseSafe also e.g. initializes all the undefined memory, which is costly and often unnecessary, as undefined is explicit and you can just code-style require it to have an explanation attached. I agree that ReleaseSafe comes closer to what feels like a good tradeoff, but the workarounds required to actually get there are somewhat noisy and I think having a wider set of assertions would express intent clearer.
I can get behind, in theory, wanting something like ReleaseSafe but with specific classes of safety checks disabled. But I need a bit more convincing about specific here! "everything's UB except asserts" doesn't feel like it, because asserts are just a small part of overall assumptions. The undefined example also feels like something you want safety-checked by default, cause in 99% cases you just "everything is an expression" initialize stuff, and the remaining 1% of 1% where that both doesn't work, and is perf sensitive, @setRuntimeSafety feels good.
The strongest case here would be perhaps for ReleaseSafe with defined integer overlow, a-la rust? Hard for me to judge --- in the context of accounting, not trapping on integer overflow is a quite a bit like using floats for money :D
I elaborated a bit in a sibling thread. as for asserts, I really think that having assert() and assume() be separate functions is useful.
I don’t have a lot of Zig experience, but at least in Rust when I initialize something to MaybeUninit::uninit() it is almost always because I am immediately passing it to some function that will fill it, like a read syscall, and I really don’t want to auto-insert initialization there. maybe there’re some Zig-specific cases where this auto-initialization would catch something.
Interesting point, are there other aspects of ReleaseSafe that you have issues with, past 0xaa undefined initialization?
I have not optimized enough Zig code to know how punishing it is to panic on overflow in every arithmetic operation, but this feels like a potential pain point. “accessing inactive member of a union” also looks suspicious, since it means every union is now a tagged union, which is costly both in memory and runtime terms.
Also interestingly, the only two relatively serious Ghostty CVEs published so far are about arbitrary command execution achieved without memory corruption, despite Ghostty being distributed in ReleaseFast.
I want to flag this as an extremely puzzling fact, which outright contradicts my understanding of how the world works.
Routinely disabling asserts in prod is suboptimal to both keeping asserts on and optimizing for performance, and I find it absurd that people would engage in this practice uncritically, while at the same time being extremely critical of ReleaseFast.
As one of the commenters linked here: this is a silly false dichotomy and a silly caricature about how I think about asserts. I wrote in another comment that I would prefer the switch to UB to be made on an assert-by-assert basis, and that my criticism of ReleaseFast is that it instead ties this to all asserts as well as all other safety checks (in a given scope).
To be clear I also agree with kristoff that disabling asserts just because you haven't fixed them and they're crashing is silly. I simply disagree that "crash or UB" are the only reasonable alternatives. The sibling comment by goldstein aligns more closely with my opinion there.
In the program analysis literature there is a duality between two forms of assertions/claims within a piece of code:
The distinction is very clear when thinking in terms of "blame" (a standard academic notion in the literature on contracts and gradual typing):
For example, at the level of a function, preconditions are "claims about the context", and postconditions are "claims about the code", but one can in fact include both in the middle of the code as well.
Some verification frameworks use assert for "claims about the code" and assume for "claims about the context". This is related to the way some testing frameworks, in particular random-testing framework, interpret them: mark the test as failed if an assertion fails, skip the test if an assumption fails.
BIND9 roughly follows a design by contract style using macros REQUIRE() to assert a precondition that must be satisfied by a function’s caller, ENSURE() to assert a postcondition that a function guarantees; plus INSIST() for intermediate checks, and INVARIANT() for loops or data structures. The documentation for a function should have “requires” and “ensures” notes corresponding to the pre- and post-conditions.
In Zig this is just not a thing because std.debug.assert is a normal function, which means that its arguments are evaluated before calling it no matter what the logic inside the function is.
The result is that you can put expressions with side-effects in your calls to assert without fear
Don't do this. This is bad practice. Also, don't use asserts for error checking (I don't think the author argues for this, TBF).
On the flipside it also means that if you have an assert that relies on performing complex computations, then those won’t necessarily be elided when building in unchecked modes, in which case you need to take care to guard the code with a comptime if
This is a good opportunity to get rid of some macro-induced PTSD and embrace simplicity
I hope the irony is not lost on the author. "Embrace the simplicity of having to consider the build mode of your program and sprinkling defensive comptime ifs."
Don't do this. This is bad practice.
Why?
Because, while written in the same language as the source program, assertions are part of the metatext, not the text of the program. They talk about the properties and relationships between the objects in your domain. You should be able to look at the assertions in isolation and they should tell a meaningful story, without having to refer to the program text. And vice versa.
From a practical standpoint, you identified the problem in your post: because, if you put program logic in assertions, you run the risk of breaking the program when you disable them. Hence my critique of comptime if to guard against it.
because, if you put program logic in assertions, you run the risk of breaking the program when you disable them.
This is, in my opinion, an incorrect negative response trained by exposure to poorly designed programming languages. Zig doesn't have macros. You can, 100% of the time, correctly determine if a piece of code will always run regardless of external conditions by only looking at that piece of code. There is no ambiguity in running assert(foo()); in Zig. The simple rules of guarantee this behaves exactly the same as writing const condition = foo(); assert(condition);. In fact, assert is fairly irrelevant in that example, it could be any function and the effects on if foo runs is unchanged. It is only exposure to rules that are not simple from other languages which would encourage someone to not think that, which needs to be untrained when learning Zig.
I don't find your classification of assert as meta-text to be a compelling argument. I'd call README or comment content meta-text, but trying to pull actual code into the definition results in too arbitrary of a distinction to be useful for creating such rules. Like, in the OP post, is the nullability of a pointer meta-text? It can be expressed via asserts or the type system, so why would one be meta-text?
Bonus: all control flow is Zig uses full words. Most notably, and and or. So in the statement assert(some_bool and foo());, it is obvious from using a keyword that foo is possibly conditionally executed, contingent on the value of some_bool. It doesn't matter if some_bool is comptime known or a runtime value; this simply says that foo will run if and only if some_bool is true. If you always want foo to run, you can swap the argument order or use a temporary.
I write some numerical code in C# and use a lot of asserts which are turned off in release. They're just too expensive to be done in all the tight loops, but in unit testing it is useful for them to blow up quickly e.g. the first time a routine sees a NaN input. These often aren't user-inputs that result in NaNs, but code bugs that result in NaNs because the optimizer wanders someplace it shouldn't (or whatever) and needs some better box constraints or something like that. Although it might be that user input needs some kind of sanitization, but that should happen at the very surface of the algorithm, not deep inside of it. It'd be nice to have a proof system, so that as the consequence of the user input sanitization you could prove the invariants deep within the algorithm rather than having asserts, but this is a side-project and nobody dies if it all blows up.
Funny how I’ve heard the same arguments about turning off asserts (and bounds checking) in production for “performance reasons” for almost 40 years now, during which time computers have become several orders of magnitude faster and software has become ever more embedded in everyone’s life!
But on a more productive note… Back in the day at Microsoft we had the usual assert, check, etc. but we also had a “reporting assert”, which I don’t often see elsewhere. It’s used when there’s a condition I don’t fully control, and I’m assuming it’s true but defensively handle the case where it’s false, and I want to know if it ever actually is false in real life (through logging/telemetry) so I can rethink my assumption if necessary. E.g., I’m assuming the user never puts more than 1000 things in this list so I used a quadratic algorithm, or I’m assuming network latency will be less than 200ms so I used a chatty protocol.