[clang-tidy] Fix false positive in cppcoreguidelines-virtual-class-destructor

Incorrectly triggers for template classes that inherit
from a base class that has virtual destructor.

Any class inheriting from a base that has a virtual destructor
will have their destructor also virtual, as per the Standard:

https://timsong-cpp.github.io/cppwp/n4140/class.dtor#9

> If a class has a base class with a virtual destructor,
> its destructor (whether user- or implicitly-declared) is virtual.

Added unit tests to prevent regression.

Fixes bug https://bugs.llvm.org/show_bug.cgi?id=51912

Differential Revision: https://reviews.llvm.org/D110614

Author: carlosgalvezp

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp

@@ -19,16 +19,29 @@ namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
+AST_MATCHER(CXXRecordDecl, hasPublicVirtualOrProtectedNonVirtualDestructor) {
+  // We need to call Node.getDestructor() instead of matching a
+  // CXXDestructorDecl. Otherwise, tests will fail for class templates, since
+  // the primary template (not the specialization) always gets a non-virtual
+  // CXXDestructorDecl in the AST. https://bugs.llvm.org/show_bug.cgi?id=51912
+  const CXXDestructorDecl *Destructor = Node.getDestructor();
+  if (!Destructor)
+    return false;
+
+  return (((Destructor->getAccess() == AccessSpecifier::AS_public) &&
+           Destructor->isVirtual()) ||
+          ((Destructor->getAccess() == AccessSpecifier::AS_protected) &&
+           !Destructor->isVirtual()));
+}
+
 void VirtualClassDestructorCheck::registerMatchers(MatchFinder *Finder) {
   ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualMethod =
       hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));
 
   Finder->addMatcher(
       cxxRecordDecl(
           anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
-          unless(anyOf(
-              has(cxxDestructorDecl(isPublic(), isVirtual())),
-              has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
+          unless(hasPublicVirtualOrProtectedNonVirtualDestructor()))
           .bind("ProblematicClassOrStruct"),
       this);
 }

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

@@ -202,3 +202,73 @@ struct NonOverridingDerivedStruct : ProtectedNonVirtualBaseStruct {
   void m();
 };
 // inherits virtual method
+
+namespace Bugzilla_51912 {
+// Fixes https://bugs.llvm.org/show_bug.cgi?id=51912
+
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 'ForwardDeclaredStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Normal Template
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'TemplatedDerived' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template <typename T>
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+TemplatedDerived<int> InstantiationWithInt;
+
+// Derived from template, base has virtual dtor
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'DerivedFromTemplateVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template <typename T>
+struct DerivedFromTemplateVirtualBaseStruct : T {
+  virtual void foo();
+};
+
+DerivedFromTemplateVirtualBaseStruct<PublicVirtualBaseStruct> InstantiationWithPublicVirtualBaseStruct;
+
+// Derived from template, base has *not* virtual dtor
+// CHECK-MESSAGES: :[[@LINE+8]]:8: warning: destructor of 'DerivedFromTemplateNonVirtualBaseStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+7]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of 'DerivedFromTemplateNonVirtualBaseStruct<PublicNonVirtualBaseStruct>' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-FIXES: struct DerivedFromTemplateNonVirtualBaseStruct : T {
+// CHECK-FIXES-NEXT: virtual ~DerivedFromTemplateNonVirtualBaseStruct() = default;
+// CHECK-FIXES-NEXT: virtual void foo();
+// CHECK-FIXES-NEXT: };
+template <typename T>
+struct DerivedFromTemplateNonVirtualBaseStruct : T {
+  virtual void foo();
+};
+
+DerivedFromTemplateNonVirtualBaseStruct<PublicNonVirtualBaseStruct> InstantiationWithPublicNonVirtualBaseStruct;
+
+// Derived from template, base has virtual dtor, to be used in a typedef
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'DerivedFromTemplateVirtualBaseStruct2' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template <typename T>
+struct DerivedFromTemplateVirtualBaseStruct2 : T {
+  virtual void foo();
+};
+
+using DerivedFromTemplateVirtualBaseStruct2Typedef = DerivedFromTemplateVirtualBaseStruct2<PublicVirtualBaseStruct>;
+DerivedFromTemplateVirtualBaseStruct2Typedef InstantiationWithPublicVirtualBaseStruct2;
+
+// Derived from template, base has *not* virtual dtor, to be used in a typedef
+// CHECK-MESSAGES: :[[@LINE+8]]:8: warning: destructor of 'DerivedFromTemplateNonVirtualBaseStruct2' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+7]]:8: note: make it public and virtual
+// CHECK-MESSAGES: :[[@LINE+6]]:8: warning: destructor of 'DerivedFromTemplateNonVirtualBaseStruct2<PublicNonVirtualBaseStruct>' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-FIXES: struct DerivedFromTemplateNonVirtualBaseStruct2 : T {
+// CHECK-FIXES-NEXT: virtual ~DerivedFromTemplateNonVirtualBaseStruct2() = default;
+// CHECK-FIXES-NEXT: virtual void foo();
+// CHECK-FIXES-NEXT: };
+template <typename T>
+struct DerivedFromTemplateNonVirtualBaseStruct2 : T {
+  virtual void foo();
+};
+
+using DerivedFromTemplateNonVirtualBaseStruct2Typedef = DerivedFromTemplateNonVirtualBaseStruct2<PublicNonVirtualBaseStruct>;
+DerivedFromTemplateNonVirtualBaseStruct2Typedef InstantiationWithPublicNonVirtualBaseStruct2;
+
+} // namespace Bugzilla_51912