Fix Issue 18419 - make all Phobos unittests version(StdUnittest)

[n8sh]

This is the purpose for which version(StdUnittest) was added. The pain is one-time, the benefit is forever. It is also a good practice for other library authors to copy.

[dlang-bot]

Thanks for your pull request, @n8sh! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	18419	make all Phobos unittests version(StdUnittest)

[jmdavis]

IMHO, if we need to do this, there is something very wrong about how D handles unit tests.

[CyberShadow]

Indeed. This change would fix a very real problem, but ... perhaps this is better fixed elsewhere.

[n8sh]

This won't interfere with someone making unittest-related improvements to other parts of the D toolchain. It may also take some of the pressure off of whoever is working on that, so they can take their time to get the design right instead of feeling like they have to rush something out the door.

[wilzbach]

For reference, the discussion is here: #6142

This change would fix a very real problem, but ... perhaps this is better fixed elsewhere.

Ok, alternatives:

• revert StdUnittest for the next release
• avoid emitting unittest blocks with -deps (or potentially other flags)

Better ideas?

---

@FeepingCreature - any chance we can add a unittest for your -deps issue? Or maybe you can simple add your project to the Project Tester?

[adamdruppe]

so now when you build the docs with dmd, you need -version=StdUnittest yes?

but i say don't let the perfect solution be the enemy of a workable stopgap. we can always fix root cause later and revert this at that time

[FeepingCreature]

edit: The important part: Yes, this pr fixes it, and another issue beside.

@FeepingCreature - any chance we can add a unittest for your -deps issue? Or maybe you can simple add your project to the Project Tester?

When you say "your project", I think you still underestimate the scale of the problem.

This is what it takes on master, to cause a failing build:

echo "import std.stdio; void main() { }" > test.d
dmd -unittest -deps test.d

Basically every phobos module is currently broken with -unittest -deps.

A feasible testcase would just consist of a single file that imports every phobos module, built with -unittest -deps. Though be warned: this may consume a lot of memory to try, if it doesn't error out immediately. If you have less than 12GB free, hold your fingers on ctrl-C and keep a watchful eye on top. edit: This issue is also fixed by this pull request! Woo hoo!

edit:

$ find dlang/phobos/std/ -name \*.d |\
sed -e "s@.*std/@std/@" -e "s@/@.@g" -e "s@\.d\$@, @" |\
xargs echo |\
sed -e 's/,$/; void main() { }/' -e 's/^/import /' -e 's/\.package//g' > test.d
dlang/dmd/generated/linux/release/64/dmd -unittest -deps test.d

I like regex! (Too bad importing std.regex increases compiletime by 12 seconds in release builds...)

Edit: tried that command line with this pr, and it seems to fix the issue, though it results in a possibly unrelated linker error.

[n8sh]

@FeepingCreature

Edit: tried that command line with this pr, and it seems to fix the issue, though it results in a possibly unrelated linker error.

I ran your script in the master branch, except without -unittest -deps. I got what I presume is the same linker error as you, so it's not related to this pull request.

Undefined symbols for architecture x86_64:
"_D3std12experimental9scripting12__ModuleInfoZ", referenced from:
_D4test12__ModuleInfoZ in test.o

[wilzbach]

You need to use the freshly compiled Phobos library when doing anything with master - so yes the "error" is unrelated.

[jmdavis]

Man. I was just forced to do this sort of thing with dxml, because folks where getting weird linker errors when they built the unit tests for their own projects which used dxml. For some reason, not all of the symbols used by dxml's unit tests were available - though dub seems to take this a step stupider than dmd does on its own. dub seems to build the actual unittest target for all dependencies not just have problems with the imports being affected by the -unittest flag. This whole situation is a mess.

[wilzbach]

This whole situation is a mess.

Agreed and it looks like we need to make a decision pretty soon (the master branch off will happen is supposed to happen in two days).

[JackStouffer]

Let's get an executive decision on this from @andralex and move on.

[andralex]

As a simple consideration, any change that adds an adornment to 4000+ lines of code in a 300 KLOC project should be approached with suspicion that there probably are 10 wrong lines of code someplace else. It's nice to see that other commenters noticed that as well.

Neither the bug nor the PR lists the problem that this change is addressing, so I'll rely on the info in #6142 and links from there. Far as I understand:

• We added version(StdUnittest) because we wanted certain things (e.g.imports, test-specific global data, unittests inside templates) to only be compiled when Phobos was being unittested, not the user code importing Phobos. For that, version(unittest) was deemed insufficient because it is active during user-level unittesting.
• Once StdUnittest started getting used (especially in imports), code in template unittests got out of sync with the versioned imports.
• There's also a DIP82 proposing a mechanism for preventing unittests in templates from being run.

There'd be a long discussion, but in short:

• Unittests in templates are a good feature because they test a template in conjunction with the user-defined types that it uses. (We've actually gotten a kudos from Matthias Felleisen himself for that.)
• In the rare cases when they shall be disabled, we already have a number of mechanisms for that (version, static if...) A language feature is not necessary; DIP82's motivation is tenuous.
• We should use StdUnittest with extreme caution and almost never for guarding imports (instead, imports should be moved down into the unittests needing them). At present there are 18 uses of StdUnittest in Phobos. If we get on them, perhaps we conclude there are 18 too many and get rid of it. That would be a great improvement.
• Please exercise good engineering sense when approaching problems. This huge change is not a one-time pain, it's an ongoing pain due to redundancy and visual noise.

[CyberShadow]

Once StdUnittest started getting used (especially in imports), code in template unittests got out of sync with the versioned imports.

That wasn't what I understood the main problem to be. My understanding was that Phobos unittests participate in building user code when -unittest is enabled, and that impacts compilation time. Sometimes the impact is egregious (e.g. with std.regex) and reaches the point where we need a mechanism to prevent Phobos unittests from being compiled at all, except when we're explicitly testing Phobos itself.

To demonstrate:

$ cat test.d
import std.regex;

$ time dmd -c test.d          
dmd -c test.d  0.21s user 0.01s system 98% cpu 0.226 total

$ time dmd -c -unittest test.d
dmd -c -unittest test.d  0.76s user 0.10s system 99% cpu 0.863 total

[n8sh]

Unittests in templates are a good feature because they test a template in conjunction with the user-defined types that it uses. (We've actually gotten a kudos from Matthias Felleisen himself for that.)

A benefit of using version(StdUnittest) instead of a change to dub or dmd is that we can selectively leave tests that like enabled.

[quickfur]

@andralex:

On the whole, I agree that version(StdUnittest) is a crutch, and an ugly one, and that we should exercise caution in taking that route. OTOH, this to me is a sign that possibly a language enhancement is necessary.

Please let's not confuse this issue with template unittests, even though the two are related. Your argument that template unittests being run in conjunction with user types is a good thing, is actually misrepresenting the real situation. Yes, in the cases where the unittest actually tests the template with the given user types, this is something beneficial. However, the majority of unittests in Phobos are of the kind:

unittest
{
    auto testData = [ 1, 2, 3 ];
    auto range = templateBeingTested(testData);
    assert(range.equals(expectedOutput));
}

Most of the time, such unittests are put inside the template because of the ddoc'd unittest feature: it automatically turns into a code example attached to the template's docs, which is a wonderful thing, because that ensures that code examples in the docs are actually tested by the autotester and verified to be runnable (and have correct results).

However, since this is inside a template, that means the same unittest, testing the same type of test data, is instantiated n times, where n is the number of different types user code might call it with. That's n-1 redundant unittest instantiations, and in user code. Does a user project really care that templateBeingTested works correctly for an array of ints? Of course not -- the autotester has already tested and verified that this case works correctly, and for all supported OS/platforms. None of the benefits you mentioned apply here: the unittest doesn't even instantiate the template with any user type; it always tests the template with an array of ints. Worse yet, it redundantly repeats this test n times, as if once on the autotester wasn't already enough to verify that the template works for this particular case. And to put this into more perspective: this is done for every user project out there that uses this template. So we're essentially making D users pay for this unittest n*numDUsers times, where once on the autotester ought to be enough already.

And this isn't even the issue we're trying to address here. The issue here is that some Phobos unittests are very expensive to run, like those in std.regex. As @DmitryOlshansky has already pointed out, it causes a degradation in compilation time of an order of magnitude, and for what? For running unittests that the D autotester has already run to death however many times it does every single time a PR is merged into Phobos. Do these unittests actually add more value? No. All the user did was to import std.regex -- he hasn't even begun using the module yet, and already he's paying for (1) decreased compilation time (so much for "fast code fast"), (2) unittest bloat in his executable, (3) runtime penalty of executing Phobos tests that, ostensibly, has already been tested to death by the D autotester, and besides isn't even the responsibility of user code to test.

And this goes beyond just Phobos alone. The way unittests are currently implemented, the same behaviour happens for all third party libraries that user code may import. If some library author A ships a D library L that contains unittests, then every single time some downstream user U imports L and compiles his code with -unittest, he has to pay for decreased compilation time, unittest bloat, and runtime penalty for running tests that ought to have been the responsibility of library author A instead. This seems to be an unreasonable cost when user U's intention is only "I want to run my own unittests in my own code". Currently, there is no way for him to do this -- either he doesn't compile with -unittest at all, or he compiles with -unittest and the very act of importing a module itself already causes library L's unittests to be compiled into his code -- unittests that he doesn't even care about because they don't concern his own code.

tl;dr: there needs to be a way for upstream library authors (including Phobos) to make it so that users compiling with -unittest don't have to pay for unittests that ostensibly said upstream library authors ought to already have run. If an in-library solution (like this PR) is not acceptable, then surely a language enhancement is in order.

[FeepingCreature]

@andralex :

Specifically, the problem I encountered is that Phobos unittests participate in user code when -unittest -deps is enabled, because the compiler has to semantically evaluate the bodies of (plain!) unittests in order to determine whether they contain imports. As such, moving imports into unittests will still leave me with horrendous compile times and memory usage (nine gigabytes for hello world) with this combination of flags (though at least it will compile) because DMD is not smart enough to determine that it does not need to output dependency information for modules in an external library, and does not have any mechanism to inform it that dependencies from non-templated Phobos unittests are irrelevant to the program being compiled. StdUnittest imports and unittests would have solved this issue while still providing the StdUnittest benefit of not requiring imports for Phobos unittests that are not emitted anyways. (Since Phobos is an external library.)

Imports in unittests in Phobos would still run into the -deps -unittest issue that DMD would have to recursively consider these imports to emit dependency information, whether or not it was actually going to run the unittests, because it couldn't tell that std/stdio.d was in a library and we didn't care about its unittests.

[andralex]

@FeepingCreature can the ddoc unittests be versioned with version(StdDdoc)?

[andralex]

@CyberShadow I agree the increase of build time in unittests is unpleasant. It seems the right long-term solution to that is making the -unittest flag more modular. Another possibility is curate Phobos imported files upon distribution. I don't think this solution is the right response.

[andralex]

tl;dr: there needs to be a way for upstream library authors (including Phobos) to make it so that users compiling with -unittest don't have to pay for unittests that ostensibly said upstream library authors ought to already have run. If an in-library solution (like this PR) is not acceptable, then surely a language enhancement is in order.

I agree. I think we need to experiment with making -unittest modular. I assume we'll run into issues, the question is how bad they are.

[quickfur]

Actually, I already have an idea that's quite simple (barring unforeseen side-effects), that should address most, if not all, of the concerns raised here:

Basically, change the compiler so that -unittest only emits unittests that are either inside a template, or found in modules that are currently being compiled on the command-line. I think something similar was done last year or the year before, where certain templates won't actually instantiate until you actually compile the module they're found in (sorry, I don't remember exactly what was done, so I'm a bit hazy on the details).

Presumably, any unittests that user code might be concerned about would be contained in a module that eventually would be passed to the compiler on the command-line, so they will end up being instantiated somewhere. Third-party libraries (or Phobos itself) would be separately compiled, and presumably distributed as binaries, so presumably they won't end up on the command-line (at least the .d files won't), so library-specific unittests will be automatically left out.

Perhaps I should reserve some time to write up a DIP about this.

[CyberShadow]

@quickfur Nice idea, though it won't work for e.g. targetType "sourceLibrary" Dub libraries, or libraries used with rdmd.

[JackStouffer]

I think the best option is to revert StdUnittest. I don't think adding in this cruft/workaround is a viable alternative to fixing the compiler in this area.

I'm going to go head and close this, and open a new PR on stable that does that. I think this conversation has been stalled for too long, and we need to fix this before we ship code with StdUnittest in it.

[JackStouffer]

#6202