http://d.puremagic.com/issues/show_bug.cgi?id=5059

           Summary: String assignment in foreach loop breaks immutability
           Product: D
           Version: D2
          Platform: Other
        OS/Version: Windows
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: andrej.mitrovich@gmail.com


--- Comment #0 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2010-10-15 12:04:00 PDT ---
Since this example uses the Registry, you need Windows to compile it:

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 resulting string array has strings that are 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 registry key name and writes more overwwritten strings like that.

The commented out code is the workaround, however string to string assignment should be safe without a need for .idup.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5059


Jesse Phillips <Jesse.K.Phillips+D@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |Jesse.K.Phillips+D@gmail.co
                   |                            |m


--- Comment #1 from Jesse Phillips <Jesse.K.Phillips+D@gmail.com> 2011-04-28 10:13:09 PDT ---
This is a bug in KeySequence's opApply. The function is reusing a char[] for every iteration, it then casts this to a string, but ends up being modified for each call to Reg_EnumKeyName_.

https://github.com/D-Programming-Language/phobos/blob/master/std/windows/registry.d#L1870

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5059



--- Comment #2 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2011-05-24 14:36:34 PDT ---
Should I just change that line to:
Key key =   m_key.getKey(sName[0 .. cchName].idup);

I think this makes the most sense if we're to trust that strings are immutable.

I can make a pull request if this fix would be ok (my tests show that this
fixes the issue).

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5059



--- Comment #3 from Steven Schveighoffer <schveiguy@yahoo.com> 2011-05-24 14:44:26 PDT ---
I think the better solution is to change it to const(char)[].  This better reflects that the data may be changing between iterations.  If you are not storing the strings from this, then it is extremely wasteful to allocate a new memory block for each iteration.  You can always idup or dup if you want to.

Note, I think the documentation needs to specifically state it reuses the buffer, so you should .dup or .idup if you wish to keep the data beyond the loop iteration.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5059



--- Comment #4 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2011-05-24 15:12:35 PDT ---
Change what exactly to const(char[])? You probably mean my "name" variable in
the example code.

I think this is again that issue of implicitly reusing a variable in a foreach loop. I really wish this was more explicit instead of relying on documentation, e.g. I'd love it if it were like this:

foreach (ref buffer; iterator()) { }  // it's reusing buffer on each iteration
foreach (buffer; iterator()) { }      // allocates new buffer on each iteration

Maybe it's easy to reason about the current foreach mechanism when you only have one foreach loop. But when you have multiple foreach statements intertwined with dozens of different lines it's easy to miss that you should have duped something.

It took me a good while before I realized what was wrong with my code until I ran it down to a simple test case like in the original post. I simply thought "hey, I'm assigning an immutable string to an immutable string, what could go wrong?".

But this is more worthy of a post in the NG then to discuss it in here I guess..

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5059



--- Comment #5 from Steven Schveighoffer <schveiguy@yahoo.com> 2011-05-24 15:28:28 PDT ---
I completely misunderstood the problem here, I thought the string was being directly returned in the opApply delegate, not a Key object.

Yes, I agree, the string should be idup'd.

Doing anything differently would require a redesign (which I think actually is in order, to allocate that much data to do a simple loop is incredibly wasteful), but since nobody is owning this, your solution is as good as we can get for now.

Sorry for the noise.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5059



--- Comment #6 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2011-05-24 16:53:41 PDT ---
Yeah from a quick glance at the registry module it does seem a little fishy overall. __gshared variables, ASCII-only WinAPI calls (what if the registry key is in UTF, if that's possible?), casts to strings.. maybe this module needs to be refactored.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5059



--- Comment #7 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2011-05-24 16:57:25 PDT ---
Actually there's 4 opApply implementations in that module and each of them is casting char[] to a string. And other methods like getKeyName, getValueName, etc.. seem to do the same thing. Ugly!

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5059



--- Comment #8 from Steven Schveighoffer <schveiguy@yahoo.com> 2011-05-24 17:08:24 PDT ---
I think this was a D1 module "casted" to D2 to get it to compile :)  Probably would be better served reimplemented, but obviously someone would have to spend some quality time doing that.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5059



--- Comment #9 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2011-05-24 17:11:28 PDT ---
Btw, I think I know what you were referring to now. The opApply at line 1984 uses a delegate that takes a ref string.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------