Fix Issue 19022 - CTorFlow: Show the line of the duplicated initializ…

…ation for const/immutable fields
dlang · Jun 27, 2018 · 24e4a0c · 24e4a0c
1 parent fb22391
commit 24e4a0c
Show file tree

Hide file tree

Showing 13 changed files with 149 additions and 86 deletions.
diff --git a/src/dmd/ctorflow.d b/src/dmd/ctorflow.d
@@ -17,6 +17,7 @@ module dmd.ctorflow;
 import core.stdc.stdio;
 
 import dmd.root.rmem;
+import dmd.globals : Loc;
 
 enum CSX : ushort
 {
@@ -30,37 +31,33 @@ enum CSX : ushort
     deprecate_18719 = 0x40,    // issue deprecation for Issue 18719 - delete when deprecation period is over
 }
 
+/// Individual field in the Ctor with information about its callees and location.
+struct FieldInit
+{
+    CSX csx; /// information about the field's callees
+    Loc loc; /// location of the field initialization
+}
+
 /***********
  * Primitive flow analysis for constructors
  */
 struct CtorFlow
 {
     CSX callSuper;      /// state of calling other constructors
 
-    CSX[] fieldinit;    /// state of field initializations
+    FieldInit[] fieldinit;    /// state of field initializations
 
     void allocFieldinit(size_t dim)
     {
-        fieldinit = (cast(CSX*)mem.xcalloc(CSX.sizeof, dim))[0 .. dim];
+        fieldinit = (cast(FieldInit*)mem.xcalloc(FieldInit.sizeof, dim))[0 .. dim];
     }
 
     void freeFieldinit()
     {
         if (fieldinit.ptr)
             mem.xfree(fieldinit.ptr);
-        fieldinit = null;
-    }
 
-    CSX[] saveFieldInit()
-    {
-        CSX[] fi = null;
-        if (fieldinit.length) // copy
-        {
-            const dim = fieldinit.length;
-            fi = (cast(CSX*)mem.xmalloc(CSX.sizeof * dim))[0 .. dim];
-            fi[] = fieldinit[];
-        }
-        return fi;
+        fieldinit = null;
     }
 
     /***********************
@@ -70,7 +67,7 @@ struct CtorFlow
      */
     CtorFlow clone()
     {
-        return CtorFlow(callSuper, saveFieldInit());
+        return CtorFlow(callSuper, fieldinit.arraydup);
     }
 
     /**********************************
@@ -82,7 +79,7 @@ struct CtorFlow
     {
         callSuper |= csx;
         foreach (ref u; fieldinit)
-            u |= csx;
+            u.csx |= csx;
     }
 
     /******************************
@@ -97,7 +94,12 @@ struct CtorFlow
         {
             assert(fieldinit.length == ctorflow.fieldinit.length);
             foreach (i, u; ctorflow.fieldinit)
-                fieldinit[i] |= u;
+            {
+                auto fi = &fieldinit[i];
+                fi.csx |= u.csx;
+                if (fi.loc == Loc.init)
+                    fi.loc = u.loc;
+            }
         }
     }
 }

diff --git a/src/dmd/declaration.d b/src/dmd/declaration.d
@@ -12,6 +12,7 @@
 
 module dmd.declaration;
 
+import core.stdc.stdio;
 import dmd.aggregate;
 import dmd.arraytypes;
 import dmd.ctorflow;
@@ -112,7 +113,8 @@ bool modifyFieldVar(Loc loc, Scope* sc, VarDeclaration var, Expression e1)
                         break;
                 }
                 assert(i < dim);
-                const fi = sc.ctorflow.fieldinit[i];
+                auto fieldInit = &sc.ctorflow.fieldinit[i];
+                const fi = fieldInit.csx;
 
                 if (fi & CSX.this_ctor)
                 {
@@ -127,9 +129,14 @@ bool modifyFieldVar(Loc loc, Scope* sc, VarDeclaration var, Expression e1)
                         // dmd.ctorflow.CSX enum.
                         // @@@DEPRECATED_2019-01@@@.
                         if (fi & CSX.deprecate_18719)
-                            .deprecation(loc, "%s field `%s` initialized multiple times", modStr, var.toChars());
+                        {
+                            .deprecation(loc, "%s field `%s` was initialized in a previous constructor call", modStr, var.toChars());
+                        }
                         else
+                        {
                             .error(loc, "%s field `%s` initialized multiple times", modStr, var.toChars());
+                            .errorSupplemental(fieldInit.loc, "Previous initialization is here.");
+                        }
                     }
                 }
                 else if (sc.inLoop || (fi & CSX.label))
@@ -143,15 +150,16 @@ bool modifyFieldVar(Loc loc, Scope* sc, VarDeclaration var, Expression e1)
                     }
                 }
 
-                sc.ctorflow.fieldinit[i] |= CSX.this_ctor;
+                fieldInit.csx |= CSX.this_ctor;
+                fieldInit.loc = e1.loc;
                 if (var.overlapped) // https://issues.dlang.org/show_bug.cgi?id=15258
                 {
                     foreach (j, v; ad.fields)
                     {
                         if (v is var || !var.isOverlappedWith(v))
                             continue;
                         v.ctorinit = true;
-                        sc.ctorflow.fieldinit[j] = CSX.this_ctor;
+                        sc.ctorflow.fieldinit[j].csx = CSX.this_ctor;
                     }
                 }
             }

diff --git a/src/dmd/dscope.d b/src/dmd/dscope.d
@@ -191,7 +191,7 @@ struct Scope
         }
         s.slabel = null;
         s.nofree = false;
-        s.ctorflow.fieldinit = ctorflow.saveFieldInit();
+        s.ctorflow.fieldinit = ctorflow.fieldinit.arraydup;
         s.flags = (flags & SCOPEpush);
         s.lastdc = null;
         assert(&this != s);
@@ -290,7 +290,7 @@ struct Scope
             foreach (i, v; ad.fields)
             {
                 bool mustInit = (v.storage_class & STC.nodefaultctor || v.type.needsNested());
-                if (!mergeFieldInit(this.ctorflow.fieldinit[i], fies[i]) && mustInit)
+                if (!mergeFieldInit(this.ctorflow.fieldinit[i].csx, fies[i].csx) && mustInit)
                 {
                     error(loc, "one path skips field `%s`", v.toChars());
                 }

diff --git a/src/dmd/expression.d b/src/dmd/expression.d
@@ -5429,7 +5429,7 @@ extern (C++) final class DotVarExp : UnaExp
                     {
                         if (f == vd)
                         {
-                            if (!(sc.ctorflow.fieldinit[i] & CSX.this_ctor))
+                            if (!(sc.ctorflow.fieldinit[i].csx & CSX.this_ctor))
                             {
                                 /* If the address of vd is taken, assume it is thereby initialized
                                  * https://issues.dlang.org/show_bug.cgi?id=15869

diff --git a/src/dmd/expressionsem.d b/src/dmd/expressionsem.d
@@ -3290,7 +3290,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
                 // initialized after this call.
                 foreach (ref field; sc.ctorflow.fieldinit)
                 {
-                    field |= CSX.this_ctor | CSX.deprecate_18719;
+                    field.csx |= CSX.this_ctor | CSX.deprecate_18719;
                 }
             }
 

diff --git a/src/dmd/globals.d b/src/dmd/globals.d
@@ -432,7 +432,7 @@ struct Loc
     static immutable Loc initial;       /// use for default initialization of const ref Loc's
 
 nothrow:
-    extern (D) this(const(char)* filename, uint linnum, uint charnum)
+    extern (D) this(const(char)* filename, uint linnum, uint charnum) pure
     {
         this.linnum = linnum;
         this.charnum = charnum;

diff --git a/src/dmd/root/rmem.d b/src/dmd/root/rmem.d
@@ -242,18 +242,64 @@ else
         }
     }
 }
+/**
+Makes a null-terminated copy of the given string on newly allocated memory.
+The null-terminator won't be part of the returned string slice. It will be
+at position `n` where `n` is the length of the input string.
+
+Params:
+    s = string to copy
 
-extern (D) static char[] xarraydup(const(char)[] s) nothrow
+Returns: A null-terminated copy of the input array.
+*/
+extern (D) char[] xarraydup(const(char)[] s) nothrow
 {
-    if (s)
-    {
-        auto p = cast(char*)mem.xmalloc(s.length + 1);
-        char[] a = p[0 .. s.length];
-        a[] = s[0 .. s.length];
-        p[s.length] = 0;    // preserve 0 terminator semantics
-        return a;
-    }
-    return null;
+    if (!s)
+        return null;
+
+    auto p = cast(char*)mem.xmalloc(s.length + 1);
+    char[] a = p[0 .. s.length];
+    a[] = s[0 .. s.length];
+    p[s.length] = 0;    // preserve 0 terminator semantics
+    return a;
 }
 
+///
+unittest
+{
+    auto s1 = "foo";
+    auto s2 = s1.xarraydup;
+    s2[0] = 'b';
+    assert(s1 == "foo");
+    assert(s2 == "boo");
+    assert(*(s2.ptr + s2.length) == '\0');
+}
+
+/**
+Makes a copy of the given array on newly allocated memory.
 
+Params:
+    s = array to copy
+
+Returns: A copy of the input array.
+*/
+extern (D) T[] arraydup(T)(const scope T[] s) nothrow
+{
+    if (!s)
+        return null;
+
+    const dim = s.length;
+    auto p = (cast(T*)mem.xmalloc(T.sizeof * dim))[0 .. dim];
+    p[] = s;
+    return p;
+}
+
+///
+unittest
+{
+    auto s1 = [0, 1, 2];
+    auto s2 = s1.arraydup;
+    s2[0] = 4;
+    assert(s1 == [0, 1, 2]);
+    assert(s2 == [4, 1, 2]);
+}
diff --git a/src/dmd/semantic3.d b/src/dmd/semantic3.d
@@ -673,7 +673,7 @@ private extern(C++) final class Semantic3Visitor : Visitor
                             else
                             {
                                 bool mustInit = (v.storage_class & STC.nodefaultctor || v.type.needsNested());
-                                if (mustInit && !(sc2.ctorflow.fieldinit[i] & CSX.this_ctor))
+                                if (mustInit && !(sc2.ctorflow.fieldinit[i].csx & CSX.this_ctor))
                                 {
                                     funcdecl.error("field `%s` must be initialized but skipped", v.toChars());
                                 }

diff --git a/src/dmd/statementsem.d b/src/dmd/statementsem.d
@@ -3236,7 +3236,7 @@ else
             foreach (i, v; ad.fields)
             {
                 bool mustInit = (v.storage_class & STC.nodefaultctor || v.type.needsNested());
-                if (mustInit && !(sc.ctorflow.fieldinit[i] & CSX.this_ctor))
+                if (mustInit && !(sc.ctorflow.fieldinit[i].csx & CSX.this_ctor))
                 {
                     rs.error("an earlier `return` statement skips field `%s` initialization", v.toChars());
                     errors = true;

diff --git a/test/fail_compilation/diag12678.d b/test/fail_compilation/diag12678.d
@@ -1,9 +1,11 @@
 /*
 TEST_OUTPUT:
 ---
-fail_compilation/diag12678.d(19): Error: const field `cf1` initialized multiple times
-fail_compilation/diag12678.d(22): Error: immutable field `if1` initialized multiple times
-fail_compilation/diag12678.d(25): Error: const field `cf2` initialization is not allowed in loops or after labels
+fail_compilation/diag12678.d(21): Error: const field `cf1` initialized multiple times
+fail_compilation/diag12678.d(20):        Previous initialization is here.
+fail_compilation/diag12678.d(24): Error: immutable field `if1` initialized multiple times
+fail_compilation/diag12678.d(23):        Previous initialization is here.
+fail_compilation/diag12678.d(27): Error: const field `cf2` initialization is not allowed in loops or after labels
 ---
 */
 

diff --git a/test/fail_compilation/diag19022.d b/test/fail_compilation/diag19022.d
@@ -0,0 +1,18 @@
+/*
+TEST_OUTPUT:
+---
+fail_compilation/diag19022.d(16): Error: immutable field `b` initialized multiple times
+fail_compilation/diag19022.d(15):        Previous initialization is here.
+---
+*/
+// https://issues.dlang.org/show_bug.cgi?id=19022
+
+struct Foo
+{
+    immutable int b;
+    this(int a)
+    {
+        b = 2;
+        b = 2;
+    }
+}
diff --git a/test/fail_compilation/fail18719.d b/test/fail_compilation/fail18719.d
@@ -4,7 +4,7 @@
 /*
 TEST_OUTPUT:
 ---
-fail_compilation/fail18719.d(29): Deprecation: immutable field `x` initialized multiple times
+fail_compilation/fail18719.d(29): Deprecation: immutable field `x` was initialized in a previous constructor call
 ---
 */