[Issue 21745] Closure created in struct constructor passed to class constructor is not heap allocated

[d-bugmail at puremagic.com]

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

Walter Bright <bugzilla at digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |INVALID

--- Comment #9 from Walter Bright <bugzilla at digitalmars.com> ---
The problem is,

struct Foo, as written, fits in registers. Therefore, it is returned from
functions in registers. The {return this.i;} lambda is indeed a closure
allocated on the heap, and the `this` pointer is put in the closure. But where
is Foo located?
    Foo getFoo(int i) { return Foo(i); }
is returned in registers. It then gets copied into the local foo. The `this`
pointer in the closure is now pointing to an expired stack frame.

Note that Foo itself is not stored in the closure, a pointer to Foo is stored.

Add a member to Foo, and what happens? It no longer is returned in registers.
It is returned via a hidden pointer to a Foo variable located in the caller's
stack frame, in this case the local main.foo. The `this` pointer in the lambda
always points to main.foo. When getFoo is called again, a separate main._TMP is
created for the return value. So this works. But only by the happenstance of
NRVO optimization putting the Foo instance in main() so it isn't expired.

The fundamental problem is the `this` is saved in a closure, but what `this` is
referring to goes out of scope and so the `this` pointer is pointing to
garbage.

D should detect this. And indeed it does, if you add @safe and compile with
-dip1000:

 @safe:

 class Bar
 {
    int delegate() dg;
    this(int delegate() @safe dg) { this.dg = dg; }
 }

 struct Foo
 {
    int i;
    Bar bar;
    this(int i)
    {
        this.i = i;
        this.bar = new Bar({ return this.i; }); // line 19
    }
 }

 Foo getFoo(int i) { return Foo(i); }

 void main() {
  auto foo = getFoo(5);
  getFoo(6);
  assert(foo.bar.dg() == 5);
 }

dmd test -dip1000 yields:

 test.d(19): Error: reference to local `this` assigned to non-scope parameter
`dg` calling test3.Bar.this

This kind of subtle stack corruption problem is what DIP1000 is designed to
detect.

I recommend:

1. redesign the code so that Foo is allocated on the heap, or at least in a
location that outlives the pointer to it in a delegate

2. consider using @safe and -dip1000

--