> A comment "this CANNOT happen" has no value on itself.

I think it does have some value: it makes clear an assumption the programmer made. I always appreciate it when I encounter comments that clarify assumptions made.

▲

AdieuToLogic 2 hours ago | parent | next [-]

>> A comment "this CANNOT happen" has no value on itself.

> I think it does have some value: it makes clear an assumption the programmer made.

To me, a comment such as the above is about the only acceptable time to either throw an exception (in languages which support that construct) or otherwise terminate execution (such as exiting the process). If further understanding of the problem domain identifies what was thought impossible to be rare or unlikely instead, then introducing use of a disjoint union type capable of producing either an error or the expected result is in order.

Most of the time, "this CANNOT happen" falls into the category of "it happens, but rarely" and is best addressed with types and verified by the compiler.

▲

addaon 13 hours ago | parent | prev | next [-]

But if you spell that `assert(false)` instead of as a comment, the intent is equally clear, but the behavior when you're wrong is well-defined.

▲

JohnFen 13 hours ago | parent | next [-]

I agree that including that assert along with the comment is much better. But the comment alone is better than nothing, so isn't without value.

▲

eterm 13 hours ago | parent | prev | next [-]

Better yet, `assert(false, message)`, with the message what you would have written in the comment.

▲

andrewf an hour ago | parent | next [-]

Swap the parameters around for C++ and similar langs where `assert(a, b)` evaluates the same as `(void) a; assert(b)`.

▲

addaon 13 hours ago | parent | prev [-]

`assert(false)` is pronounced "this can never happen." It's reasonable to add a comment with /why/ this can never happen, but if that's all the comment would have said, a message adds no value.

	▲	eterm 12 hours ago \| parent \| next [-]
		Oh I agree, literally `assert(false, "This cannot happen")` is useless, but ensuring message is always there encourages something more like, `assert(false, "This implies the Foo is Barred, but we have the Qux to make sure it never is")`. Ensuring a message encourages people to state the assumptions that are violated, rather than just asserting that their assumptions (which?) don't hold.
	▲	breatheoften 8 hours ago \| parent \| prev [-]
		what language are we talking about? If it's cpp then the pronounciation depends on compiler flags (perhaps inferred from CMAKE_BUILD_TYPE)

▲

zffr 11 hours ago | parent | prev | next [-]

At least on iOS, asserts become no-ops on release builds

▲

josephg 10 hours ago | parent | next [-]

It really depends on the language you use. Personally I like the way rust does this:

- assert!() (always checked),

- debug_assert!() (only run in debug builds)

- unreachable!() (panics)

- unsafe unreachable_unchecked() (tells the compiler it can optimise assuming this is actually unreachable)

- if cfg!(debug_assertions) { … } (Turns into if(0){…} in release mode. There’s also a macro variant if you need debug code to be compiled out.)

This way you can decide on a case by case basis when your asserts are worth keeping in release mode.

And it’s worth noting, sometimes a well placed assert before the start of a loop can improve performance thanks to llvm.

▲

addaon 9 hours ago | parent [-]

> debug_assert!() (only run in debug builds)

debug_assert!() (and it's equivalent in other languages, like C's assert with NDEBUG) is cursed. It states that you believe something to be true, but will take no automatic action if it is false; so you must implement the fallback behavior if your assumption is false manually (even if that fallback is just fallthrough). But you can't /test/ that fallback behavior in debug builds, which means you now need to run your test suite(s) in both debug and release build versions. While this is arguably a good habit anyway (although not as good a habit as just not having separate debug and release builds), deliberately diverging behavior between the two, and having tests that only work on one or the other, is pretty awful.

▲

josephg 7 hours ago | parent [-]

I hear you, but sometimes this is what I want.

For example, I’m pretty sure some complex invariant holds. Checking it is expensive, and I don’t want to actually check the invariant every time this function runs in the final build. However, if that invariant were false, I’d certainly like to know that when I run my unit tests.

Using debug_assert is a way to do this. It also communicates to anyone reading the code what the invariants are.

If all I had was assert(), there’s a bunch of assertions I’d leave out of my code because they’re too expensive. debug_assert lets me put them in without paying the cost.

And yes, you should run unit tests in release mode too.

▲

addaon 4 hours ago | parent [-]

But how do you test the recovery path if the invariant is violated in production code? You literally can’t write a test for that code path…

▲

josephg 4 hours ago | parent [-]

There is no recovery. When an invariant is violated, the system is in a corrupted state. Usually the only sensible thing to do is crash.

If there's a known bug in a program, you can try and write recovery code to work around it. But its almost always better to just fix the bug. Small, simple, correct programs are better than large, complex, buggy programs.

▲

addaon 3 hours ago | parent [-]

> Usually the only sensible thing to do is crash.

Correct. But how are you testing that you successfully crash in this case, instead of corrupting on-disk data stores or propagating bad data? That needs a test.

▲

josephg an hour ago | parent [-]

> Correct. But how are you testing that you successfully crash

In a language like rust, failed assertions panic. And panics generally aren't "caught".

> instead of corrupting on-disk data stores

If your code interacts with the filesystem or the network, you never know when a network cable will be cut or power will go out anyway. You're always going to need testing for inconvenient crashes.

IMO, the best way to do this is by stubbing out the filesystem and then using randomised testing to verify that no matter what the program does, it can still successfully open any written (or partially written) data. Its not easy to write tests like that, but if you actually want a reliable system they're worth their weight in gold.

	▲	addaon an hour ago \| parent [-]
		> In a language like rust, failed assertions panic. And panics generally aren't "caught". This thread was discussing debug_assert, where the assertions are compiled out in release code.

▲

addaon 11 hours ago | parent | prev [-]

You can (and probably should) undef NDEBUG even for release builds.

▲

fwip 9 hours ago | parent | prev [-]

I think you might have missed that they threw an exception right under the comment.

▲

dllthomas 10 hours ago | parent | prev | next [-]

Importantly, specifying reasoning can have communicative value while falling very far short of formal verification. Personally, I also try to include a cross reference to the things that could allow "this" to happen were they to change.

▲

skydhash 13 hours ago | parent | prev [-]

Such comments rot so rapidly that they're an antipattern. Such assumptions are dangerous and I would point it out in a PR.

▲

LegionMammal978 13 hours ago | parent [-]

Do you not make such a tacit assumption every time you index into an array (which in almost all languages throws an exception on bounds failure)? You always have to make assumptions that things stay consistent from one statement to the next, at least locally. Unless you use formal verification, but hardly anyone has the time and resources for that.

▲

lmm 3 hours ago | parent | next [-]

> Do you not make such a tacit assumption every time you index into an array (which in almost all languages throws an exception on bounds failure)?

Yes, which is one reason why decent code generally avoids doing that.

▲

skydhash 12 hours ago | parent | prev [-]

If such an error happens, that would be a compiler bug. Why? Because I usually do checks against the length of the array or have it done as part of the standard functions like `map`. I don't write such assumptions unless I'm really sure about the statements, and even then I don't.

▲

tosapple 12 hours ago | parent | next [-]

How does one defend against cosmic rays?

Keep two copies or three like RAID?

Edit: ECC ram helps for sure, but what else?

▲

well_ackshually 7 hours ago | parent | next [-]

>How does one defend against cosmic rays?

Unless you are in the extremely small minority of people who would actually be affected by it (in which case your company would already have bought ECC ram and made you work with three isolated processes that need to agree to proceed): you don't. You eat shit, crash and restart.

	▲	tosapple 7 hours ago \| parent [-]
		Well, bitflip errors are more of a vulnerability for longer lived values. This could effect fukushima style robots or even medical equipment. ECC implemented outside of ram would save vs triplicate but it was just a question related to the-above idea of an array access being assumed as in+bounds. Thank you.

▲

JonChesterfield 6 hours ago | parent | prev [-]

You run equivalent or equal calculations simultaneously on N computers and take majority wins, aircraft control or distributed filesystem style.

▲

LegionMammal978 11 hours ago | parent | prev | next [-]

> or have it done as part of the standard functions like `map`.

Which are all well and good when they are applicable, which is not always 100% of the time.

> Because I usually do checks against the length of the array

And what do you have your code do if such "checks" fail? Throw an assertion error? Which is my whole point, I'm advocating in favor of sanity-check exceptions.

Or does calling them "checks" instead of "assumptions" magically make them less brittle from surrounding code changes?

▲

skydhash 10 hours ago | parent [-]

A comment have no semantic value to the code. Having code that check for stuff is different from writing comments as they are executed by the machine. Not read by other humans.

	▲	LegionMammal978 7 hours ago \| parent [-]
		Of course you should put down a real assertion when you have a condition that can be cheaply checked (or even an assert(false) when the language syntax dictates an unreachable path). I'm not trying to argue against that, and I don't think anyone else here is either. I was mainly responding to TFA, which states "How many times did you leave a comment on some branch of code stating 'this CANNOT happen' and thrown an exception" (emphasis mine), i.e., an assertion error alongside the comment. The author argues that you should use error values rather than exceptions. But for such sanity checks, there's typically no useful way to handle such an error value.

▲

awesome_dude 11 hours ago | parent | prev [-]

Do you really have code that's

if array.Len > 2 { X = Y[1] }

For every CRUD to that array?

That seems... not ideal

	▲	skydhash 10 hours ago \| parent [-]
		Yes. Unless there’s some statement earlier that verify that the array has 2 items. It’s quick to do, so why not do it?