std.getopt: Add unit tests for delegates as callbacks.

[jondegenhardt]

At present there are no unit tests in std.getopt where delegates are used as callbacks. This adds a few basic tests.

[quickfur]

I like the idea of testing everything. But why is this unittest @system? As far as I can tell, there's nothing here that's un-@safe.

[quickfur]

Why @system?

[jondegenhardt]

I tried @safe initially, but it fails. Probably why most of the other unit tests are @system. The error with @safe:

std/getopt.d(1799): Error: @safe function 'std.getopt.__unittestL1777_33' cannot call @system function 'std.getopt.getopt!(string, string, void delegate() pure nothrow @nogc @safe, string, string, void delegate(string option) pure nothrow @nogc @safe, string, string, void delegate(string option, string value), string, string, void delegate() pure nothrow @nogc @safe, string, string, void delegate(string option) pure nothrow @nogc @safe, string, string, void delegate(string option, string value)).getopt'

I should have mentioned in the initial description - One rationale for adding delegates to unit tests is that delegates and functions have separate tests in the getopt code. Doesn't show up in code coverage reports because the tests are on the same code line.

[quickfur]

I'm not disagreeing that we need to add these unittests... but I think it's worth investigating where exactly it fails to be @safe. It might be a bug in the implementation of getopt.

[quickfur]

Aha! I found the problem. Well, two problems, actually, both are easily fixable:

1. TwoArgOptionHandler needs to be explicitly declared as @safe in your new unittest.
2. The non-template private function optMatch needs to be declared @safe because, not being a template function, the compiler doesn't automatically infer attributes for it, thus missing out on the fact that it's actually @safe.

Fix these two, and we're good to go!

[jondegenhardt]

Okay thanks! Would be good to make these all @safe.

I tried to find some history on this. Didn't find anything in issues, but the original PR that added @system and @safe to the unit tests was PR #4525 (@atilaneves). No discussion of the choices in the PR, but there may be background info available.

As to this PR - Personally I'd prefer to separate adding these unit tests from making std.getopt @safe generally. So, if I change optMatch to be @safe, I should also go through all the other unit tests and make them @safe. There could be a second question to address as well, in that user code, like TwoArgOptHandler, will often not be marked @safe. There could be a rationale to have both types of unit tests.

Let me know how you'd like it done. If you want them both in the same PR it might be a day or so before I get back to it, but there's nothing time critical here.

[JackStouffer]

The @safe and @system prefixes to the unit tests were added in order to make unsafe parts of Phobos grep-able, and therefore easy to direct people towards fixing. Some unit tests may have been mis-marked as @system when they were actually @safe.

[quickfur]

Let's keep it simple. If you don't feel like putting the @safe fix in this PR, then you could just take out the @system from the unittest declaration, then we can merge this PR, and in a followup PR we can apply the @safe fixes and annotations.

[jondegenhardt]

Isn't it a phobos requirement that all unittest blocks be annotated with @safe or @system? If so the @system should be kept for this PR.

[wilzbach]

Isn't it a phobos requirement that all unittest blocks be annotated with @safe or @System?

Yep - the motivation was that the unittests of new functions should be @safe which was repeatedly forgotten..

[quickfur]

Fine, then, let's merge this and fix the @safe issues in a followup PR.

[jondegenhardt]

Very good, thanks for thoughtful review.