Bug 4733 – Possible bugs caused by dynamic arrays in boolean evaluation context
Status
RESOLVED
Resolution
WONTFIX
Severity
enhancement
Priority
P2
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2010-08-26T14:12:04Z
Last change time
2024-11-29T14:19:33Z
Keywords
pull
Assigned to
No Owner
Creator
bearophile_hugs
Comments
Comment #0 by bearophile_hugs — 2010-08-26T14:12:04Z
In Python it is a normal idiom to test for collection emptiness just putting their name in a boolean evaluation context (this is an idom valid for all Python collections):
arr = [1]
assert arr
But in D similar code causes troubles, this differt code shows an example:
void main() {
int[] arr = [1];
int* orig_ptr = arr.ptr;
arr.length = 0;
assert(!!arr); // this doesn't assert
assert(arr.ptr == orig_ptr); // this doesn't assert
}
!!arr is true because while arr.length is zero, arr.ptr is not null.
To avoid possible bugs (for example for programmers coming from Python language) I suggest to turn "if(arr)" into a syntax error, forcing to use "if(arr.length)" or "if(arr.ptr)" or similar things, and you avoid possible bugs. In the uncommon cases where you want to test both fields to be empty, you may use "if(arr.length || arr.ptr)" or "if(arr.length != arr.init)" or similar things.
The semantics of "if(aa)" and "if(static_arr)", the first tests if the reference is null, the second if all items are empty.
Comment #1 by bearophile_hugs — 2012-02-19T04:57:20Z
See also issue 7539
Comment #2 by bearophile_hugs — 2013-03-25T15:59:36Z
I have created a thread about this issue in the main D newsgroup:
http://forum.dlang.org/thread/[email protected]
It was well received by almost every one. But people are split between just forbidding "if(dyn_arr)" and turning it into an equivalent of "if(dyn_arr.length)". Both proposals have advantages and disadvantages.
Both are a little breaking change, but turning something into an error allows you to find it quickly in most cases inside D code. (But D also has the compiles __trait, that does not give an error, it just silently returns false instead of true).
On the other hand the behaviour change is not meant to happen all at once. It has to be a warning, then deprecation and then a change, along months and three DMD versions. So there is time to fix old D code. And even if the old D code doesn't get changed, I think it's uncommon for code to rely on dynamic array to be actually (null,null) instead of being of zero length. So the total amount of breakage even for a semantic change is probably small. Considering the presence of the compiles __trait, the idea of the small behaviour change is not so bad.
In both cases the change path will start at the same way: warning, deprecation, and then an error or a behavour change). So for now I am asking for a warning for code like "if(dyn_arr)".
Note that the syntax "if(dyn_arr !is null)" is still allowed.
--------------------
A related issue is what to do with associative arrays:
void main() {
bool[int] aa;
assert(!aa);
aa = [1: true];
assert(aa);
aa.remove(1);
assert(aa);
assert(!aa.length);
}
Comment #3 by ds.dlang — 2013-05-11T21:08:09Z
I feel that there is an additional bug: "if(slice)" evaluates an empty slice as true (as does "!!slice"), but "cast(bool)slice" does not compile ("cannot cast slice to integral type bool").
So somehow, converting arrays to booleans isn't consistently supported, and when it compiles, the meaning is indeed unintuitive. I agree that if conversion to booleans is to be supported, then "non-empty" is the best meaning to go for.
Comment #4 by bearophile_hugs — 2013-09-21T05:19:09Z
See also issue 11080, that asks to disallow this bug-prone construct:
assert("something going wrong");
Comment #7 by bearophile_hugs — 2013-11-27T01:13:24Z
(In reply to comment #5)
> Warning:
>
> https://github.com/D-Programming-Language/dmd/pull/2885
The new warning is:
warning("converting dynamic arrays to boolean is deprecated, instead use array.ptr");
For normal D programmers the right way to tell if an array (and eventually associative array) is empty is with the std.array.empty function, that tests the length. You don't want future normal D programmers to start using .ptr everywhere they want to test array emptiness.
So perhaps a better warning is:
"converting dynamic arrays to boolean is deprecated, instead use std.array.empty"
On the other hand some programmers want really meant to use ".ptr". And some programmers want to test "arr.length || arr.ptr" and so on.
So perhaps an alternative error message is:
"converting dynamic arrays to boolean is deprecated"
Comment #8 by rswhite4 — 2013-11-27T02:05:23Z
(In reply to comment #7)
> (In reply to comment #5)
> > Warning:
> >
> > https://github.com/D-Programming-Language/dmd/pull/2885
>
> The new warning is:
>
> warning("converting dynamic arrays to boolean is deprecated, instead use
> array.ptr");
>
> For normal D programmers the right way to tell if an array (and eventually
> associative array) is empty is with the std.array.empty function, that tests
> the length. You don't want future normal D programmers to start using .ptr
> everywhere they want to test array emptiness.
>
> So perhaps a better warning is:
>
> "converting dynamic arrays to boolean is deprecated, instead use
> std.array.empty"
>
> On the other hand some programmers want really meant to use ".ptr". And some
> programmers want to test "arr.length || arr.ptr" and so on.
>
> So perhaps an alternative error message is:
>
> "converting dynamic arrays to boolean is deprecated"
Or simply: use arr.length != 0
Comment #9 by yebblies — 2013-11-27T02:12:54Z
How about:
"converting dynamic arrays to boolean is deprecated, instead use std.array.empty or array.ptr"
Comment #10 by yebblies — 2013-11-27T02:15:07Z
(In reply to comment #8)
> Or simply: use arr.length != 0
That is not the same as `if (arr)`, and I don't want this to make people accidentally break their code.
Comment #11 by yebblies — 2013-11-27T02:15:07Z
(In reply to comment #8)
> Or simply: use arr.length != 0
That is not the same as `if (arr)`, and I don't want this to make people accidentally break their code.
Comment #12 by rswhite4 — 2013-11-27T02:20:29Z
(In reply to comment #11)
> (In reply to comment #8)
> > Or simply: use arr.length != 0
>
> That is not the same as `if (arr)`, and I don't want this to make people
> accidentally break their code.
No need to talk to me twice. Then !arr.length. That is the same as std.array.empty and it saves you the import.
Comment #13 by yebblies — 2013-11-27T02:31:49Z
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #8)
> > > Or simply: use arr.length != 0
> >
> > That is not the same as `if (arr)`, and I don't want this to make people
> > accidentally break their code.
>
> No need to talk to me twice. Then !arr.length. That is the same as
> std.array.empty and it saves you the import.
`if (!arr.length) is the same as `if (array.length != 0), and neither are the same as `if (arr)`, which expands to `if (arr.ptr)`.
Comment #14 by rswhite4 — 2013-11-27T02:37:17Z
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #8)
> > > > Or simply: use arr.length != 0
> > >
> > > That is not the same as `if (arr)`, and I don't want this to make people
> > > accidentally break their code.
> >
> > No need to talk to me twice. Then !arr.length. That is the same as
> > std.array.empty and it saves you the import.
>
> `if (!arr.length) is the same as `if (array.length != 0), and neither are the
> same as `if (arr)`, which expands to `if (arr.ptr)`.
I never said that it is the same. But it's the same as std.array.empty and saves you the import. That's all.
https://github.com/D-Programming-Language/phobos/blob/master/std/array.d#L41
I would prefer that the warning consider arr.length != 0 more than std.array.empty. Nothing else. Hope you understand me now.
Comment #15 by github-bugzilla — 2013-11-27T02:39:58Z
Comment #16 by bearophile_hugs — 2013-11-27T03:16:45Z
(In reply to comment #14)
> I would prefer that the warning consider arr.length != 0 more than
> std.array.empty. Nothing else. Hope you understand me now.
Suggesting people to use length is not a good idea. For that semantics there is empty that is safer and more clean, and it's the preferred idiom in D code.
(I have an enhancement request to extend the "empty" to work on associative arrays too, Issue 6409 ).
The problem yebblies is explaining is that the code going to be disallowed is "if(arr)" that doesn't have the same semantics as "if(!arr.empty)", despite the programmer often meant that semantic.
"if(arr)" is semantically unclean, that's why I have written this tiny breaking enhancement request to disallow it. And it's right such semantic confusion that makes it harder to write a good warning message for this.
"converting dynamic arrays to boolean is deprecated, instead use std.array.empty or arr.ptr"
That message confuses a bit the "std.array" module with the "array" as the data to work on. So an alternative is to write the name of the real array used in the code:
int[] foo;
if (foo) {}
==>
"converting dynamic arrays to boolean is deprecated, instead use std.array.empty or foo.ptr"
I think this is the best simple warning I can think of for now.
Comment #17 by rswhite4 — 2013-11-27T03:19:55Z
(In reply to comment #16)
> (In reply to comment #14)
>
> > I would prefer that the warning consider arr.length != 0 more than
> > std.array.empty. Nothing else. Hope you understand me now.
>
> Suggesting people to use length is not a good idea. For that semantics there is
> empty that is safer and more clean, and it's the preferred idiom in D code.
Not in any D code I've written or I've seen so far. But thanks for explanation. I use already arr.length != 0 anyway. :)
> (I have an enhancement request to extend the "empty" to work on associative
> arrays too, Issue 6409 ).
Comment #18 by code — 2013-11-30T08:50:56Z
(In reply to comment #10)
> (In reply to comment #8)
> > Or simply: use arr.length != 0
>
> That is not the same as `if (arr)`, and I don't want this to make people
> accidentally break their code.
The difference between !!arr.ptr and !!arr.length is always a bug,
because accessing an array with non-null ptr but zero length is a range error.
Comment #19 by bugzilla — 2013-12-09T22:21:05Z
1. making this change will break existing code
2. forcing people to use if(arr.ptr) instead is just ugly
Comment #20 by bearophile_hugs — 2013-12-10T03:53:21Z
(In reply to comment #19)
> 1. making this change will break existing code
Yes, it's one of the small breaking changes, this one I have asked since 2010-08-26. There are about ten more tiny breaking changes I am asking since lot of time in Bugzilla. Do we want to close them all as WONTFIX? Having similar warts in D is not good.
> 2. forcing people to use if(arr.ptr) instead is just ugly
It's not something you do often. I don't think I have 1 case in my code where I need to do this. In 99% of the D code using ".empty" or even ".length == 0" is more clear and less dangerous (because it's more clear). The problems with finding a good error message show the old situation is not clear.
The code being a tiny bit ugly could be an advantage if it helps show unusual (low-level) qualities of the code. Even using cast(int) is a little ugly compared to (cast) but that was done for some practical reasons. Using ".ptr" shows it's special code, while using empty or length shows it's normal D user code fit for @safe functions too.
The idea that instances int[] are "pointers" is not a good idea. They are at best fat pointers, and conflating the idea of pointer with the idea of dynamic array was a design mistake that we are slowly fixing with tiny/small breaking changes. This ER is another step in that direction.
Comment #21 by bearophile_hugs — 2013-12-10T05:24:44Z
(In reply to comment #20)
> Yes, it's one of the small breaking changes, ...
As usual, sorry for the little aggressive tone of some of my answers, Walter.
Comment #22 by bearophile_hugs — 2013-12-10T05:51:18Z
> It's not something you do often. I don't think I have 1 case in my code where I
> need to do this.
I have few usages of array.ptr in my D code: for variable length structs that contain a zero-length array, for interfacing with C code, etc, but none of them come from "if(array)".
Comment #23 by hsteoh — 2013-12-10T12:30:31Z
(In reply to comment #19)
> 1. making this change will break existing code
Unclear code *should* be broken.
> 2. forcing people to use if(arr.ptr) instead is just ugly
Yes, they should use if(!arr.empty) or if(arr.length>0) instead.
Comment #24 by code — 2013-12-14T02:12:58Z
Let me restate the obvious. The following code has a bug.
if (arr)
writeln(arr[0]);
I don't understand why this causes so much discussion.
> making this change will break existing code
Making this change will find broken code.
Please come up with a valid example.
Comment #25 by code — 2013-12-14T02:13:17Z
Let me restate the obvious. The following code has a bug.
if (arr)
writeln(arr[0]);
I don't understand why this causes so much discussion.
> making this change will break existing code
Making this change will find broken code.
Please come up with a valid example.
Comment #26 by code — 2013-12-14T02:15:34Z
Let me restate the obvious. The following code has a bug.
if (arr)
writeln(arr[0]);
I don't understand why this causes so much discussion.
> making this change will break existing code
Making this change will find broken code.
Please come up with a valid example.
Comment #27 by github-bugzilla — 2013-12-15T08:53:54Z
I think it's pretty safe to say, this will not be fixed. I can't see Walter/Andrei going back on this revert. Better to not have open bugs that will never be resolved, even if we don't agree with the outcome.
Comment #36 by dlang-bugzilla — 2017-07-03T19:17:27Z
*** Issue 7539 has been marked as a duplicate of this issue. ***
Comment #37 by github-bugzilla — 2017-07-19T17:38:29Z
For the record, this post described why the revert happened:
https://forum.dlang.org/post/[email protected]
"The main reason why it caused issues is this nice idiom:
if(auto arr = someFunction())
{
// use arr
}
This would HAVE to be split out to two statements, and the arr variable would be scoped outside of the if statement."
I attempted to make it obsolete:
https://github.com/dlang/dmd/pull/15413
But that was rejected. I still think it would be good to do for editions. If that is still a no-go then it seems we need something like:
if ((auto arr = expr).ptr)
However, would `arr` then be declared in the `else` branch? If so that is still not a replacement for the feature.
Another option would be to allow `if (auto arr = expr)` but allow no other uses of an array as a boolean.
Comment #39 by dfj1esp02 — 2023-11-22T09:27:07Z
AIU some believe it's a feature, not a bug? Then it becomes a style rule. Editions are only supposed for consensus.
Comment #40 by nick — 2023-11-22T09:54:44Z
Isn't there a consensus that the feature is bug prone (at the very least for new users)?
Comment #41 by schveiguy — 2023-11-22T13:52:39Z
I would vote for `if(arr)` to be true only if length > 0, instead of making it illegal.
But some people do distinguish between null arrays and empty arrays, and this would be a breaking change for that code.
> I still think it would be good to do for editions.
Yes, please! If editions ever becomes a thing, this should be considered.
Comment #42 by snarwin+bugzilla — 2023-11-26T13:18:46Z
> If that is still a no-go then it seems we need something like:
>
> if ((auto arr = expr).ptr)
In C++17, you can write it like this:
if (auto arr = expr; arr.ptr)
Comment #43 by nick — 2024-11-29T14:00:06Z
kdevel adds detail that `if (arr.ptr)` and `if (arr)` are not the same! The difference is significant for typeid(T).initializer, which has a null pointer but non-zero length:
https://forum.dlang.org/post/[email protected]
Note that `if (arr !is null)` is equivalent to `if (arr)`. Those check both the .ptr and .length fields (i.e. `is null` means exactly the same as `is []`).
Comment #44 by snarwin+bugzilla — 2024-11-29T14:19:33Z
(In reply to Nick Treleaven from comment #43)
> Note that `if (arr !is null)` is equivalent to `if (arr)`. Those check both
> the .ptr and .length fields (i.e. `is null` means exactly the same as `is
> []`).
To be 100% explicit: both `if (arr !is null)` and `if (arr)` are equivalent to `if (a.ptr || a.length)`.