Foreach and string to string assignment (maybe a bug..)

[Steven Schveighoffer]

On Thu, 14 Oct 2010 13:01:36 -0400, Andrej Mitrovic <none at none.com> wrote:

> I'm not sure if this is a legitimate bug or one of my brainfarts.
>
> I know that if I'm using a foreach loop with a char array as a reusable  
> datatype I definitely have to create a copy if I want to store it to a  
> string.
>
> But this code is a little more subtle, check it out (This is Windows  
> only because it uses the registry, sorry):
>
> module mymodule;
>
> import std.stdio : writeln, write;
> import std.windows.registry;
>
> void main()
> {
>     Key HKLM = Registry.localMachine;
>     Key SFW = HKLM.getKey("software");
>
>     string[] names;
>
>     foreach (Key key; SFW.keys())
>     {
>         string name = key.name();
>         // string name = key.name().idup; // workaround for the issue
>         names ~= name;
>     }
>
>     writeln("results:");
>     foreach (name; names)
>     {
>         write(name, ", ");
>     }
> }
>
> The results are quite unexpected. The strings get overwritten with each  
> other, and in my case the results are similar to this:
>
> Sun Microsystems, Sun Micros, Sun , Sun Micr, Sun, Sun Mic,...
>
> And it goes like that for a hundred or so values, then switches to the  
> next name and writes more garbage like that.
>
> If I use .idup, the problem goes away. What I don't understand is why  
> assigning a string to a string isn't safe in this case? They're both  
> immutable, so I was expecting the contents of the strings never to  
> change.
>
> If it's not a bug, it certainly is a subtle issue. The original foreach  
> loop was quite big, and it took some time to figure out the problem. Are  
> we *always* supossed to be using .idup in a foreach loop? Of course, the  
> Key key variable is reused in the foreach loop, so I guess this has  
> something to do with the results.

I'm not familiar with std.windows.registry, but there are two things here.

First, unequivocally, this is a bug.  If key.name is returning an  
immutable(char)[], and later on that value is being overwritten, this is a  
violation of the type system (immutable data must never change again).

Second, I think the bug may not be that it's violating immutability, but  
rather that it's typed key.name as string.  It could probably be  
const(char)[].  Essentially, I think from the behavior described that the  
foreach loop is reusing the name buffer for each iteration of the loop.   
This is probably to save extra heap allocations in case you don't use them  
after the foreach loop is over.  I think your workaround is the correct  
way to deal with it.

This is a common problem in defining an opApply loop which streams data --  
do you make things safe or efficient?  The only reasonable solution IMO is  
to make them efficient, because safety can be had by duping the data.

-Steve