Static verification of std.getopt arguments with more helpful error messages

[ghost]

related issues:

• https://issues.dlang.org/show_bug.cgi?id=10956
• https://issues.dlang.org/show_bug.cgi?id=10955

In short, this PR proposes a way to statically check the variadic options in getOpt (it detects invalid pattern such as two consecutive recipients). The problem is that it uses pragma(msg) to output the diagnostic, but pragma(msg) is never used in phobos (so far).

The constraint works fine but maybe you'll be able to find and suggest a better way to output the diagnostic without losing the line of the error (for example with assert(0) the error origin is lost).

[JackStouffer]

pragma(msg) with static if usage can be refactored to use static assert.

[JackStouffer]

adding static in front of unittest doesn't do anything

[quickfur]

Perhaps one way to avoid needing to use pragma(msg) is to write an overload of getopt with a sig constraint that matches when something is wrong with the arguments, then it can use static assert to emit a compile-time error.

To prevent copy-pasta between the template that verifies the arguments and the template that emits the error messages, what you could do perhaps is something like this:

template OptionsChecker(T...)
{
    static if ( ... /* some checks failed */ )
    {
        enum errorMsg = "... insert error message here ";
        enum passed = false;
    }
    else
    {
        enum passed = true;
    }
}

auto getopt(Args...)(Args args)
    if (OptionsChecker!Args.passed)
{
    // real getopt implementation here
}

auto getopt(Args...)(Args args)
    if (!OptionsChecker!Args.passed)
{
    static assert(0, OptionsChecker!Args.errorMsg); // extract error message from template instance
}

[quickfur]

P.S. Of course, you'll have to refactor the checking code to use recursive templates instead, then you can propagate errors up the template instantiation chain like this:

template CheckOptionsHead(Args...)
{
    static if (... /* something is wrong */ )
    {
        enum errorMsg = "... error message here ";
        enum isValid = false;
    }
    else if ( .../* something else is wrong */)
    {
        enum errorMsg = "different error message here";
        enum isValid = false;
    }
    else
    {
        enum errorMsg = "";
        enum isValid = true; // no checks failed
    }
}

template OptionsChecker(Args...)
{
    static if (Args.length == 0)
    {
        enum errorMsg = "";
        enum isValid = true;
    }
    else static if (!checkOptionsHead!Args.isValid)
    {
        enum errorMsg = checkOptionsHead!Args.errorMsg;
        enum isValid = false;
    }
    else
    {
        alias checkTheRest = OptionsChecker!(Args[1 .. $]); // recursion
        enum errorMsg = checkTheRest.errorMsg;
        enum isValid = checkTheRest.isValid;
    }
}

[ghost]

Thanks for the last comments, using a template was the solution. It even works without recursion. The diagnotsic is now emitted correctly (initially the problem was that pragma is an optional compiler feature) and the error line is not lost.

The last thing mays be a more accurate checking of the receivers type (bool*, pointer to numeric, pointer to string, etc.). Currently the validator accepts invalid pointers or invalid delegates.

The Q. is is it worth ?, since I myself recognize that the usefulness of this PR is low. I mean that it can help someone who already knows std.getopt a bit to detect a small error but someone who starts from scratch will still have to read the manual to get that a lot of things are allowed in the variadic options.

[quickfur]

I think what you currently have is useful as-is. Adding more precise checks is just icing on top of that. At some point, if the diagnostics function becomes equally or more complex than getopt itself, then that would be time to reconsider.

On another note, I wonder if it's possible to massage getopt itself so that it emits useful errors at the point they occur, rather than have a separate diagnostics function that must be kept in sync with the actual implementation. That may be beyond the scope of this PR, though.

[ghost]

#3859 squashed and ready for decision

[quickfur]

LGTM. Thanks!

[quickfur]

Auto-merge toggled on

[ghost]

Dont esitate to ping me if you find a problem in my static checking. Theorically it sould not happen since each unittest that already exist also dependent on this...

We'll see this in 2.071...

[CyberShadow]

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=15914

[ghost]

No it's not the static checker (I'm the reporter, b2.temp, BTW). On my local repo which is at master, even if deactivate the static checker a char is not accepted anymore.

[CyberShadow]

I'm not pointing fingers or stating which part of this pull request introduced a regression, but by the simple test of "did it compile before this pull request? and after?", this pull request introduced the regression.

[ghost]

Yes it used to compile. Coedit symbol list was compiled and released two times with 2.070, and it contains an option specifier made of char:

https://github.com/BBasile/Coedit/blob/master/cesyms/cesyms.d#L40

[CyberShadow]

You can reproduce my result with these Digger settings:

////////////////// bisect.ini //////////////////
bad  = @ 2016-03-01 00:00:00
good = @ 2015-12-01 00:00:00
tester = dmd -o- test.d
[build]
components.enable.rdmd = false
//////////////////// test.d ////////////////////
import std.getopt;

void main()
{
    bool opt;
    string[] args;
    getopt(args, config.passThrough, 'a', &opt);
}
////////////////////////////////////////////////

And my result:

digger: 14ad4729fb661c591d8b3c5e9d49e191a073b0b6 is the first bad commit
commit 14ad4729fb661c591d8b3c5e9d49e191a073b0b6
Author: H. S. Teoh <hsteoh@quickfur.ath.cx>
Date:   Wed Feb 17 22:01:21 2016 -0800

    phobos: Merge pull request #3859 from BBasile/getopt-checker

    https://github.com/D-Programming-Language/phobos/pull/3859

    Static verification of std.getopt arguments with more helpful error messages

diff --git a/phobos b/phobos
index 32f032f..405858d 160000
--- a/phobos
+++ b/phobos
@@ -1 +1 @@
-Subproject commit 32f032fe65fd6362fcf7542b9b4bc66d531cf321
+Subproject commit 405858d816890edd068e494d4600a3d60b5c1184
digger: Bisection completed successfully.
Connection to k3.1azy.net closed.

[ghost]

Once again, on my local repo :

the validator is deactivated but a char is not accepted

[CyberShadow]

I don't understand what you are trying to prove with that screenshot.

Are you really actually trying to prove that the bisection result is wrong?

Like I said, the problem might not be in your code - it could be due to a change in DMD, or a latent DMD bug.

[ghost]

No I'm trying to demonstrate that this PR is not the responsible for the bug. While I can update the validator to handle chars (already done locally), I can't do anything if it's a dmd bug.

So what I suggest is to add the unittest like in the screen shot and to deactivate the static checker so that someone can work on the real bug. When it'll get fixed I'll update the validator.

[CyberShadow]

No I'm trying to demonstrate that this PR is not the responsible for the bug.

Um, looking at this patch, how can that possibly be correct? Your code never checks for chars, only strings (isSomeString) and config.

[ghost]

Was getOpt supposed to accept a char initially ? The doc doesn't mention it. All this conversation may be a useless drama. 😆

[CyberShadow]

If it broke your code, it's very likely it broke someone else's.

[CyberShadow]

So what I suggest is to add the unittest like in the screen shot and to deactivate the static checker so that someone can work on the real bug. When it'll get fixed I'll update the validator.

OK: #4187

[ghost]

No, I can post a fix very quickly. After verification the bisection is right. the unittest failed for another reason.