Merge pull request #8542 from atilaneves/fix-traits-get-unittest

Fix issue 19058 - __traits(getUnitTests) works again with separate co…

src/dmd/dmangle.d

@@ -1098,7 +1098,7 @@ public:
 }
 
 /// Returns: `true` if the given character is a valid mangled character
-package bool isValidMangling(dchar c)
+package bool isValidMangling(dchar c) nothrow
 {
     return
         c >= 'A' && c <= 'Z' ||

src/dmd/identifier.d

@@ -23,6 +23,7 @@ import dmd.root.stringtable;
 import dmd.tokens;
 import dmd.utf;
 
+
 /***********************************************************
  */
 extern (C++) final class Identifier : RootObject
@@ -124,6 +125,18 @@ nothrow:
 
     extern (C++) __gshared StringTable stringtable;
 
+    /**
+       A secondary string table is used to guarantee that we generate unique
+       identifiers per module. See generateIdWithLoc and issues
+       https://issues.dlang.org/show_bug.cgi?id=16995
+       https://issues.dlang.org/show_bug.cgi?id=18097
+       https://issues.dlang.org/show_bug.cgi?id=18111
+       https://issues.dlang.org/show_bug.cgi?id=18880
+       https://issues.dlang.org/show_bug.cgi?id=18868
+       https://issues.dlang.org/show_bug.cgi?id=19058.
+     */
+    private extern (C++) __gshared StringTable fullPathStringTable;
+
     static Identifier generateId(const(char)* prefix)
     {
         __gshared size_t i;
@@ -153,23 +166,75 @@ nothrow:
      */
     extern (D) static Identifier generateIdWithLoc(string prefix, const ref Loc loc)
     {
-        OutBuffer buf;
-        buf.writestring(prefix);
-        buf.writestring("_L");
-        buf.print(loc.linnum);
-        buf.writestring("_C");
-        buf.print(loc.charnum);
-        auto basesize = buf.peekSlice().length;
-        uint counter = 1;
-        while (stringtable.lookup(buf.peekSlice().ptr, buf.peekSlice().length) !is null)
+        import dmd.root.filename: absPathThen;
+
+        // see below for why we use absPathThen
+        return loc.filename.absPathThen!((absPath)
         {
-            // Strip the extra suffix
-            buf.setsize(basesize);
-            // Add new suffix with increased counter
-            buf.writestring("_");
-            buf.print(counter++);
-        }
-        return idPool(buf.peekSlice());
+
+            // this block generates the "regular" identifier, i.e. if there are no collisions
+            OutBuffer idBuf;
+            idBuf.writestring(prefix);
+            idBuf.writestring("_L");
+            idBuf.print(loc.linnum);
+            idBuf.writestring("_C");
+            idBuf.print(loc.charnum);
+
+            // This block generates an identifier that is prefixed by the absolute path of the file
+            // being compiled. The reason this is necessary is that we want unique identifiers per
+            // module, but the identifiers are generated before the module information is available.
+            // To guarantee that each generated identifier is unique without modules, we make them
+            // unique to each absolute file path. This also makes it consistent even if the files
+            // are compiled separately. See issues:
+            // https://issues.dlang.org/show_bug.cgi?id=16995
+            // https://issues.dlang.org/show_bug.cgi?id=18097
+            // https://issues.dlang.org/show_bug.cgi?id=18111
+            // https://issues.dlang.org/show_bug.cgi?id=18880
+            // https://issues.dlang.org/show_bug.cgi?id=18868
+            // https://issues.dlang.org/show_bug.cgi?id=19058.
+            OutBuffer fullPathIdBuf;
+
+            if (absPath)
+            {
+                // replace characters that demangle can't handle
+                for (auto ptr = absPath; *ptr != '\0'; ++ptr)
+                {
+                    // see dmd.dmangle.isValidMangling
+                    // Unfortunately importing it leads to either build failures or cyclic dependencies
+                    // between modules.
+                    if (*ptr == '/' || *ptr == '\\' || *ptr == '.' || *ptr == '?' || *ptr == ':')
+                        *ptr = '_';
+                }
+
+                fullPathIdBuf.writestring(absPath);
+                fullPathIdBuf.writestring("_");
+            }
+
+            fullPathIdBuf.writestring(idBuf.peekSlice());
+            const fullPathIdLength = fullPathIdBuf.peekSlice().length;
+            uint counter = 1;
+
+            // loop until we can't find the absolute path ~ identifier, adding a counter suffix each time
+            while (fullPathStringTable.lookup(fullPathIdBuf.peekSlice()) !is null)
+            {
+                // Strip the counter suffix if any
+                fullPathIdBuf.setsize(fullPathIdLength);
+                // Add new counter suffix
+                fullPathIdBuf.writestring("_");
+                fullPathIdBuf.print(counter++);
+            }
+
+            // `idStartIndex` is the start of the "true" identifier. We don't actually use the absolute
+            // file path in the generated identifier since the module system makes sure that the fully
+            // qualified name is unique.
+            const idStartIndex = fullPathIdLength - idBuf.peekSlice().length;
+
+            // Remember the full path identifier to avoid possible future collisions
+            fullPathStringTable.insert(fullPathIdBuf.peekSlice(),
+                                       null);
+
+            return idPool(fullPathIdBuf.peekSlice()[idStartIndex .. $]);
+        });
     }
 
     /********************************************
@@ -240,6 +305,8 @@ nothrow:
 
     static void initTable()
     {
-        stringtable._init(28000);
+        enum size = 28_000;
+        stringtable._init(size);
+        fullPathStringTable._init(size);
     }
 }

src/dmd/identifier.h

@@ -37,6 +37,7 @@ class Identifier : public RootObject
     int dyncast() const;
 
     static StringTable stringtable;
+    static StringTable fullPathStringTable;
     static Identifier *generateId(const char *prefix);
     static Identifier *generateId(const char *prefix, size_t i);
     static Identifier *idPool(const char *s, unsigned len);

src/dmd/root/filename.d

@@ -815,7 +815,7 @@ version(Windows)
      * References:
      *  https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
      */
-    package auto extendedPathThen(alias F)(const(char*) path)
+    auto extendedPathThen(alias F)(const(char*) path)
     {
         return path.toWStringzThen!((wpath)
         {
@@ -828,7 +828,7 @@ version(Windows)
                                                 null /*filePartBuffer*/);
             if (pathLength == 0)
             {
-                return F(""w.ptr);
+                return F((wchar*).init);
             }
 
             // wpath is the UTF16 version of path, but to be able to use
@@ -851,7 +851,7 @@ version(Windows)
 
             if (absPathRet == 0 || absPathRet > absPath.length - prefix.length)
             {
-                return F(""w.ptr);
+                return F((wchar*).init);
             }
 
             return F(absPath.ptr);
@@ -868,17 +868,18 @@ version(Windows)
      * Returns:
      *  The result of calling F on the UTF16 version of str.
      */
-    private auto toWStringzThen(alias F)(const(char*) str) nothrow
+    private auto toWStringzThen(alias F)(const(char)* str) nothrow
     {
         import core.stdc.string: strlen;
         import core.stdc.stdlib: malloc, free;
+        import core.sys.windows.winnls: MultiByteToWideChar;
 
         wchar[1024] buf;
         // cache this for efficiency
         const int strLength = cast(int)(strlen(str) + 1);
         // first find out how long the buffer must be to store the result
         const length = MultiByteToWideChar(0 /*codepage*/, 0 /*flags*/, str, strLength, null, 0);
-        if (!length) return F(""w.ptr);
+        if (!length) return F((wchar*).init);
 
         auto ret = length > buf.length
             ? (cast(wchar*)malloc(length * wchar.sizeof))
@@ -894,4 +895,82 @@ version(Windows)
 
         return F(ret);
     }
+
+}
+
+version(Posix)
+{
+
+    /**
+    Takes a callable F and applies it to the result of converting
+    `fileName` to an absolute file path (char*)
+
+    Params:
+        fileName = The file name to be converted to an absolute path
+    Returns: Whatever `F` returns.
+    */
+    auto absPathThen(alias F)(const(char)* fileName)
+    {
+        import core.sys.posix.stdlib: realpath, free;
+        auto absPath = realpath(fileName, null /* realpath allocates */);
+        scope(exit) free(absPath);
+        return F(absPath);
+    }
+}
+else
+{
+    /**
+    Takes a callable F and applies it to the result of converting
+    `fileName` to an absolute file path (char*)
+
+    Params:
+        fileName = The file name to be converted to an absolute path
+    Returns: Whatever `F` returns.
+     */
+    auto absPathThen(alias F)(const(char)* fileName)
+    {
+        import core.sys.windows.winnls: WideCharToMultiByte;
+        import core.stdc.stdlib: malloc, free;
+
+        return fileName.extendedPathThen!((wpath) {
+                // first find out how long the buffer must be to store the result
+                const length = WideCharToMultiByte(0,    // code page
+                                                   0,    // flags
+                                                   wpath,
+                                                   -1,   // wpath len, -1 is null terminated
+                                                   null, // multibyte output ptr
+                                                   0,    // multibyte output length
+                                                   null, // default char
+                                                   null, // if used default char
+                );
+
+                if(!length) return F((char*).init);
+
+                char[1024] buf = void;
+
+                scope multibyteBuf = length > buf.length
+                    ? (cast(char*)malloc(length * char.sizeof))
+                    : &buf[0];
+                scope (exit)
+                {
+                    if (multibyteBuf != &buf[0])
+                        free(multibyteBuf);
+                }
+
+                // now store the result
+                const length2 = WideCharToMultiByte(0,    // code page
+                                                    0,    // flags
+                                                    wpath,
+                                                    -1,   // wpath len, -1 is null terminated
+                                                    multibyteBuf,
+                                                    length,
+                                                    null, // default char
+                                                    null, // if used default char
+                );
+
+                assert(length == length2);
+
+                return F(multibyteBuf);
+        });
+    }
 }

test/runnable/imports/another_module_with_tests.d

@@ -0,0 +1,3 @@
+unittest {}
+unittest {}
+unittest { assert(false); }

test/runnable/issue16995.d

@@ -1,8 +1,9 @@
 // REQUIRED_ARGS: -unittest
 // COMPILE_SEPARATELY
-// EXTRA_SOURCES: imports/module_with_tests.d
+// EXTRA_SOURCES: imports/module_with_tests.d imports/another_module_with_tests.d
 
 import imports.module_with_tests;
+import imports.another_module_with_tests;
 import core.exception: AssertError;
 
 shared static this()
@@ -13,8 +14,8 @@ shared static this()
 
 void main()
 {
-    import module_with_tests;
-    foreach(i, ut; __traits(getUnitTests, module_with_tests)) {
+    foreach(i, ut; __traits(getUnitTests, imports.module_with_tests))
+    {
         try
         {
             ut();
@@ -25,4 +26,17 @@ void main()
             assert(i == 1, "Only 2nd unittest should fail");
         }
     }
+
+    foreach(i, ut; __traits(getUnitTests, imports.another_module_with_tests))
+    {
+        try
+        {
+            ut();
+            assert(i == 0 || i == 1, "3rd unittest should fail");
+        }
+        catch(AssertError e)
+        {
+            assert(i == 2, "Only 3rd unittest should fail");
+        }
+    }
 }