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

Jonathan M Davis <issues.dlang@jmdavisProg.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|phobos                      |dmd
           Severity|normal                      |regression

--- Comment #1 from Jonathan M Davis <issues.dlang@jmdavisProg.com> ---
After dustmiting, I've reduced it to

---
import std.range;

void main()
{
    static struct S
    {
        int val;
        ~this()
        {
            assert(val != 99, "Double Destroy");
            val = 99;
        }
    }

    auto arr = [S(0), S(1), S(2), S(3)];
    auto result = arr.mySubstitute(S(1), S(42));
    auto front = result.front;
}

auto mySubstitute(R, Substs...)(R r, Substs)
{
    struct MySubstituteElement
    {
        Substs substs;

        auto opCall(E)(E )
        {
            return substs[0];
        }
    }

    auto er = MySubstituteElement.init;

    return r.myMap!er;
}

template myMap(fun...)
{
    auto myMap(Range)(Range r)
    {
        return MyMapResult!(fun, Range)(r);
    }
}

struct MyMapResult(alias fun, Range)
{
    Range _input;

    @property empty()
    {
        return _input.empty;
    }

    void popFront()
    {
    }

    @property front()
    {
        return fun(_input);
    }
}
---

I could probably reduce it further if I understood the issue better, but any further transformations that I've tried have made the problem go away.

In any case, given that all of the Phobos code was removed aside from import std.range to get the range primitives for arrays, it looks to me like this is a compiler bug.

Also, trying it on run.dlang.org, it looks like it broke with dmd 2.068.2. Looking at the changelog - https://dlang.org/changelog/2.068.2.html - it looks like the problem was likely caused by reverting the fix for issue 14708: https://issues.dlang.org/show_bug.cgi?id=14708

I haven't dug into any of the details, so I really don't know what the exact issue is, but destructors need to not be running on an already destroyed object. It can potentially be worked around by explicitly setting a struct to its init value at the end of a destructor, but no one is going to think of doing that normally.

I suspect that the main reason that this wasn't caught previously is simply because destructors aren't used all that frequently in D, but I ran into it at work when trying to add a destructor to an existing type, and this is one of the problems that I hit.

--

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

Jonathan M Davis <issues.dlang@jmdavisProg.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code

--

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

Jonathan M Davis <issues.dlang@jmdavisProg.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|std.algorithm.substitute    |Under some circumstances,
                   |appears to be destroying an |the compiler destroys the
                   |already destroyed value     |same object more than once

--

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

kinke <kinke@gmx.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kinke@gmx.net

--- Comment #2 from kinke <kinke@gmx.net> ---
I've reduced it to this:
```
struct S {
    int val;
    ~this() {
        import core.stdc.stdio : printf;
        printf("dtor %d, %p\n", val, &this);
        val = 99;
    }
}

void main() {
    auto r = makeR();
    r.foo();
}

auto makeR() {
    auto s = S(1); // allocated in GC closure
    return R!s();
    // the frontend wrongly destructs `s` at the end of the function, see
-vcg-ast
}

struct R(alias s) {
    void foo() {
        assert(s.val != 99, "already destroyed");
    }
}
```

So the problem is that in `makeR`, `s` is correctly allocated in a GC closure (because the return value has a hidden reference to it, so the local escapes its stack frame), but the frontend still eagerly destructs it before exiting the function. So calling `r.foo()` accesses a destructed `S` instance.

AFAIK, variables in GC closures cannot be destructed, because there's no TypeInfo associated with the GC-allocated memory block. So the trade-off here would probably be that the escaping `s` local would be never destructed.

--

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

--- Comment #3 from kinke <kinke@gmx.net> ---
To be clear, this is no double-destroy issue, as can be seen when printing the addresses of the destructed instances in the 2nd test case. The problem in the 2nd test case is that `MyMapResult.front()` returns a copy of `er.substs[0]`, with `er` already having been destructed (due to the actual issue), so it returns a copy of an `S(99)`, which then gets destructed, and the check then wrongly complaining about a double-destroy, while in reality it's a destruction of a copy of a destructed S instance.

--

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tim.dlang@t-online.de

--- Comment #4 from Tim <tim.dlang@t-online.de> ---
This could be related to these issues:
Issue 11382 - Bad closure variable with scoped destruction
Issue 24120 - Closures break constructor/destructor safety

DMD also contains a comment for destructors in closures: https://github.com/dlang/dmd/blob/00df883e9f32c34a070e4ff74783c33416848dc9/compiler/src/dmd/toir.d#L800 BUG: doesn't handle destructors for the local variables. ...

--