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

           Summary: Closure outer variables in nested blocks are not
                    allocated/instantiated correctly.
           Product: D
           Version: 2.012
          Platform: PC
        OS/Version: Windows
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: bugzilla@digitalmars.com
        ReportedBy: brunodomedeiros+bugz@gmail.com


An outer variable declared in a nested (and scoped) block that is captured by a
closure will be allocated on the heap, but only once per function execution,
instead of once per block execution, which is what makes sense.
See code below:

--- ---
void delegate()[] dgList;

void test() {

        foreach(int i; [1, 2, 3]) {
                int b = 2;
                dgList ~= { writefln(b); };
                writefln(&b); // b will be the *same* on each iteration
        }
}
---


-- 

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


brunodomedeiros+bugz@gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |benoit@tionex.de




------- Comment #1 from brunodomedeiros+bugz@gmail.com  2008-07-27 12:23 -------
*** Bug 2228 has been marked as a duplicate of this bug. ***


-- 

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





------- Comment #2 from bugzilla@digitalmars.com  2008-08-14 22:05 -------
Here's a better illustration of the problem:

void delegate() dg;
void main()
{
    foreach(int i; 0 .. 2)
    {
        invariant int b = i;
        if (i == 0)
            dg = { printf("b = %d\n", b); };
        dg();
    }
    dg();
}

'b' is supposed to be invariant, yet its value (as seen by dg()) changes. One possible fix for this is to make invariants that are initialized with varying values be illegal to reference from a delegate.


-- 

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





------- Comment #3 from brunodomedeiros+bugz@gmail.com  2008-08-15 07:21 -------
(In reply to comment #2)
> Here's a better illustration of the problem:
> void delegate() dg;
> void main()
> {
>     foreach(int i; 0 .. 2)
>     {
>         invariant int b = i;
>         if (i == 0)
>             dg = { printf("b = %d\n", b); };
>         dg();
>     }
>     dg();
> }
> 'b' is supposed to be invariant, yet its value (as seen by dg()) changes. One
> possible fix for this is to make invariants that are initialized with varying
> values be illegal to reference from a delegate.


I'm assuming that a variable declared inside a loop conceptually represents not a single variable that gets reinitialized with each iteration, but instead represents several different variable "instances" (so to speak), each "created" in each iteration. I vaguely recall you confirming this notion (which is indeed what makes more sense and is more consistent), but I can't pinpoint the comment.

Given that assumption, then your code is *not* a better illustration of the problem. This isn't specific to invariant, and the problem is not exactly that b's invariance is broken, but rather that D fails to create several "instances" of b. In other words, I would expect your example to work the same as this code:

-----------

void delegate() dg;

void foreachBody(int i) {
    invariant int b = i;
    if (i == 0)
        dg = { printf("b = %d\n", b); };
    dg();
}

void main(string[] args)
{
    foreach(int i; 0 .. 2)
       foreachBody(i);
    dg();
}

-----------
I hope the example above clarifies what I mean by saying that b (and any variables inside the loop) should be "different" variables every time the scope is entered.


-- 

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





------- Comment #4 from benoit@tionex.de  2008-08-15 07:48 -------
Given this, the Java syntax makes much sense.
There, a variable that is accessible from an anonymous class must be marked as
'final'. This makes it easy, because the variable can be copied to the
anonymous class.

Perhaps D should consider to recreate a new concept for closures.

E.g. the old nested closure which are not allocated on the heap can access everything. And the new full closure can only access 'final' vars and is created with a special syntax like the suggestion from Sean "& new {}". Then not the outside stackframe is heap allocated, instead the closure has an allocated from with a copy of the accessed final vars.


-- 

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


Bruno Medeiros <bdom.pub+deebugz@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nfxjfg@gmail.com


--- Comment #5 from Bruno Medeiros <bdom.pub+deebugz@gmail.com> 2010-11-19 08:59:46 PST ---
*** Issue 4966 has been marked as a duplicate of this 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=2043


Witold Baryluk <baryluk@smp.if.uj.edu.pl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |baryluk@smp.if.uj.edu.pl


--- Comment #6 from Witold Baryluk <baryluk@smp.if.uj.edu.pl> 2011-01-01 18:20:46 PST ---
I today hit this bug. It really isn't obvious how to workaround this problem. It is not possible to easly just allocate variable or struct with variable's value manually, as it still will just by single identifier, and it will at the end still point to single entity.

Simplified case, which returns array of delegates each printing next integer.

  import std.stdio;
  alias void delegate() D;
  /* do not work, */
  auto f(int n) {
        auto tab = new D[n];
        for (int i = 0; i < n; i++) {
                // invariant ii = i; // will not help
                // struct S { int i_; }; S x = new S; x.i_ = i;
                // will also not help, as x is on stack
                tab[i] = delegate void() { writefln("i=%d", i); };
                // also using nested function, and taking address do not work
        }
        return tab;
  }

  void main() {
        auto tab = f(10);
        foreach (k, ref x; tab) {
                writef("%d : ", k);
                x();
        }
  }

It currently will print in all cases:

  0 : i=10
  1 : i=10
  2 : i=10
  3 : i=10
  4 : i=10
  5 : i=10
  6 : i=10
  7 : i=10
  8 : i=10
  9 : i=10


My current workaround involves looping using recursion (and be sure that compiler do not make it tail-recursive probably):

/* recursive loop version will work */
auto f2(int n) {
    auto tab = new D[n];
    void f2r(int i) {
        tab[i] = delegate void() { writefln("i=%d", i); };
        if (i < n-1) {
            f2r(i+1); /* recursion */
        }
    }
    f2r(0); /* enter recursion */
    return tab;
}

Which will make our constructed delegates work as desired:

  0 : i=0
  1 : i=1
  2 : i=2
  3 : i=3
  4 : i=4
  5 : i=5
  6 : i=6
  7 : i=7
  8 : i=8
  9 : i=9

(I tested if this is not just a a coincidence, but overwriting stack by random values, and it works still correctly. BTW. switch for compiler which will show what local variables are/will automatically allocated on heap will be usefull).

As of definitive solution, i think "invariant" which is referenced by escaping delegate should be allocated on heap, and its addresses should be remembered immediately in constructed delegate. Other possibility is separate syntax, like "new delegate void() ... ", which will make all variables referenced by delegate a copies of current values of variables with the same names (considering scoping rules).


Hope this will be helpful for somebody.

-- 
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=2043


timon.gehr@gmx.ch changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |timon.gehr@gmx.ch


--- Comment #7 from timon.gehr@gmx.ch 2012-06-19 09:27:41 PDT ---
(In reply to comment #6)
> ...
> Simplified case, which returns array of delegates each printing next integer.
> 
>   import std.stdio;
>   alias void delegate() D;
>   /* do not work, */
>   auto f(int n) {
>         auto tab = new D[n];
>         for (int i = 0; i < n; i++) {
>                 // invariant ii = i; // will not help
>                 // struct S { int i_; }; S x = new S; x.i_ = i;
>                 // will also not help, as x is on stack
>                 tab[i] = delegate void() { writefln("i=%d", i); };
>                 // also using nested function, and taking address do not work
>         }
>         return tab;
>   }
> 
>   void main() {
>         auto tab = f(10);
>         foreach (k, ref x; tab) {
>                 writef("%d : ", k);
>                 x();
>         }
>   }
> 
> It currently will print in all cases:
> 
>   0 : i=10
>   1 : i=10
>   2 : i=10
>   3 : i=10
>   4 : i=10
>   5 : i=10
>   6 : i=10
>   7 : i=10
>   8 : i=10
>   9 : i=10
> 
> 
> My current workaround involves looping using recursion (and be sure that
> compiler do not make it tail-recursive probably):
> [snip.]

There is a much simpler workaround:

  import std.stdio;
  alias void delegate() D;

  auto f(int n) {
        auto tab = new D[n];
        for (int i = 0; i < n; i++) (){ int i = i;
                tab[i] = delegate void() { writefln("i=%d", i); };
        }();
        return tab;
  }

  void main() {
        auto tab = f(10);
        foreach (k, ref x; tab) {
                writef("%d : ", k);
                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=2043


Russ Lewis <webmaster@villagersonline.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |webmaster@villagersonline.c
                   |                            |om


--- Comment #8 from Russ Lewis <webmaster@villagersonline.com> 2013-01-05 23:52:25 PST ---
Comment 7's workaround is a little brilliant, though terribly hard to read.
Basically, it's declaring a delegate with the
    () {
       <code>
    }
syntax, and calling it (with a trailing () ), all inline in each pass of the
for() loop.

This works around the problem because, as far as I can tell, the DMD compiler will make only 1 heap copy of any given variable *per instance of a function call*, even if the variable is a loop-local variable.  So, if you run 10 iterations of a for() loop, and create a delegate in each one, then all 10 iterations of the loop share the same heap space for their variables, even the loop-local ones (which ought to have 10 different copies).  But, if you instead call the same function 10 times in a row (as comment 7 effectively does), then you get 10 different copies of the variable on the heap.

I can understand why this is a hard problem to solve on the compiler side...but, IMHO, it's a fairly important one to solve.  This bug causes some rather hard-to-understand bugs in what would otherwise look like straightforward code.

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