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

--- Comment #36 from timon.gehr@gmx.ch ---
(In reply to Walter Bright from comment #35)
> 1. alloca() won't work, because that won't survive the end of the function.
> ...

As I said, just do it if the delegate does not escape.

> 2. Doing a heap alloc/free for every loop iteration is expensive, and since
> the cost is hidden it will come as a nasty surprise.
> ...

I don't get how this is different from other implicit heap closure allocations. A function that does such an implicit allocation not within a loop in its own body might well be called in a loop and cause a performance bottleneck. Profile.

> 3. Doing a gc allocation will generate an unbounded set of allocations, also
> coming as a nasty surprise.
> ...

This is true for all implicit heap allocations. They are also often convenient and there are already ways to flag them.

> 4. Doing my delegate rewrite will cause a "by value" which changes the
> semantics, so that won't work.
> ...

That's not true. It just gets rid of the memory corruption. The capture is still by reference, which you can easily verify by creating more than one closure over the same instance of the variable. This is the standard semantics for this kind of thing. The delegate rewrite is a very hacky way to implement it though, and probably not really the most convenient as you need to thread through control flow and the generated code will not be as good as it could be.

> I suspect the pragmatic solution is to disallow the capture of loop variables by reference by escaping delegates. The user then will be notified of the problem, and he can construct an efficient workaround that works for his application.

This fixes the memory corruption issue without much work. Deprecation might be necessary even if we fix this the right way in the end, so this is a good first step.

--

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

--- Comment #37 from Walter Bright <bugzilla@digitalmars.com> ---
(In reply to timon.gehr from comment #36)
> (In reply to Walter Bright from comment #35)
> As I said, just do it if the delegate does not escape.

Yes, I thought this was clear. This only applies if the delegate escapes.

> > 2. Doing a heap alloc/free for every loop iteration is expensive, and since
> > the cost is hidden it will come as a nasty surprise.
> > ...
> 
> I don't get how this is different from other implicit heap closure allocations. A function that does such an implicit allocation not within a loop in its own body might well be called in a loop and cause a performance bottleneck. Profile.

People have made it clear they don't particularly like hidden allocations in innocuous looking code. Hence the genesis of the @nogc attribute. For this particular issue, it would be hard to look at a random loop and see if allocations are occurring - i.e. a nasty surprise if a small change suddenly made a big hit in performance. Profiling is not the answer, as very, very few people profile code.


> > 3. Doing a gc allocation will generate an unbounded set of allocations, also
> > coming as a nasty surprise.
> > ...
> This is true for all implicit heap allocations. They are also often convenient and there are already ways to flag them.

I can hear the complaints about this already :-(


> > 4. Doing my delegate rewrite will cause a "by value" which changes the
> > semantics, so that won't work.
> > ...
> That's not true.

I believe it is:

    int i = 0;
    auto dg = { ++i; }
    dg();
    print(i);  // zero or one?

By ref would make it one, with the rewrite it would be 0 because the ref would be to a copy of i, not the original.

> The delegate rewrite is a very hacky way
> to implement it though, and probably not really the most convenient as you
> need to thread through control flow and the generated code will not be as
> good as it could be.

Things get very sticky if you've got closures from functions nested several levels deep. It turns out to be pretty hard to get all this stuff right. Doing the rewrite means I know it will be correct, as it leverages all that complex, debugged, working code.


> > I suspect the pragmatic solution is to disallow the capture of loop variables by reference by escaping delegates. The user then will be notified of the problem, and he can construct an efficient workaround that works for his application.
> 
> This fixes the memory corruption issue without much work. Deprecation might be necessary even if we fix this the right way in the end, so this is a good first step.

At least we agree on that step :-), although I am far less sanguine about adding hidden allocations inside loops being an acceptable option.

BTW, I belatedly realized that it wasn't just about loop variables. Capturing non-loop variables that get destructed when they go out of scope also cannot be done.

--

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

--- Comment #38 from timon.gehr@gmx.ch ---
(In reply to Walter Bright from comment #37)
> (In reply to timon.gehr from comment #36)
> > (In reply to Walter Bright from comment #35)
> > As I said, just do it if the delegate does not escape.
> 
> Yes, I thought this was clear. This only applies if the delegate escapes.
> 
> > > 2. Doing a heap alloc/free for every loop iteration is expensive, and since
> > > the cost is hidden it will come as a nasty surprise.
> > > ...
> > 
> > I don't get how this is different from other implicit heap closure allocations. A function that does such an implicit allocation not within a loop in its own body might well be called in a loop and cause a performance bottleneck. Profile.
> 
> People have made it clear they don't particularly like hidden allocations in innocuous looking code.

Some people. Others just want obvious code to do the obvious thing. Also, closures escaped from the loop body don't look innocuous.

> Hence the genesis of the @nogc attribute.

Exactly. I'm not proposing anything that would not be catched by @nogc.

> For this
> particular issue, it would be hard to look at a random loop and see if
> allocations are occurring

Not really. Annotate it with @nogc and you will see. This is really not a new issue. This is about consistently applying an existing rule.

> - i.e. a nasty surprise if a small change suddenly
> made a big hit in performance. Profiling is not the answer, as very, very
> few people profile code.
> ...

A "big hit in performance" is noticeable.

> 
> > > 3. Doing a gc allocation will generate an unbounded set of allocations, also
> > > coming as a nasty surprise.
> > > ...
> > This is true for all implicit heap allocations. They are also often convenient and there are already ways to flag them.
> 
> I can hear the complaints about this already :-(
> ...

There will also be complaints about missing closure support.

> 
> > > 4. Doing my delegate rewrite will cause a "by value" which changes the
> > > semantics, so that won't work.
> > > ...
> > That's not true.
> 
> I believe it is:
> 
>     int i = 0;
>     auto dg = { ++i; }
>     dg();
>     print(i);  // zero or one?
> 
> By ref would make it one, with the rewrite it would be 0 because the ref
> would be to a copy of i, not the original.
> ...

No, the rewrite would wrap the loop body and nothing else. foreach loops have a lowering similar to the following:

foreach(i; 0..n) delegates~=()=>i;

->

for(int __i=0;__i<n;__i++){
    int i=__i;
    delegates~=()=>i;
}

Here, the rewrite would be:

for(int __i=0;__i<n;__i++)(){
    int i=__i;
    delegates ~= ()=>i;
}()

This has the same semantics as before, and every closure gets its own copy of i, as this is a different variable for each of them (because it is declared in the loop body).

If you just had the for loop:

for(int i=0;i<n;i++){
    delegates~=()=>i;
}

Then the (unnecessary!) rewrite would be:

for(int i=0;i<n;i++)(){
    delegates~=()=>i;
}();

And all closures in the body get the same copy of i, as they all see the same version. In this case, in the end, every closure would point to the same i and the value of that i would be n. For this case, no additional implicit allocations (gc or non-gc) would be necessary.

> > The delegate rewrite is a very hacky way
> > to implement it though, and probably not really the most convenient as you
> > need to thread through control flow and the generated code will not be as
> > good as it could be.
> 
> Things get very sticky if you've got closures from functions nested several
> levels deep. It turns out to be pretty hard to get all this stuff right.
> Doing the rewrite means I know it will be correct, as it leverages all that
> complex, debugged, working code.
> ...

You know that that part will be correct. Wrapping the loop body into a function does not in general result in valid code though. (E.g. break, continue, early returns, interactions with scope.) Therefore, there need to be additional hacks.

> ...
> BTW, I belatedly realized that it wasn't just about loop variables.
> Capturing non-loop variables that get destructed when they go out of scope
> also cannot be done.

Yes, this is also an issue, though not related to the current one.

--

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

greenify <greeenify@gmail.com> changed:

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

--

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

--- Comment #39 from Artem Borisovskiy <kolos80@bk.ru> ---
Sorry for off-topic, but this discussion brought back memories of the classic song by Frank C Mantra:

It's a feature, not a bug,
No one really gives a *cough*,
So if you want it to be fixed,
Go on and fork it, badam badam pap-pa,
How hard can it be, badam badam pap-pa,
Trust me, it's worth it, badam badam pap-pa,
In 10 years you'll seeeeeeeeeeeeeeeeeeeeeeeeeeeee!

--

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

Bolpat <qs.il.paperinik@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |qs.il.paperinik@gmail.com

--

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

Walter Bright <bugzilla@digitalmars.com> changed:

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

--- Comment #40 from Walter Bright <bugzilla@digitalmars.com> ---
Note that dgList exceeds the lifetime of b, and should fail to compile for that reason.

Let's try it (adding import and @safe and updating the code to D2):
----
import std.stdio;

@safe:

void delegate()[] dgList;

void test() {

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

> dmd test.c -preview=dip1000
test.d(13): Error: reference to local variable `b` assigned to non-scope
parameter `_param_0`

This is correct behavior. There is no way D should allow references to variables that went out of scope.

--

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

timon.gehr@gmx.ch changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|WORKSFORME                  |---

--- Comment #41 from timon.gehr@gmx.ch ---
The correct behavior for a delegate literal that outlives a referenced variable is to allocate a closure. This case is not an exception.

--

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

--- Comment #42 from Artem Borisovskiy <kolos80@bk.ru> ---
(In reply to Walter Bright from comment #40)
> > dmd test.c -preview=dip1000

Is DIP1000 going to become the standard behaviour anytime soon? I left D, my former favourite language, for good 4 years ago because of this ancient bug (more like it has become the last straw that broke the camel's back). It turned out easier to figure out how Rust lifetimes work, than to not shoot myself in the foot in D, a GC-based language, that's supposed to be even safer w.r.t. memory.

What's even the current state of DIP1000?

--

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

Adam D. Ruppe <destructionator@gmail.com> changed:

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

--- Comment #43 from Adam D. Ruppe <destructionator@gmail.com> ---
> test.d(13): Error: reference to local variable `b` assigned to non-scope parameter `_param_0`

This error is on the *writeln* line, not the actual capture line. It has absolutely nothing to do with the actual bug.

--