www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - assert(obj) is an atrocity

reply =?ISO-8859-1?Q?Alex_R=F8nne_Petersen?= <xtzgzorex gmail.com> writes:
Hi,

As the title suggests, I'm going to be rather blunt about this. 
assert(obj) testing the invariant *without* doing a null check is insane 
for the following reasons:

1) It is not what a user expects. It is *unintuitive*.
2) assert(!obj) does an is-null check. assert(obj) is a completely 
broken opposite of this.
3) No AssertError is thrown, which is the entire point of the built-in 
assert().
4) The few added instructions for the null check hardly matter in a 
*debug* build of all things.

I don't mind assert(obj) testing the invariant of obj. In fact, that 
very much makes sense. But please, please, *please* check the object for 
null first. This is a random inconsistency in the language with no other 
justification than "seg faults are convenient in a debugger". By the 
same logic, we might as well not have array bounds checks. However, the 
state of things is that array bounds checks are emitted by default and 
users can disable them for e.g. a release build. I don't see why this 
case is any different.

- Alex
Nov 08 2011
next sibling parent bearophile <bearophileHUGS lycos.com> writes:
Alex R. Petersen:


 But please, please, *please* check the object for null first.


I tend to agree with this idea.

Bye,
bearophile
Nov 08 2011
prev sibling next sibling parent Trass3r <un known.com> writes:
+1
Nov 08 2011
prev sibling next sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 11/08/2011 11:35 PM, Alex Rønne Petersen wrote:

 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is insane
 for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object for
 null first.


+1.
Nov 08 2011
parent reply dsimcha <dsimcha yahoo.com> writes:
On 11/8/2011 7:05 PM, Timon Gehr wrote:

 On 11/08/2011 11:35 PM, Alex Rønne Petersen wrote:

 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is insane
 for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object for
 null first.


 +1.


+1.  **AND** in debug mode all pointers should be checked for null. 
Conceptually, I fail to see how a null pointer dereference is different 
from an array bounds error.  A null pointer is effectively an array with 
length zero.
Nov 08 2011
next sibling parent =?ISO-8859-1?Q?Alex_R=F8nne_Petersen?= <xtzgzorex gmail.com> writes:
On 09-11-2011 01:19, dsimcha wrote:

 On 11/8/2011 7:05 PM, Timon Gehr wrote:

 On 11/08/2011 11:35 PM, Alex Rønne Petersen wrote:

 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is insane
 for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object for
 null first.


 +1.


 +1. **AND** in debug mode all pointers should be checked for null.
 Conceptually, I fail to see how a null pointer dereference is different
 from an array bounds error. A null pointer is effectively an array with
 length zero.


I agree on that point. Though right now I'd just be happy to have sane 
asserts.

- Alex
Nov 08 2011
prev sibling parent Kapps <Kapps NotValidEmail.com> writes:
Agreed. It would be nice to have an error thrown instead of just a 
segfault that you may or may not get a reliable backtrace for and then 
spend the next few days trying to find out where it is. -_-

On 08/11/2011 6:19 PM, dsimcha wrote:

 On 11/8/2011 7:05 PM, Timon Gehr wrote:

 On 11/08/2011 11:35 PM, Alex Rønne Petersen wrote:

 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is insane
 for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object for
 null first.


 +1.


 +1. **AND** in debug mode all pointers should be checked for null.
 Conceptually, I fail to see how a null pointer dereference is different
 from an array bounds error. A null pointer is effectively an array with
 length zero.
Nov 08 2011
prev sibling next sibling parent reply "Martin Nowak" <dawg dawgfoto.de> writes:
On Tue, 08 Nov 2011 23:35:33 +0100, Alex R=C3=B8nne Petersen  =

<xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.  =



 assert(obj) testing the invariant *without* doing a null check is insa=


ne  =


 for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely  =



 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in=


  =


 assert().
 4) The few added instructions for the null check hardly matter in a  =



 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that  =



 very much makes sense. But please, please, *please* check the object f=


or  =


 null first. This is a random inconsistency in the language with no oth=


er  =


 justification than "seg faults are convenient in a debugger". By the  =



 same logic, we might as well not have array bounds checks. However, th=


e  =


 state of things is that array bounds checks are emitted by default and=


  =


 users can disable them for e.g. a release build. I don't see why this =


 =


 case is any different.

 - Alex


It does check for null.
Only it's a runtime support function (_d_invariant) and druntime is like=
ly
compiled without assertions. Are you really suggesting to add a null che=
ck  =

before
every method call?

martin
Nov 08 2011
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 11/09/2011 01:18 AM, Martin Nowak wrote:

 On Tue, 08 Nov 2011 23:35:33 +0100, Alex Rønne Petersen
 <xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is
 insane for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object
 for null first. This is a random inconsistency in the language with no
 other justification than "seg faults are convenient in a debugger". By
 the same logic, we might as well not have array bounds checks.
 However, the state of things is that array bounds checks are emitted
 by default and users can disable them for e.g. a release build. I
 don't see why this case is any different.

 - Alex


 It does check for null.
 Only it's a runtime support function (_d_invariant) and druntime is likely
 compiled without assertions. Are you really suggesting to add a null
 check before
 every method call?

 martin


No, he is suggesting to add a null check for assert(objref);, a 
construct that *looks* as if it was a null check, but does something 
almost unrelated.
Nov 08 2011
next sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 11/09/2011 01:21 AM, Timon Gehr wrote:

 On 11/09/2011 01:18 AM, Martin Nowak wrote:

 On Tue, 08 Nov 2011 23:35:33 +0100, Alex Rønne Petersen
 <xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is
 insane for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object
 for null first. This is a random inconsistency in the language with no
 other justification than "seg faults are convenient in a debugger". By
 the same logic, we might as well not have array bounds checks.
 However, the state of things is that array bounds checks are emitted
 by default and users can disable them for e.g. a release build. I
 don't see why this case is any different.

 - Alex


 It does check for null.
 Only it's a runtime support function (_d_invariant) and druntime is
 likely
 compiled without assertions. Are you really suggesting to add a null
 check before
 every method call?

 martin


 No, he is suggesting to add a null check for assert(objref);, a
 construct that *looks* as if it was a null check, but does something
 almost unrelated.


BTW, the number of occurrences of assert(objref && 1); in my code is huge.
Nov 08 2011
next sibling parent =?UTF-8?B?QWxleCBSw7hubmUgUGV0ZXJzZW4=?= <xtzgzorex gmail.com> writes:
On 09-11-2011 01:28, Timon Gehr wrote:

 On 11/09/2011 01:21 AM, Timon Gehr wrote:

 On 11/09/2011 01:18 AM, Martin Nowak wrote:

 On Tue, 08 Nov 2011 23:35:33 +0100, Alex Rønne Petersen
 <xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is
 insane for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object
 for null first. This is a random inconsistency in the language with no
 other justification than "seg faults are convenient in a debugger". By
 the same logic, we might as well not have array bounds checks.
 However, the state of things is that array bounds checks are emitted
 by default and users can disable them for e.g. a release build. I
 don't see why this case is any different.

 - Alex


 It does check for null.
 Only it's a runtime support function (_d_invariant) and druntime is
 likely
 compiled without assertions. Are you really suggesting to add a null
 check before
 every method call?

 martin


 No, he is suggesting to add a null check for assert(objref);, a
 construct that *looks* as if it was a null check, but does something
 almost unrelated.


 BTW, the number of occurrences of assert(objref && 1); in my code is huge.


Just for the record, someone on the D IRC channel suggested using 
assert(!!obj); as another workaround. Obviously still ugly as hell, but 
less so.

- Alex
Nov 08 2011
prev sibling next sibling parent reply "Martin Nowak" <dawg dawgfoto.de> writes:
On Wed, 09 Nov 2011 01:28:06 +0100, Timon Gehr <timon.gehr gmx.ch> wrote=
:


 On 11/09/2011 01:21 AM, Timon Gehr wrote:

 On 11/09/2011 01:18 AM, Martin Nowak wrote:

 On Tue, 08 Nov 2011 23:35:33 +0100, Alex R=C3=B8nne Petersen
 <xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is
 insane for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built=








-in

 assert().
 4) The few added instructions for the null check hardly matter in a=









 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, tha=








t

 very much makes sense. But please, please, *please* check the objec=








t

 for null first. This is a random inconsistency in the language with=








 no

 other justification than "seg faults are convenient in a debugger".=








 By

 the same logic, we might as well not have array bounds checks.
 However, the state of things is that array bounds checks are emitte=








d

 by default and users can disable them for e.g. a release build. I
 don't see why this case is any different.

 - Alex


 It does check for null.
 Only it's a runtime support function (_d_invariant) and druntime is
 likely
 compiled without assertions. Are you really suggesting to add a null=







 check before
 every method call?

 martin


 No, he is suggesting to add a null check for assert(objref);, a
 construct that *looks* as if it was a null check, but does something
 almost unrelated.


 BTW, the number of occurrences of assert(objref && 1); in my code is  =



 huge.


Just as a side note.
I personally consider this as harmful. All too often an assert(obj !is  =

null) is misused
as disclaimer. It pretends false safety, while the obscure case where  =

something is called
with null is not found during testing.
It also creates a coupling of the caller and the callee, because not  =

calling something with null
is now part of the in contract.
Nov 08 2011
parent Timon Gehr <timon.gehr gmx.ch> writes:
On 11/09/2011 01:46 AM, Martin Nowak wrote:

 On Wed, 09 Nov 2011 01:28:06 +0100, Timon Gehr <timon.gehr gmx.ch> wrote:


 On 11/09/2011 01:21 AM, Timon Gehr wrote:

 On 11/09/2011 01:18 AM, Martin Nowak wrote:

 On Tue, 08 Nov 2011 23:35:33 +0100, Alex Rønne Petersen
 <xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is
 insane for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object
 for null first. This is a random inconsistency in the language with no
 other justification than "seg faults are convenient in a debugger". By
 the same logic, we might as well not have array bounds checks.
 However, the state of things is that array bounds checks are emitted
 by default and users can disable them for e.g. a release build. I
 don't see why this case is any different.

 - Alex


 It does check for null.
 Only it's a runtime support function (_d_invariant) and druntime is
 likely
 compiled without assertions. Are you really suggesting to add a null
 check before
 every method call?

 martin


 No, he is suggesting to add a null check for assert(objref);, a
 construct that *looks* as if it was a null check, but does something
 almost unrelated.


 BTW, the number of occurrences of assert(objref && 1); in my code is
 huge.


 Just as a side note.
 I personally consider this as harmful. All too often an assert(obj !is
 null) is misused
 as disclaimer. It pretends false safety, while the obscure case where
 something is called
 with null is not found during testing.


What exactly do you consider harmful? If a function cannot deal with 
null it cannot. I like to have an assertion failure during debugging 
rather than a segfault. There is no "false safety", or
"true safety" for that matter, it is just as safe as not asserting 
before dereferencing the null pointer.

//...
this(Class cc)in{assert(cc&&1);}body{c=cc;}
invariant(){ assert(c&&1); }

void foo(){/* rely on c !is null. */}
//...

(Btw, I tried to verify my statement, but actually there were only about 
30 of those asserts in >6000 LOC.)


 It also creates a coupling of the caller and the callee, because not
 calling something with null
 is now part of the in contract.


You make it sound like that is a bad thing?
Nov 08 2011
prev sibling parent reply bcs <bcs example.com> writes:
On 11/08/2011 04:28 PM, Timon Gehr wrote:

 On 11/09/2011 01:21 AM, Timon Gehr wrote:

 On 11/09/2011 01:18 AM, Martin Nowak wrote:

 On Tue, 08 Nov 2011 23:35:33 +0100, Alex Rønne Petersen
 <xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is
 insane for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object
 for null first. This is a random inconsistency in the language with no
 other justification than "seg faults are convenient in a debugger". By
 the same logic, we might as well not have array bounds checks.
 However, the state of things is that array bounds checks are emitted
 by default and users can disable them for e.g. a release build. I
 don't see why this case is any different.

 - Alex


 It does check for null.
 Only it's a runtime support function (_d_invariant) and druntime is
 likely
 compiled without assertions. Are you really suggesting to add a null
 check before
 every method call?

 martin


 No, he is suggesting to add a null check for assert(objref);, a
 construct that *looks* as if it was a null check, but does something
 almost unrelated.


 BTW, the number of occurrences of assert(objref && 1); in my code is huge.


I would have used assert(!!obj) because it's shorter.
Nov 08 2011
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 11/09/2011 04:52 AM, bcs wrote:

 On 11/08/2011 04:28 PM, Timon Gehr wrote:

 On 11/09/2011 01:21 AM, Timon Gehr wrote:

 On 11/09/2011 01:18 AM, Martin Nowak wrote:

 On Tue, 08 Nov 2011 23:35:33 +0100, Alex Rønne Petersen
 <xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is
 insane for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object
 for null first. This is a random inconsistency in the language with no
 other justification than "seg faults are convenient in a debugger". By
 the same logic, we might as well not have array bounds checks.
 However, the state of things is that array bounds checks are emitted
 by default and users can disable them for e.g. a release build. I
 don't see why this case is any different.

 - Alex


 It does check for null.
 Only it's a runtime support function (_d_invariant) and druntime is
 likely
 compiled without assertions. Are you really suggesting to add a null
 check before
 every method call?

 martin


 No, he is suggesting to add a null check for assert(objref);, a
 construct that *looks* as if it was a null check, but does something
 almost unrelated.


 BTW, the number of occurrences of assert(objref && 1); in my code is
 huge.


 I would have used assert(!!obj) because it's shorter.


If you have already typed 'assert(obj', it is not shorter.
Nov 08 2011
parent bcs <bcs example.com> writes:
On 11/08/2011 11:42 PM, Timon Gehr wrote:

 On 11/09/2011 04:52 AM, bcs wrote:

 On 11/08/2011 04:28 PM, Timon Gehr wrote:

 On 11/09/2011 01:21 AM, Timon Gehr wrote:

 On 11/09/2011 01:18 AM, Martin Nowak wrote:

 On Tue, 08 Nov 2011 23:35:33 +0100, Alex Rønne Petersen
 <xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is
 insane for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the
 built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object
 for null first. This is a random inconsistency in the language
 with no
 other justification than "seg faults are convenient in a
 debugger". By
 the same logic, we might as well not have array bounds checks.
 However, the state of things is that array bounds checks are emitted
 by default and users can disable them for e.g. a release build. I
 don't see why this case is any different.

 - Alex


 It does check for null.
 Only it's a runtime support function (_d_invariant) and druntime is
 likely
 compiled without assertions. Are you really suggesting to add a null
 check before
 every method call?

 martin


 No, he is suggesting to add a null check for assert(objref);, a
 construct that *looks* as if it was a null check, but does something
 almost unrelated.


 BTW, the number of occurrences of assert(objref && 1); in my code is
 huge.


 I would have used assert(!!obj) because it's shorter.


 If you have already typed 'assert(obj', it is not shorter.


Faster to type? No. Shorter? Yes it still is. I spend enough more time 
reading and thinking about code that size is more relevant then typing 
speed.
Nov 09 2011
prev sibling parent reply "Martin Nowak" <dawg dawgfoto.de> writes:
On Wed, 09 Nov 2011 01:21:47 +0100, Timon Gehr <timon.gehr gmx.ch> wrote=
:


 On 11/09/2011 01:18 AM, Martin Nowak wrote:

 On Tue, 08 Nov 2011 23:35:33 +0100, Alex R=C3=B8nne Petersen
 <xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is
 insane for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-=






in

 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that=







 very much makes sense. But please, please, *please* check the object=







 for null first. This is a random inconsistency in the language with =






no

 other justification than "seg faults are convenient in a debugger". =






By

 the same logic, we might as well not have array bounds checks.
 However, the state of things is that array bounds checks are emitted=







 by default and users can disable them for e.g. a release build. I
 don't see why this case is any different.

 - Alex


 It does check for null.
 Only it's a runtime support function (_d_invariant) and druntime is  =





 likely
 compiled without assertions. Are you really suggesting to add a null
 check before
 every method call?

 martin


 No, he is suggesting to add a null check for assert(objref);, a  =



 construct that *looks* as if it was a null check, but does something  =



 almost unrelated.


Then just for reference, apply this patch to druntime and you'll get an =
 =

assertion instead.

diff --git a/src/rt/invariant.d b/src/rt/invariant.d
index 71337f1..bc5e53a 100644
--- a/src/rt/invariant.d
+++ b/src/rt/invariant.d
   -16,13 +16,16   
  /**
   *
   */
+import core.exception;
+
  void _d_invariant(Object o)
  {   ClassInfo c;

      //printf("__d_invariant(%p)\n", o);

      // BUG: needs to be filename/line of caller, not library routine
-    assert(o !is null); // just do null check, not invariant check
+    if (o is null)
+        throw new AssertError("_d_invariant called with null reference.=
");

      c =3D o.classinfo;
      do
Nov 08 2011
parent =?UTF-8?B?QWxleCBSw7hubmUgUGV0ZXJzZW4=?= <xtzgzorex gmail.com> writes:
On 09-11-2011 01:57, Martin Nowak wrote:

 On Wed, 09 Nov 2011 01:21:47 +0100, Timon Gehr <timon.gehr gmx.ch> wrote:


 On 11/09/2011 01:18 AM, Martin Nowak wrote:

 On Tue, 08 Nov 2011 23:35:33 +0100, Alex Rønne Petersen
 <xtzgzorex gmail.com> wrote:


 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is
 insane for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object
 for null first. This is a random inconsistency in the language with no
 other justification than "seg faults are convenient in a debugger". By
 the same logic, we might as well not have array bounds checks.
 However, the state of things is that array bounds checks are emitted
 by default and users can disable them for e.g. a release build. I
 don't see why this case is any different.

 - Alex


 It does check for null.
 Only it's a runtime support function (_d_invariant) and druntime is
 likely
 compiled without assertions. Are you really suggesting to add a null
 check before
 every method call?

 martin


 No, he is suggesting to add a null check for assert(objref);, a
 construct that *looks* as if it was a null check, but does something
 almost unrelated.


 Then just for reference, apply this patch to druntime and you'll get an
 assertion instead.

 diff --git a/src/rt/invariant.d b/src/rt/invariant.d
 index 71337f1..bc5e53a 100644
 --- a/src/rt/invariant.d
 +++ b/src/rt/invariant.d
    -16,13 +16,16   
 /**
 *
 */
 +import core.exception;
 +
 void _d_invariant(Object o)
 { ClassInfo c;

 //printf("__d_invariant(%p)\n", o);

 // BUG: needs to be filename/line of caller, not library routine
 - assert(o !is null); // just do null check, not invariant check
 + if (o is null)
 + throw new AssertError("_d_invariant called with null reference.");

 c = o.classinfo;
 do


But as that BUG note states, it still isn't sufficient/consistent. :) 
Thanks for the patch though!

- Alex
Nov 09 2011
prev sibling next sibling parent reply Jesse Phillips <jessekphillips+D gmail.com> writes:
Alex Rønne Petersen Wrote:


 Hi,


http://d.puremagic.com/issues/show_bug.cgi?id=6913

+1
Nov 08 2011
parent "Daniel Murphy" <yebblies nospamgmail.com> writes:
"Jesse Phillips" <jessekphillips+D gmail.com> wrote in message 
news:j9cikj$t8a$1 digitalmars.com...

 Alex Rønne Petersen Wrote:


 Hi,


 http://d.puremagic.com/issues/show_bug.cgi?id=6913

 +1


This is nearly 5 years old:
http://d.puremagic.com/issues/show_bug.cgi?id=796
And has a patch on github.
Nov 08 2011
prev sibling next sibling parent reply Davidson Corry <davidsoncorry comcast.net> writes:
I don't get it -- why is this even necessary? Please don't answer here. 
Swing over to D.learn, where I am starting an "assert(obj) is a mystery" 
thread...

...because answers that start with "because..." belong in a "learn" 
newsgroup.

Thanks in advance.

-- Davidson



On 11/8/2011 2:35 PM, Alex Rønne Petersen wrote:

 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is insane
 for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object for
 null first. This is a random inconsistency in the language with no other
 justification than "seg faults are convenient in a debugger". By the
 same logic, we might as well not have array bounds checks. However, the
 state of things is that array bounds checks are emitted by default and
 users can disable them for e.g. a release build. I don't see why this
 case is any different.

 - Alex
Nov 08 2011
next sibling parent =?ISO-8859-1?Q?Alex_R=F8nne_Petersen?= <xtzgzorex gmail.com> writes:
On 09-11-2011 06:23, Davidson Corry wrote:

 I don't get it -- why is this even necessary? Please don't answer here.
 Swing over to D.learn, where I am starting an "assert(obj) is a mystery"
 thread...

 ...because answers that start with "because..." belong in a "learn"
 newsgroup.

 Thanks in advance.

 -- Davidson



 On 11/8/2011 2:35 PM, Alex Rønne Petersen wrote:

 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is insane
 for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object for
 null first. This is a random inconsistency in the language with no other
 justification than "seg faults are convenient in a debugger". By the
 same logic, we might as well not have array bounds checks. However, the
 state of things is that array bounds checks are emitted by default and
 users can disable them for e.g. a release build. I don't see why this
 case is any different.

 - Alex




This has nothing to do with learning. This entire thread is a language 
design counter-argument.

- Alex
Nov 09 2011
prev sibling parent Bernard Helyer <b.helyer gmail.com> writes:
Because it's a design issue, not a learning issue?
Nov 09 2011
prev sibling next sibling parent reply Dejan Lekic <dejan.lekic gmail.com> writes:
Alex, it is a well know bug, I remember it from... ~2006... It has been 
reported as a bug ages ago... I/we totally agree with you.
Nov 09 2011
parent =?ISO-8859-1?Q?Alex_R=F8nne_Petersen?= <xtzgzorex gmail.com> writes:
On 09-11-2011 12:48, Dejan Lekic wrote:

 Alex, it is a well know bug, I remember it from... ~2006... It has been
 reported as a bug ages ago... I/we totally agree with you.


Yep, I just figured I'd bring some attention to it again, as well as 
some arguments for why the way it is now is broken.

- Alex
Nov 09 2011
prev sibling next sibling parent Bernard Helyer <b.helyer gmail.com> writes:
Agreed on all points. It's pathetic that this is still a problem. Check 
the object for null. To not do so is a _bug_, even if Walter disagrees. 
SDC will not replicate DMD's behaviour in this regard.

-Bernard.
Nov 09 2011
prev sibling parent reply =?ISO-8859-1?Q?Alex_R=F8nne_Petersen?= <xtzgzorex gmail.com> writes:
On 08-11-2011 23:35, Alex Rønne Petersen wrote:

 Hi,

 As the title suggests, I'm going to be rather blunt about this.
 assert(obj) testing the invariant *without* doing a null check is insane
 for the following reasons:

 1) It is not what a user expects. It is *unintuitive*.
 2) assert(!obj) does an is-null check. assert(obj) is a completely
 broken opposite of this.
 3) No AssertError is thrown, which is the entire point of the built-in
 assert().
 4) The few added instructions for the null check hardly matter in a
 *debug* build of all things.

 I don't mind assert(obj) testing the invariant of obj. In fact, that
 very much makes sense. But please, please, *please* check the object for
 null first. This is a random inconsistency in the language with no other
 justification than "seg faults are convenient in a debugger". By the
 same logic, we might as well not have array bounds checks. However, the
 state of things is that array bounds checks are emitted by default and
 users can disable them for e.g. a release build. I don't see why this
 case is any different.

 - Alex


I'd just like to add, regarding asserting invariants: Why not just *make 
invariants callable*? I.e. obj.invariant() invokes the invariant 
function. invariant is already a keyword, and invariants are declared 
like functions, so this seems like a logical thing to do.

- Alex
Nov 13 2011
next sibling parent reply Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 11/13/11, Alex R=F8nne Petersen <xtzgzorex gmail.com> wrote:

 I'd just like to add, regarding asserting invariants: Why not just *make
 invariants callable*? I.e. obj.invariant() invokes the invariant
 function.


I think you would have to wrap it as debug() { obj.invariant(); },
otherwise you would end up calling the invariant in release-mode.
Nov 13 2011
parent =?ISO-8859-1?Q?Alex_R=F8nne_Petersen?= <xtzgzorex gmail.com> writes:
On 13-11-2011 19:43, Andrej Mitrovic wrote:

 On 11/13/11, Alex Rønne Petersen<xtzgzorex gmail.com>  wrote:

 I'd just like to add, regarding asserting invariants: Why not just *make
 invariants callable*? I.e. obj.invariant() invokes the invariant
 function.


 I think you would have to wrap it as debug() { obj.invariant(); },
 otherwise you would end up calling the invariant in release-mode.


The compiler could easily do that work, however.

- Alex
Nov 13 2011
prev sibling parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Sunday, November 13, 2011 18:52:35 Alex R=C3=B8nne Petersen wrote:

 I'd just like to add, regarding asserting invariants: Why not just *m=


ake

 invariants callable*? I.e. obj.invariant() invokes the invariant
 function. invariant is already a keyword, and invariants are declared=



 like functions, so this seems like a logical thing to do.


Call __invariant. But like all __ functions, I don't think that it's re=
ally=20
intended to be called directly in code. You can do it though.

Really, I don't think that there's a problem with assert(obj) calling o=
bj's=20
invariant. It's just extra overhead in a non-release build which is run=
ning a=20
set of assertions which should be true anyway (assuming that it's being=
 called=20
outside of the class). It's the fact that it doesn't check for null fir=
st which=20
is so atrocious, since that _completely_ changes the semantics of what =
obj=20
normally does when implicitly converted to bool. If it checked for null=
 first,=20
then the normal semantics hold with the addition of checking the invari=
ant.

The fun part about assert(obj) though is that it doesn't work with stru=
cts -=20
not directly anyway. If obj is a struct, assert(obj) tries to convert i=
t to=20
bool like it would do normally. However, assert(&obj) _does_ call the=20=

invariant. Pointers to structs are treated exactly the same as referenc=
es to=20
classes in this case (complete with the lack of a null check). So, it's=
=20
arguably consistent, but it's a bit weird.

In any case, what really needs to be done is to add the extra null chec=
k. It=20
won't silently break code to do that, and it'll avoid the surprise and=20=

confusiong of assert(obj) not checking for null and blowing up as a res=
ult.

- Jonathan M Davis
Nov 13 2011
next sibling parent bearophile <bearophileHUGS lycos.com> writes:
Jonathan M Davis:


 The fun part about assert(obj) though is that it doesn't work with structs - 
 not directly anyway. If obj is a struct, assert(obj) tries to convert it to 
 bool like it would do normally. However, assert(&obj) _does_ call the 
 invariant. Pointers to structs are treated exactly the same as references to 
 classes in this case (complete with the lack of a null check). So, it's 
 arguably consistent, but it's a bit weird.


I didn't remember this about pointer to struct.

Generally a good language needs to be designed with a more explicit syntax
(unless the operation is very commonly done in this language, or among other
languages too). If I am a newbie that doesn't know D a lot, and I see those
asserts, for class instances, struct instances, and struct instances with &, I
can't tell what they mean. This is not so good.

Bye,
bearophile
Nov 13 2011
prev sibling parent reply =?UTF-8?B?QWxleCBSw7hubmUgUGV0ZXJzZW4=?= <xtzgzorex gmail.com> writes:
On 13-11-2011 22:38, Jonathan M Davis wrote:

 On Sunday, November 13, 2011 18:52:35 Alex Rønne Petersen wrote:

 I'd just like to add, regarding asserting invariants: Why not just *make
 invariants callable*? I.e. obj.invariant() invokes the invariant
 function. invariant is already a keyword, and invariants are declared
 like functions, so this seems like a logical thing to do.


 Call __invariant. But like all __ functions, I don't think that it's really
 intended to be called directly in code. You can do it though.

 Really, I don't think that there's a problem with assert(obj) calling obj's
 invariant. It's just extra overhead in a non-release build which is running a
 set of assertions which should be true anyway (assuming that it's being called
 outside of the class). It's the fact that it doesn't check for null first which
 is so atrocious, since that _completely_ changes the semantics of what obj
 normally does when implicitly converted to bool. If it checked for null first,
 then the normal semantics hold with the addition of checking the invariant.


I ran into a funny issue today. I wanted to assert that all items in a 
collection are not null. Naturally, I just did something like:

invariant()
{
     foreach (item; _store)
         assert(item);
}

That was a bad idea, however! Because this assert calls the invariant of 
item, *it can end up causing an infinite loop*. This can happen in a 
scenario such as:

class A
{
     Collection!B col;

     invariant()
     {
         assert(col);
     }
}

class B
{
     A a;

     invariant()
     {
         assert(a);
     }
}

auto a = new A(); a.col = new Collection!B();
auto b = new B(); b.a = a; a.col.add(b);

assert(b);

The call sequence will now look something like:

1: B.invariant()
2: A.invariant()
3: Collection!B.invariant()
4: *B.invariant()*
5: ... and so on ...

I just thought it was worth mentioning how calling the invariant in 
assert can lead to very unexpected results. Of course, the solution is 
simple: assert(item !is null); in the collection invariant. Still, I 
think this is just as unintuitive as the missing null check.

I think we need to keep it simple here.


 The fun part about assert(obj) though is that it doesn't work with structs -
 not directly anyway. If obj is a struct, assert(obj) tries to convert it to
 bool like it would do normally. However, assert(&obj) _does_ call the
 invariant. Pointers to structs are treated exactly the same as references to
 classes in this case (complete with the lack of a null check). So, it's
 arguably consistent, but it's a bit weird.

 In any case, what really needs to be done is to add the extra null check. It
 won't silently break code to do that, and it'll avoid the surprise and
 confusiong of assert(obj) not checking for null and blowing up as a result.

 - Jonathan M Davis


- Alex
Nov 19 2011
parent bearophile <bearophileHUGS lycos.com> writes:
Alex R. Petersen:


 I just thought it was worth mentioning how calling the invariant in 
 assert can lead to very unexpected results.


Python Zen says "Explicit is better than implicit.". Calling the invariant with
an explicit syntax is better than calling it implicitly with an assert syntax
that looks like it does something different.

Bye,
bearophile
Nov 19 2011