A better assert() [was: Re: std.unittests [updated] for review]

[Jonathan M Davis]

On Saturday 05 February 2011 02:43:56 spir wrote:
> On 02/05/2011 08:29 AM, Jonathan M Davis wrote:
> > On Friday 04 February 2011 13:29:38 bearophile wrote:
> >> Jonathan M Davis:
> >>> assert(0) has the advantage of being a normal assertion in non-release
> >>> mode.
> >> 
> >> What is this useful for? To me this looks like a significant
> >> disadvantage. If I want a HALT (to tell the compiler a point can't be
> >> reached, etc) I want it in every kind of compilation of the program.
> >> 
> >>> It also makes it clear that that code path should _never_ be reached.
> >> 
> >> The replacement for assert(0) is meant to be more clear in its purpose
> >> compared to assert(0). It may be named thisCantHappen(), or
> >> assertZero(), etc.
> > 
> > assert(0) will actually give a stack trace with a file and line number.
> > It will also give you a message if you include one with it. HALT just
> > kills the program. I _much_ prefer that assert(0) be a normal assert in
> > no-release mode. Leaving it in as a HALT has the advantage that the
> > program will just die if it reaches that point in release mode rather
> > than trying to continue with the assert gone, but I very much want a
> > normal assert in non-release mode. It's much more useful.
> > 
> >>> The real question though is whether you can convince Walter (which I
> >>> doubt, but I don't know).
> >> 
> >> This topic was already discussed, and I think the result of the
> >> discussion was that this change of assert(false) is not worth it. But
> >> if asserts gets inproved for other purposes, then this is a chance to
> >> work on improving assert(0) too.
> >> 
> >>> Still, making such a change _would_ contradict TDPL, which is supposed
> >>> to be a major no-no at this point.<
> >> 
> >> I like TDPL, I respect Andrei and you, I agree that TDPL is a kind of
> >> reference for D2, but please stop using TDPL as a The Bible in many of
> >> your posts. Not even Andrei himself looks so religiously attached as you
> >> to the contents of TDPL :-) A little flexibility is acceptable.
> > 
> > I believe that Walter and Andrei have made it fairly clear that if we do
> > anything that contradicts TDPL, it needs to have a very good reason to be
> > done. TDPL is _supposed_ to have been the final word. Unfortunately, the
> > implementation is behind, so _it_ is possible that we're going to have
> > to make changes which contradict it. However, if we do, those changes
> > have to be needed or at least really merit the cost of contradicting
> > TDPL. Something as small as changing assert(0) is unlikely to do that.
> > 
> >> struct Foo {
> >> 
> >>      int x;
> >>      invariant() { assert(x == 1); }
> >> 
> >> }
> >> void main() {
> >> 
> >>      Foo f;
> >>      assert(f);
> >> 
> >> }
> >> 
> >> 
> >> DMD 2.051:
> >> test.d(7): Error: expression f of type Foo does not have a boolean value
> > 
> > Actually, the more I think about it, the less I see
> > assert(class_instance) to be a problem. Normally it would check that the
> > reference was non-null. Assuming that the feature isn't buggy, it'll
> > still do that, but it'll check the invariant in addition. And since the
> > invariant is always supposed to be true, that shouldn't be a problem.
> > 
> > I really don't think that assert needs to be fundamentally changed with
> > regards to assert(0) or assert(class_instance).
> > 
> > - Jonathan M Davis
> 
> All right, I guess I get your point. I still think that checking a class's
> invariant should be explicit. Assert(whatever) means for me "check whatever
> is not (equivalent to) false", not "check whatever is not (equivalent to)
> false and whatever's invariant condition, if any, is fulfilled".
> Note that an object's invariant is *not* part of its regular truth value
> (1st assertion in unittest below indeed passes). Thus, it is clearly
> incorrect that assert(x) checks its invariant implicitely:
>      assert ( x );
>      assert ( cast(bool)x == true ) ;
> should always have the same outcome.
> Thus, the idiom to check an object's invariant should be different, at
> least in a visible detail.
> 
> A side-issue is the following code:
> 
> class C {
>      int i;
>      invariant () { assert(i == 1); }
> }
> unittest {
>      auto c = new C();
>      assert ( cast(bool)c == true ) ;
>      assert ( c ) ;
> }
> 
> throws a plain "Assertion error" with no additional comment. No mention of
> invariant checking, thus no hint to debug. I guess the problem would be
> more acceptable if asserts in invariants had at least slightly different
> error message forms, if only "Invariant assertion error".
> (Yes, one can customize error messages, sure; but defaults should be as
> helpful as possible.)

The AssertError should give the file and line number which should then tell you 
exactly which assert failed. Also, if stack traces work (which they do on Linux, 
but I don't think that they do on Windows yet), then you get a stack trace. It 
might be nice if the AssertError also said that it was in an invariant, but I 
don't think that it much matters where the assert was when an assert fails - be 
in a function, or in an invariant, or in a in block, or wherever. What matters 
is knowing which assert failed so that you can deal with it. The file and line 
number give you that. The fact that an assert was in an invariant as opposed to 
anywhere else is kind of irrelevant as far as the error message goes.

- Jonathan M Davis