Skip to content

Fix issue 16995 - __traits(getUnitTests) works with separate compilation #6727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
WalterBright merged 2 commits into from
Sep 30, 2017
Merged

Fix issue 16995 - __traits(getUnitTests) works with separate compilation #6727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f527810
Fix Issue 16995 - __traits(getUnittests) doesn't work with separate c…
atilaneves Apr 24, 2017
0f26862
Address review comments
atilaneves Sep 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/ddmd/dmodule.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,12 @@ extern (C++) final class Module : Package
int isDocFile; // if it is a documentation input file, not D source
bool isPackageFile; // if it is a package.d
int needmoduleinfo;

/**
How many unit tests have been seen so far in this module. Makes it so the
unit test name is reproducible regardless of whether it's compiled
separately or all at once.
*/
uint unitTestCounter; // how many unittests have been seen so far
int selfimports; // 0: don't know, 1: does not, 2: does

/*************************************
Expand Down
6 changes: 6 additions & 0 deletions src/ddmd/dsymbolsem.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5144,6 +5144,12 @@ extern(C++) final class DsymbolSemanticVisitor : Visitor

override void visit(UnitTestDeclaration utd)
{
// The identifier has to be generated here in order for it to be possible
// to link regardless of whether the files were compiled separately
// or all at once. See:
// https://issues.dlang.org/show_bug.cgi?id=16995
utd.setIdentifier();

if (utd.semanticRun >= PASSsemanticdone)
return;
if (utd._scope)
Expand Down
44 changes: 33 additions & 11 deletions src/ddmd/func.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3156,16 +3156,6 @@ extern (C++) final class InvariantDeclaration : FuncDeclaration
}
}

/***********************************************************
* Generate unique unittest function Id so we can have multiple
* instances per module.
*/
private Identifier unitTestId(Loc loc)
{
OutBuffer buf;
buf.printf("__unittestL%u_", loc.linnum);
return Identifier.generateId(buf.peekString());
}

/***********************************************************
*/
Expand All @@ -3178,7 +3168,10 @@ extern (C++) final class UnitTestDeclaration : FuncDeclaration

extern (D) this(Loc loc, Loc endloc, StorageClass stc, char* codedoc)
{
super(loc, endloc, unitTestId(loc), stc, null);
// Id.empty can cause certain things to fail, so we create a
// temporary one here that serves for most purposes with
// createIdentifier. There is no scope to pass so we pass null.
Copy link
Member

@WalterBright WalterBright Sep 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes me uneasy to start with one identifier, and finish with another. What fails with an empty identifier?

Copy link
Contributor Author

@atilaneves atilaneves Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't like having to do it either, my first attempt was to use Id.empty, but when I did that the unit tests failed. I can't remember which ones or why now, but they do.

super(loc, endloc, createIdentifier(loc, null), stc, null);
this.codedoc = codedoc;
}

Expand All @@ -3189,6 +3182,35 @@ extern (C++) final class UnitTestDeclaration : FuncDeclaration
return FuncDeclaration.syntaxCopy(utd);
}

/**
Sets the "real" identifier, replacing the one created in the contructor.
The reason for this is that the "real" identifier can only be generated
properly in the semantic pass. See:
https://issues.dlang.org/show_bug.cgi?id=16995
*/
final void setIdentifier()
{
ident = createIdentifier(loc, _scope);
}

/***********************************************************
* Generate unique unittest function Id so we can have multiple
* instances per module.
*/
private static Identifier createIdentifier(Loc loc, Scope* sc)
{
OutBuffer buf;
auto index = sc ? sc._module.unitTestCounter++ : 0;
buf.printf("__unittest_%s_%u_%d", loc.filename, loc.linnum, index);
Copy link
Member

@andralex andralex Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a difference without a distinction between %u and %d - just use %u for both, or better yet %s for all (assuming they're all unsigned as they should).

Copy link
Member

@WalterBright WalterBright Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

printf doesn't work with %s for integers :-)

Copy link
Member

@andralex andralex Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait I thought it's writef...


// replace characters that demangle can't handle
auto str = buf.peekString;
for(int i = 0; str[i] != 0; ++i)
if(str[i] == '/' || str[i] == '\\' || str[i] == '.') str[i] = '_';
Copy link
Member

@WalterBright WalterBright Sep 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More efficient as:

foreach (ref c; buf.peekSlice())
{
    if (c == '/' || c == ...) c = '_';
}

Copy link
Member

@WalterBright WalterBright Sep 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even: buf.peekSlice()[11 .. $-4] :-)

Copy link
Contributor Author

@atilaneves atilaneves Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

peekSlice returns const(char)[] so that doesn't compile. The latter version is also too hardcoded, even though I think it's unlikely anybody will change the way unit tests are named in the future.

Copy link
Member

@andralex andralex Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's one of those places where among is useful (though we can't use it here)

Copy link
Member

@WalterBright WalterBright Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so that doesn't compile

Oopsie-daisy!

Copy link
Member

@ibuclaw ibuclaw Nov 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not strong enough and various testsuite tests fail if compiling to assembly (such as what gdc does).

This logic should match what is done for pragma(mangle).

dmd/src/ddmd/dsymbolsem.d

Lines 3033 to 3045 in 32830de

if (c < 0x80)
{
if (c >= 'A' && c <= 'Z' || c >= 'a' && c <= 'z' || c >= '0' && c <= '9' || c != 0 && strchr("$%().:?@[]_", c))
{
++i;
continue;
}
else
{
pd.error("char 0x%02x not allowed in mangled name", c);
break;
}
}

Copy link
Member

@ibuclaw ibuclaw Nov 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance, unittests compiled that are inside the directory extra-files fail because the assembler stumbles over on the dash symbol.


return Identifier.idPool(buf.peekSlice());
}

override AggregateDeclaration isThis()
{
return null;
Expand Down
7 changes: 6 additions & 1 deletion src/ddmd/module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,12 @@ class Module : public Package
int isDocFile; // if it is a documentation input file, not D source
bool isPackageFile; // if it is a package.d
int needmoduleinfo;

/**
How many unit tests have been seen so far in this module. Makes it so the
unit test name is reproducible regardless of whether it's compiled
separately or all at once.
*/
unsigned unitTestCounter;
int selfimports; // 0: don't know, 1: does not, 2: does
bool selfImports(); // returns true if module imports itself

Expand Down
1 change: 1 addition & 0 deletions test/compilable/imports/module_with_tests.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
unittest {} unittest {}
13 changes: 13 additions & 0 deletions test/compilable/issue16995.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// EXTRA_SOURCES: imports/module_with_tests.d
// REQUIRED_ARGS: -unittest
// COMPILE_SEPARATELY


import imports.module_with_tests;

void main() {
import module_with_tests;
foreach(ut; __traits(getUnitTests, module_with_tests)) {
ut();
}
}
16 changes: 8 additions & 8 deletions test/fail_compilation/fail7848.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@
/*
TEST_OUTPUT:
---
fail_compilation/fail7848.d(35): Error: pure function 'fail7848.C.__unittestL33_$n$' cannot call impure function 'fail7848.func'
fail_compilation/fail7848.d(35): Error: @safe function 'fail7848.C.__unittestL33_$n$' cannot call @system function 'fail7848.func'
fail_compilation/fail7848.d(35): Error: @nogc function 'fail7848.C.__unittestL33_$n$' cannot call non-@nogc function 'fail7848.func'
fail_compilation/fail7848.d(35): Error: pure function 'fail7848.C.__unittest_fail_compilation_fail7848_d_33_0' cannot call impure function 'fail7848.func'
fail_compilation/fail7848.d(35): Error: @safe function 'fail7848.C.__unittest_fail_compilation_fail7848_d_33_0' cannot call @system function 'fail7848.func'
fail_compilation/fail7848.d(35): Error: @nogc function 'fail7848.C.__unittest_fail_compilation_fail7848_d_33_0' cannot call non-@nogc function 'fail7848.func'
fail_compilation/fail7848.d(35): Error: function `fail7848.func` is not nothrow
fail_compilation/fail7848.d(33): Error: nothrow function `fail7848.C.__unittestL33_$n$` may throw
fail_compilation/fail7848.d(40): Error: pure function 'fail7848.C.__invariant2' cannot call impure function 'fail7848.func'
fail_compilation/fail7848.d(40): Error: @safe function 'fail7848.C.__invariant2' cannot call @system function 'fail7848.func'
fail_compilation/fail7848.d(40): Error: @nogc function 'fail7848.C.__invariant2' cannot call non-@nogc function 'fail7848.func'
fail_compilation/fail7848.d(33): Error: nothrow function `fail7848.C.__unittest_fail_compilation_fail7848_d_33_0` may throw
fail_compilation/fail7848.d(40): Error: pure function 'fail7848.C.__invariant1' cannot call impure function 'fail7848.func'
fail_compilation/fail7848.d(40): Error: @safe function 'fail7848.C.__invariant1' cannot call @system function 'fail7848.func'
fail_compilation/fail7848.d(40): Error: @nogc function 'fail7848.C.__invariant1' cannot call non-@nogc function 'fail7848.func'
fail_compilation/fail7848.d(40): Error: function `fail7848.func` is not nothrow
fail_compilation/fail7848.d(38): Error: nothrow function `fail7848.C.__invariant2` may throw
fail_compilation/fail7848.d(38): Error: nothrow function `fail7848.C.__invariant1` may throw
fail_compilation/fail7848.d(45): Error: pure allocator 'fail7848.C.new' cannot call impure function 'fail7848.func'
fail_compilation/fail7848.d(45): Error: @safe allocator 'fail7848.C.new' cannot call @system function 'fail7848.func'
fail_compilation/fail7848.d(45): Error: @nogc allocator 'fail7848.C.new' cannot call non-@nogc function 'fail7848.func'
Expand Down
2 changes: 1 addition & 1 deletion test/fail_compilation/ice14424.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
/*
TEST_OUTPUT:
---
fail_compilation/ice14424.d(12): Error: `tuple` has no effect in expression `tuple(__unittestL3_$n$)`
fail_compilation/ice14424.d(12): Error: `tuple` has no effect in expression `tuple(__unittest_fail_compilation_imports_a14424_d_3_0)`
---
*/

Expand Down