www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 9087] New: Value modified in foreach warning

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087

           Summary: Value modified in foreach warning
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: DMD
        AssignedTo: nobody puremagic.com
        ReportedBy: bearophile_hugs eml.cc


--- Comment #0 from bearophile_hugs eml.cc 2012-11-27 18:41:03 PST ---
Currently (DMD 2.061alpha) this code generates a warning:

// program#1
void main() {
    foreach (i; 0 .. 10)
        i++;
}

test.d(4): Warning: variable modified in foreach body requires ref storage
class


This is a rather common and well known source of bugs in D code (C# disallows
such mutation):

// program#2
struct S { int x; }
void main() {
    auto items = [S(1), S(2), S(3)];
    foreach (it; items)
        it.x++;
}


(The bug is that the programmer thinks she has modified the contents of "items"
array, while the changes are just in the "it" copy, and such changes get lost
silently.)

So to both help avoid those common bugs, and improve consistency between the
two cases, I suggest to generate a warning in program#2 too, similar to:

test.d(6): Warning: mutable value modified in foreach body requires ref storage
class

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 27 2012
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087



--- Comment #1 from Kenji Hara <k.hara.pg gmail.com> 2012-11-27 19:31:53 PST ---
(In reply to comment #0)

 Currently (DMD 2.061alpha) this code generates a warning:
 
 // program#1
 void main() {
     foreach (i; 0 .. 10)
         i++;
 }
 
 test.d(4): Warning: variable modified in foreach body requires ref storage
 class


This is the *temporary* warning in progress of fixing bug 6652. Finally, the
warning will be disappeared and modifying non-ref foreach key will be
acceptable.

This means that the warning is not for the purpose you hope.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 27 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087



--- Comment #2 from bearophile_hugs eml.cc 2012-11-27 19:49:45 PST ---
(In reply to comment #1)


 This means that the warning is not for the purpose you hope.


I see, thank you for the answer Hara.

But I think the warning I am asking here is one of the better solutions to
avoid those bugs.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 27 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087



--- Comment #3 from Kenji Hara <k.hara.pg gmail.com> 2012-11-27 20:23:02 PST ---
(In reply to comment #2)

 But I think the warning I am asking here is one of the better solutions to
 avoid those bugs.


Yes, in many case those *bugs* will be avoided by your enhancement.
However, in few cases, if a user really wants to use the foreach key as a
mutable copy of iterated element, your enhancement will reject such *correct*
code. At least, that is a point described in the bug 6652 comment#0.

Therefore, this is a design selection. One is avoiding "non-effect
modification" bugs with blocking a few valid code. Another is to keep language
expressive with a little more user's learning. I stand on latter, in basic.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 27 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087



--- Comment #4 from bearophile_hugs eml.cc 2012-11-28 05:13:30 PST ---
(In reply to comment #1)


 This is the *temporary* warning in progress of fixing bug 6652. Finally, the
 warning will be disappeared and modifying non-ref foreach key will be
 acceptable.


Are you saying this will be accepted with no warnings nor errors?

void main() {
foreach (i; 0 .. 10)
    i++;
}


This means extending a bug-prone behavour of D.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 28 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087



--- Comment #5 from bearophile_hugs eml.cc 2012-11-28 05:14:12 PST ---
(In reply to comment #3)


 Another is to keep language expressive with a little more user's learning.


1) Learning to avoid bugs is a very painful process, it takes lot of time, and
some bugs escape even the most precise programmers. Experience shows that
automatic tools (like the compiler) are needed.
2) Modifying a copy of a struct/value in a foreach is a rare operation. And
there is no significant decrease in D expressiveness, because there are other
ways to do it:

// program#3
struct S { int x; }
void main() {
    auto items = [S(1), S(2), S(3)];
    foreach (it; items) {
        auto itc = it;
        itc.x++;
    }
    for (size_t i = 0; i < items.length; i++) {
        auto it = items[i];
        it.x++;
    }
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 28 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087


Jonathan M Davis <jmdavisProg gmx.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmdavisProg gmx.com


--- Comment #6 from Jonathan M Davis <jmdavisProg gmx.com> 2012-11-28 08:20:30
PST ---
And why should we take a performance hit with a required copy just because
you're worried about the stray mistake where someone mutated a foreach
variable? Honestly, I don't think that I have _ever_ seen a bug caused by this.
That doesn't mean that it can't happen or that it hasn't happened, but I don't
think that it's even vaguely worth worrying about. You have a habit of trying
to get warnings created for every single stray mistake that you think a
programmer might make, and I think that that approach will ultimately lead to a
language that is extremely annoying to use. There comes a point where you have
to let the programmer do their job and assume that they're not going to be an
idiot.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 28 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087



--- Comment #7 from bearophile_hugs eml.cc 2012-11-28 14:17:31 PST ---
(In reply to comment #6)

Thank you for your answers, Jonathan.


 And why should we take a performance hit with a required copy


I think you are missing something. If you use "ref" or "const ref" no copy is
performed:

// program#4
struct S { int x; }
void main() {
    auto items = [S(1), S(2), S(3)];
    foreach (ref it; items)
        it.x++;
}


In program#3 there was a copy because in that case the programmer do wants a
copy.



 Honestly, I don't think that I have _ever_ seen a bug caused by this.


Assuming you have understood what's the bug-class I am talking about (and I am
not sure of this, given what I think is the above mistake of yours), yours is
only one data point. Mine is another data point, and in my code the unwanted
modification of a struct in a foreach loop has caused me several bugs. And in
the D newsgroup some other persons have said they make mistakes about this.
Another important data point is the design of C#, that in the case discussed in
this enhancement request disallows mutation. So in the end it's quite more one
person against your case.



 That doesn't mean that it can't happen or that it hasn't happened, but I don't
 think that it's even vaguely worth worrying about.


It seems to be a common enough situation that's recognized as bug prone. So
it's worth worrying about, because this class of problems is not hard to
prevent.



 You have a habit of trying to get warnings created for every
 single stray mistake that you think a programmer might make,


I agree that raising a warning here is a suboptimal solution. I think the right
solution is to do something more similar to C#, and disallow the struct
mutation in foreach unless you put a "ref" tag.

The total number of warnings I am asking for in Bugzilla is really small,
probably around 3-4.



 There comes a point where you have
 to let the programmer do their job and assume that they're not going to be an
 idiot.


We are *very* far from that in D, for a D programmer there are plenty of things
to take care of to write correct programs :-)

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 28 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087



--- Comment #8 from bearophile_hugs eml.cc 2012-11-28 14:28:39 PST ---
(In reply to comment #7)


 and in my code the unwanted modification of a struct in a foreach loop


The problem is the opposite, sorry. I was trying to modify the struct, but no
actual change of the original struct was happening.

I have explained this topic very well elsewhere to Walter, and it seems that
even him has a hard time understanding this very simple problem and situation.
See for some of my explanations:

http://forum.dlang.org/thread/znbtczbgipqqzllafmzk forum.dlang.org

http://forum.dlang.org/thread/k7afq6$2832$1 digitalmars.com?page=7#post-kothvmmjkrgtcfzcxohj:40forum.dlang.org

http://forum.dlang.org/thread/k7afq6$2832$1 digitalmars.com?page=8#post-hrfjeobyzdmajlhjdivf:40forum.dlang.org

http://forum.dlang.org/thread/k7afq6$2832$1 digitalmars.com?page=10#post-zhrnkzjtaaosmizmqdtk:40forum.dlang.org

http://forum.dlang.org/thread/k7afq6$2832$1 digitalmars.com?page=12#post-hqdihcnzzgnlfvznhqbc:40forum.dlang.org

Take a look especially at the first and last of those links.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 28 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087



--- Comment #9 from Jonathan M Davis <jmdavisProg gmx.com> 2012-11-28 15:08:58
PST ---

 I think you are missing something. If you use "ref" or "const ref" no copy is
 performed:


If don't want a copy, then use ref. If you want a copy, then don't use ref.
Disallowing mutating the loop variable if not using ref would be annoying. It
would force you to manually copy the loop variable if you wanted to mutate it.
If we were doing that, we might as well make the loop variable always ref. I
think that that's a horrible idea. And warning about something is as good as
disallowing, because it's an error for anyone compiling with -w and it's
terrible practice to leave any warnings in your code anyway.

I understand that there may be bugs caused by failing to use ref when you meant
to, but you're basically proposing that a foreach loop always use ref and that
anyone wanting it to not be ref must copy the loop variable themselves. Not to
mention, in some cases, you _can't_ use ref (e.g. if iterating over a range,
and its front doesn't return by ref), which would mean that you'd have to make
a useless copy of a non-ref variable in that case. So, I think that a warning
like you propose is completely untenable.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 28 2012
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9087


bearophile_hugs eml.cc changed:

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


--- Comment #10 from bearophile_hugs eml.cc 2012-11-28 16:41:27 PST ---
(In reply to comment #9)


 So, I think that a warning
 like you propose is completely untenable.


Thank you again for the comments Jonathan. I agree that a warning is not good
enough, so I close this. I will open a new enhancement request if and when I
will find a more usable idea.

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