[clang-tools-extra] [clang-tidy] Add misc-forbid-non-virtual-base-dtor check (PR #183384)

[via cfe-commits]

CarvedCoder wrote:

> Thanks for implementing this. I’m not a clang-tidy maintainer, so please take this as one reviewer’s opinion — I mainly want to flag the risks I think maintainers will inherit if this lands upstream.
> 
> I agree the underlying issue is real and dangerous: deleting a derived object through a base pointer when the base destructor isn’t virtual is undefined behavior, because the derived destructor won’t run.
> 
> My concern is that the rule being implemented here is a declaration-level heuristic. It effectively tries to predict “someone might delete through `Base*`” from the shape of a class hierarchy. In practice I expect it to be **very noisy, with a lot of false positives**, because it will warn on many perfectly valid hierarchies that never do polymorphic deletion.
> 
> Here is the real bug we want to catch:
> 
> ```cpp
> struct Base { ~Base() = default; };   // non-virtual, public
> struct Derived : Base {
>   int *p = new int[1024];
>   ~Derived() { delete[] p; }
> };
> 
> Base *b = new Derived();
> delete b;  // UB: Base::~Base() runs, Derived::~Derived() does not
> ```
> 
> But the same “shape” also appears in code that is safe by construction:
> 
> ```cpp
> struct Base { ~Base() = default; };      // intentionally non-virtual
> struct Derived : Base { int data; };
> 
> void f() {
>   Derived d;                            // value semantics
>   auto p = std::make_unique<Derived>(); // ownership stays concrete
> } // no delete through Base*
> ```
> 
> The condition “derived adds data members” doesn’t make the signal much better. It doesn’t tell you whether deletion through `Base*` happens; it mostly decides who gets warned. The likely outcome is warning fatigue, or people “fixing” it by making destructors virtual everywhere, which can be the wrong design and can be ABI-breaking.
> 
> There’s also a broader upstream fit question. clang-tidy intentionally hosts checks for different code styles (and some are even mutually conflicting), but the successful upstream checks usually map to something widely accepted in major style guides or to a well-scoped bug pattern with low noise. This rule doesn’t really seem enforced by any major C++ style guide in that form. It feels more like an “XY problem”: we want to catch *polymorphic deletion with a non-virtual base destructor* (the real bug), but we’re enforcing “avoid certain inheritance shapes” as a proxy. That proxy is where the false positives and design pressure come from.
> 
> Fix-its are also hard to make safe. “Make the destructor virtual” can change layout/ABI. “Make it protected” changes the API surface and can break legitimate uses. “Change ownership / use a custom deleter” is not something clang-tidy can safely automate.
> 
> Most importantly, the bug is flow and ownership dependent: the unsafe part is the delete site and whether the pointer may hold a derived dynamic type. That’s the kind of reasoning clang-tidy is not great at, and it’s exactly what the analyzer is meant to do.
> 
> If we want an upstream solution, we already have better-fitting tools. Clang Static Analyzer has a checker for this pattern: `alpha.cplusplus.DeleteWithNonVirtualDtor`. And for runtime coverage of real consequences, sanitizers are effective when the derived destructor would have freed memory/resources: `-fsanitize=address` (ASan) and `-fsanitize=leak` (LSan) can report leaks when `~Derived()` doesn’t run and allocations are left behind.
> 
> Given all that, my suggestion (again, as a non-maintainer trying to flag downstream cost) is to not merge this as an upstream clang-tidy check in its current form. The bug is real, but this heuristic targets the wrong layer and risks becoming noisy, and accumulating special-cases over time. A better upstream direction would be improving and graduating the analyzer checker, rather than adding a declaration-based clang-tidy rule.
> 

After considering the concerns about noise, upstream fit, and the declaration-level nature of this heuristic I agree that this isn’t the right direction for an upstream clang-tidy check in its current form.

I think closing this pr would be a better choice rather than implementing something that will cause problems in the long run. If I revisit this problem I will explore improvements on the analyzer checker instead 

I really appreciate the detailed review it was very helpful 

https://github.com/llvm/llvm-project/pull/183384