Is this supposed to be allowed:

ubyte[] a;
ubyte[16] b;
a = b;
assert(a.ptr == b.ptr);

Because if so that makes it terribly easy to do a bug like this (as I just saw in IRC):

struct A
{
    ubyte[] a;
    this(ubyte c)
    {
        ubyte[16] b;
        b[] = c;
        this.a = b;  // a now points at an immediately invalid static array
    }
}

For reference, here was what user soos on IRC was doing that caused him to hit this <http://dpaste.dzfl.pl/08ee7b76#>:

import std.digest.md;
import std.stdio;

struct Hash {
  ubyte[] hash1;
  ubyte[] hash2;

  this (string str) {
    auto md5 = new MD5Digest();
    this.hash1 = md5.digest(str);
    this.hash2 = digest!MD5(str);
  }
};

void main() {
  auto h = Hash("hello world!");
  writeln(toHexString(h.hash1));
  writeln(toHexString(h.hash2));
}


It's not obvious at all what the problem is.

On Thursday, April 18, 2013 23:06:32 Brad Anderson wrote:
> Is this supposed to be allowed:
> 
> ubyte[] a;
> ubyte[16] b;
> a = b;
> assert(a.ptr == b.ptr);
> 
> Because if so that makes it terribly easy to do a bug like this
> (as I just saw in IRC):
> 
> struct A
> {
> ubyte[] a;
> this(ubyte c)
> {
> ubyte[16] b;
> b[] = c;
> this.a = b; // a now points at an immediately invalid
> static array
> }
> }

Yes, that's legal, though it should arguably be @system, since it's doing essentially the same thing as

int i;
int* p = &i;

which _is_ @system. The compiler doesn't currently consider slicing a static array in general to be @system though, much as it should ( http://d.puremagic.com/issues/show_bug.cgi?id=8838 ). I could see an argument that it should have to be

a = b[];

so that the slicing is explicit instead of just

a = b;

where it's implicit, but AFAIK, that's not currently required. You have to be very careful when slicing static arrays. If it were up to me, exlicit slicing would _always_ be required when slicing a static array, even when calling functions which take a dynamic array, but that's not how it works unfortunately.

- Jonathan M Davis

On 04/18/2013 02:06 PM, Brad Anderson wrote:
> Is this supposed to be allowed:
>
> ubyte[] a;
> ubyte[16] b;
> a = b;
> assert(a.ptr == b.ptr);
>
> Because if so that makes it terribly easy to do a bug like this (as I
> just saw in IRC):
>
> struct A
> {
>      ubyte[] a;
>      this(ubyte c)
>      {
>          ubyte[16] b;
>          b[] = c;
>          this.a = b;  // a now points at an immediately invalid static
> array
>      }
> }

There is a similar problem with the automatically generated array arguments.

The following constructor takes any number of ints that come in array form:

import std.stdio;

struct S
{
    int[] a;

    this(int[] args...)
    {
        a = args;
    }

    void foo()
    {
        writeln(a);
    }
}

void main()
{
    S[] a;

    foreach (i; 0 .. 2) {
        a ~= S(i, i, i);  // <-- WARNING temporary array
    }

    foreach (e; a) {
        e.foo();
    }
}

The program prints the following because the temporary arrays that are generated when calling the constructors are long gone:

[1, 1, 1]
[1, 1, 1]

The programmer *may have* ;) expected the following output:

[1, 1, 1]
[2, 2, 2]

Ali

Brad Anderson:

> Is this supposed to be allowed:
>
> ubyte[] a;
> ubyte[16] b;
> a = b;
> assert(a.ptr == b.ptr);

Yes, this is supposed to be allowed with the current design of D. But with the latest dmd 2.063alpha that code doesn't work, see below.

The Rust language removes this source of bugs because it manages accurately the memory zones. D will probably improve its error detection capabilities for such simple cases, but I think it will not solve this problem in general, unless it improves its type system, similarly to Rust.


> import std.digest.md;
> import std.stdio;
>
> struct Hash {
>   ubyte[] hash1;
>   ubyte[] hash2;
>
>   this (string str) {
>     auto md5 = new MD5Digest();
>     this.hash1 = md5.digest(str);
>     this.hash2 = digest!MD5(str);
>   }
> };
>
> void main() {
>   auto h = Hash("hello world!");
>   writeln(toHexString(h.hash1));
>   writeln(toHexString(h.hash2));
> }
>
>
> It's not obvious at all what the problem is.

Now that code gives a warning:

temp.d(11): Warning: explicit slice assignment this.hash2 = (digest(str))[] is better than this.hash2 = digest(str)

(This is the result of a small battle I have started years ago that is now half work, thanks to the work of Hara implementing my enhancement request. I am glad to see our time was not wasted.)

To remove that warning you have to write:

this.hash1 = md5.digest(str);
this.hash2 = digest!MD5(str)[];

Now it's visible that for hash2 you are slicing something that's probably a fixed-sized array. This gives you a significant help in understanding what's going on.

Bye,
bearophile

Jonathan M Davis:

> I could see an argument that it should have to be
>
> a = b[];
>
> so that the slicing is explicit instead of just
>
> a = b;
>
> where it's implicit, but AFAIK, that's not currently required.

It's currently a warning, and it will be required.

--------------------------

Ali Çehreli:

> There is a similar problem with the automatically generated array arguments.
>
> The following constructor takes any number of ints that come in array form:
>
> import std.stdio;
>
> struct S
> {
>     int[] a;
>
>     this(int[] args...)
>     {
>         a = args;
>     }
>
>     void foo()
>     {
>         writeln(a);
>     }
> }
>
> void main()
> {
>     S[] a;
>
>     foreach (i; 0 .. 2) {
>         a ~= S(i, i, i);  // <-- WARNING temporary array

This is a known problem, I have put it in Bugzilla since lot of time and it seems there are various persons that agree with me and you on it. So hopefully this source of bugs will be removed, allocating that data on the heap.

Bye,
bearophile

On Thursday, 18 April 2013 at 21:45:56 UTC, bearophile wrote:
> Yes, this is supposed to be allowed with the current design of D. But with the latest dmd 2.063alpha that code doesn't work, see below.
>
> [snip]
>
> Now that code gives a warning:
>
> temp.d(11): Warning: explicit slice assignment this.hash2 = (digest(str))[] is better than this.hash2 = digest(str)
>
> (This is the result of a small battle I have started years ago that is now half work, thanks to the work of Hara implementing my enhancement request. I am glad to see our time was not wasted.)
>
> To remove that warning you have to write:
>
> this.hash1 = md5.digest(str);
> this.hash2 = digest!MD5(str)[];
>
> Now it's visible that for hash2 you are slicing something that's probably a fixed-sized array. This gives you a significant help in understanding what's going on.
>

That's good to hear, Bearophile. Thanks Kenji.

> There is a similar problem with the automatically generated array arguments.
>
> The following constructor takes any number of ints that come in array form:
>
> import std.stdio;
>
> struct S
> {
>      int[] a;
>
>      this(int[] args...)
>      {
>          a = args;
>      }
>
>      void foo()
>      {
>          writeln(a);
>      }
> }
>
> void main()
> {
>      S[] a;
>
>      foreach (i; 0 .. 2) {
>          a ~= S(i, i, i);  // <-- WARNING temporary array
>      }
>
>      foreach (e; a) {
>          e.foo();
>      }
> }
>
> The program prints the following because the temporary arrays that are generated when calling the constructors are long gone:
>
> [1, 1, 1]
> [1, 1, 1]
>
> The programmer *may have* ;) expected the following output:
>
> [1, 1, 1]
> [2, 2, 2]

There is no guarantee that the incoming array from a variadic function is heap-based.

But an interesting way to deal with it is that you can overload with an explicit slice parameter, and the variadic version will ONLY bind to a variadic call.

For example, in dcollections' ArrayList I have two constructors:

   /**
     * Create an array list based on a number of elements.
     */
    this(V[] elems...)
    {
        _array = elems.dup;
    }

    /**
     * Use an array as the backing storage.  This does not duplicate the
     * array.  Use new ArrayList(storage.dup) to make a distinct copy.  Beware
     * of using a stack-based array when using this constructor!
     */
    this(V[] storage)
    {
        _array = storage;
    }

The first version binds only to cases where the parameter is not *explicitly* a slice, so I am guaranteed that the array is allocated on the stack!  The second binds to slices, and I assume from my comment, fixed-sized arrays (been a while since I looked at that code).

And the correct expectation for your code should be:

[0, 0, 0]
[1, 1, 1]

:)

-Steve

Steven Schveighoffer:

> There is no guarantee that the incoming array from a variadic function is heap-based.
>
> But an interesting way to deal with it is that you can overload with an explicit slice parameter, and the variadic version will ONLY bind to a variadic call.
>
> For example, in dcollections' ArrayList I have two constructors:
>
>    /**
>      * Create an array list based on a number of elements.
>      */
>     this(V[] elems...)
>     {
>         _array = elems.dup;
>     }
>
>     /**
>      * Use an array as the backing storage.  This does not duplicate the
>      * array.  Use new ArrayList(storage.dup) to make a distinct copy.  Beware
>      * of using a stack-based array when using this constructor!
>      */
>     this(V[] storage)
>     {
>         _array = storage;
>     }

To avoid those bugs I have suggested the simpler possible thing: (V[] elems...) to dup the data on the heap every time. In theory if you write "(scope V[] elems...)" it will be free to not dup the data, avoiding the heap allocation and the associated little performance loss. In practice as you know "scope" is not yet implemented. D2 language is not close to being fully implemented.

Bye,
bearophile

On 04/18/2013 02:54 PM, Steven Schveighoffer wrote:

>> The program prints the following because the temporary arrays that are
>> generated when calling the constructors are long gone:
>>
>> [1, 1, 1]
>> [1, 1, 1]
>>
>> The programmer *may have* ;) expected the following output:
>>
>> [1, 1, 1]
>> [2, 2, 2]
>
> There is no guarantee that the incoming array from a variadic function
> is heap-based.
>
> But an interesting way to deal with it is that you can overload with an
> explicit slice parameter, and the variadic version will ONLY bind to a
> variadic call.

Interesting.

Personally, I would not even bother with the second one and expect the caller to simply put square brackets around the arguments:

import std.stdio;

struct S
{
    int[] a;

    this(int[] args)  // <-- now requires a slice
    {
        a = args;
    }

    void foo()
    {
        writeln(a);
    }
}

void main()
{
    S[] a;

    foreach (i; 0 .. 2) {
        a ~= S([i, i, i]); // <-- minor inconvenience
    }

    foreach (e; a) {
        e.foo();
    }
}

It is now safe, right?

> And the correct expectation for your code should be:
>
> [0, 0, 0]
> [1, 1, 1]
>
> :)
>
> -Steve

Wow! I made an off-by-6 error there. :)

Ali