[phobos] enforce() vs. assert() for range primitives

[Andrei Alexandrescu]

On 08/18/2010 05:49 PM, David Simcha wrote:
> While I was debugging std.range and std.algorithm I noticed that there's
> tons of inconsistency about when enforce() vs. assert() is used for
> checking input to range primitives. I think this needs to be made
> consistent, and there are good arguments for both.

As mentioned, I've studied this exchange, thought a lot about it, and 
also discussed it with Walter.

Clearly right now Phobos is inconsistent. Walter and other used asserts 
and contracts and other means etc. My approach since I started has been 
to sprinkle enforce() everywhere and worry about it later, and that 
later has come.

One problem with that approach is that, as Lars mentioned, there are 3-4 
checks on a simple operation due to the use of higher-order ranges that 
wrap other ranges, like Stride!Chain!Whatever. This issue is not 
difficult to tackle: we can introduce in Phobos the notion that a range 
wrapping another range defers the checking to the wrapped range. That 
way the wrapper only uses assert() and essentially trusts that the other 
range protects its own integrity. Consider as an example Take's 
implementation of popFront:

     void popFront()
     {
         enforce(_maxAvailable > 0,
             "Attempting to popFront() past the end of a "
             ~ Take.stringof);
         original.popFront();
         --_maxAvailable;
     }

The enforce is redundant - we could decide that Take defers checking to 
original. So this code should be allowed:

     void popFront()
     {
         assert(_maxAvailable > 0, ...);
         original.popFront();
         --_maxAvailable;
     }

But (very careful) Take should still protect its _own_ integrity, 
assuming original also does. So the following code would be incorrect:

     void popFront()
     {
         assert(_maxAvailable > 0, ...);
         --_maxAvailable;
         original.popFront();
     }

If original.popFront throws, _maxAvailable has already been decremented 
so Take is in an inconsistent state. So rule #1 could be something like:

Rule #1: A higher-order entity in Phobos can generally assume that the 
lower-order entities it builds on protect their own integrity, and 
protect its own under that premise.

I am sure there would be fuzzy corners to this rule, but by and large it 
shold cut down on a lot of enforce calls. (We could and should replace 
them with asserts).

The second rule concerns safety. To my dismay, this program still 
compiles and runs "successfully" with -release:

import std.stdio;
void main(string[] args)
{
     writeln(args[10]);
}

It should throw, and it should only run with -noboundscheck enabled. 
That flag was defined by Walter under my pressure, and I think it's 
misnamed - it should really be -safe.

Safety is one matter I am extremely rigid about. There is a point where 
we must draw a line in the sand regarding checks, and safety is it. 
Since there is a simple and clear definition of it, we can't redefine it 
as we'd prefer. So I'd say:

Rule #2: All checks protecting memory safety must be done with enforce() 
or rely on built-in array indexing (which in turn obeys -noboundscheck).

Finally, we have several places where we call APIs such as the I/O code 
etc. For those I think it's fine to mandate use of enforce() for several 
reasons, a simple one being that such calls would not be slowed down 
considerably by the extra test. So:

Rule #3: All external API calls must be protected by enforce().

Of course there are many case-by-case judgments, but I think having such 
guidelines in mind should be helpful. Thoughts?


Andrei