www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 5059] New: String assignment in foreach loop breaks immutability

reply d-bugmail puremagic.com writes:
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: -------
Oct 15 2010
next sibling parent d-bugmail puremagic.com writes:
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: -------
Apr 28 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
May 24 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
May 24 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
May 24 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
May 24 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
May 24 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
May 24 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
May 24 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
May 24 2011
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=5059


Andrej Mitrovic <andrej.mitrovich gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


--- Comment #10 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2011-07-11
08:27:55 PDT ---
Fixed in 2.054

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