https://issues.dlang.org/show_bug.cgi?id=2043

Artem Borisovskiy <kolos80@bk.ru> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kolos80@bk.ru
           Severity|normal                      |major

--- Comment #9 from Artem Borisovskiy <kolos80@bk.ru> ---
I write a simple app that reads a binary log file, decrypts the messages in parrallel and prints them when all the decrypting threads are finished. Here's a piece of my code:

--------------
auto workers = new ThreadGroup;
int i;
foreach (group; logEntries.chunks(averageChunkSize)) () {
    auto data = group;
    auto ii = i;
    auto workerThread = new Thread(
    {
        writefln("[%s] %s", ii, data.length);
    });
    workers.add(workerThread);
    workerThread.start;
    ++i;
}();
workers.joinAll;
--------------

As you can see, I tried to use hack from the code above, but it didn't help until I created "data" and "ii" variables. Just using "i" and "group" gives wrong results, just as described by Bruno. The same code works perfectly in Java, and it should in D, because it's the way closures work.

Please, fix this bug, it's real PITA and it's 6 years old after all.

--

https://issues.dlang.org/show_bug.cgi?id=2043

Gabor Mezo <gabor.mezo@outlook.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gabor.mezo@outlook.com

--- Comment #10 from Gabor Mezo <gabor.mezo@outlook.com> ---
Actually, I find this style of workaround mutch more readable than we seen before:

foreach (idx; 1 .. data.length)
{
 (idx)
 {
   // Do stuff
 }(idx);
}

It not looks too ugly if you have some JS background. :)

Please fix this bug ASAP. I really like this language but this POS always makes me sad if I take a look at my code.

--

https://issues.dlang.org/show_bug.cgi?id=2043

Kenji Hara <k.hara.pg@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrei@erdani.com

--- Comment #11 from Kenji Hara <k.hara.pg@gmail.com> ---
*** Issue 8621 has been marked as a duplicate of this issue. ***

--

https://issues.dlang.org/show_bug.cgi?id=2043

entheh@cantab.net changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |entheh@cantab.net
            Summary|Closure outer variables in  |Closure outer variables in
                   |nested blocks are not       |nested blocks are not
                   |allocated/instantiated      |allocated/instantiated
                   |correctly.                  |correctly: should have
                   |                            |multiple instances but only
                   |                            |have one.

--- Comment #12 from entheh@cantab.net ---
Expanded summaries a little on this bug and on 8621 to prevent further confusion - they are not duplicates.

--

https://issues.dlang.org/show_bug.cgi?id=2043

hsteoh@quickfur.ath.cx changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hsteoh@quickfur.ath.cx

--- Comment #13 from hsteoh@quickfur.ath.cx ---
Note that this bug breaks immutability:

---------
import std.stdio;

void main() {
    void delegate()[] a;
    foreach (i; 1 .. 10) {
        immutable j = i;
        writeln(j);
        a ~= { writeln(j); };
    }
    writeln("---");
    foreach (f; a) {
        f();
    }
}
---------

Output:
---------
1
2
3
4
5
6
7
8
9
---
9
9
9
9
9
9
9
9
9
---------

Notice that in each iteration of the loop, we are closing on the immutable value j, which is set to the loop counter. However, the output shows that the value of j has changed since the delegates were initialized. In fact, it changes at every iteration. This violates the guarantee of immutable.

--

https://issues.dlang.org/show_bug.cgi?id=2043

--- Comment #14 from hsteoh@quickfur.ath.cx ---
Note that the same thing applies if you make the loop counter immutable, which the compiler also readily accepts. But the delegates will all capture the same address which is reused over and over.

So, basically, the logical scope of the loop counter i should be restricted to a *single* iteration (otherwise we must reject immutable loop counters!), but currently, its actual scope is the loop body *across* all iterations. The delegates exacerbate this problem by extending this scope even further, outside the loop body. Ideally, the compiler should allocate each new loop counter value on the heap, unless it can prove that there are no escaping references to it at the end of the iteration, in which case it is then safe to put it on the stack (and reuse the same memory for subsequent iterations).

--

https://issues.dlang.org/show_bug.cgi?id=2043

--- Comment #15 from hsteoh@quickfur.ath.cx ---
P.S., printing out the addresses of local variables outside the loop as well as the loop counter, reveals that if we comment out the line that initializes the delegates closing over the loop counter, then the loop counter is allocated on the stack, but with the closures, the compiler actually allocates the loop counter on the heap. However, it fails to recognize that multiple iterations of the loop will reuse the same heap variable.

So it looks like the compiler is already halfway there; it's already detecting the closure and putting the loop counter on the heap instead of the stack. Now the only thing that remains, is for it to detect that it needs a new instance of the loop counter per iteration, as opposed to just a single instance.

--

https://issues.dlang.org/show_bug.cgi?id=2043

--- Comment #16 from entheh@cantab.net ---
Unfortunately I have a feeling there will be people relying on being able to do this kind of thing (though I'm not 100% sure if it currently works):

foreach (i; 0..10) {
  ...
  if (somethingStrangeHappens) {
    //Try again with the same 'i'
    i--;
    continue;
  }
}

If 'i' became a new variable for each iteration, then the modify-assign would not have the effect intended. Do we need to make 'i' implicitly final to protect against this?

--

https://issues.dlang.org/show_bug.cgi?id=2043

Ketmar Dark <ketmar@ketmar.no-ip.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ketmar@ketmar.no-ip.org

--- Comment #17 from Ketmar Dark <ketmar@ketmar.no-ip.org> ---
(In reply to entheh from comment #16)
i believe that modifying `i` should be allowed here. but without 'ref' `i` is a
copy of internal counter, so your sample correctly iterates 10 times, updating
`i` to correct value on each iteration. but this works (and it's correct too):

  foreach (ref i; 0..10) {
    ++i; // skip one
    writeln(i);
  }

this outputs 1, 3, 5, 7, 9.

> If 'i' became a new variable for each iteration, then the modify-assign would not have the effect intended.
it works this way now. ;-)

--

https://issues.dlang.org/show_bug.cgi?id=2043

--- Comment #18 from hsteoh@quickfur.ath.cx ---
If the loop index is mutable, then I think it's OK to make it shared across loop iterations. Creating a new instance of a loop variable per iteration is only necessary if (1) the loop index is immutable, and (2) it is being closed over.

(I also think modifying the loop counter of a foreach loop is a very bad idea
-- you should be using a good ole for(...) loop or while loop instead -- but
that's beside the point.)

--