Fix issue 19058 - __traits(getUnitTests) works again with separate co… by atilaneves · Pull Request #8542 · dlang/dmd

atilaneves · 2018-08-07T14:57:30Z

I tried creating the PR to stable instead of master but for reasons I don't understand a whole lot of commits came along with it. And yes, I'd rebased to upstream/stable.

dlang-bot · 2018-08-07T14:57:32Z

Thanks for your pull request and interest in making D better, @atilaneves! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19058	regression	__traits(getUnitTests) stops working with separate compilation in dmd 2.081.0

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + dmd#8542"

wilzbach · 2018-08-07T15:20:51Z

I tried creating the PR to stable instead of master but for reasons I don't understand a whole lot of commits came along with it. And yes, I'd rebased to upstream/stable.

Try using --onto the next time:

git rebase --onto upstream/stable upstream/master

I just rebased this to stable for you ;-)

atilaneves · 2018-08-07T16:19:39Z

I did --onto. I don't know what happened.

Anyway, the semaphore and buildkite failures are due to stable/master differences (I got that locally) between dmd and druntime. If druntime is at master and dmd at stable... this error occurs. It doesn't if druntime is also on stable. I don't know how these CI pipelines are set up. My old PR for this fix just passed on master, and the errors are nowhere near any files I touched.

DAutoTest seems to be entering a pathological dub dependency graph that has nothing to do with this either.

atilaneves · 2018-08-07T16:28:39Z

I'd rather get this merged to master and miss dmd 2.081.2 than deal with buildkite and semaphoreci failing for no good reason.

jacob-carlborg · 2018-08-07T18:54:21Z

src/dmd/identifier.d

+                {
+                    if(*ptr == '/' || *ptr == '\\' || *ptr == '.' || *ptr == '?' || *ptr == ':')
+                        *ptr = '_';
+                }


Can the linker (or any other tools that deal with mangled symbols) handle these symbols? I'm asking because of the comment and why demangle isn't updated instead.

I don't know if the linker can handle it. Maybe updating demangle is a good idea, but even if it is it's not the responsibility of this PR to handle that.

Similar code existed before in the long chain of bug fixes mentioned in the comments.

jacob-carlborg · 2018-08-07T18:55:45Z

src/dmd/identifier.d

+            // are compiled separately. See issues
+            // 16995, 18097, 18111, 18880, 18868, 19058.
+            OutBuffer fullPathIdBuf;
+            if(absPath)


Space between if/for/while etc. and the opening parenthesis.

jacob-carlborg · 2018-08-07T18:57:10Z

src/dmd/identifier.d

+            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)


Please add a const(char)[] overload to lookup.

I thought about it, but again, PRs are supposed to be focused. I only made the changes to filename.d because I had to.

We should avoid adding new code that uses C strings or passing pointer and length separately. We're trying to improve the code, not digging the hole deeper we are already in.

In this case it's very simple to add an overload that forwards to the existing method. Even if the actual implementation isn't using a D string it will reduce the surface of where C string are used.

I agree that it's simple. I agree that it makes the code better. I agree that it should be done. However, it goes against what dlang-bot posted above: "My PR is as minimal as possible".

It also makes the code better to localise imports, yet in this same review you asked me to delete them.

I'm trying to fix a bug, not change the interface to StringTable.

Feel free to submit a PR that adds slice overloads.

I've already submitted a PR that adds those overloads #8547.

An overload taking const(char)[] has been added and merged. Please rebase and use the new overload.

It's been merged to master, this is targeting stable.

Oh, my mistake.

Master has been merged into stable now https://github.com/dlang/dmd/blob/stable/src/dmd/root/stringtable.d#L127.

jacob-carlborg · 2018-08-07T18:58:50Z

src/dmd/identifier.d

+            const idStartIndex = fullPathIdLength - idBuf.peekSlice().length;
+
+            // Remember the full path identifier to avoid possible future collisions
+            fullPathStringTable.insert(fullPathIdBuf.peekString(),


Same here, add an overload for insert taking const(char)[].

Same reply as the previous comment.

An overload taking const(char)[] has been added and merged. Please rebase and use the new overload.

jacob-carlborg · 2018-08-07T19:00:05Z

src/dmd/identifier.d

+       identifiers per module. See generateIdWithLoc and issues
+       16995, 18097, 18111, 18880, 18868, 19058
+     */
+    private extern (C++) static __gshared StringTable fullPathStringTable;


If this is private there shouldn't be a reason for it being extern(C++).

I don't know why the other one is. Maybe because of the backend. Speaking of which, I need to add this to the .h file.

I'm following the existing pattern.

I recommend not following existing patterns. We need to get rid of this cruft somehow.

There are header files here for a reason. Unless they disappear, I think extern(C++) is the way to go.

static and __gshared are redundant. Dscanner which is in master would have complained about this.

jacob-carlborg · 2018-08-07T19:02:44Z

src/dmd/root/filename.d

     */
    private int _mkdir(const(char)* path) nothrow
    {
+        import core.sys.windows.winbase: CreateDirectoryW;


Why is this necessary?

It isn't, but I copied the code around and it failed to compile because of the import. It was one my one concession to "do only what you must". I don't mind deleting it.

Please do, it's already imported elsewhere.

jacob-carlborg · 2018-08-07T19:02:50Z

src/dmd/root/filename.d

-    package auto extendedPathThen(alias F)(const(char*) path)
+    auto extendedPathThen(alias F)(const(char*) path)
    {
+        import core.sys.windows.winbase: GetFullPathNameW;


Why is this necessary?

Same reply as above.

jacob-carlborg · 2018-08-07T19:05:54Z

src/dmd/root/filename.d

+        return F(absPath);
+    }
+
+    extern(C) char* realpath(const(char)* file_name, char* resolved_name) @nogc nothrow;


Why not import this from druntime?

This should be private

That didn't occur to me. I didn't even know importing from druntime was allowed in dmd.

Agreed.

That didn't occur to me. I didn't even know importing from druntime was allowed in dmd.

To get access the the C bindings it is. Phobos is not allowed though.

jacob-carlborg · 2018-08-07T19:10:30Z

src/dmd/root/filename.d

+     * 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)


Can a const(char)[] parameter be used instead.

That would complicate the code given that the Win32 API only deals in null-terminated strings.

We would prefer all new functions have full DDoc headers including Params and Returns. Also see https://dlang.org/dstyle.html#phobos_documentation

Although a const(char)[] parameter would complicate the code, that complication would be encapsulated in this function, and that's good IMO.

Loc.filename isn't char[], it's const(char)*. That would require a lot of conversions. const(char)* is used all over the codebase from the time it was written in C++. Converting const(char)* to char[] back to a C string for both the Windows and Posix APIs doesn't make much sense to me.

I'm trying to fix a bug, not refactor dmd. Yes, one should refactor as one goes, but C strings are so pervasive right now I'd need dozens of PRs just to get to the point that I can fix this regression, which seriously affects my workflow and is stopping us from upgrading the compiler version at work.

jacob-carlborg · 2018-08-07T19:11:25Z

src/dmd/root/filename.d

+                scope (exit)
+                {
+                    if (multibyteBuf != &buf[0])
+                        free(multibyteBuf);


Is there anything preventing the parameter to F from escaping?

I changed auto to scope.

wilzbach · 2018-08-07T21:01:14Z

I don't know how these CI pipelines are set up.
I'd rather get this merged to master and miss dmd 2.081.2 than deal with buildkite and semaphoreci failing for no good reason.

Fix for Buildkite: dlang/ci#271
Not sure how to fix SemaphoreCI as it doesn't expose the target branch and we probably can only find it out via the GitHub API.

DAutoTest seems to be entering a pathological dub dependency graph that has nothing to do with this either.

Yeah sadly that one is known :/

I'd rather get this merged to master and miss dmd 2.081.2 than deal with buildkite and semaphoreci failing for no good reason.

Fair enough, but keep in mind that the master-stable branch-off will happen around 15.8 (i.e. in one week). So if this hasn't been merged by then, you might need to rebase to stable again if you want this to be part of 2.082.

Sorry for the CI annoyances here :/

atilaneves · 2018-08-08T07:48:45Z

I hope it gets merged soon but that's out of my control. It is a regression fix though...

wilzbach · 2018-08-13T17:20:42Z

Not sure how to fix SemaphoreCI as it doesn't expose the target branch and we probably can only find it out via the GitHub API.

Fix: #8560

atilaneves · 2018-08-17T07:58:31Z

Green and ready to go.

JinShil · 2018-08-17T10:17:47Z

src/dmd/root/filename.d

+     * Params:
+     *     fileName: The file name to be converted to an absolute path
+     * Returns: Whatever `F` returns.
+     */


Please remove the "*" prefix, and put a newline above "Params" and "Returns". See the example at https://dlang.org/dstyle.html#phobos_documentation

JinShil · 2018-08-17T10:20:15Z

test/runnable/issue16995.d

 {
-    import module_with_tests;
-    foreach(i, ut; __traits(getUnitTests, module_with_tests)) {
+    foreach(i, ut; __traits(getUnitTests, imports.module_with_tests)) {


We prefer consistent coding conventions even in the tests. i.e. { on a new line.

JinShil · 2018-08-17T10:20:56Z

test/runnable/issue16995.d

        }
    }
+
+    foreach(i, ut; __traits(getUnitTests, imports.another_module_with_tests)) {


{ curly brace on new line.

wilzbach · 2018-08-22T11:06:17Z

buildkite is being weird again

Sorry, my fault. Should be fixed now. I restarted the build.

MartinNowak · 2018-08-22T11:36:23Z

I'd be happy to see a solution that could also be applied to 14894 – mangling of mixins and lambdas is not unique and depends on compilation flags, basically the same problem.

Including absolute paths in mangling has a quite a smell (and also bloats symbols sizes, though that's less important for debug builds). It would likely break hard-linked folders and copies of sources (vendored folders). As you already said yourself, module names should be unique already (https://forum.dlang.org/post/krkinfxikztqvudjopao@forum.dlang.org).

Also I'm not sure that we need to support lookup of unittests by their symbol name (hence they start with __). We might wanna have a second look at the resolution of 18097 – [REG2.077] Unittest function is undefined identifier.

Overall we probably should spent more time on this and fix it once and for all, and at best also the similar problem with mixins and lambdas.

Short summary of the suffix approaches and their problems:
Line/Column number: possibly not suitable due to differences in .di/.d files (or only for unittests which are assumed to not be in .di files)
Counter: not suitable as it requires full semantic analysis when importing (but not compiling) a module and also increments depend on exact compilation flags
Hash: would prolly work, but we'd need a consistent and fast hash

At least for unittests the suggested line/column number + counter on collision approach might be worth a second thought. If the importer can already see a unittest, why wouldn't it see all the other ones at the same loc/col?

atilaneves · 2018-08-22T12:18:34Z

I'd be happy to see a solution that could also be applied to 14894

That's great, but that's not what this PR is about. It's about fixing 19058. It's hard enough to keep things focussed as it is.

Including absolute paths in mangling has a quite a smell (and also bloats symbols sizes, though that's less important for debug builds

This isn't what this PR does. It includes the absolute path iff there's a collision. Notice that none of the Phobos tests that check for error messages that print out unittest names had to be changed. And they didn't have to be changed because the absolute path isn't part of the symbol. Imagine having to write tests so that the absolute paths were part of the symbol that didn't depend on the filesystem layout of the autotester - it wouldn't be fun.

As you already said yourself, module names should be unique already

I also said that modules aren't available until semantic, which is far too late to generate the identifier, which was the cause of some of the bugs referenced in the comments of this PR.

Without completely restructuring dmd, I don't see how these multiple issues can be solved simultaneously.

Also I'm not sure that we need to support lookup of unittests by their symbol name (hence they start with __). We might wanna have a second look at the resolution of 18097 – [REG2.077] Unittest function is undefined identifier.

That wasn't the problem in that bug. It was the fact that linking failed after trying to get a reference to a unittest function. The included implementation-defined names were used as an illustration. There's nothing weird at all in trying to get a reference to a unittest function without using the actual identifier.

Overall we probably should spent more time on this and fix it once and for all, and at best also the similar problem with mixins and lambdas.

And, in the meanwhile, this bug has completely broken my workflow and is currently preventing us from upgrading the dmd version at work - we're still stuck on 2.080.1 on the figurative eve of the release of 2.082.0.

This bug is not a detail. It's a regression blocking us at work. It breaks legitimate uses of the language's functionality and is currently taking a major vesion bump of 2 (or 3 or more if this carries on long enough) to get fixed despite a small PR with a fix that passes all tests.

At least for unittests the suggested line/column number + counter on collision approach might be worth a second thought. If the importer can already see a unittest, why wouldn't it see all the other ones at the same loc/col?

This demonstrably does not, and cannot, work. It's literally the "fix" that caused this regression in the first place. The counter would need to be per module, and modules don't exist yet when the identifier is generated.

jacob-carlborg · 2018-08-22T16:14:18Z

Would it be possible to change or set the identifier later? I mean, we do have anonymous functions.

atilaneves · 2018-08-22T18:07:21Z

@jacob-carlborg No, we can't set the identifier later. That's what I did in my original fix exactly as a suggestion from @klickverbot because lambdas are done that way, and it ended up causing issue 18097. There's a long list of issues related to this, which I chronicled in the already mentioned blog post where I asked for ideas to solve this problem. The laundry list of issues is also in the diffs of this PR.

This fix works. This fix is small. This fix doesn't affect the vast majority of symbols. I've considered this every which way I can, asked for help, and nobody has come up with a better idea that also works.

I don't understand why this hasn't been merged yet. Did I mention this is a work-blocking regression?

JinShil · 2018-08-23T13:23:20Z

@wilzbach The buildkite failure looks suspicious. Any ideas?

Geod24 · 2018-08-23T13:59:23Z

It was broken last night but is fixed now. Retriggering the build did the trick.

kinke · 2018-08-23T21:24:02Z

At least for unittests the suggested line/column number + counter on collision approach might be worth a second thought. If the importer can already see a unittest, why wouldn't it see all the other ones at the same loc/col?

This demonstrably does not, and cannot, work. It's literally the "fix" that caused this regression in the first place. The counter would need to be per module, and modules don't exist yet when the identifier is generated.

I proposed including the (unique) file path as replacement for the missing module. Isn't that enough?

Edit: After a quick glance, this seems to be more or less how I imagined it. Lgtm.

kinke · 2018-08-23T21:29:07Z

src/dmd/root/filename.d

+
+                if(!length) return F((char*).init);
+
+                char[1024] buf;


It's a minor optimization. No need to default-initialize it if you're just going to overwrite it.

kinke · 2018-08-23T21:49:14Z

src/dmd/identifier.d

+
+            fullPathIdBuf.writestring(idBuf.peekSlice());
+            const fullPathIdLength = fullPathIdBuf.peekSlice().length;
+            uint counter = 1;


I think the counter/StringTable could be simplified and possibly accelerated/requiring less memory via __gshared uint[string][Loc] counters, indexed by counters[loc][prefix]; checking if there's an existing entry and incrementing/setting it to 1 when done. I might give that a try as a follow-up.

…mpilation

atilaneves · 2018-08-24T10:48:30Z

I'm going on holiday today. I'm not going to reply to anything until I'm back on the 12th of September.

JinShil · 2018-08-24T11:13:10Z

I would like to merge this before the next release. If there are any objections, please make them known soon. If there are no show-stoppers, but you have some recommended changes, let's do them as followups.

MartinNowak · 2018-08-25T11:49:36Z

Also I'm not sure that we need to support lookup of unittests by their symbol name (hence they start with __). We might wanna have a second look at the resolution of 18097 – [REG2.077] Unittest function is undefined identifier.

That wasn't the problem in that bug. It was the fact that linking failed after trying to get a reference to a unittest function. The included implementation-defined names were used as an illustration. There's nothing weird at all in trying to get a reference to a unittest function without using the actual identifier.

What fails with 2.077.0 is auto a = &mixin(__traits(identifier, __traits(parent, { })));, but not auto b = &__traits(parent, { });. So it's only lookup via the internal unittest name (__unittest_bug_d_5_1) that has a problem and that might be acceptable.

Rework #8542 wrt. deterministic IDs via generateIdWithLoc() merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

dlang-bot added the Severity:Bug Fix label Aug 7, 2018

wilzbach changed the base branch from master to stable August 7, 2018 15:19

dlang-bot added the Severity:Enhancement label Aug 7, 2018

wilzbach force-pushed the fix-traits-get-unittest branch from 520fa2c to 6017634 Compare August 7, 2018 15:20

jacob-carlborg reviewed Aug 7, 2018

View reviewed changes

jacob-carlborg suggested changes Aug 7, 2018

View reviewed changes

atilaneves force-pushed the fix-traits-get-unittest branch from 6017634 to 20234cd Compare August 7, 2018 20:19

atilaneves requested review from dnadlinger and ibuclaw as code owners August 7, 2018 20:19

atilaneves force-pushed the fix-traits-get-unittest branch from 20234cd to 4787f3f Compare August 8, 2018 07:25

atilaneves force-pushed the fix-traits-get-unittest branch from 4787f3f to 9be2380 Compare August 8, 2018 13:53

atilaneves force-pushed the fix-traits-get-unittest branch from 9be2380 to 45942c8 Compare August 16, 2018 14:29

atilaneves force-pushed the fix-traits-get-unittest branch from 45942c8 to e75fd78 Compare August 17, 2018 10:09

JinShil reviewed Aug 17, 2018

View reviewed changes

JinShil approved these changes Aug 23, 2018

View reviewed changes

kinke reviewed Aug 23, 2018

View reviewed changes

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

3a686c0

…mpilation

atilaneves force-pushed the fix-traits-get-unittest branch from 31c4894 to 3a686c0 Compare August 24, 2018 10:48

JinShil added 3 commits August 24, 2018 19:52

void initialize temporary buffer in filename.d

cd351e9

Update DDoc comments to use standard conventions in filename.d

3fddb2e

Update copy-pasta comment in filename.d

5507f12

JinShil mentioned this pull request Aug 24, 2018

Cleanup and refactor some filename-related code to use D slices #8585

Merged

JinShil approved these changes Aug 24, 2018

View reviewed changes

JinShil added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Aug 24, 2018

MartinNowak merged commit 758c5ac into dlang:stable Aug 25, 2018

kinke added a commit to kinke/dmd that referenced this pull request Aug 28, 2018

Rework dlang#8542 wrt. deterministic IDs via generateIdWithLoc()

4ce08b4

kinke added a commit to kinke/dmd that referenced this pull request Aug 29, 2018

Rework dlang#8542 wrt. deterministic IDs via generateIdWithLoc()

9a5b054

kinke added a commit to kinke/dmd that referenced this pull request Aug 29, 2018

Rework dlang#8542 wrt. deterministic IDs via generateIdWithLoc()

5c6e203

atilaneves deleted the fix-traits-get-unittest branch September 12, 2018 10:10

kinke added a commit to kinke/dmd that referenced this pull request Feb 2, 2019

Rework dlang#8542 wrt. deterministic IDs via generateIdWithLoc()

bced5bf

kinke mentioned this pull request Apr 14, 2019

Rework #8542 wrt. deterministic IDs via generateIdWithLoc() #8633

Merged

dlang-bot added a commit that referenced this pull request Apr 15, 2019

Merge pull request #8633 from kinke/stableIDs

07f379f

Rework #8542 wrt. deterministic IDs via generateIdWithLoc() merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

Uh oh!

Conversation

atilaneves commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

wilzbach commented Aug 7, 2018

Uh oh!

atilaneves commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atilaneves commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

atilaneves commented Aug 7, 2018 •

edited

Loading

dlang-bot commented Aug 7, 2018 •

edited

Loading

atilaneves commented Aug 7, 2018 •

edited

Loading

jacob-carlborg Aug 8, 2018 •

edited

Loading

JinShil Aug 17, 2018 •

edited

Loading

JinShil Aug 17, 2018 •

edited

Loading