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

Stanislav Blinov <stanislav.blinov@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |stanislav.blinov@gmail.com
           Severity|major                       |critical

--- Comment #9 from Stanislav Blinov <stanislav.blinov@gmail.com> ---
(In reply to deadalnix from comment #8)

> You'll not that C++'s std::function will allocate on heap if you capture.

??? std::function MAY allocate on the heap, and whether it will would depend on its implementation and size of lambda's state. A decent implementation of std::function surely would not allocate if all you capture is a single reference.

> The equivalent code in C++ WILL allocate in a loop too.

Whether std::function would allocate is irrelevant. Equivalent C++ code would, generally speaking, print unspecified values for all but the string:

#include <functional>
#include <vector>
#include <iostream>

int main(int argc, char** argv) {
    std::vector<std::function<void()>> dgs;

    for (int i = 0; i < 10; i++) {
        // Capture by reference, as that's D's semantics
        dgs.emplace_back([&i] () {
            std::cout << i << "\n";
        });
    }

    dgs.emplace_back([] () {
        std::cout << "With cached variables" << "\n";
    });

    for (int i = 0; i < 10; i++) {
        int index = i;
        // Capture by reference, as that's D's semantics
        dgs.emplace_back([&index] () {
            std::cout << index << "\n";
        });
    }

    for (auto& dg: dgs) {
        dg();
    }

    return 0;
}

Debug build with clang prints 10s followed by 9s. Debug build with gcc prints garbage values. Optimized builds with either print garbage values. Walter's rewrite clearly demonstrates why. D's output is at least predictable non-garbage, if not expected.

Note the comments, as that's the crux of the problem. D's captures are always by reference, so equivalent C++ code should do the same. If you capture by copy, then sure, you'll see a printout of [0, 10) after "With cached variables", but then the code would not be equivalent.

Back to D land, as mentioned both here and in https://issues.dlang.org/show_bug.cgi?id=2043, a, uh... "workaround" would be to force a new frame by turning loop body into e.g. an anonymous lambda call:

    for (int i = 0; i < 10; i++) (int index) {
        dgs ~= () {
            import std.stdio;
            writeln(index);
        };
    } (i);

...but then bye-bye go break and continue :\

Ideally, it'd be great to see by-copy and moving captures in D.

I do support the concern that the behavior is not at all obvious at a glance, however, mimicking the behavior of C# would be too restrictive, and would make a different problem not obvious at a glance, as it would hide allocations. Not that you can't detect that, just that detection won't be "at a glance".

...C++ does this right: forbids implicit captures and makes the programmer specify exactly how a capture should be performed. IMO, D should follow suit.

But at the very least, as Vladimir suggested, forming a closure that captures loop variables by reference should be @system. Code from first message should fail to compile in @safe, reporting "Closure over variable `<insert_variable_name_here>` defined in loop scope is not allowed in @safe code". Further language enhancements should follow, allowing for solutions that don't require silly wrappers. There shouldn't be ad-hoc fixes though. D has enough special cases as is.

Could someone make an executive decision as to which of the two reports should stay open? That this issue is nearing second half of its second decade should also warrant this to become critical.

--

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

--- Comment #10 from Walter Bright <bugzilla@digitalmars.com> ---
Replying to comment #1:

The observed behavior is a direct result of lambdas in D not capturing by value, they capture by reference.

It is also a direct result of loop variables, `i` and `index` although disappearing at the end of the scope, are recreated in the exact same place. Hence all the lambda references refer to the same location where each of the references points to.

The bug here is that the lambda's lifetime, in both loops, exceeds the lifetime of the variables `i` and `index`.

The error should be emitted when the first lambda, which refers to `i`, is assigned to dgs[], which has a lifetime longer than `i`. The same goes for the second lambda and `index`.

Hence, the following simplified code should fail to compile:

    @safe
    void test() {
        int delegate() dg;
        foreach (i; 0 .. 10) {
            dg = () { return i; };
        }
    }

--

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

--- Comment #11 from Walter Bright <bugzilla@digitalmars.com> ---
Even allocating a closure on the gc heap is not a solution here, because only one allocation will be made which will be shared by each lambda, exhibiting the same behavior.

--

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

Tim <tim.dlang@t-online.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tim.dlang@t-online.de
           See Also|                            |https://issues.dlang.org/sh
                   |                            |ow_bug.cgi?id=2043

--

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

--- Comment #12 from deadalnix <deadalnix@gmail.com> ---
(In reply to Stanislav Blinov from comment #9)
> (In reply to deadalnix from comment #8)
> 
> > You'll not that C++'s std::function will allocate on heap if you capture.
> 
> ??? std::function MAY allocate on the heap, and whether it will would depend on its implementation and size of lambda's state. A decent implementation of std::function surely would not allocate if all you capture is a single reference.
> 
> > The equivalent code in C++ WILL allocate in a loop too.
> 
> Whether std::function would allocate is irrelevant. Equivalent C++ code would, generally speaking, print unspecified values for all but the string:
> 
> #include <functional>
> #include <vector>
> #include <iostream>
> 
> int main(int argc, char** argv) {
>     std::vector<std::function<void()>> dgs;
> 
>     for (int i = 0; i < 10; i++) {
>         // Capture by reference, as that's D's semantics
>         dgs.emplace_back([&i] () {
>             std::cout << i << "\n";
>         });
>     }
> 
>     dgs.emplace_back([] () {
>         std::cout << "With cached variables" << "\n";
>     });
> 
>     for (int i = 0; i < 10; i++) {
>         int index = i;
>         // Capture by reference, as that's D's semantics
>         dgs.emplace_back([&index] () {
>             std::cout << index << "\n";
>         });
>     }
> 
>     for (auto& dg: dgs) {
>         dg();
>     }
> 
>     return 0;
> }
> 

This code is invalid C++, whatever you deduce from it engages only yourself and does not involve C++ in any way. In fact, this code could format your hard drive and and would still be within the C++ spec.

--

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

--- Comment #13 from deadalnix <deadalnix@gmail.com> ---
(In reply to Walter Bright from comment #10)
> Replying to comment #1:
> 
> The observed behavior is a direct result of lambdas in D not capturing by value, they capture by reference.
> 

This is true of the first exemple, for which things behave apropriately.

This is not true of the second one, as, even though it capture by reference, it capture a reference to a new variable every time.

> Even allocating a closure on the gc heap is not a solution here, because only one allocation will be made which will be shared by each lambda, exhibiting the same behavior.

What's nice when most options are incorrect, is that it's fairly obvious what
the correct ones are. in this cases, there are 3:
 - All variables have function scope, like in python.
 - A new closure is allocated for every loop iteration, like in JavaScript or
C#.
 - Don't pretend you are a safe language in any way like C++ and put the burden
of getting things right on the user.

Considering D as it stands, it is obvious that the second choice is the correct one. But can also chose to do away with RAII and go for the first option. Or we can get rid of @safe .

Or we can ignore the issue for one more decade and hope it goes away as everybody moves to rust.

--

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

--- Comment #14 from Walter Bright <bugzilla@digitalmars.com> ---
I don't much care for the "allocate every iteration of the loop" because it may not be obvious to the user that this is happening, and may cause an awful lot of memory to be allocated.

--

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

--- Comment #15 from Walter Bright <bugzilla@digitalmars.com> ---
The solution I outlined in https://issues.dlang.org/show_bug.cgi?id=21929#c10 which is making it an error is the most reasonable one for the way D works.

--

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

--- Comment #16 from deadalnix <deadalnix@gmail.com> ---
It's probably suboptimal, but nevertheless consistent with current semantic.

--

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

Iain Buclaw <ibuclaw@gdcproject.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P1                          |P2

--