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

Author: atilaneves

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,13 @@ nothrow:
 
     extern (C++) static __gshared StringTable stringtable;
 
+    /**
+       A secondary string table is used to guarantee that we generate unique
+       identifiers per module. See generateIdWithLoc and issues
+       16995, 18097, 18111, 18880, 18868, 19058
+     */
+    private extern (C++) static __gshared StringTable fullPathStringTable;
+
     static Identifier generateId(const(char)* prefix)
     {
         static __gshared size_t i;
@@ -153,23 +161,67 @@ 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
+            // 16995, 18097, 18111, 18880, 18868, 19058.
+            OutBuffer fullPathIdBuf;
+            if(absPath)
+            {
+                // replace characters that demangle can't handle
+                for(auto ptr = absPath; *ptr != '\0'; ++ptr)
+                {
+                    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().ptr, fullPathIdBuf.peekSlice().length) !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.peekString(),
+                                       fullPathIdBuf.peekSlice().length,
+                                       null);
+
+            return idPool(fullPathIdBuf.peekSlice()[idStartIndex .. $]);
+        });
     }
 
     /********************************************
@@ -240,6 +292,8 @@ nothrow:
 
     static void initTable()
     {
-        stringtable._init(28000);
+        enum size = 28_000;
+        stringtable._init(size);
+        fullPathStringTable._init(size);
     }
 }

src/dmd/root/filename.d

@@ -793,6 +793,7 @@ version(Windows)
      */
     private int _mkdir(const(char)* path) nothrow
     {
+        import core.sys.windows.winbase: CreateDirectoryW;
         const createRet = path.extendedPathThen!(p => CreateDirectoryW(p,
                                                                        null /*securityAttributes*/));
         // different conventions for CreateDirectory and mkdir
@@ -810,8 +811,10 @@ 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)
     {
+        import core.sys.windows.winbase: GetFullPathNameW;
+
         return path.toWStringzThen!((wpath)
         {
             // GetFullPathNameW expects a sized buffer to store the result in. Since we don't
@@ -823,7 +826,7 @@ version(Windows)
                                                 null /*filePartBuffer*/);
             if (pathLength == 0)
             {
-                return F(""w.ptr);
+                return F(cast(wchar*) null);
             }
 
             // wpath is the UTF16 version of path, but to be able to use
@@ -846,7 +849,7 @@ version(Windows)
 
             if (absPathRet == 0 || absPathRet > absPath.length - prefix.length)
             {
-                return F(""w.ptr);
+                return F(cast(wchar*) null);
             }
 
             return F(absPath.ptr);
@@ -867,13 +870,14 @@ version(Windows)
     {
         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(cast(wchar*) null);
 
         auto ret = length > buf.length
             ? (cast(wchar*)malloc(length * wchar.sizeof))
@@ -889,4 +893,77 @@ 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*)
+     */
+    auto absPathThen(alias F)(const(char)* fileName)
+    {
+        char[1024] realPathBuffer;
+        auto absPath = realpath(fileName, &realPathBuffer[0]);
+        return F(absPath);
+    }
+
+    extern(C) char* realpath(const(char)* file_name, char* resolved_name) @nogc nothrow;
+}
+else
+{
+    /**************************
+     * Takes a callable F and applies it to the result of converting
+     * `fileName` to an absolute file path (char*)
+     */
+    auto absPathThen(alias F)(const(char)* fileName)
+    {
+        import core.sys.windows.winnls: WideCharToMultiByte;
+        import core.stdc.stdlib: malloc, free;
+        import std.stdio: writeln;
+        import std.conv: text;
+
+        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(cast(char*) null);
+
+                char[1024] buf;
+
+                auto multibyteBuf = length > buf.length
+                    ? (cast(char*)malloc(length * char.sizeof))
+                    : &buf[0];
+                scope (exit)
+                {
+                    if (multibyteBuf != &buf[0])
+                        free(multibyteBuf);
+                }
+
+                // first find out how long the buffer must be to 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,7 @@ 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 +25,16 @@ 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");
+        }
+    }
 }