Issue 7444 - Require [] for array copies too
Summary: Require [] for array copies too
Status: REOPENED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P4 enhancement
Assignee: No Owner
URL:
Keywords: pull
Depends on:
Blocks:
 
Reported: 2012-02-05 11:10 UTC by bearophile_hugs
Modified: 2024-07-19 17:31 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description bearophile_hugs 2012-02-05 11:10:57 UTC
This is derived from issue 3971 see there for more (messy) discussions.

In 2.058 this compiles:


int[100] foo() {
    int[100] a;
    return a;
}
void main() {
    int[10_000] a, b;
    auto c = new int[10_000];
    a = 1;
    a = b;
    a = c;
    auto d = foo();
}


But for consistency of the vector ops syntax, and to help the programmer spot where the code is doing a potentially long copy, I suggest to turn that first program into a compile-time error, and require [] on lvalues everywhere you perform a vector operation, like when you copy a int[N] on another int[N], etc:


int[100] foo() {
    int[100] a;
    return a;
}
void main() {
    int[10_000] a, b;
    auto c = new int[10_000];
    a[] = 1;
    a[] = b;
    a[] = c;
    auto d[] = foo(); // currently an error
}

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

But note that normally all array ops require a [] even on the rvalue:


void main() {
    int[10_000] a, b;
    a[] += b[];
}



So an alternative stricter proposal is to require [] on the right too:


int[100] foo() {
    int[100] a;
    return a;
}
void main() {
    int[10_000] a, b;
    auto c = new int[10_000];
    auto d = new int[10_000];
    a[] = 1;
    a[] = b[];
    a[] = c[];
    c[] = d[];
    auto e[] = foo()[]; // currently an error
}



It helps the programmer tell apart two different cases, that currently are usable with the same syntax (from an example by Don):


void main() {
    int[][3] x;
    int[]    y;
    int[]    z;

    x[] = z; // copies just the z pointer
    y[] = z; // copies the elements in z
}



Requiring [] on the right (rvalue) for the copy of many items avoids the ambiguity:

void main() {
    int[][3] x;
    int[]    y;
    int[]    z;
    x[] = z;
    y[] = z[];
}
Comment 1 timon.gehr 2012-02-05 15:34:04 UTC
While I agree that the syntax should be enforced more strictly in general, I still completely disagree with requiring [] on static array copies. Static arrays are value types of fixed size, and should be treated as such. Requiring [] is just wrong.

void foo(int[4] x){}
void foo(int[] y){}

void main(){
    int[4] x, y;
    struct S{int[4] x;}
    S a, b;
    x = y;
    a = b; // why should this work if the above does not?
    foo(x);   // copies, you want this to be an error
    foo(x[]); // calls the other overload, does not copy
}
Comment 2 Kenji Hara 2012-02-09 05:33:52 UTC
(In reply to comment #1)
> While I agree that the syntax should be enforced more strictly in general, I
> still completely disagree with requiring [] on static array copies. Static
> arrays are value types of fixed size, and should be treated as such. Requiring
> [] is just wrong.
> 
> void foo(int[4] x){}
> void foo(int[] y){}
> 
> void main(){
>     int[4] x, y;
>     struct S{int[4] x;}
>     S a, b;
>     x = y;
>     a = b; // why should this work if the above does not?
>     foo(x);   // copies, you want this to be an error
>     foo(x[]); // calls the other overload, does not copy
> }

I completely agree with Timon.
When x and y are declared with same type T, x = y is identity assignment, and it is valid syntax even if T is static array type.
Comment 3 Kenji Hara 2012-02-09 06:31:34 UTC
I'd like to provide an exhaustive test that should compile after fixing the enhancement suggested by me and timon.

----
// X: Changed accepts-invalid to rejects-invalid by this issue
// a: slice assginment
// b: element-wise assignment

static assert(!__traits(compiles, { sa   = e; }));      // X
static assert( __traits(compiles, { sa[] = e; }));      // b
static assert(!__traits(compiles, { da   = e; }));
static assert( __traits(compiles, { da[] = e; }));      // b

// lhs is static array
static assert( __traits(compiles, { sa   = sa;   }));   // b == identity assign
static assert(!__traits(compiles, { sa   = sa[]; }));   // X
static assert(!__traits(compiles, { sa[] = sa;   }));   // X
static assert( __traits(compiles, { sa[] = sa[]; }));   // b

static assert(!__traits(compiles, { sa   = da;   }));   // X
static assert(!__traits(compiles, { sa   = da[]; }));   // X
static assert( __traits(compiles, { sa[] = da;   }));   // b
static assert( __traits(compiles, { sa[] = da[]; }));   // b

// lhs is dynamic array
static assert(!__traits(compiles, { da   = sa;   }));   // X
static assert( __traits(compiles, { da   = sa[]; }));   // a
static assert(!__traits(compiles, { da[] = sa;   }));   // X
static assert( __traits(compiles, { da[] = sa[]; }));   // b

static assert( __traits(compiles, { da   = da;   }));   // a == identity assign
static assert( __traits(compiles, { da   = da[]; }));   // a
static assert( __traits(compiles, { da[] = da;   }));   // b
static assert( __traits(compiles, { da[] = da[]; }));   // b
----

bearophile says that sa = sa should be rejected and must replace it to sa[] = sa[].
But I and timon disagree it.
Comment 4 timon.gehr 2012-02-09 07:01:17 UTC
Maybe sa[] = da and da[] = da should be '// X' too.
Comment 5 Kenji Hara 2012-02-10 04:23:32 UTC
(In reply to comment #4)
> Maybe sa[] = da and da[] = da should be '// X' too.

Hmm, OK. If it is an element-wise assignment, we should add explicit slicing both side of AssignExp.

I've posted a pull to implement this enhancement:
https://github.com/D-Programming-Language/dmd/pull/702

Updated test:
https://github.com/D-Programming-Language/dmd/pull/702/files#L6R250
Comment 6 Walter Bright 2012-02-17 23:27:14 UTC
The enhancement is to disallow:

  sa = da; // use sa[] = da[] instead
  sa = e;  // use sa[] = e instead

I'm not sure this is worth the existing code breakage.
Comment 7 bearophile_hugs 2012-02-18 05:02:07 UTC
(In reply to comment #6)

> I'm not sure this is worth the existing code breakage.

I thin it's worth it, if you want with warning / deprecation / error stages.

But if want, I'll ask for a little voting in the main D newsgroup.
Comment 8 bearophile_hugs 2012-02-22 16:07:54 UTC
See why a consistent syntax matters: Issue 7564
Comment 9 github-bugzilla 2012-10-14 19:19:00 UTC
Commits pushed to master at https://github.com/D-Programming-Language/druntime

https://github.com/D-Programming-Language/druntime/commit/a77c82772e7e7b2d1d863b1fb56b614b9d4bc6a1
fix Issue 7444 - Require [] for array copies too

https://github.com/D-Programming-Language/druntime/commit/be3a7fa1bc726b453203c058ff2fa8c81dcfcab1
Merge pull request #314 from 9rnsr/fix7444

Supplemental changes for Issue 7444 - Require [] for array copies too
Comment 10 github-bugzilla 2012-11-20 09:55:06 UTC
Commits pushed to master at https://github.com/D-Programming-Language/phobos

https://github.com/D-Programming-Language/phobos/commit/28dedee456e741f02f08a944f41daa9e46236224
Issue 7444 - Require [] for array copies too

https://github.com/D-Programming-Language/phobos/commit/9a6dad8a2ac5841bdcfa2b86082450818f6eefab
Merge pull request #960 from 9rnsr/fix7444

Supplemental changes for Issue 7444 - Require [] for array copies too
Comment 11 github-bugzilla 2012-11-21 20:12:48 UTC
Commits pushed to master at https://github.com/D-Programming-Language/druntime

https://github.com/D-Programming-Language/druntime/commit/e5895640c83d8b5b4c8b2404b4e639ba6fdf2243
Additional fix Issue 7444 in Posix platforms

https://github.com/D-Programming-Language/druntime/commit/a54f41c98aa26eeb70274e8b78d8abfb575bcebc
Merge pull request #352 from 9rnsr/fix7444

Additional fix Issue 7444 in Posix platforms
Comment 12 github-bugzilla 2013-03-06 22:53:51 UTC
Commits pushed to master at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/57b770ed49379c5af726d23356e0f75818a3f859
Issue 7444 - Require [] for array copies too

https://github.com/D-Programming-Language/dmd/commit/ba1009c5561b51b8f18d9c869fde9bd45cb7ebc7
Merge pull request #702 from 9rnsr/fix7444

Issue 7444 - Require [] for array copies too
Comment 13 bearophile_hugs 2013-03-07 04:22:27 UTC
(In reply to comment #12)
> Commits pushed to master at https://github.com/D-Programming-Language/dmd
> 
> https://github.com/D-Programming-Language/dmd/commit/57b770ed49379c5af726d23356e0f75818a3f859
> Issue 7444 - Require [] for array copies too
> 
> https://github.com/D-Programming-Language/dmd/commit/ba1009c5561b51b8f18d9c869fde9bd45cb7ebc7
> Merge pull request #702 from 9rnsr/fix7444
> 
> Issue 7444 - Require [] for array copies too

I have tried this change, and now the first test case of this ER:


int[100] foo() {
    int[100] a;
    return a;
}
void main() {
    int[10_000] a, b;
    auto c = new int[10_000];
    a = 1;
    a = b;
    a = c;
    auto d = foo();
}



gives a ICE:

temp.d(8): Warning: explicit element-wise assignment (a)[] = 1 is better than a = 1
temp.d(10): Warning: explicit element-wise assignment (a)[] = (c)[] is better than a = c
Assertion failure: '0' on line 1193 in file 'glue.c'
Comment 14 bearophile_hugs 2013-03-07 04:58:19 UTC
(In reply to comment #13)

> gives a ICE:

Smaller test case:


void main() {
    int[1] a;
    a = 1;
}
Comment 15 Kenji Hara 2013-03-07 05:14:27 UTC
(In reply to comment #14)
> (In reply to comment #13)
> 
> > gives a ICE:
> 
> Smaller test case:
> 
> 
> void main() {
>     int[1] a;
>     a = 1;
> }

What version and compiler switch do you use? I cannot reproduce the ICE.
Comment 16 bearophile_hugs 2013-03-07 12:10:52 UTC
(In reply to comment #15)

> What version and compiler switch do you use? I cannot reproduce the ICE.

I am using the GIT head compiler, I have downloaded and compiled dmd few hours ago, after this patch was merged.

I am on Windows 32 bit, and I have compiled the code with:

dmd -wi test.d
Comment 17 Kenji Hara 2013-03-07 15:41:56 UTC
(In reply to comment #16)
> (In reply to comment #15)
> 
> > What version and compiler switch do you use? I cannot reproduce the ICE.
> 
> I am using the GIT head compiler, I have downloaded and compiled dmd few hours
> ago, after this patch was merged.
> 
> I am on Windows 32 bit, and I have compiled the code with:
> 
> dmd -wi test.d

Thanks. -w option does not reproduce it, but -wi does.
Comment 18 Kenji Hara 2013-03-07 17:00:31 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > 
> > > What version and compiler switch do you use? I cannot reproduce the ICE.
> > 
> > I am using the GIT head compiler, I have downloaded and compiled dmd few hours
> > ago, after this patch was merged.
> > 
> > I am on Windows 32 bit, and I have compiled the code with:
> > 
> > dmd -wi test.d
> 
> Thanks. -w option does not reproduce it, but -wi does.

OK, I opened a new report bug 9663 for the regression.
Comment 19 Walter Bright 2013-10-20 09:38:57 UTC
This pull:

https://github.com/D-Programming-Language/dmd/pull/2673

undoes it with explanation. Reclassified as "wontfix".
Comment 20 bearophile_hugs 2013-10-20 10:49:58 UTC
(In reply to comment #19)
> This pull:
> 
> https://github.com/D-Programming-Language/dmd/pull/2673
> 
> undoes it with explanation. Reclassified as "wontfix".

What? After all the energy time and work spent on fixing this problem!?

If you take a look at the patch by Kenji you see:

-  else if (global.params.warnings && !global.gag && op == TOKassign &&
+  else if (0 && global.params.warnings && !global.gag && op == TOKassign && 

That means the code is commented out, but it's meant to be activated back in dmd 2.065 to try to fix this issue better. I will probably re-open this issue later.
Comment 21 bearophile_hugs 2013-10-20 12:29:43 UTC
Reopened, this will wait for dmd 2.065, to see if Kenji or others are able to find a solution. If no solution will be found, we'll close this again.
Comment 22 anonymous4 2014-09-11 06:40:48 UTC
That's the reason for an unambiguous syntax a[*]=b[*];
Comment 23 Peter Alexander 2018-08-24 12:14:31 UTC
I just lost a bunch of time because of this issue (T[n] = e).

I understand this is a breaking change. Is there perhaps a deprecation path to solving this?
Comment 24 Nick Treleaven 2024-07-19 17:16:41 UTC
I think `[]` should be required at least for assigning a single element to a static array, like it is for a dynamic array.

Another issue is with an AA of static array key type:
https://forum.dlang.org/post/dokvozlfivgerkwhupvs@forum.dlang.org

// meant to create key "test" and copy slice as value
lookup["test"] = dynArray[0 .. $];
// instead causes a missing key error as it becomes
lookup["test"][0 .. $] = dynArray[0 .. $];
Comment 25 Nick Treleaven 2024-07-19 17:31:21 UTC
> Another issue is with an AA of static array key type:

Sorry, static array *value* type:

string[3][string] lookup;
string[] dynArray = ["d", "e", "f"];