(I'm referring to Scott's 2014 DConf talk: https://www.youtube.com/watch?v=KAWA1DuvCnQ)

I was actually preparing a DIP about this when Manu posted to the forums about his own related problems with C++ interop.

I traced a bug in some of my D code to my own misunderstanding of how D's destructors actually work.  So I did some research and discovered a bunch of edge cases with using __dtor, __xdtor and hasElaborateDestructor.  I tried reviewing the packages on code.dlang.org and some elsewhere (thankfully only about 1% of D packages use these functions directly) and it turns out I'm not the only one confused.  I don't have time to file bug reports for every package, so, if you're responsible for code that handles destructors manually, please do a review.  There's a *lot* of buggy code out there.

I'm starting this thread to talk about ways to make things better, but first the bad news.  Let's use this idiomatic code as an example of typical bugs:

void foo(T)(auto ref T t)
{
    ...
    static if (hasElaborateDestructor!T)
    {
        t.__dtor();
    }
    ...
}

Gotcha #1:
__dtor calls the destructor defined by T, but not the destructor defined by any members of T (if T is a struct or class).  I know, some of you might be thinking, "Silly sarn, that's what __xdtor is for, of course!"  Turns out this isn't that widely known or understood (__dtor is used in examples in the spec --- https://dlang.org/spec/struct.html#assign-overload).  A lot of code is only __dtor-aware, and there's at least some code out there that checks for both __dtor and __xdtor and mistakenly prefers __dtor.  __xdtor only solves the specific problem of also destroying members.

Gotcha #2:
The destructor will never be run for classes because hasElaborateDestructor is only ever true for structs.  This is actually per the documentation, but it's also not well known "on the ground" (e.g., a lot of code has meaningless is(T == class) or is(T == struct) around hasElaborateDestructor).  The code example is obviously a leak if t was emplace()d in non-GC memory, but even for GCed classes, it's important for containers to be explicit about whether or not they own reference types.

Gotcha #3:
Even if __dtor is run on a class instance, it generally won't run the correct destructor because it's not virtual.  (I.e., if T is a base class, no destructors for derived classes will be called.)  The spec says that D destructors are virtual, but this is emulated using runtime type information rather than by using the normal virtual function implementation.  __xdtor is the same.

Gotcha #4:
Even if the right derived class __dtor is run, it won't run the destructors for any base classes.  The spec says that destructors automatically recurse to base classes, but, again, this is handled manually by walking RTTI, not by making the destructor function itself recurse.

Gotcha #5:
The idiom of checking if something needs destruction before destroying it is often implemented incorrectly.  As before, hasElaborateDestructor works for structs, but doesn't always work as expected for classes.  hasMember!(T, "__dtor") seems to work for classes, but doesn't work for a struct that doesn't define a destructor, but requires destruction for its members (a case that's easy to miss in testing).

It looks like most D programmers think that D destructors work like they typically do in C++, just like I did.

Here are some recommendations:
* Really try to just use destroy().  Manually working with __dtor/__xdtor is a minefield, and I haven't seen any code that actually reimplements the RTTI walk that the runtime library does.  (Unfortunately destroy() currently isn't zero-overhead for plain old data structs because it's based on RTTI, but at least it works.)
* Avoid trying to figure out if something needs destruction.  Just destroy everything, or make it clear you don't own classes at all, or be totally sure you're working with plain old data structs.
* Some code uses __dtor as a way to manually run cleanup code on an object that will be used again.  Putting this cleanup code into a normal method will cause fewer headaches.

The one other usage of these low-level destructor facilities is checking if a type is a plain old data struct.  This is an important special case for some code, but everyone seems to do the check a different way.  Maybe a special isPod trait is needed.

On 23/05/2018 1:59 PM, sarn wrote:
> (I'm referring to Scott's 2014 DConf talk: https://www.youtube.com/watch?v=KAWA1DuvCnQ)
> 
> I was actually preparing a DIP about this when Manu posted to the forums about his own related problems with C++ interop.
> 
> I traced a bug in some of my D code to my own misunderstanding of how D's destructors actually work.  So I did some research and discovered a bunch of edge cases with using __dtor, __xdtor and hasElaborateDestructor.  I tried reviewing the packages on code.dlang.org and some elsewhere (thankfully only about 1% of D packages use these functions directly) and it turns out I'm not the only one confused.  I don't have time to file bug reports for every package, so, if you're responsible for code that handles destructors manually, please do a review.  There's a *lot* of buggy code out there.
> 
> I'm starting this thread to talk about ways to make things better, but first the bad news.  Let's use this idiomatic code as an example of typical bugs:
> 
> void foo(T)(auto ref T t)
> {
>      ...
>      static if (hasElaborateDestructor!T)
>      {
>          t.__dtor();
>      }
>      ...
> }
> 
> Gotcha #1:
> __dtor calls the destructor defined by T, but not the destructor defined by any members of T (if T is a struct or class).  I know, some of you might be thinking, "Silly sarn, that's what __xdtor is for, of course!"  Turns out this isn't that widely known or understood (__dtor is used in examples in the spec --- https://dlang.org/spec/struct.html#assign-overload).  A lot of code is only __dtor-aware, and there's at least some code out there that checks for both __dtor and __xdtor and mistakenly prefers __dtor.  __xdtor only solves the specific problem of also destroying members.
> 
> Gotcha #2:
> The destructor will never be run for classes because hasElaborateDestructor is only ever true for structs.  This is actually per the documentation, but it's also not well known "on the ground" (e.g., a lot of code has meaningless is(T == class) or is(T == struct) around hasElaborateDestructor).  The code example is obviously a leak if t was emplace()d in non-GC memory, but even for GCed classes, it's important for containers to be explicit about whether or not they own reference types.
> 
> Gotcha #3:
> Even if __dtor is run on a class instance, it generally won't run the correct destructor because it's not virtual.  (I.e., if T is a base class, no destructors for derived classes will be called.)  The spec says that D destructors are virtual, but this is emulated using runtime type information rather than by using the normal virtual function implementation.  __xdtor is the same.
> 
> Gotcha #4:
> Even if the right derived class __dtor is run, it won't run the destructors for any base classes.  The spec says that destructors automatically recurse to base classes, but, again, this is handled manually by walking RTTI, not by making the destructor function itself recurse.
> 
> Gotcha #5:
> The idiom of checking if something needs destruction before destroying it is often implemented incorrectly.  As before, hasElaborateDestructor works for structs, but doesn't always work as expected for classes.  hasMember!(T, "__dtor") seems to work for classes, but doesn't work for a struct that doesn't define a destructor, but requires destruction for its members (a case that's easy to miss in testing).
> 
> It looks like most D programmers think that D destructors work like they typically do in C++, just like I did.
> 
> Here are some recommendations:
> * Really try to just use destroy().  Manually working with __dtor/__xdtor is a minefield, and I haven't seen any code that actually reimplements the RTTI walk that the runtime library does.  (Unfortunately destroy() currently isn't zero-overhead for plain old data structs because it's based on RTTI, but at least it works.)
> * Avoid trying to figure out if something needs destruction. Just destroy everything, or make it clear you don't own classes at all, or be totally sure you're working with plain old data structs.
> * Some code uses __dtor as a way to manually run cleanup code on an object that will be used again.  Putting this cleanup code into a normal method will cause fewer headaches.
> 
> The one other usage of these low-level destructor facilities is checking if a type is a plain old data struct.  This is an important special case for some code, but everyone seems to do the check a different way.  Maybe a special isPod trait is needed.

I would consider the current state with classes a bug.
So ticket please, it should not require a DIP to change (although Walter may disagree).

On 22 May 2018 at 18:59, sarn via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
> (I'm referring to Scott's 2014 DConf talk:
> https://www.youtube.com/watch?v=KAWA1DuvCnQ)
>
> I was actually preparing a DIP about this when Manu posted to the forums about his own related problems with C++ interop.
>
> I traced a bug in some of my D code to my own misunderstanding of how D's destructors actually work.  So I did some research and discovered a bunch of edge cases with using __dtor, __xdtor and hasElaborateDestructor.  I tried reviewing the packages on code.dlang.org and some elsewhere (thankfully only about 1% of D packages use these functions directly) and it turns out I'm not the only one confused.  I don't have time to file bug reports for every package, so, if you're responsible for code that handles destructors manually, please do a review.  There's a *lot* of buggy code out there.
>
> I'm starting this thread to talk about ways to make things better, but first the bad news.  Let's use this idiomatic code as an example of typical bugs:
>
> void foo(T)(auto ref T t)
> {
>     ...
>     static if (hasElaborateDestructor!T)
>     {
>         t.__dtor();
>     }
>     ...
> }
>
> Gotcha #1:
> __dtor calls the destructor defined by T, but not the destructor defined by
> any members of T (if T is a struct or class).  I know, some of you might be
> thinking, "Silly sarn, that's what __xdtor is for, of course!"  Turns out
> this isn't that widely known or understood (__dtor is used in examples in
> the spec --- https://dlang.org/spec/struct.html#assign-overload).  A lot of
> code is only __dtor-aware, and there's at least some code out there that
> checks for both __dtor and __xdtor and mistakenly prefers __dtor.  __xdtor
> only solves the specific problem of also destroying members.
>
> Gotcha #2:
> The destructor will never be run for classes because hasElaborateDestructor
> is only ever true for structs.  This is actually per the documentation, but
> it's also not well known "on the ground" (e.g., a lot of code has
> meaningless is(T == class) or is(T == struct) around
> hasElaborateDestructor).  The code example is obviously a leak if t was
> emplace()d in non-GC memory, but even for GCed classes, it's important for
> containers to be explicit about whether or not they own reference types.
>
> Gotcha #3:
> Even if __dtor is run on a class instance, it generally won't run the
> correct destructor because it's not virtual.  (I.e., if T is a base class,
> no destructors for derived classes will be called.)  The spec says that D
> destructors are virtual, but this is emulated using runtime type information
> rather than by using the normal virtual function implementation.  __xdtor is
> the same.
>
> Gotcha #4:
> Even if the right derived class __dtor is run, it won't run the destructors
> for any base classes.  The spec says that destructors automatically recurse
> to base classes, but, again, this is handled manually by walking RTTI, not
> by making the destructor function itself recurse.
>
> Gotcha #5:
> The idiom of checking if something needs destruction before destroying it is
> often implemented incorrectly.  As before, hasElaborateDestructor works for
> structs, but doesn't always work as expected for classes.  hasMember!(T,
> "__dtor") seems to work for classes, but doesn't work for a struct that
> doesn't define a destructor, but requires destruction for its members (a
> case that's easy to miss in testing).
>
> It looks like most D programmers think that D destructors work like they typically do in C++, just like I did.
>
> Here are some recommendations:
> * Really try to just use destroy().  Manually working with __dtor/__xdtor is
> a minefield, and I haven't seen any code that actually reimplements the RTTI
> walk that the runtime library does.  (Unfortunately destroy() currently
> isn't zero-overhead for plain old data structs because it's based on RTTI,
> but at least it works.)
> * Avoid trying to figure out if something needs destruction.  Just destroy
> everything, or make it clear you don't own classes at all, or be totally
> sure you're working with plain old data structs.
> * Some code uses __dtor as a way to manually run cleanup code on an object
> that will be used again.  Putting this cleanup code into a normal method
> will cause fewer headaches.
>
> The one other usage of these low-level destructor facilities is checking if a type is a plain old data struct.  This is an important special case for some code, but everyone seems to do the check a different way.  Maybe a special isPod trait is needed.
>

All of these things!
I learned a lot of them over the last few days while trying to
implement destructor linkage and virtual destructors for extern(C++).

If you're in the mood to prepare a DIP, I think you should prepare this dip:

Support the syntax:
  classInstance.~this();

Which **does the right thing**. That is, aggregate of all the gotchas above!
It should recurse the ClassInfo calling the dtors there to perform a
full virtual destruction.
It should also work for structs.
It should also work for extern(C++), which is going to behave yet
differently to the cases you've described (since it needs to match C++
semantics)

On Wednesday, 23 May 2018 at 02:15:25 UTC, Manu wrote:
> It should recurse the ClassInfo calling the dtors there to perform a
> full virtual destruction.

Why? That seems restrictive as there is possibility that the parent class have an empty destruction with no attributes which your suggestion to be attribute unfriendly.(Unless the attributes are determine at compile time)

Alexander

On 22 May 2018 at 19:39, 12345swordy via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
> On Wednesday, 23 May 2018 at 02:15:25 UTC, Manu wrote:
>>
>> It should recurse the ClassInfo calling the dtors there to perform a full virtual destruction.
>
>
> Why? That seems restrictive as there is possibility that the parent class have an empty destruction with no attributes which your suggestion to be attribute unfriendly.(Unless the attributes are determine at compile time)

... what?

On Wednesday, 23 May 2018 at 01:59:50 UTC, sarn wrote:
> I'm starting this thread to talk about ways to make things better, but first the bad news.  Let's use this idiomatic code as an example of typical bugs:
>

Keep up the good work of explaining surprising things!
Someone has to do Scott's job for D.

On 5/22/18 10:15 PM, Manu wrote:
> If you're in the mood to prepare a DIP, I think you should prepare this dip:
> 
> Support the syntax:
>    classInstance.~this();

Isn't this just classInstance.destroy() ?

-Steve

On 5/22/18 9:59 PM, sarn wrote:
> (I'm referring to Scott's 2014 DConf talk: https://www.youtube.com/watch?v=KAWA1DuvCnQ)
> 
> I was actually preparing a DIP about this when Manu posted to the forums about his own related problems with C++ interop.
> 
> I traced a bug in some of my D code to my own misunderstanding of how D's destructors actually work.  So I did some research and discovered a bunch of edge cases with using __dtor, __xdtor and hasElaborateDestructor.  I tried reviewing the packages on code.dlang.org and some elsewhere (thankfully only about 1% of D packages use these functions directly) and it turns out I'm not the only one confused.  I don't have time to file bug reports for every package, so, if you're responsible for code that handles destructors manually, please do a review.  There's a *lot* of buggy code out there.
> 

I think it's very good to bring attention to these pitfalls, thanks!

> 
> Here are some recommendations:
> * Really try to just use destroy().  Manually working with __dtor/__xdtor is a minefield, and I haven't seen any code that actually reimplements the RTTI walk that the runtime library does. 

This is advice you need to follow. Using the underlying __functions are error prone, and subject to weird errors as you have found.

The other thing to do is to follow the example of what Phobos functions do when dealing with these low-level functions.

In terms of classes, the RTTI walk is callable via the function _rt_finalize (an extern(C) function, you can prototype it anywhere). I highly recommend just calling destroy as it will do this.

> (Unfortunately destroy() currently isn't zero-overhead for plain old data structs because it's based on RTTI, but at least it works.)

Hm.. that should be fixed. I don't see why we can't just do = T.init, we should at least be optimizing to this for small enough structs.

Please file an enhancement request.

> * Avoid trying to figure out if something needs destruction. Just destroy everything, or make it clear you don't own classes at all, or be totally sure you're working with plain old data structs.

Ideally, destroy should do the most efficient thing. So this is good advice as well.

> * Some code uses __dtor as a way to manually run cleanup code on an object that will be used again.  Putting this cleanup code into a normal method will cause fewer headaches.

Using __dtor is a very bad idea in almost all cases. Putting cleanup code into a normal function can have some of the same pitfalls, however (what if you forget to call the super version of the method?). The only *correct* way to destroy an object is to follow the runtime's example or call the function directly.

The destructor also has the nice feature of being called when the struct goes out of scope.

Best advice -- just use destroy on types to clean them up.

> 
> The one other usage of these low-level destructor facilities is checking if a type is a plain old data struct.  This is an important special case for some code, but everyone seems to do the check a different way.  Maybe a special isPod trait is needed.
> 

isPod as you have described it is not difficult:

enum isPod(T) = is(T == struct) && !hasElaborateDestructor!T;

But this is not necessarily the definition of POD. Generally this means it has no postblit, and some people may even be expecting such a thing to have no methods as well. So I'm not sure we want to add such a definition to the library.

Clearly destroy is preferred to checking or doing anything else, but maybe in some cases we want a more efficient destruction that cuts corners. For instance, there's no reason to blit the init value back to the object when its memory is being reclaimed. That building block is somewhat inextricable from destroy at the moment.

Ideally, you shouldn't have to worry about it as a normal user -- some genius library author should take care of these details. Unfortunately, the only place where this happens is on the stack or in the GC. All other memory allocation schemes require you to get your hands dirty with destruction.

-Steve

On Wednesday, 23 May 2018 at 03:45:39 UTC, Manu wrote:
> On 22 May 2018 at 19:39, 12345swordy via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
>> On Wednesday, 23 May 2018 at 02:15:25 UTC, Manu wrote:
>>>
>>> It should recurse the ClassInfo calling the dtors there to perform a full virtual destruction.
>>
>>
>> Why? That seems restrictive as there is possibility that the parent class have an empty destruction with no attributes which your suggestion to be attribute unfriendly.(Unless the attributes are determine at compile time)
>
> ... what?

Why? That seems restrictive as there is possibility that the parent class have an empty destruction with no attributes which causes your classInstance.~this() suggestion to be attribute unfriendly.(Unless the parent class attributes are determine at compile time)

*There I fixed it! Really wish this is a proper web forum.

On 5/23/18 9:12 AM, Steven Schveighoffer wrote:
> On 5/22/18 9:59 PM, sarn wrote:

>> (Unfortunately destroy() currently isn't zero-overhead for plain old data structs because it's based on RTTI, but at least it works.)
> 
> Hm.. that should be fixed. I don't see why we can't just do = T.init, we should at least be optimizing to this for small enough structs.

Coincidentally, this JUST changed due to a different reason: https://github.com/dlang/druntime/pull/2178

> Please file an enhancement request.

I still think it could be better, so I added a further issue:
https://issues.dlang.org/show_bug.cgi?id=18899

-Steve