https://issues.dlang.org/show_bug.cgi?id=18172

Seb <greensunny12@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |18110


Referenced Issues:

https://issues.dlang.org/show_bug.cgi?id=18110
[Issue 18110] most of phobos should be @safe-ly useable
--

https://issues.dlang.org/show_bug.cgi?id=18172

Jack Stouffer <jack@jackstouffer.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |safe
                 CC|                            |jack@jackstouffer.com

--- Comment #1 from Jack Stouffer <jack@jackstouffer.com> ---
Working on this, I've come to the realization that you have to give up most of the static argument checking that currently exists in getopt for this to work.

Once you allow non-pointers to be used with the auto ref variadic parameters, it's practically impossible to tell the difference between an invalid signature and a valid one.

--

https://issues.dlang.org/show_bug.cgi?id=18172

--- Comment #2 from Jack Stouffer <jack@jackstouffer.com> ---
Actually the problem is way worse. Consider

========
string file1 = "file.dat";
string file2 = "file2.dat";

getopt(args, "file1", "info about arg", file1,
    "file2", file2);
========

It's actually, not practically, impossible to know if the string at the third arg is to be written to or is to be used as the help info. All of the arguments look like identical strings to the code.

One possible fix is to use tuples to break the arguments into groups

=======
string file1;
string file2;

getopt(
    args,
    tuple("arg1", "info about arg", file1),
    tuple("arg2", file2)
);
=======

--

https://issues.dlang.org/show_bug.cgi?id=18172

Jonathan M Davis <issues.dlang@jmdavisProg.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |issues.dlang@jmdavisProg.co
                   |                            |m

--- Comment #3 from Jonathan M Davis <issues.dlang@jmdavisProg.com> ---
I'd suggest looking into how DIP 1000 can fix this problem without needing ref, since with DIP 1000, taking the address of a local variable isn't necessarily @system.

--

https://issues.dlang.org/show_bug.cgi?id=18172

--- Comment #4 from Jack Stouffer <jack@jackstouffer.com> ---
(In reply to Jonathan M Davis from comment #3)
> I'd suggest looking into how DIP 1000 can fix this problem without needing ref, since with DIP 1000, taking the address of a local variable isn't necessarily @system.

Despite marking the parameters as scope and using `@safe` to check for any security holes, creating a scoped pointer still yields an error

=====
@safe unittest
{
    auto args = ["prog", "--foo", "-b"];

    bool foo;
    bool bar;
    scope bool* f = &foo;
    scope bool* b = &bar;
    auto rslt = getopt(args, "foo|f", "Some information about foo.", f,
"bar|b",
        "Some help message about bar.", b);
}
=====

std/getopt.d(452): Error: cannot take address of local foo in @safe function
__unittest_L446_C7
std/getopt.d(453): Error: cannot take address of local bar in @safe function
__unittest_L446_C7

--

https://issues.dlang.org/show_bug.cgi?id=18172

--- Comment #5 from Jonathan M Davis <issues.dlang@jmdavisProg.com> ---
You have to compile with -dip1000, which Phobos isn't right now. If I try it locally with -dip1000, I get

q.d(10): Error: scope variable f assigned to non-scope parameter _param_3
calling std.getopt.getopt!(string, string, bool*, string, string, bool*).getopt
q.d(11): Error: scope variable b assigned to non-scope parameter _param_6
calling std.getopt.getopt!(string, string, bool*, string, string, bool*).getopt

and I get a similar error if I take the address when passing to getopt like you normally would. getopt itself probably needs to have the parameters marked with scope to fix that. Regardless, it seems to me that either it's possible to tweak getopt to make this work with @safe in -dip1000, or it should be discussed with Walter how to improve DIP 1000 to make it work. Conceptually, I don't see any reason why getopt couldn't be made to work with scope and -dip1000 in @safe, because it doesn't escape the pointers. The question is what needs to be done to make it work.

--

https://issues.dlang.org/show_bug.cgi?id=18172

--- Comment #6 from Jack Stouffer <jack@jackstouffer.com> ---
Created attachment 1683
  --> https://issues.dlang.org/attachment.cgi?id=1683&action=edit
getopt.patch

--

https://issues.dlang.org/show_bug.cgi?id=18172

Jack Stouffer <jack@jackstouffer.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1683|application/mbox            |text/plain
          mime type|                            |

--

https://issues.dlang.org/show_bug.cgi?id=18172

--- Comment #7 from Jack Stouffer <jack@jackstouffer.com> ---
(In reply to Jonathan M Davis from comment #5)
> You have to compile with -dip1000, which Phobos isn't right now. If I try it locally with -dip1000, I get
> 
> q.d(10): Error: scope variable f assigned to non-scope parameter _param_3
> calling std.getopt.getopt!(string, string, bool*, string, string,
> bool*).getopt
> q.d(11): Error: scope variable b assigned to non-scope parameter _param_6
> calling std.getopt.getopt!(string, string, bool*, string, string,
> bool*).getopt
> 
> and I get a similar error if I take the address when passing to getopt like you normally would. getopt itself probably needs to have the parameters marked with scope to fix that. Regardless, it seems to me that either it's possible to tweak getopt to make this work with @safe in -dip1000, or it should be discussed with Walter how to improve DIP 1000 to make it work. Conceptually, I don't see any reason why getopt couldn't be made to work with scope and -dip1000 in @safe, because it doesn't escape the pointers. The question is what needs to be done to make it work.

I should have emphized more the fact that I had already modified getopt. I've attached my changes as a patch

--

https://issues.dlang.org/show_bug.cgi?id=18172

--- Comment #8 from Jonathan M Davis <issues.dlang@jmdavisProg.com> ---
(In reply to Jack Stouffer from comment #7)
> I should have emphized more the fact that I had already modified getopt. I've attached my changes as a patch

Without compiling with -dip1000, nothing will be fixed, and Phobos isn't compiled with -dip1000, so the unit tests don't tell us anything. If I test your changes locally (minus the unit test changes) and compile your example in a separate program compiled with -dip1000, then I get this error:

q.d(9): Error: @safe function D main cannot call @system function
std.getopt.getopt!(string, string, bool*, string, string, bool*).getopt

which indicates that the signature for getopt works just fine at that point, making the caller @safe. It's just that getopt's internals still need some fixing.

If I slap @trusted on getopt to make it shut up, then I get linker errors about stuff in std.conv being undefined, but there are no more errors from the compiler itself. I think that that happens sometimes when some code is compiled with -dip1000 and some without, but I'm not sure. Unfortunately, Phobos as a whole does not work with -dip1000 yet, and I haven't done much with -dip1000. But from the looks of it, I'd say that it looks likely that we can fix getopt to be @safe with -dip1000 and scope without breaking the API. There may turn out to be bugs there that Walter will need to fix to get it fully working, but it looks promising.

Either way, you can't update the unit tests right now to be @safe, because Phobos isn't compiled with -dip1000.

--