D's Destructors are What Scott Meyers Warned Us About

[rikki cattermole]

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).