Bug 11343 – [2.064 beta] Error: multiple field initialization

Status
RESOLVED
Resolution
INVALID
Severity
regression
Priority
P2
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2013-10-24T12:56:00Z
Last change time
2013-10-30T16:49:40Z
Assigned to
nobody
Creator
rswhite4

Comments

Comment #0 by rswhite4 — 2013-10-24T12:56:06Z
Code: ---- import std.stdio; enum Foo { A = 1, B = 2 } struct Test { public: const ubyte[Foo] test; this(Foo f) { if (Foo.A & f) this.test[Foo.A] = 1; if (Foo.B & f) this.test[Foo.B] = 2; } } void main() { Test t = Test(Foo.A); } ---- Flag_TEst.d(16): Error: multiple field test initialization Tested with the latest beta. Worked in 2.063.2
Comment #1 by rswhite4 — 2013-10-24T14:02:05Z
Consider also this: ---- import std.stdio; struct Test { public: const int[] test; this(int i) { this.test ~= i; this.test ~= i + 1; } } void main() { Test t = Test(42); } ---- Same error And maybe related: http://forum.dlang.org/thread/[email protected]%2Fissues%2F
Comment #2 by k.hara.pg — 2013-10-24T17:18:06Z
(In reply to comment #0) > Code: > ---- > import std.stdio; > > enum Foo { > A = 1, > B = 2 > } > > struct Test { > public: > const ubyte[Foo] test; > > this(Foo f) { > if (Foo.A & f) > this.test[Foo.A] = 1; > if (Foo.B & f) > this.test[Foo.B] = 2; > } > } > > void main() { > Test t = Test(Foo.A); > } > ---- > > Flag_TEst.d(16): Error: multiple field test initialization > > Tested with the latest beta. > Worked in 2.063.2 It's intended behavior change introduced by fixing bug 9665. Non-mutable field initialization is now allowed just only once. So, in this case, you need to create a temporary mutable AA, then initialize Test.test by that. struct Test { public: const ubyte[Foo] test; this(Foo f) { ubyte[Foo] tmp; if (Foo.A & f) tmp[Foo.A] = 1; if (Foo.B & f) tmp[Foo.B] = 2; this.test = tmp; } }
Comment #3 by k.hara.pg — 2013-10-24T17:22:40Z
(In reply to comment #1) > Consider also this: > > ---- > import std.stdio; > > struct Test { > public: > const int[] test; > > this(int i) { > this.test ~= i; > this.test ~= i + 1; > } > } > > void main() { > Test t = Test(42); > } > ---- > > Same error The code modifies a non-mutable array field 'const(int[]) test' twice. Create temporary int[] and initialize Test.test by that.
Comment #4 by rswhite4 — 2013-10-25T00:01:24Z
After my imagination, a constant member can be assigned as many times as I want, as long as it happens in the constructor. This is only logical. Everything else just makes it complex and completely non intuitive, especially for newbies.
Comment #5 by k.hara.pg — 2013-10-25T00:52:00Z
(In reply to comment #4) > After my imagination, a constant member can be assigned as many times as I > want, as long as it happens in the constructor. This is only logical. > Everything else just makes it complex and completely non intuitive, especially > for newbies. A const reference may refer immutable data which comes from the out of the constructor. If so, modifying const data inside constructor is definitely illegal operation. By disallowing multiple initialization, compiler can validate the field initializing correctness or not, at the first initializing place. ==== Additionally, it is necessary to make immutable field data initialization more flexible. Today, if a pure function returns mutable data, it could be implicitly convertible to immutable data. int[] make(int n) pure { int[] a = new int[](n); foreach (i; 0 .. n) a[i] = i + 1; return a; // [1, 2, ..., n] } immutable int[] arr = make(n); // OK From 2.064, by fixing issue 9665, compiler can detect the only once initializing point for the object fields. Combine pure function, you can initialize immutable data field as follows. struct S { immutable int[] arr; this(int n) { static int[] make(int n) pure { int[] a = new int[](n); foreach (i; 0 .. n) a[i] = i + 1; return a; // [1, 2, ..., n] } arr = make(n); // This line is now initialization (not assignment), so // conversion from mutable to immutable is allowed } } void main() { auto s = S(10); assert(s.arr[0] == 0); assert(s.arr[$-1] == 10); static assert(is(typeof(s.arr) == immutable int[])); }
Comment #6 by k.hara.pg — 2013-10-25T02:08:50Z
Mark as invalid, as same as bug 11346.
Comment #7 by bearophile_hugs — 2013-10-28T15:19:49Z
(In reply to comment #5) > A const reference may refer immutable data which comes from the out of the > constructor. If so, modifying const data inside constructor is definitely > illegal operation. By disallowing multiple initialization, compiler can > validate the field initializing correctness or not, at the first initializing > place. To reduce a little the disruption of formerly compilable code is it possible and a good idea to make the detector smarter, and allow code like this? struct Foo { immutable int[3] arr; this(int x) { arr[0] = x; arr[1] = x + 1; arr[2] = x + 2; } } void main() { auto f = Foo(5); } Currently it gives: test.d(5): Error: multiple field arr initialization test.d(6): Error: multiple field arr initialization The idea is to see and track every cell of a fixed-size array as an independent variable, like this, that compiles: struct Foo { immutable int arr0, arr1, arr2; this(int x) { arr0 = x; arr1 = x + 1; arr2 = x + 2; } } void main() { auto f = Foo(5); } If this is possible and good, then I could open an enhancement request on this. (Once that's possible, in future we could even allow some simple forms of initialization in loops, but that's for later).
Comment #8 by k.hara.pg — 2013-10-29T18:51:45Z
(In reply to comment #7) > To reduce a little the disruption of formerly compilable code is it possible > and a good idea to make the detector smarter, and allow code like this? > > struct Foo { > immutable int[3] arr; > this(int x) { > arr[0] = x; > arr[1] = x + 1; > arr[2] = x + 2; > } > } I think this is difficult if one of the index is given in runtime. For example: this(int x, size_t i) { arr[0] = x; arr[1] = x + 1; arr[i] = x + 2; // which index will be initialized in runtime? } Once arr[i] is used inside constructor, compiler should reject *all other* initializing of arr (arr[0]=x; and arr[1]=x+1;) to avoid multiple initialization. I think there's not benefits commensurate with the cost to implement it. So I won't work for that. Instead, I'd like to recommend an alternative way how to initialize immutable int[3] field. Today, NRVO also works for static array data, and you can convert mutable data to immutable implicitly by using pure *creator* function. this(int x) { static int[3] make(int x) pure { int[3] a; a[0] = x; a[1] = x + 1; a[2] = x + 2; } this.arr = make(x); // NRVO works } In near the future, you will be able to use a pure lambda instead of the static 'make' function.
Comment #9 by rswhite4 — 2013-10-30T01:24:58Z
This is annoying. If I want to initialize an const array I must write a new function? No way. The easier way (and I'm sure the most users will do this) is to get rid of const. That change may guarantee more safety but it will also lead that const is used less.
Comment #10 by rswhite4 — 2013-10-30T01:44:17Z
Couldn't the compiler to this dirty trick for me? If he found such multiple init case, he could rewrite the code. So that this code: ---- this(int x, size_t i) { arr[0] = x; arr[1] = x + 1; arr[i] = x + 2; // which index will be initialized in runtime? } ---- if rewritten to this: this(int x, size_t i) { Unqual!typeof(arr) __arr = null; __arr[0] = x; __arr[1] = x + 1; __arr[i] = x + 2; this.arr = __arr; /// only _one_ assignment }
Comment #11 by k.hara.pg — 2013-10-30T01:49:54Z
(In reply to comment #10) > Couldn't the compiler to this dirty trick for me? > If he found such multiple init case, he could rewrite the code. So that this > code: > ---- > this(int x, size_t i) { > arr[0] = x; > arr[1] = x + 1; > arr[i] = x + 2; // which index will be initialized in runtime? > } > ---- > > if rewritten to this: > > this(int x, size_t i) { > Unqual!typeof(arr) __arr = null; > > __arr[0] = x; > __arr[1] = x + 1; > __arr[i] = x + 2; > > this.arr = __arr; /// only _one_ assignment > } That would be a bad behavior when the size of arr field is quite big, because it will silently consume stack memory. I can believe someone will hate it.
Comment #12 by rswhite4 — 2013-10-30T01:52:32Z
(In reply to comment #11) > (In reply to comment #10) > > Couldn't the compiler to this dirty trick for me? > > If he found such multiple init case, he could rewrite the code. So that this > > code: > > ---- > > this(int x, size_t i) { > > arr[0] = x; > > arr[1] = x + 1; > > arr[i] = x + 2; // which index will be initialized in runtime? > > } > > ---- > > > > if rewritten to this: > > > > this(int x, size_t i) { > > Unqual!typeof(arr) __arr = null; > > > > __arr[0] = x; > > __arr[1] = x + 1; > > __arr[i] = x + 2; > > > > this.arr = __arr; /// only _one_ assignment > > } > > That would be a bad behavior when the size of arr field is quite big, because > it will silently consume stack memory. I can believe someone will hate it. Who? It's the best approach I have currently. There must be a way to weaken the effects of this change.
Comment #13 by bearophile_hugs — 2013-10-30T02:43:17Z
(In reply to comment #8) > this(int x) { > static int[3] make(int x) pure { > int[3] a; > a[0] = x; > a[1] = x + 1; > a[2] = x + 2; > } > this.arr = make(x); // NRVO works > } A little test program: struct Foo { int[3] arr; this(int x) pure nothrow { arr[0] = x; arr[1] = x + 1; arr[2] = x + 2; } } struct Bar { immutable int[3] arr; this(int x) pure nothrow { static int[3] make(in int x) pure nothrow { typeof(return) a; a[0] = x; a[1] = x + 1; a[2] = x + 2; return a; } this.arr = make(x); // NRVO works } } void main() { auto f = Foo(5); auto b = Bar(5); } The asm of its struct constructors using dmd: dmd -O -release -inline -noboundscheck test.d Foo.__ctor: push EBX mov EDX, EAX mov EBX, 8[ESP] push ESI lea ECX, 1[EBX] lea ESI, 2[EBX] mov [EDX], EBX mov 4[EDX], ECX mov 8[EDX], ESI pop ESI pop EBX ret 4 Bar.__ctor: sub ESP, 010h xor EDX, EDX push EBX mov EBX, 018h[ESP] push ESI lea ESI, 1[EBX] push EDI lea EDI, 2[EBX] mov 018h[ESP], EAX push 0Ch lea ECX, 010h[ESP] mov [ECX], EDX mov 4[ECX], EDX mov 8[ECX], EDX mov 010h[ESP], EBX mov 014h[ESP], ESI mov 018h[ESP], EDI push ECX push EAX call near ptr _memcpy add ESP, 0Ch mov EAX, 018h[ESP] pop EDI pop ESI pop EBX add ESP, 010h ret 4
Comment #14 by k.hara.pg — 2013-10-30T03:00:47Z
(In reply to comment #12) > Who? Embedded system programmers, for example. --- We can add some acceptable cases certainly. For example, by the following rule: "If the static array field is initialized 1. with different compile-time constant indices, and 2. each initializing expressions does not have overlapping with each other over the whole control-flow paths in the constructor" the bearophile's case may be allowed. But, it's very complex and heuristic rule, and will covers just only a few cases. We must reject heuristic rule, because it is hard to maintain and verify language semantics. That's the matter of cost-benefit ratio.
Comment #15 by k.hara.pg — 2013-10-30T03:12:27Z
(In reply to comment #13) > (In reply to comment #8) > > A little test program: [snip] Hmm, strange. After moving out the nested static function 'make' to module level, I got this generated code. struct Foo { int[3] arr; this(int x) pure nothrow { arr[0] = x; arr[1] = x + 1; arr[2] = x + 2; } } // move out to module level static int[3] make(in int x) pure nothrow { typeof(return) a; a[0] = x; a[1] = x + 1; a[2] = x + 2; return a; } struct Bar { immutable int[3] arr; this(int x) pure nothrow { this.arr = make(x); // NRVO works? } } void main() { auto f = Foo(5); auto b = Bar(5); } Assembly: C:\Users\khara\d\test.d:23 void main() { 0040209c: 83ec24 sub esp, 0x24 0040209f: 31c9 xor ecx, ecx 004020a1: ba05000000 mov edx, 0x5 004020a6: 53 push ebx 004020a7: bb06000000 mov ebx, 0x6 004020ac: 56 push esi C:\Users\khara\d\test.d:24 auto f = Foo(5); 004020ad: 8d442408 lea eax, [esp+0x8] 004020b1: be07000000 mov esi, 0x7 004020b6: 8908 mov [eax], ecx 004020b8: 894804 mov [eax+0x4], ecx 004020bb: 894808 mov [eax+0x8], ecx C:\Users\khara\d\test.d:4 arr[0] = x; 004020be: 8d442414 lea eax, [esp+0x14] 004020c2: 89542408 mov [esp+0x8], edx C:\Users\khara\d\test.d:5 arr[1] = x + 1; 004020c6: 31d2 xor edx, edx 004020c8: 895c240c mov [esp+0xc], ebx C:\Users\khara\d\test.d:6 arr[2] = x + 2; 004020cc: 89742410 mov [esp+0x10], esi C:\Users\khara\d\test.d:25 auto b = Bar(5); 004020d0: 6a0c push 0xc C:\Users\khara\d\test.d:20 this.arr = make(x); // NRVO works? 004020d2: 8908 mov [eax], ecx 004020d4: 894804 mov [eax+0x4], ecx 004020d7: 894808 mov [eax+0x8], ecx 004020da: 8d4c2424 lea ecx, [esp+0x24] 004020de: 8911 mov [ecx], edx 004020e0: 895104 mov [ecx+0x4], edx 004020e3: 895108 mov [ecx+0x8], edx C:\Users\khara\d\test.d:11 a[0] = x; 004020e6: c744242405000000 mov dword [esp+0x24], 0x5 C:\Users\khara\d\test.d:12 a[1] = x + 1; 004020ee: 895c2428 mov [esp+0x28], ebx C:\Users\khara\d\test.d:13 a[2] = x + 2; 004020f2: 8974242c mov [esp+0x2c], esi C:\Users\khara\d\test.d:10 typeof(return) a; 004020f6: 51 push ecx 004020f7: 50 push eax 004020f8: e8e3110100 call 0x4132e0 _memmove // still exists...! 004020fd: 83c40c add esp, 0xc C:\Users\khara\d\test.d:26 } 00402100: 31c0 xor eax, eax 00402102: 5e pop esi 00402103: 5b pop ebx 00402104: 83c424 add esp, 0x24 00402107: c3 ret In 2.064a, the bug 10094 has been fixed. However we need to fix these (at least two orthogonal) bugs for the next release. I'll open new issues for them.
Comment #16 by bearophile_hugs — 2013-10-30T16:49:40Z
(In reply to comment #15) > In 2.064a, the bug 10094 has been fixed. However we need to fix these (at least > two orthogonal) bugs for the next release. > > I'll open new issues for them. Something perhaps related: import std.algorithm: copy; immutable int[2] data; static this() { [10, 20].copy(data[]); // Error. } void main() {} Thread: http://forum.dlang.org/thread/[email protected]