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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------