Issue 15246 - Destructor inheritance doesn't inherit attributes properly
Summary: Destructor inheritance doesn't inherit attributes properly
Status: NEW
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: x86_64 Linux
: P2 critical
Assignee: No Owner
URL:
Keywords: accepts-invalid, safe
: 17297 17592 17867 20582 (view as issue list)
Depends on:
Blocks: 13972
  Show dependency treegraph
 
Reported: 2015-10-25 13:47 UTC by Andrei Alexandrescu
Modified: 2022-12-17 10:40 UTC (History)
9 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Andrei Alexandrescu 2015-10-25 13:47:57 UTC
Consider:

void* g;

class Widget {
	~this() {
		import std.stdio;
		writeln("Calling Widget's destructor");
		g = cast(void*) this;
		throw new Exception("");
	}
}

class Gadget : Widget {
	pure @safe @nogc nothrow ~this() {}
}

void main() {
	auto w = new Gadget;
}

This code compiles and allows "pure @safe @nogc nothrow" code to call into code that's neither.
Comment 1 Marco Leise 2015-10-26 20:05:43 UTC
You are assuming that destructors are inherited and there is an implicit super-call, but they are actually chained and called one after another from outer to inner. Under the current language semantics this bug report is invalid.
Comment 2 Marco Leise 2015-10-26 20:11:16 UTC
Here is the loop that iterates the inheritance chain from up to Object and calls destructors (if defined):
https://github.com/D-Programming-Language/druntime/blob/v2.069.0-b2/src/rt/lifetime.d#L1366
Comment 3 Andrei Alexandrescu 2015-10-26 20:57:50 UTC
(In reply to Marco Leise from comment #1)
> You are assuming that destructors are inherited and there is an implicit
> super-call, but they are actually chained and called one after another from
> outer to inner. Under the current language semantics this bug report is
> invalid.

I agree I am using terminology loosely; newly introduced destructors don't only override the existing ones, they also call them. But I don't see how that makes the bug report invalid.
Comment 4 Marco Leise 2015-10-27 09:19:56 UTC
The destructors do *neither* inherit *nor* call their parent destructors. Not as a matter of terminology, but because in D they are not called recursively, but in sequence, starting from the runtime type's dtor and working its way up the inheritance chain. Take a look at the druntime source I linked above and you will understand what happens.

To make the bug report valid we would have to introduce destructor inheritance to the language to begin with. Right now the only functions affected by the destructor attributes would be the attribute-less external(C) functions `rt_finalize` and `rt_finalize2`. (In client code we call `rt_finalize` as `destroy(Object obj)` for deterministic object destruction).
Comment 5 Andrei Alexandrescu 2015-10-27 11:14:28 UTC
(In reply to Marco Leise from comment #4)
> The destructors do *neither* inherit *nor* call their parent destructors.
> Not as a matter of terminology, but because in D they are not called
> recursively, but in sequence, starting from the runtime type's dtor and
> working its way up the inheritance chain. Take a look at the druntime source
> I linked above and you will understand what happens.
> 
> To make the bug report valid we would have to introduce destructor
> inheritance to the language to begin with. Right now the only functions
> affected by the destructor attributes would be the attribute-less
> external(C) functions `rt_finalize` and `rt_finalize2`. (In client code we
> call `rt_finalize` as `destroy(Object obj)` for deterministic object
> destruction).

I understand what happens technically (each dtor is distinct, and code external to the destructor calls them all).

What happens conceptually is that destructors in derived classes both override and call destructors in base classes. There is no need to change the language to make the bug report valid. Destructors must typecheck as if they override and call the base class destructors.
Comment 6 Marco Leise 2015-10-27 14:13:17 UTC
I'd agree with you if this was all not observable, but your change causes friction that I object against. It should be either inheritance all the way or just sequential calls as right now. Otherwise destructors will be perceived as inheriting while type-checking and as stand-alone when called directly. To illustrate this:

	import core.stdc.stdio;
	void main()
	{
		// Get a vanilla 'Ext' object without calling
		// constructors and spoiling the output.
		void[__traits(classInstanceSize, Ext)] buf = void;
		buf[] = typeid(Ext).init[];
		// Show constructor/destructor semantics.
		Ext ext = cast(Ext) buf.ptr;
		ext.__ctor();
		ext.__dtor();
	}
	class Base { 
		void* v;
		this() { printf("Base ctor\n"); }
		~this() { printf("Base dtor\n"); } // never called
	}
	class Ext : Base { 
		this() { printf("Ext ctor\n"); }
		~this() @nogc nothrow { printf("Ext dtor\n"); }
	}

Prints:
  Base ctor
  Ext ctor
  Ext dtor

In particular in this example with D's semantics it is correct to have Ext's destructor be @nogc nothrow while the Base destructor is not. It may be surprising depending on programming language background, but at least it is consistently implemented as far as I can tell.
Comment 7 Andrei Alexandrescu 2015-10-27 14:24:05 UTC
@mleise that is surprising. I assumed calling __dtor also invokes the base destructors.  What is the canonical way to destroy a D class properly?

(This bug report stands.)
Comment 8 Marco Leise 2015-10-27 15:46:35 UTC
We "finalize" them through the helper function `rt_finalize` mentioned earlier, which calls the __dtor's in sequence and is the only place that also handles destruction of the hidden "monitor" field if it was used. It is wrapped in object.d as:

	void destroy(T)(T obj) if (is(T == class))
	{
		rt_finalize(cast(void*)obj);
	}

destroy() complete object finalization + memory reinitialized to .init
__xdtor   same as __dtor + destroys any RAII members
__dtor    the ~this() function as defined in the source
(That's how I remember it. May be inaccurate.)
Comment 9 Andrei Alexandrescu 2015-10-27 16:02:03 UTC
That's quite the bummer because rt_finalize in all likelihood doesn't know anything about attributes.
Comment 10 Marco Leise 2015-10-27 17:24:01 UTC
Nope, in fact it is an extern(C) function, and I assume part of the druntime API that people replace when writing bare metal runtimes etc.

Since destructors don't need to inherit attributes for anything based on the current implementation, this bug report has no basis. If attributes were inherited, it would not solve any use case or problem. If you meant to streamline the language semantics with this, it was well meant, but actually adds to the cognitive load.

Or did you have some specific use case in mind, like being able to deterministically destroy objects in @safe functions? That's not going to happen either way. As it stands now, `destroy(obj)` and `rt_finalize`, which is neither @safe nor @nogc nor nothrow nor pure. And if we did in fact have virtual destructors like C++, the general rule with inheritance applies: The derived thing must be usable everywhere the base class is used. That disallows the removal of attributes on virtual function overrides:

class C {
   ~this() @safe
}

class D {
   override ~this(); // @system
}

void foo(C c) @safe 
{
   // Destroying D is unsafe, but we can't statically check,
   // because for all we know it is at least a C.
   destroy(c);
}

void main()
{
   foo(new D);
}
Comment 11 Rainer Schuetze 2017-04-21 18:53:16 UTC
See http://dlang.org/spec/class.html#destructors: "The destructor for the super class automatically gets called when the destructor ends. There is no way to call the super destructor explicitly."

I.e. the net effect of destructing an Object is the same as C++ virtual destructor. It doesn't really matter how it is implemented, and the usage of double underscores suggests that, too. Poking at the internals of __dtor, __xdtor or rt_finalize should not have an effect on the language definition. 

Changing Andrei's example slightly with

void main() pure nothrow @safe @nogc {
	scope w = new Gadget;
}

compiles and generates the direct call to rt_finalize violating all the attributes of main (but throws a FinalizeError due to throwing an exception - this should only happen when called by the GC).

One option for keeping annotated class destructors useful without adding more attributes: when checking attributes of the destructor of the derived class, assume a trailing call to the base class destructor, but infer it's attributes from the code. If no code is available for inference only the explicit attributes are assumed (similar to declared (but not defined) functions in templates!).
Comment 12 Stanislav Blinov 2017-05-21 13:52:45 UTC
I agree with Andrei and Rainer. It should not matter in what order the runtime calls the destructors. If it does call them, it calls all of them. So the net observable effect should be as if all of them are called.
User code is not expected to call dtors explicitly. While we *can* (and should be able to) do that, it should not allow attributes to conflict.
That means the strictest attribute set "wins", and derived class should not be allowed to loosen the restrictions.

Effectively, the first class in the hierarchy to define a destructor has final authority over the dtor attributes, and no derived classes after that can define dtor with different attributes, or implicitly violate the attributes (via members). This is not covariance, we're not looking at a virtual call.

If the class doesn't explicitly define a dtor, it should be inferred from non-reference members (structs, fixed-size arrays, etc). As of right now, members don't have any effect on the class dtor:

struct S { ~this() @nogc {} }

class A { S s; } // compiles
class B { S s;  ~this() {} } // compiles, but shouldn't

For a, ~this() @nogc should be inferred.
For B, it should be a compile-time error.

There should be a strict agreement on dtors throughout the hierarchy and within each definition. Otherwise, we're free to violate attributes however we please. For example, this also compiles (note, these are structs, not classes):

struct A { ~this() {} }
struct B { A a; ~this() @nogc {} }

But this function will not:

void snafu(B b) @nogc {}

Note the error message:
Error: @nogc function snafu cannot call non-@nogc destructor B.~this.

What??? So *there* it catches that B's dtor is not really @nogc!

While B's dtor in itself might not make any GC calls, destruction of B implies destruction of all members. A's dtor is not @nogc, and so B's shouldn't be @nogc.

Members that are reference types (classes, interfaces, dynamic arrays...) should be excluded from that check, as user code is expected to explicitly destruct them by calling the destroy() function, and so if users want to destruct them with the object, they'd have to define a destructor, which in turn will have to be checked against the destroy() calls made within:

class A { ~this() {} }

class B { A a; ~this() @nogc {} } // fine, a leaks (i.e. reliance on GC)
class C { A a; ~this() @nogc { destroy(a); } } // error, destroy() is not @nogc

class D : B { ~this() {} } // error, base dtor is @nogc

struct S { ~this() @nogc {} }

class E : A { S s; }  // error, E's dtor has to be @nogc, but A's dtor isn't

class F { ~this() @nogc {} }
class G { S s; F f; ~this() @nogc { destroy(f); } } // fine, S dtor is @nogc, destroy(f) is inferred @nogc

Same goes for safety, purity, nothrow, etc:

class A { ~this() {} }

class B { A a; ~this() nothrow {} } // fine
class C { A a; ~this() { destroy(a); } } //error, destroy() may throw

class D { ~this() @safe pure {} }
class E : D { ~this() pure {} } // error, D.~this is @safe, E.~this should also be @safe

...and so on.

We *could* allow covariance between @safe and @trusted, but once any of those are in the hierarchy, @system dtors should be out the window.

rt_finalize does not need to change. destroy(obj) should statically typecheck the hierarchy from obj up, cast a pointer to rt_finalize to a function with appropriate set of attibutes, and call it. But we need to make sure that rt_finalize doesn't perform any GC calls (it doesn't look like it does, the monitor is not a GC-allocated object).

The only special cases here are if destroy(obj) is called with an Object (or an interface, which just casts to Object anyway). For these, it looks like destroy() should treat Object as if it was a class with this dtor:

~this() @system {}

If destroy() is called with an actual user-defined class, Object should be ignored.

All of the above should ensure that destroy(obj) can safely infer attributes from the hierarchy [typeof(obj), Object), as anything derived from typeof(obj) is not allowed to violate those attributes.
Comment 13 Simen Kjaeraas 2018-02-07 12:51:14 UTC
*** Issue 17867 has been marked as a duplicate of this issue. ***
Comment 14 Simen Kjaeraas 2018-02-07 12:51:14 UTC
*** Issue 17592 has been marked as a duplicate of this issue. ***
Comment 15 Simen Kjaeraas 2018-02-07 12:51:17 UTC
*** Issue 17297 has been marked as a duplicate of this issue. ***
Comment 16 Simen Kjaeraas 2018-04-23 00:17:43 UTC
*** Issue 17867 has been marked as a duplicate of this issue. ***
Comment 17 alexanderheistermann 2018-05-11 02:00:41 UTC
This bug blocks certain Phobos functions from being @nogc as it involves the destroy function. I am bumping the importance of this to critical status, as the current workarounds I seen involves taking a sledge hammer approach, as it forces the compiler to assume @nogc.

Here an example of what I was referring to.
https://www.auburnsounds.com/blog/2016-11-10_Running-D-without-its-runtime.html
Comment 18 ponce 2018-05-11 08:05:36 UTC
Hopefully fixed in ProtoObject! ;)
Comment 19 alexanderheistermann 2018-07-21 19:24:03 UTC
PhotoObject does not solve this issue, as it involved predefined functions toString, toHash, opCmp, and opEquals.
See this link: https://github.com/andralex/DIPs/blob/ProtoObject/DIPs/DIPxxxx.md
Comment 20 Andrei Alexandrescu 2018-07-22 22:47:32 UTC
Yah, the DIP should definitely amended to include ~this() on the list of functions to look at. Assigning to Razvan so we don't forget about this.
Comment 21 alexanderheistermann 2018-07-24 00:40:03 UTC
I am in the middle process of writing a DIP that address this:
https://github.com/dlang/DIPs/pull/120
Why not drop by and give me some feedback?
Comment 22 Andrei Alexandrescu 2018-07-24 15:32:23 UTC
(In reply to alexanderheistermann from comment #21)
> I am in the middle process of writing a DIP that address this:
> https://github.com/dlang/DIPs/pull/120
> Why not drop by and give me some feedback?

Having a typo in the title is a hitch on the way to ensuring the DIP is taken seriously, let alone ready for review.
Comment 23 alexanderheistermann 2018-07-24 19:15:07 UTC
(In reply to Andrei Alexandrescu from comment #22)
> (In reply to alexanderheistermann from comment #21)
> > I am in the middle process of writing a DIP that address this:
> > https://github.com/dlang/DIPs/pull/120
> > Why not drop by and give me some feedback?
> 
> Having a typo in the title is a hitch on the way to ensuring the DIP is
> taken seriously, let alone ready for review.

Please leave any comments regarding the DIP on the pull request itself and I will address/fix it as soon I have the time.
Comment 24 alexanderheistermann 2020-01-03 01:59:52 UTC
This bug is relevant to DIP "Safe by default" 1028.
Comment 25 moonlightsentinel 2020-02-15 14:27:41 UTC
*** Issue 20582 has been marked as a duplicate of this issue. ***
Comment 26 timon.gehr 2020-06-09 16:49:37 UTC
Also see issue 20914.