Fix #22709 - Use of extern(C++) destructor is hidden

[dkorpel]

Closes #22709

The initial fix was simply "if (extern(c++) destructor) continue;" on the hidden method check, but that felt odd because it's a weirdly special case, and the bug only manifests with the empty B class inbetween. So after asking Claude to investigate, this deeper bug surfaced where the search for an inherited destructor is too shallow.

The correction looks right to me, but could really use a review from someone with C++ experience.

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#22931"

[thewilsonator]

What exactly was "hidden" about this? along explanation about this in the commit message would be good.
Also that test case doesn't seem to test for much I would expect that level of brevity for an ICE. Is it possible to test observed behaviour, with possibly one of the classes implemented in C++ (or several permutations of implementation on C++/D)?

[dkorpel]

When you add a method to a derived class with the exact same signature, you must add the override keyword to specify it will override the vtbl entry of the base class:

class C {
    int f(int x) => 3;
}

class D : C {
    override int f(int x) => 4;
}

// vtbl = [int f(int x)]

When the signature is different, instead of overriding you can add a new method with the same name as a different overload:

class C {
    int f(int x) => 3;
}

class D : C {
    int f(string x) => 4;
}

// vtbl = [int f(int x), int f(string x)]

But what if the parameter types implicitly convert?

class C {
    int f(uint x) => 3;
}

class D : C {
    int f(int x) => 4;
}

// vtbl = [int f(uint x), int f(int x)]

Even an exact match uint will always match D.f(x) first through implicit conversions, so C.f is considered hidden by D.f. Hence dmd raises an error and suggests adding C.f to an overload set with D.f using an alias.

Now with C++ destructors, from what I gather, the intention is that ~this() implicitly overrides the parent destructor. But this search is shallow, so in the issues test case with classes A, B and C, the resulting vtable is:

extern(C++):
class A
{
    ~this();
}
class B : A
{
}
class C : B
{
    ~this();
}

// vtbl = [A.~this, C.~this]

And then dmd considers A.~this hidden by C.~this. This makes no sense because A. the destructor is supposed to be called automatically, not manually, and B. there should only be 1 destructor overload. So this PR makes class C find the destructor in class A and override it, resulting in:

// vtbl = [C.~this]

That makes sense to me, but I'm not familiar with the exact C++ destructor overriding / chaining semantics hence my question for review.

I had Claude write a cxx-runnable test, but it's currently hard-coded for the itanium C++ abi, so I'll try to make a cross-platform test case that doesn't pry in vtbl internals.

[tim-dlang]

The change looks good. I think there could be other problems, if cppDtorVtblIndex is not set for C++ classes without explicit destructor, like this:

import std.stdio;

extern(C++):
struct S
{
    ~this()
    {
        writeln("S.~this");
    }
}

class A
{
    ~this()
    {
        writeln("A.~this");
    }
}

class B : A
{
    S s;
}

void main()
{
    A x = new B;
    x.destroy();
}

It only runs the destructor for A and does not run the destructor for S, because B has no explicit destructor. See

dmd/compiler/src/dmd/dsymbolsem.d

Lines 5842 to 5854 in 1133fe0

	if (cldec.classKind == ClassKind.cpp && cldec.cppDtorVtblIndex != -1)
	{
	// now we've built the aggregate destructor, we'll make it virtual and assign it to the reserved vtable slot
	cldec.dtor.vtblIndex = cldec.cppDtorVtblIndex;
	cldec.vtbl[cldec.cppDtorVtblIndex] = cldec.dtor;
	if (target.cpp.twoDtorInVtable)
	{
	// TODO: create a C++ compatible deleting destructor (call out to `operator delete`)
	// for the moment, we'll call the non-deleting destructor and leak
	cldec.vtbl[cldec.cppDtorVtblIndex + 1] = cldec.dtor;
	}
	}

It could be better, if the semantic analysis for C++ classes without destructor also sets cppDtorVtblIndex if a base class has it set. Maybe the loop over all base classes would then not be needed. but I'm not sure, if the order of semantic analysis always allows this.

[dkorpel]

Thanks for the review @tim-dlang, is that field destructor issue filed already?

[tim-dlang]

Thanks for the review @tim-dlang, is that field destructor issue filed already?

I have created an issue now: #22942