Support parsing only specific unittests (e.g. @betterc-test)

[wilzbach]

This adds support for parsing only a subset of specifically annotated unittests from Phobos.

A potential example:

@bettercTest unittest
{
	assert(1);
}

The only downside of this approach is that we would have to define such a dummy symbol somewhere, so maybe the string approach is better:

@("betterc") unittest
{
	assert(1);
}

Anyhow, this can be decided on the PR for Phobos as the attribute support is generic enough to support both use cases.

[dlang-bot]

Thanks for your pull request, @wilzbach!

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 + tools#357"

[wilzbach]

@ZombineDev or @JinShil - you seem to be two most interested in having @betterC annotated unittests, so let's move forward with this, s.t. we can start to integrate it in Phobos?

[JinShil]

Given that this has a @betterc attribute, shoudn't the body be something like this?

version(D_BetterC)
{
    assert(4 == 4);
}
else
{
    assert(0);
}

[wilzbach]

No. The idea here is just to selectively filter out unittests. Such rewrites would just complicate the job of this extraction tool and mess up the line matching.

[JinShil]

But I don't see the test proving that the extraction tool only extracts those test with the correct attribute, as all tests in this file succeed. Though, I also don't understand the purpose of the .ext file either.

[JinShil]

Ah, now I get it. Ok it's good!

[wilzbach]

For the record: the .ext files are the result of applying the extraction script on this file.

[JinShil]

I think the name should be betterC

[wilzbach]

This is just a test to check whether the attribute filter works. It could be any attribute. See the -a betterc line in posix.mak

[JinShil]

I'm not yet convinced that this is the right approach, but I'm not convinced it's not either.

For example, perhaps what we should be doing instead is running the unittests twice (once with -betterC and once without), and then segregating the betterC-supported code from the betterC-unsupported code with version(D_BetterC). The downside of that is every bit of code that is not compatible with -betterC will need to be in a version(D_NoBetterC) scope, and I don't know if that's going to scale well.

Also, I think this should be added to druntime first before adding it to Phobos. I have my doubts that much of anything in Phobos even works in betterC given how limiting it is.

[JinShil]

Also, I don't clearly understand what this PR is implementing. Are you saying that @("betterc") and @betterc are treated identically? This PR doesn't seem to decided on what the attribute name should be, only that attributing unittests becomes possible. Anyway, please clarify.

[wilzbach]

Also, I think this should be added to druntime first before adding it to Phobos.

Well, we have been using this tests_extractor for almost two years now at Phobos, but we haven't managed to use it for druntime (see e.g. dlang/druntime#2087), so I think it's easier to start with Phobos.

I'm not yet convinced that this is the right approach, but I'm not convinced it's not either.

I think that's because what you want for druntime is a bit different to what I want to have for Phobos ;-)

The downside of that is every bit of code that is not compatible with -betterC will need to be in a version(D_NoBetterC) scope, and I don't know if that's going to scale well.

I agree, this probably won't scale well.

I have my doubts that much of anything in Phobos even works in betterC given how limiting it is.

You would be suprised, but much of std.algorithm and std.range does work in -betterC :)
https://github.com/dlang/phobos/blob/master/test/betterC/algorithm.d

Also, I don't clearly understand what this PR is implementing. Are you saying that @("betterc") and @BetterC are treated identically?

Yes, this a generic approach to it.

This PR doesn't seem to decided on what the attribute name should be, only that attributing unittests becomes possible. Anyway, please clarify.

Yes, this just implements the extraction. The decision which attribute name it should be is somewhat arbitrary and I opened a NG thread for this: https://forum.dlang.org/post/irovmywdhafdromajrwc@forum.dlang.org

[JinShil]

Other than versioning code (which has its own problems), I can't think of a better way.

Gah! I'm not 100% on this yet. I need to give it more thought. Sorry!

[wilzbach]

BTW I thought a bit more about this and instead of performing dirty hacks in the Phobos Makefile, I think it's better if we directly transform the unittest blocks in a -betterC compatible format.

Of course, on the long run it would be great if dmd could do this out of the box, but for now I think the best place to do the AST rewrites is here in the transformer as we already have access to the AST and can just emit function declarations instead of unittest declarations for --betterC.

I added this as another commit.

[wilzbach]

I finally got around making the respective PR for Phobos: dlang/phobos#6645

I hope this helps to clear up the plans I have for this PR and how it's supposed to be integrated with Phobos and later-on Druntime.

[jacob-carlborg]

Of course, on the long run it would be great if dmd could do this out of the box, but for now I think the best place to do the AST rewrites is here in the transformer as we already have access to the AST and can just emit function declarations instead of unittest declarations for --betterC

I'm most likely missing something, but the compiler already transforms unit test blocks to functions.

[wilzbach]

I'm most likely missing something, but the compiler already transforms unit test blocks to functions.

Yep, but we miss ModuleInfo to be able to iterate them like we do in druntime:

https://github.com/dlang/druntime/blob/251cab752c25b9dcaf5bacb1344d2a26b49ed80c/src/core/runtime.d#L636

See also:

• https://run.dlang.io/is/4xgcg6
• https://run.dlang.io/is/hvit1L (-betterC)

[jacob-carlborg]

What about __traits(getUnitTests)?

[wilzbach]

What about __traits(getUnitTests)?

Okay, --betterC will now only add this:

extern(C) void main()
{
    static foreach(u; __traits(getUnitTests, __traits(parent, main)))
        u();
}

In theory this could also be added during the build process, but as this will be required often, I think the tests_extractor is a good place for this.

[wilzbach]

Also as running unittests in -betterC was listed in the "Not Available" features sections, I opened a PR for the spec: dlang/dlang.org#2436

[wilzbach]

Maybe it's better to rename this to --main as it's now more analogous to the -main switch of DMD?

[wilzbach]

So can we move forward with this? I would love to give this a try at Phobos.

[JinShil]

I'm still more in favor of versioning the code. I know it has scaling problems being introduced "after the fact", but it's more "correct" IMO.

There are also issues with -betterC's janky design and implementation. The fact that it imports everything at compile-time, but then pulls the rung out at link time causes issues, as features are available at CTFE that aren't available at runtime. And since there is no version(CTFE) possible, we can't really do anything about it. We need to make a decision about whether -betterC at compile-time has feature parity with -betterC at runtime before we pursue this direction.

-betterC is fragmenting the D language, but it's necessary given the poor design decisions of the past. I hope in the long run -betterC just turns out to be a transition feature to a more scalable language.

I'm also concerned about the shoehorning here, trying to make Phobos and betterC work together when both were designed without each other in mind. I really think we need to consider moving Phobos modules to DUB, and force it to compete in the broader ecosystem. Without "blessed" implementations, it opens the flood gates for competing implementations that can then take things like -betterC into consideration from the beginning. That is out of scope of what's being proposed here, but we need to keep the future in mind as we make decisions.

I see the practicality of the solution in this PR, though I warn that when we get in the habit of using the more expedient option, we just create technical debt for the future. That being said, given the available options, I can't think of a more practical solution. Since I have a completely different vision for where we should be going, I will recuse myself from reviewing this PR.

[wilzbach]

before we pursue this direction.

I think we have different directions in mind. I just want to prevent betterC regressions in Phobos and ensure that most of std.algorithm and std.range can work in -betterC.

-betterC is fragmenting the D language, but it's necessary given the poor design decisions of the past.

Agreed.

I hope in the long run -betterC just turns out to be a transition feature to a more scalable language.

It doesn't look like it at the moment, because we still haven't found good -betterC answer to D's more interesting features like slices or classes. I think only if -betterC gets very close to normal D (slices, classes, ...) it has a chance of getting used more seriously and wider adoption.

I really think we need to consider moving Phobos modules to DUB, and force it to compete in the broader ecosystem. Without "blessed" implementations, it opens the flood gates for competing implementations that can then take things like -betterC into consideration from the beginning. That is out of scope of what's being proposed here, but we need to keep the future in mind as we make decisions.

We had such a discussion 1 1/2 years ago and we weren't able to convince Andrei that splitting up Phobos is a good idea. However, I agree that Phobos is in deep need of a "do-over".

I see the practicality of the solution in this PR, though I warn that when we get in the habit of using the more expedient option, we just create technical debt for the future.

I agree in general, but I don't think that adding a few @betterC attributes will create technical debt. And if we ever decide to revert this decision, it's a simple RegExp to remove then.

That being said, given the available options, I can't think of a more practical solution.

Yep, me neither. I'm not sure whether I have already mentioned this, but I did talk with Andrei about this last week very briefly and he's in favour of adding @betterC attributes to unittest blocks in Phobos.

[JinShil]

I think we have different directions in mind. I just want to prevent betterC regressions in Phobos and ensure that most of std.algorithm and std.range can work in -betterC.

Yes, that's probably the core of the disagreement here. I think we should tell users to "use Phobos in -betterC at your own risk". The fact that some things work in -betterC seems more accidental than by design.

We had such a discussion 1 1/2 years ago and we weren't able to convince Andrei that splitting up Phobos is a good idea.

Thanks for that link; I'd forgotten all about it. It's probably going to take several motivated individuals working outside of the D Language Foundation to make it happen. I'm slowly working towards it, but I doubt I'll get there on my own.

It doesn't look like it at the moment, because we still haven't found good -betterC answer to D's more interesting features like slices or classes.

Yeah, I know, but I do have a few ideas. Right now I'm just trying to get a few basic features of arrays working in -betterC, and it's not easy.

[wilzbach]

I did talk with Andrei about this last week very briefly and he's in favour of adding @BetterC attributes to unittest blocks in Phobos.

BTW he's not the only one who wants to see @betterC regression testing and improvement for Phobos:

So can we please move forward with this? Anyone around for a quick merge?

I think we should tell users to "use Phobos in -betterC at your own risk". The fact that some things work in -betterC seems more accidental than by design.

Well, the long-term plan is that we can say for certain functions (or even modules) that they will work in -betterC and adding basic testing for it is a step forward.

Thanks for that link; I'd forgotten all about it. It's probably going to take several motivated individuals working outside of the D Language Foundation to make it happen. I'm slowly working towards it, but I doubt I'll get there on my own.

I actually also want to see this happen and as I'm a bit involved with the DLF, I might be able to push from the inside a bit ;-)

Yeah, I know, but I do have a few ideas. Right now I'm just trying to get a few basic features of arrays working in -betterC, and it's not easy.

Another idea is that rcstring and its underlying array class can be used with -betterC, but as it currently uses classes for the allocator interface, that's still a dream for the future.
Anyhow, one step at a time.

[PetarKirov]

I agree with @JinShil on most points, but in this particular case, I believe that incremental progress towards making phobos functions more and more independent is very much worth pursuing.

Plus I really like how general is the attribute extraction - we can easily reuse it for other purposes, besides @betterc.

The only thing that I would prefer is if we could find a clean way to define test runners externally not hard code them in the test_extractor itself, but this could improved in the future.

[PetarKirov]

(I guess no point in waiting for Jenkins anymore ;) )