[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

Iain Buclaw  changed:

   What|Removed |Added

   Priority|P1  |P4

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

--- Comment #11 from Jack Stouffer  ---
(In reply to Walter Bright from comment #10)
> (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
> 
> Please submit this as a Pull Request to https://github.com/dlang/phobos

It's at https://github.com/dlang/phobos/pull/6281

It's currently stalled due to a copying issue inherent in DIP1000's design that
I'm planning on making a forum post on.

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

Walter Bright  changed:

   What|Removed |Added

 CC||bugzi...@digitalmars.com

--- Comment #10 from Walter Bright  ---
(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

Please submit this as a Pull Request to https://github.com/dlang/phobos

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

--- Comment #9 from Seb  ---
> 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. 

-dip1000 is ABI-incompatible with normal D, but Walter (nor anyone else) seems
to care.

https://github.com/dlang/phobos/pull/5915#issuecomment-350426192

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

--- Comment #8 from Jonathan M Davis  ---
(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.

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

--- Comment #7 from Jack Stouffer  ---
(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

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

Jack Stouffer  changed:

   What|Removed |Added

   Attachment #1683|application/mbox|text/plain
  mime type||

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

--- Comment #6 from Jack Stouffer  ---
Created attachment 1683
  --> https://issues.dlang.org/attachment.cgi?id=1683=edit
getopt.patch

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

--- Comment #5 from Jonathan M Davis  ---
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.

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

--- Comment #4 from Jack Stouffer  ---
(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 = 
scope bool* b = 
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

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

Jonathan M Davis  changed:

   What|Removed |Added

 CC||issues.dl...@jmdavisprog.co
   ||m

--- Comment #3 from Jonathan M Davis  ---
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.

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

--- Comment #2 from Jack Stouffer  ---
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)
);
===

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

Jack Stouffer  changed:

   What|Removed |Added

   Keywords||safe
 CC||j...@jackstouffer.com

--- Comment #1 from Jack Stouffer  ---
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.

--

[Issue 18172] std.getopt should allow taking parameters by `ref` (like std.format.formattedRead), s.t. it can be used in @safe

[d-bugmail--- via Digitalmars-d-bugs]

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

Seb  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
--