Fix issue 15829 - hasElaborateDestructor doesn't work for classes

[PetarKirov]

No description provided.

[dlang-bot]

Fix	Bugzilla	Description
✓	15829	hasElaborateDestructor doesn't work for classes

[PetarKirov]

I'm not sure if I should add support for interfaces.

[JackStouffer]

@andralex It seems you're the original writer of this, so was there any specific reason this was limited to structs?

[ghost]

@ZombineDev, about interfaces. No need to support them: interfaces can't have a __dtor and at compile time you cannot get the static type of the class that might or not implement the interface.

[PetarKirov]

@bbasile that's also what I thought, but I didn't have time to investigate last night, so I decided to ask.

BTW, in .NET there is a special interface called IDisposable that is recognized by the compiler / GC. It has a Dispose method which sort of works like a destructor - the GC will call it before collecting the object. Also you can call it manually for a deterministic resource handling behavior. For example:

interface Scheduler
{
    IDisposable schedule(Task t, Duration dueTime);
}

When you schedule a task(function) for async execution, the Scheduler gives you back a handle through which you can cancel the execution, by calling the dispose method.

I'm wondering if it is a good idea to allow interfaces that define ~this() method to achieve the same in D. (Another option is to copy the interface from .NET - the difference is that it allow releasing part of the object's resources without destroying the object as a whole). So instead of relying on TypeInfo.dtor (includingTypeInfo and ModuleInfo in the executable may not be desirable or possible at all in constrained environments), the runtime or the user can make a dynamic cast from Object to IDisposable to find out if the object in question needs special care.

[PetarKirov]

It seems that this would be a breaking change if it's accepted. I'll investigate if there is much work needed to make the whole Phobos test suite pass.

[DmitryOlshansky]

I'd argue that classes don't have a destruct but rsther finalizer. As such finalizer is not called on copy( reference type) unlike struct dtor which is what is checked by this trait everywhere.

[PetarKirov]

@DmitryOlshansky the problem is that existing code already relies on hasElaborateDestructor to work for any type. For exmaple calling dispose on array of classes. (I'll file a bug for that soon).

[PetarKirov]

We can either fix all code in existance that incorrectly uses it and put a warning in the documentation of hasElaborateDestructor, or we can fix it here.

[ntrel]

put a warning in the documentation of hasElaborateDestructor

There pretty much is already:

Elaborate destructors are introduced by defining ~this() for a struct.
Classes and unions never have elaborate destructors, even though classes may define ~this().

[PetarKirov]

So we need to add a new function hasElaborateDestructor2 which works also with classes? And we need to fix all generic code to use it instead?

[ntrel]

I can see how the current trait is useful deliberately by not supporting class references. If we need a trait that supports them too, it probably shouldn't use the word elaborate - I think that word is how the existing design differentiates its destructor terminology from class destructors. I accept elaborate isn't the clearest word though.

[wilzbach]

Ping as a rebase is needed. How about we deprecate hasElaborateDestructor and replace it with the fixed version?

[DmitryOlshansky]

So we need to add a new function hasElaborateDestructor2 which works also with classes?

The thing is - elaborate destructor is specifically about struct-style destructor that is called on out of scope. So adding a hasElaborateFinalizer might be better....

[wilzbach]

The thing is - elaborate destructor is specifically about struct-style destructor that is called on out of scope.

Then fix the name of elaborateDestructor (seems to be a unique D creation) ;-)

it probably shouldn't use the word elaborate -
I accept elaborate isn't the clearest word though.

+1

So adding a hasElaborateFinalizer might be better....

Destructor?

[PetarKirov]

I would like to get @andralex's opinion on this issue.

I really don't see why anyone would want a trait that works only with structs, especially when if you needed that you could do (hasElaborateDestructor!T && is(T == struct)).
OTOH, at least 90% of the time you need hasElaborateDestructor to tell you if you need to run dtors when manually managing memory (no matter if T is a class or a struct) and if it returns false for e.g. a container of classes with dtors, you get a nice big memory/resource leak.
And this happens after you went through all the trouble of writing @nogc containers and smart pointers to ensure deterministic memory / resource reclamation.
This trait is currently useless because you need to reimplement it every time you want correct behaviour.

@DmitryOlshansky dtors have nothing to do with value vs. ref semantics (opAssign has - structs can define it, but classes can't). dtors matter only when the object is destroyed.
Calling class dtors "finalizers" is just as wrong as calling dtors of heap-allocated structs "finalizers" is. A dtor is a dtor no matter the memory management scheme.
Well, maybe before @nogc was introduced, calling class dtors "finalizers" was instructive for newbies, because that way it was easier to remember that you shouldn't rely on them, because with GCs you never know if it is safe, but now that @nogc is here to stay, relying on class dtors is perfectly ok, provided that you use scoped, some sort of smart pointer or a collection/container to do the deterministic lifetime management for you.

[DmitryOlshansky]

To hell with nogc, the whole point is determinism. Take a look at std.algo.move - when moving things around manually one has to know if there is struct destruct not class (class is safe to bit-blit, it's a pointer)

[PetarKirov]

move is correct and will remain correct after this change because in all the places hasElaborateDestructor it is inside static if (is(T == struct)).

[PetarKirov]

As I said earlier, the dtor matters only when the object is destroyed. move for ref types only swaps the pointers, but if you have container that stores classes inplace (instead of storing only the references) container.move(idx, elem) would need to destroy the target just like move does for structs. In that case how would you check that the target needs to be destroyed first? Relying on a trait in std is much more portable, because if something in the language changes, Phobos would need to be updated too.

(If you are wondering why would you need a container of possibly polymorphic types to store it's elements inplace - it's a common perf optimization - see http://bannalia.blogspot.bg/2014/05/fast-polymorphic-collections.html?m=1 )

[DmitryOlshansky]

@ZombineDev makes sense, I think I'm convinced

[ntrel]

move is correct and will remain correct after this change because in all the places hasElaborateDestructor it is inside static if (is(T == struct)).

move doesn't work for static arrays of structs with destructors:
https://issues.dlang.org/show_bug.cgi?id=8067

So to truly replace hasElaborateDestructor you would need isValueType!T && hasDestructor!T.
I would add a new symbol hasDestructor which works with classes and then maybe deprecate hasElaborateDestructor once isValueType is added to Phobos.

[JackStouffer]

@ZombineDev If you're waiting on @andralex's opinion, then you should email him

[andralex]

The intent of hasElaborateDestructor is to specialize code that needs special handling of values. I agree there is a consistency argument for adding it to classes, but that would go more with the letter than the spirit of the primitive. I'll close this. Thanks!

[PetarKirov]

@andralex but what should we do about code like in std.experimental.allocator.dispose? Currently if you try to dispose an array of classes, dispose won't call their destructors! And that's just an example. There are other places where code incorrectly assumes that hasElaborateDestructor handles all types (including classes), which leads to all kinds of bugs such as resource leaks.

[nordlow]

I agree completely with @ZombineDev in this regard! I'm also missing hasElaborateDestructor-support for classes. What about putting this trait for classes into a new name, say hasElaborateFinalizer or hasFinalizer, or hasElabrateClassDestructor?

And if hasElaborateDestructor doesn't apply to classes why isn't it an error to apply that trait on classes?