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

           Summary: Make == null a warning for arrays
           Product: D
           Version: unspecified
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: jmdavisProg@gmx.com


--- Comment #0 from Jonathan M Davis <jmdavisProg@gmx.com> 2011-07-18 12:51:46 PDT ---
The correct way to test whether something in null in D is to use is. Using == null with strings is doing the same as checking whether their length == 0. This is _not_ what most people are going to expect. So, if you actually understand the rules, then the correct way to check for null is is null, and the correct way to check for empty is to either use length == 0 or std.array.empty. As such, there is no point in using == and if it's in the code, there's a good chance that it's a bug.

It appears that there is _already_ such a warning when dealing with classes. I think that it would be far less error-prone to make the same warning apply to arrays. I don't think that there is really a reasonable case for use == null in D at all, so in all cases, it should generate a warning. is null is what actually checks for null, and there are better, clearer ways to check for empty with arrays.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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


bearophile_hugs@eml.cc changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bearophile_hugs@eml.cc


--- Comment #1 from bearophile_hugs@eml.cc 2011-07-18 12:54:45 PDT ---
(In reply to comment #0)

> I don't think that there is really a reasonable case for use == null in D at all,

If this is true then I suggest to statically disallow such syntax, turning it into an error, instead of producing a warning.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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


Steven Schveighoffer <schveiguy@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |schveiguy@yahoo.com


--- Comment #2 from Steven Schveighoffer <schveiguy@yahoo.com> 2011-07-18 13:25:45 PDT ---
I disagree with the idea that == null is so bad to require a warning.

Note that .empty requires importing std.array, and arr.length == 0 doesn't read as well as array == null.

Also note that the problem with classes (actually this has been fixed) is that obj == null would cause a segfault if obj actually was null.  The distinction between arr == null and arr is null is far more subtle and far less dangerous.

Also, this change makes code behave inconsistently.

For example:

foo(int[] y)
{
  int[] x = null;
  if(y == x) // ok
  {}
  else if(y == null) // warning???
}

What happens if the former is folded into the latter during optimization?

I'd argue this is not worth a change to the language warnings.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #3 from Jonathan M Davis <jmdavisProg@gmx.com> 2011-07-18 13:43:18 PDT ---
Well, that's why I said warning rather than error. But I would consider it bad practice to use == null anywhere with arrays. I'm automatically go to look at it and wonder whether the programmer really meant is null (and they probably did). And since I'd expect most code to import std.array if it's doing much with arrays at all, I'd fully expect it to have access to empty. And while arr.length == 0 is not as short as arr == length, it's far less error prone. I think that the used of arr == null implies a fundamental misunderstanding on the part of the programmer using it. Now, maybe they do understand that it's the same as arr.length == 0 or arr.empty, but the odds are so high that they really meant is null that I do _not_ think that code should be using == null. If you're talking about "code smells" as Bearophile likes to, arr == null _definitely_ smells. Sure, it _might_ be that it does what the programmer intended, but odds are that it doesn't.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #4 from Steven Schveighoffer <schveiguy@yahoo.com> 2011-07-18 13:54:46 PDT ---
There is a flaw in your argument, however.  For the coder to have meant they want to check specifically if the pointer is null, they have to know there is a difference between null and an "empty but non-null" slice.  If this is the case, then wouldn't they use is null instead of == null?

Most coders don't even know that an empty slice could have a non-null pointer, or would even care.  They just know that comparing an empty slice to null results in true.  Consider that there are very few, if at all, functions which make a distinction between null and "empty but not null" slices.  So where exactly does doing == null lead to a problem?

If you want to focus on confusing spots, the two problems I see are:

if(arr is null)

and

if(arr)

Both of these act *differently* based on whether the pointer is null or not. Now that is confusing.

Note that this can be fixed by doing:

if(arr is null) => warn, recommend using if(arr.ptr is null)

if(arr) => should always be false if arr.length == 0

But I think == null is perfectly safe, and 99% of the time exactly what the writer intended.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #5 from Jonathan M Davis <jmdavisProg@gmx.com> 2011-07-18 14:46:24 PDT ---
In pretty much every other language that I've ever used, null is _not_ the same thing as empty _ever_. Someone coming from C, C++, Java etc. is going to expect dynamic arrays to be able to be null. The way that you check for null in all of those languages is either == 0 or == null (or == NULL or == nullptr). is does not exist in any other language that I've ever seen. I fully expect programmers from C, C++, Java etc. to expect == null to be checking whether an array is null - that is whether is null is true. But it doesn't do that. It checks whether length == 0.

The fact that an empty array an null are almost synonymous in D is incredibly bizarre. It simplifies declaring an array and then appending it or setting its length, but it is _not_ what your average programmer is going to expect coming from other C-based languages. As such, I expect that == null risks being a frequent newbie mistake. The only reason it wouldn't be is if not that many newbies need to or care about checking for null.

The fact that

if(arr)

and

if(arr is null)

don't do the same thing is a related but separate issue, and should probably also be addressed. But I really think that == null is a problem which is going to throw off a lot of people. If I see it in code, I'm either going to point it out or change it (depending on the circumstances). The odds are just too high that it doesn't do what the programmer intended or that someone else reading the code will misinterpret what it does.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #6 from Steven Schveighoffer <schveiguy@yahoo.com> 2011-07-19 05:31:19 PDT ---
(In reply to comment #5)
> In pretty much every other language that I've ever used, null is _not_ the same thing as empty _ever_. Someone coming from C, C++, Java etc. is going to expect dynamic arrays to be able to be null. The way that you check for null in all of those languages is either == 0 or == null (or == NULL or == nullptr).

php just uses if(!arr) or if(arr == array()) or if(arr == null).  All of those
return true for empty arrays.

But the point I was making is, for D null *is* synonymous with empty.  D is not every other language, and shouldn't be bound to the rules of those languages. The larger question to answer here is, is it likely to be *wrong* when someone enters:

if(arr == null)

and expects this to check if the pointer is null.  The answer is very much no. It's likely to be either *exactly* what you want, or harmless.  The super-subtle difference between null arrays and empty-but-not-null arrays is seldom, if ever, used.  Now, I agree that we could have a better built-in moniker for is this array empty, but do we really need a warning?  The code is clear, unambiguous, and exactly what the developer wanted, or close enough to be effective.

In java or C, checking an array for a null pointer is needed because an unallocated array throws an exception or segfault if you try to use it.  This is not the case in D.  D does not have this problem.  Anyone's code who thinks it does is bound to be obvious:

if(arr == null)
{
   arr = new arr[5];
}

No need to warn here, the code is fine (works exactly how the user wants).  But the easy optimization is just to remove the if check.

> is does
> not exist in any other language that I've ever seen.

It exists in at least C# (Object.ReferenceEquals), php (===) and C has no need
for it, since operators can't be overloaded.

> I fully expect programmers
> from C, C++, Java etc. to expect == null to be checking whether an array is
> null - that is whether is null is true. But it doesn't do that. It checks
> whether length == 0.

Yeah, but the point I'm trying to make is, checking for an actual null pointer means nothing in D!  So checking if an array is empty is a much more useful implementation.  Null arrays are not bad things in D, they don't throw exceptions or segfaults simply because they are null.  If anything, the check for null is probably extraneous, and can be removed.

> But I really think that == null is a problem which is going
> to throw off a lot of people. If I see it in code, I'm either going to point it
> out or change it (depending on the circumstances). The odds are just too high
> that it doesn't do what the programmer intended or that someone else reading
> the code will misinterpret what it does.

I completely disagree, the odds are very low that it's damaging or incorrect at all.  Experienced D coders use this feature all the time.  You just gotta start thinking in D instead of your previous languages.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #7 from Steven Schveighoffer <schveiguy@yahoo.com> 2011-07-19 05:32:02 PDT ---
(In reply to comment #5)
> In pretty much every other language that I've ever used, null is _not_ the same thing as empty _ever_. Someone coming from C, C++, Java etc. is going to expect dynamic arrays to be able to be null. The way that you check for null in all of those languages is either == 0 or == null (or == NULL or == nullptr).

php just uses if(!arr) or if(arr == array()) or if(arr == null).  All of those
return true for empty arrays.

But the point I was making is, for D null *is* synonymous with empty.  D is not every other language, and shouldn't be bound to the rules of those languages. The larger question to answer here is, is it likely to be *wrong* when someone enters:

if(arr == null)

and expects this to check if the pointer is null.  The answer is very much no. It's likely to be either *exactly* what you want, or harmless.  The super-subtle difference between null arrays and empty-but-not-null arrays is seldom, if ever, used.  Now, I agree that we could have a better built-in moniker for is this array empty, but do we really need a warning?  The code is clear, unambiguous, and exactly what the developer wanted, or close enough to be effective.

In java or C, checking an array for a null pointer is needed because an unallocated array throws an exception or segfault if you try to use it.  This is not the case in D.  D does not have this problem.  Anyone's code who thinks it does is bound to be obvious:

if(arr == null)
{
   arr = new int[5];
}

No need to warn here, the code is fine (works exactly how the user wants).  But the easy optimization is just to remove the if check.

> is does
> not exist in any other language that I've ever seen.

It exists in at least C# (Object.ReferenceEquals), php (===) and C has no need
for it, since operators can't be overloaded.

> I fully expect programmers
> from C, C++, Java etc. to expect == null to be checking whether an array is
> null - that is whether is null is true. But it doesn't do that. It checks
> whether length == 0.

Yeah, but the point I'm trying to make is, checking for an actual null pointer means nothing in D!  So checking if an array is empty is a much more useful implementation.  Null arrays are not bad things in D, they don't throw exceptions or segfaults simply because they are null.  If anything, the check for null is probably extraneous, and can be removed.

> But I really think that == null is a problem which is going
> to throw off a lot of people. If I see it in code, I'm either going to point it
> out or change it (depending on the circumstances). The odds are just too high
> that it doesn't do what the programmer intended or that someone else reading
> the code will misinterpret what it does.

I completely disagree, the odds are very low that it's damaging or incorrect at all.  Experienced D coders use this feature all the time.  You just gotta start thinking in D instead of your previous languages.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #8 from Steven Schveighoffer <schveiguy@yahoo.com> 2011-07-19 05:33:14 PDT ---
Sorry for the duplicate comment, I wanted the first to be overwritten by the
second (bugzilla told me that's what would happen), but it didn't :(  Ignore
comment 6 (/me wishes for comment editing/deleting)

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #9 from Jonathan M Davis <jmdavisProg@gmx.com> 2011-07-19 11:21:49 PDT ---
Except that null and empty are _not_ the same for arrays. True, checking for null is not as necessary in D, but you _can_ write code which relies on whether arrays are null or not. And the most likely thing that someone is going to do if they're writing code like that is to use == null, unless they were paying a lot of attention when reading the online docs or TDPL and remembered is and how it differs from ==. If == null and is null were identical, it wouldn't be an issue, but they're not. It can matter whether a function returns null or "" (or []). It can matter whether you use == or is. And I wouldn't expect many people new to D to correctly use is with null. Rather, if they're checking for null, they'll use == null. And since there is _zero_ need for == null given the alternatives and given how it's likely that a newbie would use == where they should use is, I see no reason that good D code should be using == null (at best, it's a bit prettier than length == 0, and given that empty matters with containers, empty should be encouraged over length == 0 anyway). So, given that it's going to cause problems (and bugs) for newbies, and anyone who knows what they're doing doesn't need it, I really think that there should be a warning for using == null.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------