www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Proposed Enhancement: Deprecate std.conv.text With 0 Arguments

reply Meta <jared771 gmail.com> writes:
This is a small but annoying problem: std.conv.text is defined 
like this:

string text(T...)(T args) { return textImpl!string(args); }

private S textImpl(S, U...)(U args)
{
     static if (U.length == 0)
     {
         return null;
     }
     else
     {
         auto result = to!S(args[0]);
         foreach (arg; args[1 .. $])
             result ~= to!S(arg);
         return result;
     }
}

If it is called with 0 arguments it will return null. This 
behaviour has caused several bugs in my code because combined 
with optional parens and UFCS, it is easy to accidentally call 
text with 0 args but have it look like passing a variable to a 
function. An example bug that I recently found in my code:

Token getNextToken(ref string input)
{
     assert(!input.empty);

     while (input.front.isSpace)
     {
         input.advance(1);
     }

     if (input.front == '$' || input.front.isAlpha)
     {
         if (input.front == '$')
         {
             //BUG
             text.advance(1);
         }
         auto identStr = input.getNextWord();
         if (keywords.canFind(identStr))
         {
             return new Keyword(input.front == '$' ? '$' ~ 
identStr : identStr);
         }
         else
         {
             return new Identifier(identStr);
         }
     }
     else if (input.front.isDigit || input.front == '-' || 
input.front == '+')
     {
         //etc.
     }
     else if (input.front == '"')
     {
         //etc.
     }
     //etc...
}

void advance(ref string input, size_t n)
{
     foreach (_; 0..n)
     {
         if (input.empty)
         {
             break;
         }
         input.popFront();
     }
}

The problem is that `advance` was not advancing the input, 
causing bugs in the lexing stage. Usually the compiler would warn 
me that no such variable `text` exists, but as I imported 
std.conv, `text` was in scope. Since `text` can take 0 or more 
arguments, and due to optional parens, `text.advance(1)` is a 
valid expression that does nothing. If std.conv.text took only 1 
or more arguments and refused to compile with 0 arguments, then 
the compiler would signal an error as normal and I would not have 
to spend an hour hunting down this bug.

I would like to make a pull request to fix this, deprecating the 
0-argument case and then eventually making it an error, but would 
this PR be accepted considering it technically breaks 
backwards-compatibility?
Jun 22 2016
next sibling parent reply Seb <seb wilzba.ch> writes:
On Wednesday, 22 June 2016 at 15:39:11 UTC, Meta wrote:

 This is a small but annoying problem: std.conv.text is defined 
 like this:

 [...]


Imho that's a great idea! I don't see any problem in deprecating 
it as std.conv.text with 0 arguments is clearly unintended 
behavior. Hence I would call this a bug fix ;-)
Jun 22 2016
parent reply Jack Stouffer <jack jackstouffer.com> writes:
On Wednesday, 22 June 2016 at 15:48:06 UTC, Seb wrote:

 I don't see any problem in deprecating it as std.conv.text
 with 0 arguments is clearly unintended behavior


I don't see how you can make that claim, take a look at the code 
again:

private S textImpl(S, U...)(U args)
{
     static if (U.length == 0)
     {
         return null;
     }
     else
     {
         auto result = to!S(args[0]);
         foreach (arg; args[1 .. $])
             result ~= to!S(arg);
         return result;
     }
}

The zero argument result was clearly an intentional special case
Jun 22 2016
parent reply Meta <jared771 gmail.com> writes:
On Wednesday, 22 June 2016 at 16:47:53 UTC, Jack Stouffer wrote:

 On Wednesday, 22 June 2016 at 15:48:06 UTC, Seb wrote:

 I don't see any problem in deprecating it as std.conv.text
 with 0 arguments is clearly unintended behavior


 I don't see how you can make that claim, take a look at the 
 code again:

 private S textImpl(S, U...)(U args)
 {
     static if (U.length == 0)
     {
         return null;
     }
     else
     {
         auto result = to!S(args[0]);
         foreach (arg; args[1 .. $])
             result ~= to!S(arg);
         return result;
     }
 }

 The zero argument result was clearly an intentional special case


Intentional or not, I don't see any downside to disallowing 
calling text with 0 args (aside from backwards-compatibility 
concerns). It doesn't even return anything useful, just null.
Jun 22 2016
parent reply Jack Stouffer <jack jackstouffer.com> writes:
On Wednesday, 22 June 2016 at 17:04:54 UTC, Meta wrote:

 Intentional or not, I don't see any downside to disallowing 
 calling text with 0 args (aside from backwards-compatibility 
 concerns). It doesn't even return anything useful, just null.


Well, the idea is that the function is a string constructor so it 
returns d/w/string.init with nothing passed in.
Jun 22 2016
parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 6/22/16 1:12 PM, Jack Stouffer wrote:

 On Wednesday, 22 June 2016 at 17:04:54 UTC, Meta wrote:

 Intentional or not, I don't see any downside to disallowing calling
 text with 0 args (aside from backwards-compatibility concerns). It
 doesn't even return anything useful, just null.


 Well, the idea is that the function is a string constructor so it
 returns d/w/string.init with nothing passed in.


I think it's worthy of a change. if(something); used to be valid code. 
But it's more likely to be an erroneous semi-colon. Making this an error 
was a huge win.

Same thing with text with no args. You're actually better off using "" 
or string.init.

-Steve
Jun 23 2016
prev sibling next sibling parent Gary Willoughby <dev nomad.so> writes:
On Wednesday, 22 June 2016 at 15:39:11 UTC, Meta wrote:

 If it is called with 0 arguments it will return null. This 
 behaviour has caused several bugs in my code because combined 
 with optional parens and UFCS, it is easy to accidentally call 
 text with 0 args but have it look like passing a variable to a 
 function.


It's really annoying:

https://issues.dlang.org/show_bug.cgi?id=12357
Jun 22 2016
prev sibling parent reply deadalnix <deadalnix gmail.com> writes:
On Wednesday, 22 June 2016 at 15:39:11 UTC, Meta wrote:

 If it is called with 0 arguments it will return null.


No it will  return empty string.


 This  behaviour has caused several bugs in my code because
 combined with optional parens and UFCS, it is easy to 
 accidentally
 call text with 0 args but have it look like passing a variable 
 to a function. An example bug that I recently found in my code:


This is a problem with optional (), not text.

test takes n parameters and return a string representation of 
these parameters. returning an empty string when no parameters is 
provided is very much expected.

The alternative is to add special case. And here you get into the 
bad design land. When some bad design is introduced it causes 
problems. Now there is the option to introduce more and more bad 
design to work around the original bad design, or adopt a 

be using C++ or PHP, which both fit that spot better than D.

When the solution to a problem is adding more special casing, you 
can be sure you are not moving in the right direction.
Jun 23 2016
parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 6/23/16 6:46 PM, deadalnix wrote:

 On Wednesday, 22 June 2016 at 15:39:11 UTC, Meta wrote:

 If it is called with 0 arguments it will return null.


 No it will  return empty string.


null is an empty string. And it does return null specifically.


 This  behaviour has caused several bugs in my code because
 combined with optional parens and UFCS, it is easy to accidentally
 call text with 0 args but have it look like passing a variable to a
 function. An example bug that I recently found in my code:


 This is a problem with optional (), not text.


A valid point. But we aren't going to have this change.


 test takes n parameters and return a string representation of these
 parameters. returning an empty string when no parameters is provided is
 very much expected.


If it has to be valid, this is what I would expect, yes. But it doesn't 
have to be valid.

The only place I can see this being useful is generic code which itself 
takes a tuple.


 The alternative is to add special case. And here you get into the bad
 design land. When some bad design is introduced it causes problems. Now
 there is the option to introduce more and more bad design to work around
 the original bad design, or adopt a principled approach. I'd rather go

 spot better than D.


It's not a special case to require a minimum number of parameters.


 When the solution to a problem is adding more special casing, you can be
 sure you are not moving in the right direction.


There is already a special case for zero arguments -- a whole static if 
branch, in fact.

-Steve
Jun 23 2016
parent reply deadalnix <deadalnix gmail.com> writes:
On Thursday, 23 June 2016 at 23:16:23 UTC, Steven Schveighoffer 
wrote:

 This is a problem with optional (), not text.


 A valid point. But we aren't going to have this change.


Spreading the shit doesn't make it smell good.


 test takes n parameters and return a string representation of 
 these
 parameters. returning an empty string when no parameters is 
 provided is
 very much expected.


 If it has to be valid, this is what I would expect, yes. But it 
 doesn't have to be valid.

 The only place I can see this being useful is generic code 
 which itself takes a tuple.


Exactly.


 The alternative is to add special case. And here you get into 
 the bad
 design land. When some bad design is introduced it causes 
 problems. Now
 there is the option to introduce more and more bad design to 
 work around
 the original bad design, or adopt a principled approach. I'd 
 rather go

 fit that
 spot better than D.


 It's not a special case to require a minimum number of 
 parameters.


You are just pushing the problem up one level. Now every template 
that call text with a sequence need to do a length check (and 
none will).

The very problem is that the optional () make the argumentless 
thing a special case. Special cases are like cancer, they spread. 
Making more argumentless thing special cases is just making the 
problem worse.

But it is pushed on somebody else, so I guess that's alright...

That reminds me of https://www.youtube.com/watch?v=aI0euMFAWF8


 When the solution to a problem is adding more special casing, 
 you can be
 sure you are not moving in the right direction.


 There is already a special case for zero arguments -- a whole 
 static if branch, in fact.


That's an implementation detail.

string test(T...)(T args) {
     auto ret = "";
     foreach (a; args) {
         ret ~= testImpl(a);
     }

     return ret;
}

Or it can be seen as the bottom turtle.

string test(T...)(T args) {
     static if (args.length == 0) {
         return "";
     } else {
         return text(a[0 .. $ - 1]) ~ textImpl(a[$ - 1]);
     }
}

Anyway, looking at the implementation to figure out what the 
semantic should be is completely ass backward. The implementation 
should match desired semantic not the other way around.
Jun 23 2016
next sibling parent Meta <jared771 gmail.com> writes:
Thank you for the input but considering that the deprecation 
message for this wasn't triggered a single time while compiling 
Phobos, I suspect that very few will be bothered by this special 
case. I have submitted a PMR, so we can all fight over it there:

https://github.com/dlang/phobos/pull/4460
Jun 23 2016
prev sibling next sibling parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 6/23/16 7:46 PM, deadalnix wrote:

 On Thursday, 23 June 2016 at 23:16:23 UTC, Steven Schveighoffer wrote:

 This is a problem with optional (), not text.


 A valid point. But we aren't going to have this change.


 Spreading the shit doesn't make it smell good.


I once cared a lot about making parentheses required for non- property 
functions that take no args. I'm not so keen on it any more, it doesn't 
add a lot of clarity in cases of no-arg functions.

The one thing that can be said here is that text is a terrible name for 
a function, since it's a likely name for a variable as well. asText or 
toText would have been better.


 It's not a special case to require a minimum number of parameters.


 You are just pushing the problem up one level. Now every template that
 call text with a sequence need to do a length check (and none will).


And likely if they do happen to send zero args to text, it was a bug in 
the first place (and they should have been checking the length).

Also, you don't have to do much to work around:

foo(T...)(T args) {
text("", args);
}


 The very problem is that the optional () make the argumentless thing a
 special case. Special cases are like cancer, they spread. Making more
 argumentless thing special cases is just making the problem worse.


The cases where you are going to call text with an unknown quantity of 
parameters is pretty small. I think they can deal with it. The fact that 
meta's PR caused zero breakage should tell you something.

-Steve
Jun 24 2016
prev sibling parent reply Ola Fosheim =?UTF-8?B?R3LDuHN0YWQ=?= writes:
On Thursday, 23 June 2016 at 23:46:47 UTC, deadalnix wrote:

 You are just pushing the problem up one level. Now every 
 template that call text with a sequence need to do a length 
 check (and none will).


Indeed, D should try to focus on making generic programming less 
tedious and more principled.

The problem Meta had was largely due to lack of restrictions in 
how name lookup/import and functions as properties work. Which is 
kinda problematic for programming-in-the-large which D is meant 
to do well.

This is just hacking on band-aids over the symptoms rather than 
fixing the cause.
Jun 24 2016
parent reply Meta <jared771 gmail.com> writes:
On Friday, 24 June 2016 at 14:15:46 UTC, Ola Fosheim Grøstad 
wrote:

 On Thursday, 23 June 2016 at 23:46:47 UTC, deadalnix wrote:

 You are just pushing the problem up one level. Now every 
 template that call text with a sequence need to do a length 
 check (and none will).


 Indeed, D should try to focus on making generic programming 
 less tedious and more principled.

 The problem Meta had was largely due to lack of restrictions in 
 how name lookup/import and functions as properties work. Which 
 is kinda problematic for programming-in-the-large which D is 
 meant to do well.

 This is just hacking on band-aids over the symptoms rather than 
 fixing the cause.


Unfortunately they are symptoms we are stuck with so a band-aid 
is the only solution.
Jun 24 2016
next sibling parent Ola Fosheim =?UTF-8?B?R3LDuHN0YWQ=?= writes:
On Friday, 24 June 2016 at 16:34:19 UTC, Meta wrote:

 Unfortunately they are symptoms we are stuck with so a band-aid 
 is the only solution.


But I think name resolution can be fixed and that existing code 
bases can be updated automatically by semantic analysis of 
regular code? In most cases a tool can look up symbols using old 
rules and add explicit prefixing using new rules? Granted, this 
does not work on string-based generic code, but such code would 
fail at compile-time, or?
Jun 24 2016
prev sibling parent deadalnix <deadalnix gmail.com> writes:
On Friday, 24 June 2016 at 16:34:19 UTC, Meta wrote:

 Unfortunately they are symptoms we are stuck with so a band-aid 
 is the only solution.


Bullshit.
Jun 24 2016