[BLOCKED][WIP] Don't instantiate unittest templates in libraries

[marler8997]

NOTE: This change no longer has a functional impact on users other than they should see a performance increase when compiling with -unittest.

[dlang-bot]

Thanks for your pull request and interest in making D better, @marler8997! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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 "master + dmd#8124"

[marler8997]

This PR modifies the code to determine if an import should be compiled earlier than before (right after parsing the module declaration instead of after parsing the whole file). This way, we know if we are going to compile unittests before parsing the file. To that end, I found out that the parser depends on importedFrom being set correctly meaning we can no longer "hijack" this field by resetting it when an imported module is compiled. The simple solution is to add a new field that indicates when an imported module should be compiled, and to leave the importedFrom field intact so it can be used during the parse stage.

[wilzbach]

This is really cool. Thanks you so much for pushing this!!
I just assume that this is still WIP, but one thing I already noticed is that you are missing tests.
A bash script + grep should be able to check for this?

[marler8997]

Yeah still WIP (making sure the concept works). Thanks for the suggested test...I'll see if I can get some tests going.

[andralex]

cc @WalterBright @atilaneves @schveiguy

[schveiguy]

Very cool! A few things:

1. How does this affect version(unittest)? My understanding is that this will still compile those in, as it seems you are only skipping parsing/semantic on unittest blocks.
2. It has been suggested in the forums that __traits(getUnitTests) override the fact that it's not a root, but the way you have done it makes this very difficult. But I may not understand the way the compiler works here.

On the other hand, __traits(getUnitTests) may work just fine for modules that are actually passed on the command line (which may be all @atilaneves needs).

[schveiguy]

How does this affect version(unittest)?

Did more research, indeed the unittest version is more blunt, it determines if the scope should be included based on the global flag only. I can think of a way to make it work more like I had expected, but now I'm not sure if that's the correct course.

There are cases when version(unittest) alters the layout of a type, but this really is a bad idea in most cases. version(unittest) is bound to be rare, and it may be bad to set the precedent of having a version that isn't global for the compilation.

The one case where I'm concerned is when version(unittest) does extra imports, and therefore causes a slowdown. However, we can migrate code away from that and towards using local imports inside unittests (which would be elided by this PR). It's a different case for the unittests themselves, as those are functions that definitely aren't going to be emitted for modules that are simply imported.

[marler8997]

We could make version(unittest) be true only when the module's unittests are being compiled, and then create a new version for the global unittest flag version(some_unittests_enabled)?

[jacob-carlborg]

If I understand correctly, the unittest blocks are basically implemented as functions. At some point the compiler will merge all those functions into one function and make that, combined unit test function, available in the module info for later use at runtime.

An alternative approach that, I think, would not affect __traits(getUnitTests) would be to compile everything as usual, but don't create the combined unit test function.

[marler8997]

The slowdown is in analyzing the unittests, not "combining them/generating code" as you describe. And I don't think it "combines" them into one function. I think it leaves them as separate functions and just calls them in succession.

[jacob-carlborg]

The slowdown is in analyzing the unittests, not "combining them/generating code" as you describe.

Yes. Not sure what I was thinking.

And I don't think it "combines" them into one function. I think it leaves them as separate functions and just calls them in succession.

That might be the case.

[quickfur]

I didn't look too closely at how this was implemented, but the fact that it is implemented gets a big applause from me. :-) We badly, badly need this. Thanks for taking the time to do it rather than merely talk about it, like the rest of us. :-D

[marler8997]

NOTE: THIS IDEA IS NOT BACKWARD COMPATIBLE, SEE OTHER IDEA IN OTHER COMMENT BELOW

Or maybe we reverse the logic, have version(unittest) mean that we are compiling with the -unittest flag...and then create version(unittesting_this_module) mean that the compiler is compiling unittests for the current module. This is a distinction we should support because there are times when many unittests share the same common functions or data structures.

So...code that is used outside the module meant for unittesting would go in one type type of version, and code within the module meant for unittestting would go in another type of version.

Given this description, maybe another good name would be

version(unittest_library)
{
    // gets compiled if `-unittest` was given on the command line
    // provides code to support unittests inside and outside this module
}

version(unittest)
{
    // only gets compiled if this module is being unittested
    // provides code to support unittests in this module only
}

[jmdavis]

I don't know exactly what we want to do with version(unittest), but I can guarantee that if we make it so that version(unittest) is not compiled in when the module is imported, then existing code will break - e.g. when I've had helpers for unit tests, I've sometimes put them in a separate module where the unit tests can then import them, and I've generally put them in a version(unittest) block, because they're intended to only be used as part of the unit tests. So, blindly making it so that version(unittest) isn't compiled in when the module is imported will break code.

Now, we can choose to do something like print a deprecation message if you ever import anything that's in a version(unittest) block (though I don't know how easy that would be to implement), and then later, we can treat version(unittest) as only applying when the module is directly compiled with -unittest, but I'm opposed to simply changing the behavior of version(unittest) without some sort of deprecation mechanism.

Regardless, if we want to make it so that version(unittest) only affects the module being compiled and not those it's importing, I don't think that we then need to add a version identifier for unit tests that affects all modules. Any project looking to do something like that can create its own version identifier, and in that case, it won't spill over into other projects.

[marler8997]

@jmdavis

Yeah, this PR takes the conservative approach, leaving inversion(unittest) for now. The idea is maybe later we could add another version(unittesting_this_module) that would only get instantiated if both the -unittest flag is provide AND unittests are being compiled for this module. This is where you could define code that is common between unittests in the same module.

Without this special version, each unittest block would have to define all their imports/data structures/functions that aren't included in the non-unittest code, none of it could be shared between unittests in the same module.

version(unittest)
{
    // gets compiled if -unittest is given on the
    // command line
}
version(unittesting_this_module)
{
    // gets compiled if -unittest is given on the
    // command line AND unittests are being
    // compiled for this module
}

[jmdavis]

The slowdown is in analyzing the unittests, not "combining them/generating code" as you describe. And I don't think it "combines" them into one function. I think it leaves them as separate functions and just calls them in succession.

If you have an assertion fails in a unittest block, and you look at the stack, then you can see the functions involved, and the function for the unittest block includes the line number, clearly indicating that there is indeed a function specifically for that specific unittest block. e.g.

core.exception.AssertError@q.d(8): unittest failure
----------------
??:? _d_unittestp [0x42be75]
??:? void q.__unittest_L5_C1() [0x42bced]
??:? void q.__modtest() [0x42bd90]
??:? int core.runtime.runModuleUnitTests().__foreachbody2(object.ModuleInfo*) [0x42e77b]
??:? int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda2(immutable(object.ModuleInfo*)) [0x4351a6]
??:? int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))).__foreachbody2(ref rt.sections_elf_shared.DSO) [0x444a75]
??:? int rt.sections_elf_shared.DSO.opApply(scope int delegate(ref rt.sections_elf_shared.DSO)) [0x4329e0]
??:? int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))) [0x444a01]
??:? int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)) [0x43517d]
??:? runModuleUnitTests [0x42e559]
??:? void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).runAll() [0x42c190]
??:? void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).tryExec(scope void delegate()) [0x42c11b]
??:? _d_run_main [0x42c086]
??:? main [0x42bda1]
??:? _start [0x42bbae]
??:? ??? [0x68dfff]
1/1 unittests FAILED

[atilaneves]

Unless it's no longer true, I'd keep the part of the comment that says "This is used to determine the ownership of templaet instantiation".

[marler8997]

The information was just moved to the isRoot function.

[atilaneves]

If the next module is printingUnittest, this one should be pragmaMsgVersionUnittest, no?

[marler8997]

meh, not a big deal, kind of a long name

[atilaneves]

The error code for this isn't 0, but it won't cause the whole script to fail.

[marler8997]

ah...I see you're not familiar with the latest work on DMD's Bash tests...see test/tools/sh_do_test.sh :)

[JackStouffer]

Are there bug reports for these mangling issues?

[marler8997]

I saw a bunch of bugs listed in phobos/dip1000.mak, but it's hard to say exactly which the bugs are exposed by this PR. There's multiple problems and I think I know the reasons for some of them but there's more that I haven't investigated yet....

[schveiguy]

So, I have figured out (somewhat) the issues here, and we have this peculiar notion that when
-unittest is passed on the command line, templates that appear to be handled by imported modules are "moved" to a root module, exactly as @marler8997 was saying.

An example:

importme.d:

module importme;
struct S(T)
{
   T t;
   void method() {}
}

void baz(S!int) {}

void foo()() {
    S!int x;
    x.method();
}

importer.d:

import importme;

void main() { foo(); }

Compile with dmd importer.d, and you get a linker failure -- method cannot be found. What I'm guessing happens here is that the compiler assumes that since importme has instantiated S!int, it doesn't need to put the code for method inside the importer.o object, since it should technically be in importme.o.

But when I execute dmd -unittest importer.d it links! This is because even though importme.d instantiates S!int, it's "moved" to the root module, meaning importer now contains the object code for method.

Indeed, compiling separately, both .o modules contain a definition of method. The linker simply discards the extra definition (I guess?).

Doing some research, I found that this "moving" actually ALWAYS used to happen without -unittest or -debug (even before this if statement was introduced). Here is the original pull that removed the "moving" situation for all code but certain cases (including unittests inexplicably): #2550

And here is the one that adds moving for -debug back into the mix: #2661

So IMO, I think we need to do 2 things:

1. Remove all cases in debug and unittesting code where druntime/phobos is instantiated with:
   a) different layout
   b) different symbols
2. Remove the special case "moving" of template instantiations into root modules for both -unittest and -debug.

These 2 fixes will help fix the extra imports due to template unittests in some cases, but not all. In cases where the instantiation is happening inside a root module, you will still see unittests be compiled (and even included).

Then I think we can move forward with a PR that kills semantic/emitting of unittests for imported modules. Preferably, this would allow __traits(getUnittest) to override this setting for a specific import.

I don't think we should disable version(unittest) parsing, as I think the effect would be too detrimental -- often the declarations inside these are helper functions that are useful outside the module that defines them. All we can do is clean our own house (phobos) and show people the correct way to do things.

[schveiguy]

Yeah, I think the mangling issue has to do with the lack of "moving" to root modules. Note that the debug build works! This is because -debug is passed, and you haven't turned off the moving for -debug.

What it looks like is happening is the "moving" is re-compiling the code as if it were in the root module, and this masks any incorrect symbols in the library object. So @marler8997 is correct that this is "uncovering" dip1000 issues, albeit probably only when doing separate compilation. It's kind of alarming that we have bugs in separate compilation, which is how we compile unittests for phobos, but we ship the code compiled all-at-once! And all of it is masked over by moving all template instantiations local 😝

I wonder how much the autotester will speed up when it's properly ignoring imported templates it shouldn't be compiling.

[wilzbach]

So @marler8997 is correct that this is "uncovering" dip1000 issues, albeit probably only when doing separate compilation.

For the record, this issue of separate mangling for dip1000 is "well-known" (at least among Phobos developers) and definitely a blocker for it (sadly not the only one), because it means that we once we ship Phobos compiled with -dip1000 everyone has to use -dip1000 too (and of course in the other way around: as long as we don't ship a -dip1000 version of Phobos, it's really difficult to us Phobos with -dip1000).

[marler8997]

because it means that we once we ship Phobos compiled with -dip1000 everyone has to use -dip1000 too

I'm sure the path would be to make dip 1000 enabled by default before shipping phobos with dip1000 enabled.

[schveiguy]

Am I understanding that we are running the auto tester with dip1000, but shipping without it? Is this really a good thing? I mean, dip1000 isn't being shipped, isn't properly working, yet it's impeding other work (such as this PR). Worse yet, if we have issues that are somehow masked by dip1000, they will get through the tester.

I understand and agree that the dip1000 issues are important, but at the cost of preventing other things from being worked on, I'd prefer it be tested in the background instead of being a blocker. You could run only the master branch with -dip1000 and still see when things went bad. Most of the time, dip1000 issues aren't going to regress anyway.

[wilzbach]

Am I understanding that we are running the auto tester with dip1000, but shipping without it?

Partially. Only on some machines.

Is this really a good thing?

Yes (see answers below).

Worse yet, if we have issues that are somehow masked by dip1000, they will get through the tester.

That's why it's only run on some CIs.

I understand and agree that the dip1000 issues are important, but at the cost of preventing other things from being worked on

If that's really blocking this PR, you could remove the $(aa[$(subst /,.,$(basename $<))]) substitution from this line. Though we would need to ensure that then the *.test targets are run at dlang/dmd too (at the moment they are only run at dlang/phobos via CircleCi). Shouldn't be much work though.

I'd prefer it be tested in the background instead of being a blocker.

No warnings don't work. Everyone ignores them.
A recent example: #8018

You could run only the master branch with -dip1000 and still see when things went bad.

Not sure how this be different from the status quo? (dip1000.mak isn't in stable yet).
Also -dip1000 modules needs to be enforced, because (1) otherwise it will regress and (2) it makes approving -dip1000 PRs a lot easier.

Most of the time, dip1000 issues aren't going to regress anyway.

No. People love to break -dip1000, because no one (except Walter maybe), really understands it.
There were quite a few regressions until dlang/phobos#6278.

[schveiguy]

because no one (except Walter maybe), really understands it.

This isn't encouraging when it's a blocker in some cases. How can there be regressions if it wasn't being tested before that PR?

You could run only the master branch with -dip1000 and still see when things went bad.

Not sure how this be different from the status quo?

I mean not run it on any PRs, just the master build. And I don't mean the real master, I mean like a separate build, similar to how @yebblies had his own build branch until we flipped the DDMD switch.

This is similar to the broken cycle situation we had. Until the cycle tester was fixed, any PR could mysteriously break the build for weird reasons, and then the PR is rejected for reasons having nothing to do with the PR changes. Having the status quo be a broken build is not conducive to productivity.

That's why it's only run on some CIs.

Which ones? The only ones that matter here are the autotester (where it seems every platform but Windows is using -dip1000). It'd be fine if there was a different CI that tested dip1000 issues and we could override it individually, but the time to build Phobos with separate compilation probably rules that out.

[wilzbach]

Though we would need to ensure that then the *.test targets are run at dlang/dmd too (at the moment they are only run at dlang/phobos via CircleCi). Shouldn't be much work though.

-> dlang/phobos#6443
@marler8997 - this should unblock your PR?

[marler8997]

I'll check to see if that phobos PR fixes the issues. I also pulled out some of the changes in this PR and moved them into here #8151 ...waiting for that one to get merged.

[CyberShadow]

Would be nice to expand this with an explanation of which situations are adversely affected by this change, and what are the steps for fixing said situations.

[marler8997]

Actually...I should update this. Turns out the unittests already weren't being compiled in (thanks @schveiguy for looking into this). What's actually happening is that version(unittest) blocks are importing and instantiating templates that aren't instantiated without -unittest. Then these instantiations are being moved to root modules. With this new information, as far as I know this change shouldn't have any "functional" impact on users other than a performance increase when compiling with -unittest.

[CyberShadow]

So this just fixes sync between whether unittest blocks and version(unittest) blocks are being compiled, right?

[marler8997]

Actually no. This is preventing template instances inside libraries from being moved to root modules when compiling with -unittest. This is what's causing the compilation slowdown because the template instances that are being moved cause ALOT of phobos modules to be loaded and analyzed that wouldn't be otherwise.

version(unittest) blocks still get analyzed, however, @schveiguy is working on a phobos PR that should prevent those blocks from causing too much slowdown. This PR along with the phobos PR should be everything we need to make -unittest compile just as fast as without -unittest (when the app has no unittesting itself).

NOTE: this PR is also adding a small optimization where it won't even create a parser AST for the code inside unittest blocks. But the unittests don't get included even if they are parsed.

[marler8997]

I slightly modified dlang/phobos#6443 (see my comment in there Seb) and was able to successfully run the phobos unittests. Good stuff.

[marler8997]

@schveiguy

Any progress with phobos? Could you create a WIP PR with what you have now so we can monitor progress?

[schveiguy]

Any progress with phobos?

I want to split my single giant commit into smaller PRs for easier review. Will do soon.

[dnadlinger]

Here is the original pull that removed the "moving" situation for all code but certain cases (including unittests inexplicably): #2550

As far as "inexplicably" goes: The reason -unittest disables elision of template instances is that selectively compiling only some files with -unittest (or -debug) violates what otherwise is D's compilation model, namely that all modules directly or indirectly imported are in fact compiled and linked, and that all were compiled with the same version flags.

This assumption is what allows template instances to be elided if they are already known to be used in an imported module. For example, if Phobos already uses sort!(int[]) internally, the instance doesn't need to be compiled again in client code, speeding up compilation.

Obviously, this can only work if those templates are actually emitted into the library/other object files linked in. However, typical libraries (including Phobos) instantiate more/other templates in unit test builds than; hence -unittest flags would need to match between the time the modules are imported and when the object files linked to were built.

This is obviously problematic for unit testing user code that links against Phobos, since we don't ship a Phobos version with -unittest enabled (plus we would need to change the test runner not to run imported tests by default). The same problem would of course occur for any other D library.

Hence, as a concession to practicality a hack was introduced, whereby template instances are never assumed to be present in imported modules for -unittest and -debug builds.

---

This is a huge hack, and the reliance of user code on these Frankensteinian builds to work has created all sorts of trouble for LDC in the past when it comes to cross-module inlining and things like that.

It might seem that changing -unittest such that it only affects currently compiled modules would be a solution. While no longer triggering version (unittest) in non-root modules would be a breaking change, it might arguably be worth it, plus there could be a deprecation period for accessing symbols versioned as such.

However, note that this would also make any -unittest-based coordination between multiple compiler invocations impossible. For example, say the layout of a struct used across multiple modules in a library is changed for debugging purposes. If separate compilation is used for that library, the change would have to been triggered as a separate version, as there is no way for the compiler to know which modules should be affected by -unittest (unless we introduce a syntax such as -unittest=mylib.*).

[schveiguy]

@klickverbot I think I understand what you are saying. An imported module may instantiate more templates when compiled with -unittest. The mechanism that says "I don't need to instantiate this template locally because it's already done inside the import" will NOT move the instantiation local (given this PR is accepted). So if you haven't actually compiled the imported module with -unittest, then there is no instantiation anywhere, and you get linker errors.

BUT, with this PR, the only place this should happen is in version(unittest) blocks, since unittests in imported modules aren't parsed or examined. So what we need to ensure is that version(unittest) blocks do not instantiate any templates that aren't already instantiated. I personally think we should get rid of as much version(unittest) as possible in any case. I've been doing so, but without this understanding, so may need a revisit once I'm done.

I'm very concerned about eliminating version(unittest) in imported modules, as it's used in some places to provide common harnesses for unittesting. For example, allocator.common.testAllocator.

[wilzbach]

As the Phobos PR has been merged this is starting to pass on some machines, but interestingly only for the 32-bit ones.
It still results in a linker error for 64-bit builds:

generated/freebsd/release/64/unittest/libphobos2-ut.so: undefined reference to `_D3std4conv__T7toCharsVii10TaVEQBd5ascii10LetterCasei1TkZQBrFNaNbNiNfkZ6Result8popFrontMFNaNbNiNfZv'

[marler8997]

The demangled symbol looks like this:

pure nothrow @nogc @safe void std.conv.toChars!(10, char, 1, uint).toChars(uint).Result.popFront()

Something to look into...

[schveiguy]

A shame we can't get this to work.

Likely the symbol failure is because DMD gets confused about which symbols are already generated. The fact that it's a uint, and a 64-bit system has size_t being ulong, makes me think that in the 32-bit case it doesn't fail because the uint and size_t versions are the same, and size_t is used somewhere else that makes it link.