www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Re: Against enforce()

reply Kagamin <spam here.lot> writes:
Walter Bright Wrote:


 1. Asserts and contracts are for detecting program BUGS. They are not for 
 validating user input, checking for disk full, file not found errors, etc.
 
 2. Enforce is for validating user input, checking for disk full, file not
found 
 errors, etc. Enforce is NOT for use in contracts or checking for program bugs.
 
 
 Any use of enforce in Phobos that is checking for program bugs is itself a bug 
 and should be entered into bugzilla for fixing.



So this is a bug? This is a contract, not a validation of user input.

struct Iota(N, S) if ((isIntegral!N || isPointer!N) && isIntegral!S)
 {
    private N current, pastLast;
    private S step;
    this(N current, N pastLast, S step)
    {
        enforce((current <= pastLast && step > 0) ||
                (current >= pastLast && step < 0));
        this.current = current;
        this.step = step;
Mar 18 2011
next sibling parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Fri, 18 Mar 2011 04:14:12 -0400, Kagamin <spam here.lot> wrote:


 Walter Bright Wrote:


 1. Asserts and contracts are for detecting program BUGS. They are not  
 for
 validating user input, checking for disk full, file not found errors,  
 etc.

 2. Enforce is for validating user input, checking for disk full, file  
 not found
 errors, etc. Enforce is NOT for use in contracts or checking for  
 program bugs.


 Any use of enforce in Phobos that is checking for program bugs is  
 itself a bug
 and should be entered into bugzilla for fixing.



 So this is a bug? This is a contract, not a validation of user input.

 struct Iota(N, S) if ((isIntegral!N || isPointer!N) && isIntegral!S)
  {
     private N current, pastLast;
     private S step;
     this(N current, N pastLast, S step)
     {
         enforce((current <= pastLast && step > 0) ||
                 (current >= pastLast && step < 0));
         this.current = current;
         this.step = step;



This is a good example of why it's difficult to decide what "user input"  
is.  One could consider that the 'user' in this case is the developer  
using the library, but I don't think that's the right choice.

I'd say it's a bug, this is clearly a contract, since the data being  
passed into the ctor can easily not be user input (i.e. it's most likely  
two literals that will never depend on a user).  If it is user input, the  
caller of the ctor should enforce the user input before passing it to iota.

-Steve
Mar 18 2011
next sibling parent reply spir <denis.spir gmail.com> writes:
On 03/18/2011 01:37 PM, Steven Schveighoffer wrote:

 This is a good example of why it's difficult to decide what "user input" is.
 One could consider that the 'user' in this case is the developer using the
 library, but I don't think that's the right choice.

 I'd say it's a bug, this is clearly a contract, since the data being passed
 into the ctor can easily not be user input (i.e. it's most likely two literals
 that will never depend on a user).  If it is user input, the caller of the ctor
 should enforce the user input before passing it to iota.



This is indeed a difficult topic. I'm a bit bluffed when reading people 
confidently asserting apparently clear positions about the use of enforce vs 
assert vs contracts and such, or whether such checks should or not stay or not 
in various distribution builds (mainly -release).
I can see at least 5 cases, and am far to be sure what the proper tool is in 
every case, and in which builds it should stay. In each case, there is 
potential "wrong" input; but note the variety of cases does seems orthogonal 
(lol) to what kind of harm it may cause:

* colleague: my code is called by code from the same app (same dev team); 
typically, wrong input logically "cannot" happen
* friend: my code is called by code designed to cooperate with it; there is a 
kind of moral contract
In both cases, wrong input reveals a bug; but in the first case, it's my own 
(team's) bug. I guess, but am not sure, these cases are good candidates for 
asserts (or contracts?), excluded from release build.

* lib call: my code is a typical lib; thus, I have zero control on caller.
I would let the check in release mode, thus use enforce. Or, use assert if it 
remains when the *caller* is compiled in debug mode. There is something unclear 
here, I guess. Maybe there are two sub-cases:
	~ the caller logically should be able to prove its args correct
	~ or not
In the first case, checks should dispappear in release mode. But there is 
always a risk. So, what to choose if my func really requires correct input?
By the way, I don't understand the diff between enforce and alwaysAssert; I 
thought enforce was precisely an alwaysAssert.

* user input: cope with it.
It's the only clear case for me.

Denis
-- 
_________________
vita es estrany
spir.wikidot.com
Mar 18 2011
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/18/11 11:07 AM, Steven Schveighoffer wrote:

 On Fri, 18 Mar 2011 11:35:22 -0400, spir <denis.spir gmail.com> wrote:


 On 03/18/2011 01:37 PM, Steven Schveighoffer wrote:

 This is a good example of why it's difficult to decide what "user
 input" is.
 One could consider that the 'user' in this case is the developer
 using the
 library, but I don't think that's the right choice.

 I'd say it's a bug, this is clearly a contract, since the data being
 passed
 into the ctor can easily not be user input (i.e. it's most likely two
 literals
 that will never depend on a user). If it is user input, the caller of
 the ctor
 should enforce the user input before passing it to iota.



 This is indeed a difficult topic. I'm a bit bluffed when reading
 people confidently asserting apparently clear positions about the use
 of enforce vs assert vs contracts and such, or whether such checks
 should or not stay or not in various distribution builds (mainly
 -release).
 I can see at least 5 cases, and am far to be sure what the proper tool
 is in every case, and in which builds it should stay. In each case,
 there is potential "wrong" input; but note the variety of cases does
 seems orthogonal (lol) to what kind of harm it may cause:

 * colleague: my code is called by code from the same app (same dev
 team); typically, wrong input logically "cannot" happen
 * friend: my code is called by code designed to cooperate with it;
 there is a kind of moral contract
 In both cases, wrong input reveals a bug; but in the first case, it's
 my own (team's) bug. I guess, but am not sure, these cases are good
 candidates for asserts (or contracts?), excluded from release build.

 * lib call: my code is a typical lib; thus, I have zero control on
 caller.
 I would let the check in release mode, thus use enforce. Or, use
 assert if it remains when the *caller* is compiled in debug mode.
 There is something unclear here, I guess. Maybe there are two sub-cases:
 ~ the caller logically should be able to prove its args correct
 ~ or not



 See, this is where I feel we have issues. The clear problem with
 *always* checking is the iota example. One may use iota like this:

 foreach(i; iota(0, 5))

 Why should checks in iota remain for iota to prove that 0 is less than
 5? It always will be less than 5, and the check is not necessary.




This is the kind of job that the compiler could and should do. Whether 
it's assert and enforce, an inlining pass followed by value range 
propagation should simply eliminate the unnecessary tests.


Andrei
Mar 18 2011
prev sibling next sibling parent Kagamin <spam here.lot> writes:
Steven Schveighoffer Wrote:


 This is a good example of why it's difficult to decide what "user input"  
 is.  One could consider that the 'user' in this case is the developer  
 using the library, but I don't think that's the right choice.
 
 I'd say it's a bug, this is clearly a contract, since the data being  
 passed into the ctor can easily not be user input (i.e. it's most likely  
 two literals that will never depend on a user).  If it is user input, the  
 caller of the ctor should enforce the user input before passing it to iota.



You can't validate all user input, so external data ends up spead across your
entire application. So I don't understand obsession with -release switch,
because contracts most of the time do validate user input. If we think about
-release switch as a HP-hack for exotic code, there will be no ideological
difference between assert and enforce.
Mar 18 2011
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Fri, 18 Mar 2011 11:35:22 -0400, spir <denis.spir gmail.com> wrote:


 On 03/18/2011 01:37 PM, Steven Schveighoffer wrote:

 This is a good example of why it's difficult to decide what "user  
 input" is.
 One could consider that the 'user' in this case is the developer using  
 the
 library, but I don't think that's the right choice.

 I'd say it's a bug, this is clearly a contract, since the data being  
 passed
 into the ctor can easily not be user input (i.e. it's most likely two  
 literals
 that will never depend on a user).  If it is user input, the caller of  
 the ctor
 should enforce the user input before passing it to iota.



 This is indeed a difficult topic. I'm a bit bluffed when reading people  
 confidently asserting apparently clear positions about the use of  
 enforce vs assert vs contracts and such, or whether such checks should  
 or not stay or not in various distribution builds (mainly -release).
 I can see at least 5 cases, and am far to be sure what the proper tool  
 is in every case, and in which builds it should stay. In each case,  
 there is potential "wrong" input; but note the variety of cases does  
 seems orthogonal (lol) to what kind of harm it may cause:

 * colleague: my code is called by code from the same app (same dev  
 team); typically, wrong input logically "cannot" happen
 * friend: my code is called by code designed to cooperate with it; there  
 is a kind of moral contract
 In both cases, wrong input reveals a bug; but in the first case, it's my  
 own (team's) bug. I guess, but am not sure, these cases are good  
 candidates for asserts (or contracts?), excluded from release build.

 * lib call: my code is a typical lib; thus, I have zero control on  
 caller.
 I would let the check in release mode, thus use enforce. Or, use assert  
 if it remains when the *caller* is compiled in debug mode. There is  
 something unclear here, I guess. Maybe there are two sub-cases:
 	~ the caller logically should be able to prove its args correct
 	~ or not



See, this is where I feel we have issues.  The clear problem with *always*  
checking is the iota example.  One may use iota like this:

foreach(i; iota(0, 5))

Why should checks in iota remain for iota to prove that 0 is less than 5?   
It always will be less than 5, and the check is not necessary.

checks should only be in place during release when the input to the  
function cannot be proven at compile time.  When it can be proven, then  
the checks should go away.

The problem I see is it's iota's responsibility to do those checks, but it  
has no idea where the data comes from.

What I would suggest is to check at the point the argument data is  
created, not at the point where it's used.  So for instance, if you get  
the parameters for iota from an input file, then you need to check those  
arguments before passing to iota.

This is a difficult problem to solve, because one party knows whether the  
arguments need to be checked, and the other party knows how to check the  
arguments.  I don't know if there is a clean way to do this.

My thoughts are that phobos should only use enforce where it can prove the  
arguments are runtime-generated, and rely on asserts otherwise.  The  
obvious pitfall is that one could pass runtime-generated data to a phobos  
function which uses asserts, and the program could crash on an otherwise  
recoverable error because the user of phobos did not validate the input  
first.  I think the risk here is less important than the reduction in  
performance that occurs when enforce is used instead.


 By the way, I don't understand the diff between enforce and  
 alwaysAssert; I thought enforce was precisely an alwaysAssert.



Assert throws an unrecoverable error, and its indication is that there is  
a problem in the code.  An enforce throws a recoverable exception, and  
indicates there is a problem in the runtime input.

-Steve
Mar 18 2011
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Fri, 18 Mar 2011 12:31:23 -0400, Andrei Alexandrescu  
<SeeWebsiteForEmail erdani.org> wrote:


 On 3/18/11 11:07 AM, Steven Schveighoffer wrote:

 On Fri, 18 Mar 2011 11:35:22 -0400, spir <denis.spir gmail.com> wrote:


 On 03/18/2011 01:37 PM, Steven Schveighoffer wrote:

 This is a good example of why it's difficult to decide what "user
 input" is.
 One could consider that the 'user' in this case is the developer
 using the
 library, but I don't think that's the right choice.

 I'd say it's a bug, this is clearly a contract, since the data being
 passed
 into the ctor can easily not be user input (i.e. it's most likely two
 literals
 that will never depend on a user). If it is user input, the caller of
 the ctor
 should enforce the user input before passing it to iota.



 This is indeed a difficult topic. I'm a bit bluffed when reading
 people confidently asserting apparently clear positions about the use
 of enforce vs assert vs contracts and such, or whether such checks
 should or not stay or not in various distribution builds (mainly
 -release).
 I can see at least 5 cases, and am far to be sure what the proper tool
 is in every case, and in which builds it should stay. In each case,
 there is potential "wrong" input; but note the variety of cases does
 seems orthogonal (lol) to what kind of harm it may cause:

 * colleague: my code is called by code from the same app (same dev
 team); typically, wrong input logically "cannot" happen
 * friend: my code is called by code designed to cooperate with it;
 there is a kind of moral contract
 In both cases, wrong input reveals a bug; but in the first case, it's
 my own (team's) bug. I guess, but am not sure, these cases are good
 candidates for asserts (or contracts?), excluded from release build.

 * lib call: my code is a typical lib; thus, I have zero control on
 caller.
 I would let the check in release mode, thus use enforce. Or, use
 assert if it remains when the *caller* is compiled in debug mode.
 There is something unclear here, I guess. Maybe there are two  
 sub-cases:
 ~ the caller logically should be able to prove its args correct
 ~ or not



 See, this is where I feel we have issues. The clear problem with
 *always* checking is the iota example. One may use iota like this:

 foreach(i; iota(0, 5))

 Why should checks in iota remain for iota to prove that 0 is less than
 5? It always will be less than 5, and the check is not necessary.




 This is the kind of job that the compiler could and should do. Whether  
 it's assert and enforce, an inlining pass followed by value range  
 propagation should simply eliminate the unnecessary tests.



In this simple case yes, but inlining is not forceable, so you should not  
rely on it for optimizing out asserts or enforce.  Inlining also only goes  
so deep, it doesn't make sense to inline a whole program, so at some  
point, you are going to use a function call, even though it can be proven  
the data is within range.

 From what I can see, we have 3 cases:

1. those where we can prove beyond a doubt that whether a value is valid  
is runtime dependent.  Those cases should obviously use enforce.
2. Those where we can prove beyond a doubt that whether a value is valid  
does not depend on runtime data.  Those cases should obviously use assert.
3. You can't prove beyond a doubt where those values come from.

It's case 3 that is the troublesome one, not because it's hard to prove  
whether it's case 3 or not, but because the code that knows what the data  
could be (the caller) is separate from the code which knows whether its  
valid (the callee).

It's also case 3 which is the most common.  The most common case is a  
library function which wants to validate its input.

So with the tools we have at hand today, which one should be applied to  
case 3?  My preference is to use assert because then the caller has  
control over whether the data is checked or not.  If you use enforce, the  
caller has no way of saying "no really, I checked that value already!".   
Note also that phobos functions may be double checking data ALREADY if  
they make calls to each other.

-Steve
Mar 18 2011
prev sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Friday, March 18, 2011 09:08:38 Kagamin wrote:

 Steven Schveighoffer Wrote:

 This is a good example of why it's difficult to decide what "user input"
 is.  One could consider that the 'user' in this case is the developer
 using the library, but I don't think that's the right choice.
 
 I'd say it's a bug, this is clearly a contract, since the data being
 passed into the ctor can easily not be user input (i.e. it's most likely
 two literals that will never depend on a user).  If it is user input, the
 caller of the ctor should enforce the user input before passing it to
 iota.



 You can't validate all user input, so external data ends up spead across
 your entire application. So I don't understand obsession with -release
 switch, because contracts most of the time do validate user input. If we
 think about -release switch as a HP-hack for exotic code, there will be no
 ideological difference between assert and enforce.



The idea is that if an assertion fails, there is a bug in your program. You 
_cannot_ rely on an assertion being run, because it could be compiled out. It
is 
_only_ for verifying that your code is correct.

Exceptions, on the other hand, are used for handling errors. If something which 
results in an error but which is _not_ a logic bug in your program, then it 
should be handled by an exception. User input would be a classic example of 
this. If you get bad user input, that's not the program's fault. It can verify 
that the user input is valid and then assume that it's valid after that (at 
which point, using assert would make sense, because it's supposed to be 
guaranteed that the values are valid, since they were validated). But in 
handling the user input, it would throw on error, not assert.

enforce is merely a shorthand way of testing and throw exceptions on failure.
It 
makes exception handling look like assertions. But it is an _entirely_
different 
thing.

As has been point out, the problem is in cases where it's not clear whether you 
should treat input as user input (and therefore needs to _always_ be checked
and 
have exceptions thrown on error) or whether you should treat input as being
from 
your program and guaranteed to be valid (at which point you use assert to check 
that that guarantee actually holds).

Assertions are _not_ for error handling. They _kill_ your program on failure 
(since they throw an Assert_Error_, not an exception). Exceptions (and
therefore 
enforce) are for error handling. There is a _clear_ and _distinct_ difference 
between the two. The confusion is not between assert and enforce. The confusion 
is when you have a situation where whether assert or enforce is appropriate 
depends on how the function is being used.

- Jonathan M Davis
Mar 18 2011
prev sibling next sibling parent spir <denis.spir gmail.com> writes:
On 03/18/2011 05:07 PM, Steven Schveighoffer wrote:

 On Fri, 18 Mar 2011 11:35:22 -0400, spir <denis.spir gmail.com> wrote:


 On 03/18/2011 01:37 PM, Steven Schveighoffer wrote:

 This is a good example of why it's difficult to decide what "user input" is.
 One could consider that the 'user' in this case is the developer using the
 library, but I don't think that's the right choice.

 I'd say it's a bug, this is clearly a contract, since the data being passed
 into the ctor can easily not be user input (i.e. it's most likely two literals
 that will never depend on a user). If it is user input, the caller of the ctor
 should enforce the user input before passing it to iota.



 This is indeed a difficult topic. I'm a bit bluffed when reading people
 confidently asserting apparently clear positions about the use of enforce vs
 assert vs contracts and such, or whether such checks should or not stay or
 not in various distribution builds (mainly -release).
 I can see at least 5 cases, and am far to be sure what the proper tool is in
 every case, and in which builds it should stay. In each case, there is
 potential "wrong" input; but note the variety of cases does seems orthogonal
 (lol) to what kind of harm it may cause:

 * colleague: my code is called by code from the same app (same dev team);
 typically, wrong input logically "cannot" happen
 * friend: my code is called by code designed to cooperate with it; there is a
 kind of moral contract
 In both cases, wrong input reveals a bug; but in the first case, it's my own
 (team's) bug. I guess, but am not sure, these cases are good candidates for
 asserts (or contracts?), excluded from release build.

 * lib call: my code is a typical lib; thus, I have zero control on caller.
 I would let the check in release mode, thus use enforce. Or, use assert if it
 remains when the *caller* is compiled in debug mode. There is something
 unclear here, I guess. Maybe there are two sub-cases:
 ~ the caller logically should be able to prove its args correct
 ~ or not



 See, this is where I feel we have issues. The clear problem with *always*
 checking is the iota example. One may use iota like this:

 foreach(i; iota(0, 5))

 Why should checks in iota remain for iota to prove that 0 is less than 5? It
 always will be less than 5, and the check is not necessary.

 checks should only be in place during release when the input to the function
 cannot be proven at compile time. When it can be proven, then the checks should
 go away.

 The problem I see is it's iota's responsibility to do those checks, but it has
 no idea where the data comes from.

 What I would suggest is to check at the point the argument data is created, not
 at the point where it's used. So for instance, if you get the parameters for
 iota from an input file, then you need to check those arguments before passing
 to iota.

 This is a difficult problem to solve, because one party knows whether the
 arguments need to be checked, and the other party knows how to check the
 arguments. I don't know if there is a clean way to do this.

 My thoughts are that phobos should only use enforce where it can prove the
 arguments are runtime-generated, and rely on asserts otherwise. The obvious
 pitfall is that one could pass runtime-generated data to a phobos function
 which uses asserts, and the program could crash on an otherwise recoverable
 error because the user of phobos did not validate the input first. I think the
 risk here is less important than the reduction in performance that occurs when
 enforce is used instead.



Yes, I think you are correctly surrounding the issue.

But my choice would rather be safety as default. Would you really let the func 
called by a/b "non-check" b!=0? Thus, I would consider allowing the caller 
stating "don't check this call", not the opposite.
Another issue is this creates one more complication in the language; and one 
orthogonal to the whole set of func-calls; with syntax needed, I guess.

Another path is decision via compiler analysis: if arguments can be proved 
constant (I guess /this/ point can be made), then check is removed for release, 
automatically; else it cannot be removed at all.

[Thanks for the precision about enforce vs alwaysAssert.]

Denis
-- 
_________________
vita es estrany
spir.wikidot.com
Mar 18 2011
prev sibling parent Gerrit Wichert <gwichert yahoo.com> writes:
I would say it is a bug in the contract.
The signature is not normalized and the user gets a chance to provide
conflicting parameters. I think that it would be best to deduce the 
direction
from the order of the start and end parameters. Then the stepsize can be 
made
absolute.

Am 18.03.2011 09:14, schrieb Kagamin:

 So this is a bug? This is a contract, not a validation of user input.
 struct Iota(N, S) if ((isIntegral!N || isPointer!N)&&  isIntegral!S)
   {
      private N current, pastLast;
      private S step;
      this(N current, N pastLast, S step)
      {
          enforce((current<= pastLast&&  step>  0) ||
                  (current>= pastLast&&  step<  0));
          this.current = current;
          this.step = step;
Mar 18 2011