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 principled approach. I'd rather go for #2 . if I wanted #1, I'd 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
 for #2 . if I wanted #1, I'd be using C++ or PHP, which both fit that
 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
 for #2 . if I wanted #1, I'd be using C++ or PHP, which both 
 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