www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - assert vs. enforce in Phobos code

reply Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
Hello all,

This is a subject I think we've visited before, but I can't find the old 
discussion threads :-(

As some of you know I'm working on a successor to std.random -- it's been put
on 
hold the last 8 weeks or so by country/life/work changes, but now I'm back onto 
it.  One of the things I'm examining in the course of that is the error
checking.

For example, a call to uniform(a, b) sees the function ensure that the interval 
is appropriate via an enforce(a <= b).  The result is that passing the wrong 
values to uniform() will bring your program down.  There are many other 
comparable cases in the module.

Part of me feels this is absolutely right and that such errors should be 
strictly objected to in this way.  On the other hand, this seems a wrong 
approach given that these kinds of functions are surely going to be called deep 
within the program -- we're not verifying user input here, it would be a
logical 
error in the program to pass wrong bounds to uniform() and so on.

My inclination is to clean this stuff up, to generally replace enforce's with 
asserts, to provide in- and out-contracts where appropriate, and so on.

Thoughts, remarks, ... ?

Thanks & best wishes,

      -- Joe
Feb 23 2014
next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Sunday, 23 February 2014 at 09:55:29 UTC, Joseph Rushton 
Wakeling wrote:
 Hello all,

 This is a subject I think we've visited before, but I can't 
 find the old discussion threads :-(

 As some of you know I'm working on a successor to std.random -- 
 it's been put on hold the last 8 weeks or so by 
 country/life/work changes, but now I'm back onto it.  One of 
 the things I'm examining in the course of that is the error 
 checking.

 For example, a call to uniform(a, b) sees the function ensure 
 that the interval is appropriate via an enforce(a <= b).  The 
 result is that passing the wrong values to uniform() will bring 
 your program down.  There are many other comparable cases in 
 the module.

 Part of me feels this is absolutely right and that such errors 
 should be strictly objected to in this way.  On the other hand, 
 this seems a wrong approach given that these kinds of functions 
 are surely going to be called deep within the program -- we're 
 not verifying user input here, it would be a logical error in 
 the program to pass wrong bounds to uniform() and so on.

 My inclination is to clean this stuff up, to generally replace 
 enforce's with asserts, to provide in- and out-contracts where 
 appropriate, and so on.

 Thoughts, remarks, ... ?

 Thanks & best wishes,

      -- Joe

As a rule of thumb, "enforce" is not necessarily for things "user-input" but for things "outside the programmer's control" eg: "things that can legitimately fail", Notably, IO, threads, databases etc... If you see any phobos code that validates the range of its inputs via an enforce, please knock yourself out and assert it/contract it.
Feb 23 2014
prev sibling next sibling parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 23/02/2014 17:10, monarch_dodra wrote:
 As a rule of thumb, "enforce" is not necessarily for things "user-input" but
for
 things "outside the programmer's control" eg: "things that can legitimately
 fail", Notably, IO, threads, databases etc...

It seems to me that "enforce" is used extensively in Phobos for other purposes than that, and quite often seems to be used for obligatory runtime checks.
 If you see any phobos code that validates the range of its inputs via an
 enforce, please knock yourself out and assert it/contract it.

Well, these are some examples where it seems to me both understandable that someone wanted to really, really make sure the values were correct, and at the same time to fall outside the scope of "things that can legitimately fail": https://github.com/D-Programming-Language/phobos/blob/master/std/random.d#L359-L360 https://github.com/D-Programming-Language/phobos/blob/master/std/random.d#L1253-L1255 https://github.com/D-Programming-Language/phobos/blob/master/std/random.d#L1334-L1335 https://github.com/D-Programming-Language/phobos/blob/master/std/random.d#L1345-L1347 https://github.com/D-Programming-Language/phobos/blob/master/std/random.d#L1357-L1359 https://github.com/D-Programming-Language/phobos/blob/master/std/random.d#L1629 https://github.com/D-Programming-Language/phobos/blob/master/std/random.d#L1703 https://github.com/D-Programming-Language/phobos/blob/master/std/random.d#L2061-L2070 I referred to checking user input because the only rationale I can see for these statements being enforce is if the values being checked were being treated as user input and therefore something that could "legitimately fail". Or have I missed something about when "enforce" is appropriate? ;-) Assuming I'm right about these enforce statements being inappropriate, I'll make a patch.
Feb 23 2014
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
On Sunday, 23 February 2014 at 17:51:34 UTC, Joseph Rushton 
Wakeling wrote:
 Assuming I'm right about these enforce statements being 
 inappropriate, I'll make a patch.

I believe you are correct, but the larger issue is that Phobos doesn't come with a debug build. So while they probably should be contracts, they will not be verified since the contracts will be compiled out.
Feb 23 2014
prev sibling next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Sunday, 23 February 2014 at 20:00:48 UTC, Jesse Phillips wrote:
 On Sunday, 23 February 2014 at 17:51:34 UTC, Joseph Rushton 
 Wakeling wrote:
 Assuming I'm right about these enforce statements being 
 inappropriate, I'll make a patch.

I believe you are correct, but the larger issue is that Phobos doesn't come with a debug build. So while they probably should be contracts, they will not be verified since the contracts will be compiled out.

It's "less" of an issue with phobos, which massivelly templated. Only very few things are non-template (AFAIK). The bigger issue is things such as druntime, which is basically 99% pre-compiled. That, and there's also the broader issue of the conditionality of contracts should be decided by the "calling" library.
Feb 23 2014
prev sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Sunday, February 23, 2014 16:10:02 monarch_dodra wrote:
 As a rule of thumb, "enforce" is not necessarily for things
 "user-input" but for things "outside the programmer's control"
 eg: "things that can legitimately fail", Notably, IO, threads,
 databases etc...
 
 If you see any phobos code that validates the range of its inputs
 via an enforce, please knock yourself out and assert it/contract
 it.

assert is used on input to a function when you consider it a programming bug for it to be given input that does not fulfill that condition. Exceptions are used when it's considered reasonable for the invalid input to be passed to the function. Where exactly to draw that line depends on what the function does and how it's likely to be used. A function that's likely to be used on data that was input into the program is more likely to use exceptions, whereas on whose parameters would not have originated from I/O at all are much more likely to use assertions. Unfortunately, knowing when to use one or the other is a bit of an art and definitely subjective. However, in general, I would have thought that std.random would use assertions, given that its initialization and use does not normally involve operating on anything that originated from the user even indirectly, and as such, it's reasonable to require that the programmer give it valid input. - Jonathan M Davis
Feb 25 2014