Convert all assertions to verify

[gavin-norman-sociomantic]

As discussed many time in the past, the different behaviour of assertions in D1/D2 is problematic. To quote some previous discussion:

The main problem with the proposed "design by contract" model is the assumption that once the program is sufficiently tested in debug mode it is safe to remove assertions and contracts without compromising correctness (for better performance).

However, in the case of a server application working primarily with external data, it is very hard to get to the point of such confidence, especially if any error can have a potentially very costly impact on business. Even full test coverage can be misleading. This has resulted in some application never using the -release flag out of fear of possible uncaught programmer errors.

After some discussion, it was agreed that we need a more practical approach to using contracts, even if it will violate the intended DbC ideology. Most known issues seem to be associated with in contracts, because those do not notify the programmer when violation of some internal assumption occurs but rather at some later point when the violation is about to cause application corruption and must be protected against at all costs.

At the same time, the actual performance penalty of calling such safety checks is often very small, being as simple as checking for null pointers or integer boundaries. Any possible performance gain is simply not worth the added risk of removing the check (one case where it can make a difference, though, is in very small functions where extra operations can make the difference between inlining or not).

Indeed, we are now at the stage of running important server apps in D2 builds and need to address this issue (the change of assertion behaviour) before progressing.

[leandro-lucarella-sociomantic]

Related to #213, I wonder if we should finally introduce this programming error before doing this conversion.

[mathias-lang-sociomantic]

How do we deal with bounds checking ?

[nemanja-boric-sociomantic]

It was discussed to move with the ocean compiled as a library with -release.

[mathias-lang-sociomantic]

It means we have to compile the whole project with -release or am I missing something ?

[mihails-strasuns-sociomantic]

How do we deal with bounds checking ?

I don't see way out of it other than protect all places where it may happen with preceding enforce/verify of some sort.

It means we have to compile the whole project with -release or am I missing something ?

I think it should be the goal eventually but of course that requires similar adjustments of other libs and thus will take plenty of time. Idea of providing copy-paste makd snippet to link ocean as -release built static library is one way to make such slow transition less painful.

[leandro-lucarella-sociomantic]

I don't see way out of it other than protect all places where it may happen with preceding enforce/verify of some sort.

This really sucks and I think is not practical. OK, we can minimize then the Error problem, but we can't rely on getting rid of it completely, which will still be somehow problematic.

[mihails-strasuns-sociomantic]

I actually don't think it is that bad. We rarely try to index arrays with some data that is not directly derived from its .length without previously doing some sanity checking on it - and that sounds like a reasonable approach overall. Code like this one https://github.com/sociomantic-tsunami/ocean/blob/v2.x.x/src/ocean/util/serialize/contiguous/Deserializer.d#L314 seems common.

[leandro-lucarella-sociomantic]

But the whole point of this is for times when someone forgets to do the sanity checks :)
I see your point about not being the end of the world though, but I think it is still risky to have cases where the program can explode in your face when it is avoidable.

[mihails-strasuns-sociomantic]

I agree with that but to me it seems much more likely to be caught at (unit)testing stage than failed contract and thus much further in the realm of acceptable.

[gavin-norman-sociomantic]

PR updating the error handling guidelines: sociomantic-tsunami/sociomantic#10

[mihails-strasuns-sociomantic]

Also related - #96

[gavin-norman-sociomantic]

Breakdown of assert occurrences by top-level package:

• core: 472
• io: 586
• math: 900
• meta: 191
• net: 202
• stdc: 14
• sys: 54
• task: 32
• text: 1339
• time: 601
• util: 1187

It might be a good idea to strip out unused code before doing this. (Or deprecate and not bother assessing deprecated code for assertions.)

[gavin-norman-sociomantic]

Some easy cases exist in old unittests that were never converted to use test instead of assert.

[gavin-norman-sociomantic]

Some easy cases exist in old unittests that were never converted to use test instead of assert.

It might be possible to auto-convert these using d1to2fix. @mihails-strasuns-sociomantic what do you think?

[mihails-strasuns-sociomantic]

Shouldn't be too hard. I'll have a look.

[gavin-norman-sociomantic]

Excellent. Let's try to do that first, then we can assess what remains.

[gavin-norman-sociomantic]

After merging #241:

• core: 168
• io: 340
• math: 322
• meta: 29 (already done)
• net: 67
• stdc: 2
• sys: 37
• task: 38 (already done)
• text: 312
• time: 29
• util: 562

[gavin-norman-sociomantic]

(Adjusted numbers above, filtering out static assert.)

[gavin-norman-sociomantic]

Looks like splitting it six ways might work:

• io
• math
• text
• util part 1
• util part 2
• The rest

[mihails-strasuns-sociomantic]

I'd like to take core :)

[gavin-norman-sociomantic]

core is included in "the rest" ;)

[mathias-lang-sociomantic]

I volunteer as a tribute for text :)

[gavin-norman-sociomantic]

Thanks!

So we have:

• @mathias-lang-sociomantic text. Verification of ocean.text #276
• Me util.container. Convert assert -> verify in util.container #253
• @david-eckardt-sociomantic math Convert assert to verify/test in math #259
• @mihails-strasuns-sociomantic core, net, sys, time. Switch in contracts to verify in core #255, Verification of ocean.sys #270, Verify ocean.net #265 (time doesn't contain anything worth changing)
• @nemanja-boric-sociomantic io. Replace assertions with verifies in ocean.io.* #251
• @nemanja-boric-sociomantic util excluding util.container. Convert asserts to verify in ocean.util.[^container] #264

[gavin-norman-sociomantic]

The easiest way to split util would be util.container and everything else. container has 387 / 562 asserts.

I'm happy to take container.

[mihails-strasuns-sociomantic]

This finished for initial conversion, right?

[mathias-lang-sociomantic]

I haven't done my share yet 😊

[mihails-strasuns-sociomantic]

@mathias-lang-sociomantic do you think you can get to it in next few days? Otherwise I am going to take it over as I want to release 3.5.0 this Friday :)

[mathias-lang-sociomantic]

I definitely won't be able to start before Thursday at best (Brighton), so maybe it's better if you take over, sorry :(

[mihails-strasuns-sociomantic]

No problem!

[mihails-strasuns-sociomantic]

ocean.text merged - #276

That should be it for the first stage.