5 ms·

Putting code with side effects into an assert is asking for trouble. Compile with NDEBUG set and the effects mysteriously disappear! Anything beyond an equality

Putting code with side effects into an assert is asking for trouble. Compile with NDEBUG set and the effects mysteriously disappear! Anything beyond an equality expression or straight boolean should be avoided.

nyc_pizzadev 6mo ago
This is just a symptom of a bad assert() implementation, which funny enough is the standard. If you properly (void) it out, side effects are maintained. https://github.com/fiberfs/fiberfs/blob/7e79eaabbb180b0f1a7942b318e981293318a6da/src/utils/fbr_assert.h#L45 https://github.com/fiberfs/fiberfs/blob/7e79eaabbb180b0f1a79...
- omoikane 6mo ago
  assert() is meant to be compiled away if NDEBUG is defined, otherwise it shouldn't be called assert(). Given that assert() may be compiled away, it makes sense not to give it anything that has side effects. Abseil has the convention where instead of assert(), users call "CHECK" for checks that are guaranteed to happen at run time, or "DCHECK" for checks that will be compiled away when NDEBUG is defined. https://github.com/abseil/abseil-cpp/blob/0093ac6cac892086a6d7d09c55421a2a4c2cdb2e/absl/log/check.h#L21 https://github.com/abseil/abseil-cpp/blob/0093ac6cac892086a6... https://github.com/abseil/abseil-cpp/blob/0093ac6cac892086a6d7d09c55421a2a4c2cdb2e/absl/log/internal/check_impl.h#L39 https://github.com/abseil/abseil-cpp/blob/0093ac6cac892086a6...
  - nmilo 6mo ago
    If your assert compiles down to `if (condition) {}` in production then the compiler will optimize away the condition while keeping any side effects.
    - IshKebab 6mo ago
      Yeah which may not be what you want. E.g. `assert(expensive_to_compute() == 0)`. The correct way to solve this is with debug asserts (as in Rust, or how the parent described).
      - nyc_pizzadev 6mo ago
        Genuine question, does Rust know if `expensive_to_compute()` has side effects? There are no params, so could it be compiled out if the return value is ignored? Ex: `expensive_to_compute()` What about: `(void) expensive_to_compute()`?
        IshKebab 6mo ago
        No, Rust is the same as C++ in terms of tracking side effects. It doesn't matter that there are no parameters. It could manipulate globals or call other functions that have side effects (e.g. printing).
        functional_dev 6mo ago
        What about rust const fn()? I think it guarantees there are no side effects
        IshKebab 6mo ago
        I think you're right. Equivalent to C++'s constexpr.
        aw1621107 6mo ago
        No, in general Rust doesn't (and can't) know whether an arbitrary function has side effects. The compiler does arguably have a leg up since Rust code is typically all built from source, but there's still things like FFI that act as visibility barriers for the compiler.
      - nmilo 6mo ago
        Compilers are very good these days. If it has no side effects it will likely be compiled out.
jmalicki 6mo ago
Side effects are bad of course, but anything beyond a straight boolean or equality is bad? `assert(vector.size() < 3)` is ridiculous to you?
nealabq 6mo ago
I don't mean to be that guy, but for "functional" programmers a print statement has "side effects". But your meaning is clear. In an assert expression, don't call functions that might change the program/database state. Be as "const" as possible.
- toxik 6mo ago
  Not just for functional programmers. Prints and other I/O operations absolutely are side effects. That's not running counter to the point being made. Print in an assert and NDEBUG takes away that behavior.
  - nealabq 6mo ago
    You're right of course. I was thinking specifically of printing log/debug statements in the assert(..), but that usually only happens if the assert(..) fails and exits, and in that case the "no side effects" rule no longer matters.
usrnm 6mo ago
I once spent several days debugging that same mistake. Stuff worked perfectly in tests but broke misteriously in production builds. Couldn't stop laughing for a few minutes when I finally figured it out.
maccard 6mo ago
Indeed. bool is_even(int* valPtr) { assert(valPtr != nullptr); return *valPtr % 2; } Does not do what you think it does with nullptr. A major game engine [0] has a toggle to enable asserts in shipping builds, mostly for this reason [0] https://dev.epicgames.com/documentation/en-us/unreal-engine/asserts-in-unreal-engine https://dev.epicgames.com/documentation/en-us/unreal-engine/...
- dccsillag 6mo ago
  I'm sorry, but what exactly is the problem with the code? I've been staring at it for quite a while now and still don't see what is counterintuitive about it.
  - IshKebab 6mo ago
    There's nothing wrong with it. It does exactly what you think it does when passed null.
    - jmalicki 6mo ago
      A lot of compilers will optimize out a NULL pointer check because dereferencing a NULL pointer is UB. Because assert will not run the following code in the case of a NULL pointer, AFAIK this exact code is still defined behavior, but if for some reason some code dereferenced the NULL pointer before, it would be optimized out - there are some corner cases that aren't obvious on the surface. This kind of thing was always theoretically allowed, but really started to become insidious within the past 5-10 years. It's probably one of the more surprising UB things that bites people in the field. GCC has a flag "-fno-delete-null-pointer-checks" to specifically turn off this behavior. https://qinsb.blogspot.com/2018/03/ub-will-delete-your-null-checks.html https://qinsb.blogspot.com/2018/03/ub-will-delete-your-null-... This is an actual Linux kernel exploit caused by this behavior where the compiler optimized out code that checked for a NULL pointer and returned an error. https://lwn.net/Articles/342330/ https://lwn.net/Articles/342330/
      - IshKebab 6mo ago
samiv 6mo ago
That's why you define your own assert macro and keep in on unconditionally. Your programs will be better for it.
- jandrewrogers 6mo ago
  An assertion can be arbitrarily expensive to evaluate. This may be worth the cost in a debug build but not in a release build. If all of assertions are cheap, they likely are not checking nearly as much as they could or should.
  - samiv 6mo ago
    Possibly but I've never seen it in practice that some assert evaluation would be the first thing to optimize. Anyway should that happen then consider removing just that assert. That being said being slow or fast is kinda moot point if the program is not correct. So my advisor to leave always all asserts in. Offensive programming.
andrepd 6mo ago
Rust has assert and debug_assert, which are self-explanatory. But it also has an assert_unchecked, which is what other languages incl C++ call an "assume" (meaning "this condition not holding is undefined behaviour"), with the added bonus that debug builds assert that the condition is true.
- tialaramex 6mo ago
  Notably, like most things with "unchecked" in their name `core::hint::assert_unchecked` is unsafe, however it's also constant, that is, we can do this at compile time, it's just promising that this condition will turn out to be true and so you should use it only as an optimisation. Necessarily, in any language, you should not optimise until you have measured a performance problem. Do not write this because "I think it's faster". Either you measured, and you know it's crucial to your desired performance, or you didn't measure and you are wasting everybody's time. If you just scatter such hints in your code because "I think it's faster" and you're wrong about it being true the program has UB, if you're wrong about it being faster the program may be slower or just harder to maintain.
bluGill 6mo ago
Related our logging system has a debug which is not logged by default but can be turned on if a problem in an area is found (in addition to the normal error/info which is logged). I had the idea that if a test fails we should print all these debugs - easy enough to turn on but a number of tests failed because of side effects that didn't show up when off. i'm trying to think of how/if we can run tests with all logging off to find the error and info logs with side effects.
saagarjha 6mo ago
I actually feel like asserts ended up in the worst situation here. They let you do one line quick checks which get compiled out which makes them very tempting for those but also incredibly frustrating for more complex real checks you’d want to run in debug builds but not in release.

nyc_pizzadev 6mo ago

This is just a symptom of a bad assert() implementation, which funny enough is the standard. If you properly (void) it out, side effects are maintained. https://github.com/fiberfs/fiberfs/blob/7e79eaabbb180b0f1a7942b318e981293318a6da/src/utils/fbr_assert.h#L45 https://github.com/fiberfs/fiberfs/blob/7e79eaabbb180b0f1a79...

omoikane 6mo ago
assert() is meant to be compiled away if NDEBUG is defined, otherwise it shouldn't be called assert(). Given that assert() may be compiled away, it makes sense not to give it anything that has side effects. Abseil has the convention where instead of assert(), users call "CHECK" for checks that are guaranteed to happen at run time, or "DCHECK" for checks that will be compiled away when NDEBUG is defined. https://github.com/abseil/abseil-cpp/blob/0093ac6cac892086a6d7d09c55421a2a4c2cdb2e/absl/log/check.h#L21 https://github.com/abseil/abseil-cpp/blob/0093ac6cac892086a6... https://github.com/abseil/abseil-cpp/blob/0093ac6cac892086a6d7d09c55421a2a4c2cdb2e/absl/log/internal/check_impl.h#L39 https://github.com/abseil/abseil-cpp/blob/0093ac6cac892086a6...
- nmilo 6mo ago
  If your assert compiles down to `if (condition) {}` in production then the compiler will optimize away the condition while keeping any side effects.
  - IshKebab 6mo ago
    Yeah which may not be what you want. E.g. `assert(expensive_to_compute() == 0)`. The correct way to solve this is with debug asserts (as in Rust, or how the parent described).
    - nyc_pizzadev 6mo ago
      Genuine question, does Rust know if `expensive_to_compute()` has side effects? There are no params, so could it be compiled out if the return value is ignored? Ex: `expensive_to_compute()` What about: `(void) expensive_to_compute()`?
      - IshKebab 6mo ago
        No, Rust is the same as C++ in terms of tracking side effects. It doesn't matter that there are no parameters. It could manipulate globals or call other functions that have side effects (e.g. printing).
        functional_dev 6mo ago
        What about rust const fn()? I think it guarantees there are no side effects
        IshKebab 6mo ago
        I think you're right. Equivalent to C++'s constexpr.
      - aw1621107 6mo ago
        No, in general Rust doesn't (and can't) know whether an arbitrary function has side effects. The compiler does arguably have a leg up since Rust code is typically all built from source, but there's still things like FFI that act as visibility barriers for the compiler.
    - nmilo 6mo ago
      Compilers are very good these days. If it has no side effects it will likely be compiled out.

jmalicki 6mo ago

Side effects are bad of course, but anything beyond a straight boolean or equality is bad? `assert(vector.size() < 3)` is ridiculous to you?

nealabq 6mo ago

I don't mean to be that guy, but for "functional" programmers a print statement has "side effects". But your meaning is clear. In an assert expression, don't call functions that might change the program/database state. Be as "const" as possible.

toxik 6mo ago
Not just for functional programmers. Prints and other I/O operations absolutely are side effects. That's not running counter to the point being made. Print in an assert and NDEBUG takes away that behavior.
- nealabq 6mo ago
  You're right of course. I was thinking specifically of printing log/debug statements in the assert(..), but that usually only happens if the assert(..) fails and exits, and in that case the "no side effects" rule no longer matters.

usrnm 6mo ago

I once spent several days debugging that same mistake. Stuff worked perfectly in tests but broke misteriously in production builds. Couldn't stop laughing for a few minutes when I finally figured it out.

maccard 6mo ago

Indeed. bool is_even(int* valPtr) { assert(valPtr != nullptr); return *valPtr % 2; } Does not do what you think it does with nullptr. A major game engine [0] has a toggle to enable asserts in shipping builds, mostly for this reason [0] https://dev.epicgames.com/documentation/en-us/unreal-engine/asserts-in-unreal-engine https://dev.epicgames.com/documentation/en-us/unreal-engine/...

dccsillag 6mo ago
I'm sorry, but what exactly is the problem with the code? I've been staring at it for quite a while now and still don't see what is counterintuitive about it.
- IshKebab 6mo ago
  There's nothing wrong with it. It does exactly what you think it does when passed null.
  - jmalicki 6mo ago
    A lot of compilers will optimize out a NULL pointer check because dereferencing a NULL pointer is UB. Because assert will not run the following code in the case of a NULL pointer, AFAIK this exact code is still defined behavior, but if for some reason some code dereferenced the NULL pointer before, it would be optimized out - there are some corner cases that aren't obvious on the surface. This kind of thing was always theoretically allowed, but really started to become insidious within the past 5-10 years. It's probably one of the more surprising UB things that bites people in the field. GCC has a flag "-fno-delete-null-pointer-checks" to specifically turn off this behavior. https://qinsb.blogspot.com/2018/03/ub-will-delete-your-null-checks.html https://qinsb.blogspot.com/2018/03/ub-will-delete-your-null-... This is an actual Linux kernel exploit caused by this behavior where the compiler optimized out code that checked for a NULL pointer and returned an error. https://lwn.net/Articles/342330/ https://lwn.net/Articles/342330/
    - IshKebab 6mo ago

samiv 6mo ago

That's why you define your own assert macro and keep in on unconditionally. Your programs will be better for it.

jandrewrogers 6mo ago
An assertion can be arbitrarily expensive to evaluate. This may be worth the cost in a debug build but not in a release build. If all of assertions are cheap, they likely are not checking nearly as much as they could or should.
- samiv 6mo ago
  Possibly but I've never seen it in practice that some assert evaluation would be the first thing to optimize. Anyway should that happen then consider removing just that assert. That being said being slow or fast is kinda moot point if the program is not correct. So my advisor to leave always all asserts in. Offensive programming.

andrepd 6mo ago

Rust has assert and debug_assert, which are self-explanatory. But it also has an assert_unchecked, which is what other languages incl C++ call an "assume" (meaning "this condition not holding is undefined behaviour"), with the added bonus that debug builds assert that the condition is true.

tialaramex 6mo ago
Notably, like most things with "unchecked" in their name `core::hint::assert_unchecked` is unsafe, however it's also constant, that is, we can do this at compile time, it's just promising that this condition will turn out to be true and so you should use it only as an optimisation. Necessarily, in any language, you should not optimise until you have measured a performance problem. Do not write this because "I think it's faster". Either you measured, and you know it's crucial to your desired performance, or you didn't measure and you are wasting everybody's time. If you just scatter such hints in your code because "I think it's faster" and you're wrong about it being true the program has UB, if you're wrong about it being faster the program may be slower or just harder to maintain.

bluGill 6mo ago

Related our logging system has a debug which is not logged by default but can be turned on if a problem in an area is found (in addition to the normal error/info which is logged). I had the idea that if a test fails we should print all these debugs - easy enough to turn on but a number of tests failed because of side effects that didn't show up when off. i'm trying to think of how/if we can run tests with all logging off to find the error and info logs with side effects.

saagarjha 6mo ago

I actually feel like asserts ended up in the worst situation here. They let you do one line quick checks which get compiled out which makes them very tempting for those but also incredibly frustrating for more complex real checks you’d want to run in debug builds but not in release.