Bug 6681 – struct constructor call is converted to struct literal that breaks union initialization

In D1 code like this {{{ module t; template MTuple( TList... ) { alias TList MTuple; } struct V{ union { double[2] cell; version(v2) {} else { MTuple!(double,double) tuple; } struct { union { double x; double r; } union { double y; double g; } } } static const V zero={x:0, y:1}; } V a=V.zero; version(v2) { struct Q { union { struct { double x, y; } V xyzw; } const static Q id = { x: 0, y:1 }; } Q b=Q.id; } }}} fails with duplicate union initialization, which is incorrect. This happens both with and without -version=v2 which shows that the error is not just the tuple. Closely related errors are present also in D2, even if one uses constructors: {{{ module t; template MTuple( TList... ) { alias TList MTuple; } struct V{ this(double a,double b){ x=a; y=b; } union { double[2] cell; version(v2) {} else { MTuple!(double,double) tuple; } struct { union { double x; double r; } union { double y; double g; } } } static immutable V zero=V(0,1); } V a=V.zero; version(v2) { struct Q { union { struct { double x, y; } V xyzw; } immutable static Q id =Q(0,1); } Q b=Q.id; } }}} similar errors seem to be very old: http://www.digitalmars.com/d/archives/digitalmars/D/bugs/6271.html and there are related or very similar errors are already present in bugzilla: http://d.puremagic.com/issues/show_bug.cgi?id=4241 which basically uses the same code as me (omg derived vector structs), but just complains about line number, seemingly accepting the error (which is bogus imho. I have also tried to sprinkle around some =void but I just managed to end up with "Error: no initializer for union". http://d.puremagic.com/issues/show_bug.cgi?id=1432 (using initializers in the union) but this one at least with D1 is a regression from 1.067 at least

There have been several patches to 'fix' struct/union initialization. Evidently, we need to step back a bit and rethink/reengineer it. Something along the lines of: 1. Create a list of all the fields, in lexical order. Each field will have a beginning offset and an ending offset. One field 'overlaps' another if its offset range overlaps the other. 2. Examine list of initializers. Unnamed initializers will be associated with a field as follows: 1. if it's the first initializer, it's the first field. Done. 2. start with the previous field that was initialized. Move forward through the field list and pick the first field that does not overlap with that previous field. That will be the field associated with that initializer. 3. If any initialized field overlaps with any other initialized field, error. 4. Go back through the field list again, in order. If a field does not have an initializer, and does not overlap with any other initialized field, assign it the default initializer. At this point, I wish to defer this to the next update.

Ok, the first test case reduces to this: struct V{ union { double[2] cell; double x; } static immutable V zero=V(0,1); } The problem being that the struct literal gets turned into: this(a, b) { cell = 0; x = 1; } ie. it passes the first argument to the first member, and the second argument to the second. This is sort of what I'd expect to happen, but the error message is completely valid for what it's trying to do. If anyone has a better idea of how struct literals should map to unions, please open another bug report about it.

Got my test cases a little mixed up there, but it's still mostly valid. All of the non-struct-literal struct construction seems to be converted into struct literals. eg. struct S { this(int a, int b) { this.a = b; this.b = a; } union { ulong g; struct {int a, b; }; } } static immutable S s = S(0, 1); Prints: (with a little extra debug output) StructLiteralExp::semantic('S(0LU,1,0)') S Error: duplicate union initialization for a Error: duplicate union initialization for b As you can see, it make a struct literal with every field accounted for. So this is a bug in the constfolding/ctfe code.

(In reply to comment #3) > > As you can see, it make a struct literal with every field accounted for. > > So this is a bug in the constfolding/ctfe code. Not exactly. It's a compiler structural problem: there's no way to specify a struct literal with missing fields. Struct static initializers can do it, but struct literals cannot. I think the solution is to merge struct literals with struct static initializers, as it says in a TODO in the code.

(In reply to comment #4) > Not exactly. It's a compiler structural problem: there's no way to specify a > struct literal with missing fields. Struct static initializers can do it, but > struct literals cannot. > > I think the solution is to merge struct literals with struct static > initializers, as it says in a TODO in the code. One of the D1 cases seems to have the same problem with struct static initializers. Can't this be done by just nulling out the untouched fields in the Expressions array and ensuring at least one field gets initialized?

(In reply to comment #5) > (In reply to comment #4) > > Not exactly. It's a compiler structural problem: there's no way to specify a > > struct literal with missing fields. Struct static initializers can do it, but > > struct literals cannot. > > > > I think the solution is to merge struct literals with struct static > > initializers, as it says in a TODO in the code. > > One of the D1 cases seems to have the same problem with struct static > initializers. Can't this be done by just nulling out the untouched fields in > the Expressions array and ensuring at least one field gets initialized? Maybe. The order of fields in a struct is fixed, so in theory that ought to work. It's a while since I last looked at it, but I remember there were severe problems with anonymous unions nested inside anonymous unions. There's code elsewhere in the compiler which tries to identify fields based on their type + offset, but that cannot work. It appears to work at the moment, but only because it assumes when fields are initialized in order with no gaps. Still, I've fixed some of those compiler bugs recently, so maybe it's more possible now.

Ok, I'll take a look at it tomorrow unless you want it. I know there are at least two places it checks for overlapping union initialization, one in expression.c and one somewhere in the glue, maybe e2ir?

(In reply to comment #7) > Ok, I'll take a look at it tomorrow unless you want it. I know there are at > least two places it checks for overlapping union initialization, one in > expression.c and one somewhere in the glue, maybe e2ir? The big one is in init.c. Around line 340 there's code I wrote (to replace the code in 320..340). Walter disabled that code a bit later, but he didn't say why. Would be great if you could take a fresh look at it.

I think for this to work, the interpreter needs to be able to handle uninitialized values, and unions need to default to void initializers. I have a patch for this that is nearly ready, and solves issue 6438 at the same time.

>It's a compiler structural problem: there's no way to specify a struct literal with missing fields. I don't know about in CTFE, but in the rest of the compiler the code is in place to just have elements[i] be NULL for missing fields.

(In reply to comment #10) > >It's a compiler structural problem: there's no way to specify a > struct literal with missing fields. > > I don't know about in CTFE, but in the rest of the compiler the code is in > place to just have elements[i] be NULL for missing fields. I have branch for this that mostly works, but no time to work on it at the moment. https://github.com/yebblies/dmd/tree/ctfeunion

https://github.com/D-Programming-Language/phobos/pull/493 and then https://github.com/D-Programming-Language/dmd/pull/803

Thanks for doing this. I think my branch was still letting you return partially uninitialized arrays/structs from ctfe. I also think the following should work: union U { int a, b; } int func() { U u; u.a = 3; assert(u.b == 3); return 1; } static assert(func()); But I don't know how to implement it. (it might not be worth it) Umm, test cases. (some pass, some fail, some pulled out of other test cases in dmd/phobos.) The last one should fail, I doubt it's useful leave a variable partially initialized. /* version(none) { struct S { this(int a, int b) { this.a = b; this.b = a; } union { ulong g; struct { int a; int b; } } } static immutable S s = S(1, 0); extern(C) int printf(const char *, ...); void main() { S s = .s; printf("%d %d %d\n", s.g, s.a, s.b); } } version(none) { union in6_addr { private union _in6_u_t { ubyte[16] u6_addr8; ushort[8] u6_addr16; uint[4] u6_addr32; } _in6_u_t in6_u; ubyte[16] s6_addr8; ushort[8] s6_addr16; uint[4] s6_addr32; alias s6_addr8 s6_addr; } const in6_addr IN6ADDR_ANY = { s6_addr8: [0] }; } version(none) { struct Zadok { char [4] s = void; } int quop() { Zadok pong; pong.s = ['z','x','f', 'g']; return 1; } static assert(quop()==1); static assert(quop()==1); // check for clobbering } //version = testc; version(testc) { union U { int a; int b; } int testxx() { U u; u.a = 7; u.b = 4; assert(u.a == 7); assert(u.b == 4); return 1; } static assert(testxx()); } //version = testb; version(testb) { void fillWithZero(T)(T[] arr) { foreach(ref x; arr) x = 7; } T[4] f(T)() { T[4] stackSpace = void; T[4] x = stackSpace; int y = x[0]; //int z = y + y; fillWithZero(stackSpace[]); return stackSpace; } static assert(f!int() == [7,7,7,7]); } //version = testa; version(testa) { interface SomeInterface { int daz(); float bar(char); int baz(); } interface SomeOtherInterface { int xxx(); } class TheBase : SomeInterface, SomeOtherInterface { int q = 88; int rad = 61; int a = 14; int somebaseclassfunc() { return 28;} int daz() { return 0; } int baz() { return 0; } int xxx() { return 762; } int foo() { return q; } float bar(char c) { return 3.6; } } class SomeClass : TheBase, SomeInterface { int gab = 9; int fab; int a = 17; int b = 23; int foo() { return gab + a; } float bar(char c) { return 2.6; } int something() { return 0; } int daz() { return 0; } int baz() { return 0; } } class Unrelated : TheBase { this(int x) { a = x; } } auto classtest1(int n) { SomeClass c = new SomeClass; assert(c.a == 17); assert(c.q == 88); TheBase d = c; assert(d.a == 14); assert(d.q == 88); if (n==7) { // bad cast -- should fail Unrelated u = cast(Unrelated)d; assert(u is null); } SomeClass e = cast(SomeClass)d; d.q = 35; assert(c.q == 35); assert(c.foo() == 9 + 17); ++c.a; assert(c.foo() == 9 + 18); assert(d.foo() == 9 + 18); d = new TheBase; SomeInterface fc = c; SomeOtherInterface ot = c; assert(fc.bar('x') == 2.6); assert(ot.xxx() == 762); fc = d; ot = d; assert(fc.bar('x') == 3.6); assert(ot.xxx() == 762); Unrelated u2 = new Unrelated(7); assert(u2.a == 7); return 6; } static assert(classtest1(1)); static assert(classtest1(2)); static assert(classtest1(7)); // bug 7154 } //version = testd; version(testd) { struct XY { union { int x, y; } } struct AHolder { XY aa; void a(XY x) { aa = x; } } struct AB { AHolder aHolder; XY b; void a(XY x) { aHolder.a(x); } } struct Main { AB ab; void setB() { ab.b = XY(); } void f() { ab.a(XY.init); setB(); } } } //version = teste; version(teste) { union U { int a; long b; } long test() { U u; u.a = 3; u.b = 8; return u.a + u.b; } static assert(test() == 11); } //version = testf; version(testf) { int[5] test() { int[5] var = void; var[0] = 6; var[2] = 6; var[4] = 6; return var; } pragma(msg, test()); } */

Commit pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/7cecf0090f7d22caf0efd2e1a558171013a387a5 Merge pull request #493 from donc/bug6681 Supplemental change required by regression bug 6681

Commit pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/296b99db347ace5e166120564146277788957eaf Merge pull request #803 from donc/ctfeunion6681yebblies Fix issue 6681 - struct constructor call is converted to struct literal ...

Commit pushed to dmd-1.x at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/f8fcbf1f44af6999b87f452f70265d5e47eed188 merge D2 pull #803 fix Issue 6681